server: add git server implementation #190

smola · 2016-12-19T11:51:34Z

No description provided.

smola · 2016-12-19T11:51:53Z

Depends on #178

* add package with generic server implementation. * implement git-upload-pack and git-receive-pack commands. * add git-receive-pack and git-upload-pack examples. * format/packfile: add UpdateObjectStorage function, extracted from Remote. * capability: add Check.

mcuadros · 2016-12-20T09:15:29Z

examples/git-receive-pack/main.go

+		os.Exit(129)
+	}
+
+	if err := file.DefaultServer.Serve(


Use CheckIfError from the example library

I have not used it here so that the example behaves exactly the same as the official git-receive-pack.

but is a example so we don't need to replicate nothing

mcuadros · 2016-12-20T09:16:59Z

plumbing/protocol/packp/capability/list.go

@@ -167,6 +167,18 @@ func (l *List) Delete(capability Capability) {
 	}
 }

+// Check checks that all capabilities set in the given list are supported
+// in this list.
+func (l *List) Check(ol *List) error {


Maybe instead of doing this you can turn Supports in variadic

Variadic for a slice of capabilities? Yep, we can do that, although we would need to add another method All() to get a slice of all capabilities.

mcuadros · 2016-12-20T09:18:12Z

plumbing/protocol/packp/uppackresp.go


+	Packfile   io.ReadCloser


So that it can be set by the server when building the response.

mcuadros · 2016-12-20T09:18:48Z

plumbing/transport/file/git-receive-pack

+# See examples/git-upload-pack.
+
+path="$(dirname $0)"
+exec go run "$path/../../../examples/git-receive-pack/main.go" "$@"


what is this?

if is for testing write it with the test, or something, or put in a fixture folder

ok, writing it in SetUpSuite

mcuadros · 2016-12-20T09:20:44Z

plumbing/transport/file/server.go

+
+var DefaultServer = NewServer(server.DefaultHandler)
+
+type Server struct {


having the server at this level don't allow to you to having a server of multiple repositories, I was thinking on having a server package at porcelain level

You can call Serve multiple times with different arguments for different repositories.
I am thinking of modularizing it further by decoupling repository finding/loading from the server itself, just as with the server.Handler. Something like server.StorerLoader. The default one would load repositories by file system path and could be shared by default implementations, but the user could pass a loader getting repositories from a database. What do you think?

mcuadros · 2016-12-20T09:21:15Z

plumbing/transport/file/upload_pack_test.go

+	Server        *Server
+	Path          string
+	URL           string
+	UploadPackBin string


Just bin, since the test is for uploadpack

mcuadros · 2016-12-20T09:23:08Z

plumbing/transport/server/server.go

+}
+
+func (s *rpSession) reportStatus() *packp.ReportStatus {
+


extra space

mcuadros · 2016-12-20T09:23:30Z

plumbing/transport/server/server.go

+		return err
+	}
+
+	if err := c.Set(capability.ReportStatus); err != nil {


this can be returned directly

mcuadros · 2016-12-20T09:24:06Z

plumbing/transport/server/server.go

+		}
+
+		ref, err = storer.ResolveReference(s, ref.Target())
+		if err == plumbing.ErrReferenceNotFound {


duplicate code, function?

codecov-io · 2016-12-20T15:51:20Z

Current coverage is 76.09% (diff: 59.87%)

Merging #190 into master will decrease coverage by 1.53%

@@             master       #190   diff @@
==========================================
  Files            92         96     +4   
  Lines          5940       6224   +284   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4611       4736   +125   
- Misses          835        970   +135   
- Partials        494        518    +24

Powered by Codecov. Last update f599ee3...03438c5

mcuadros · 2016-12-20T16:53:10Z

plumbing/transport/server/loader.go

+	// Load loads a storer.Storer given an optional host and a path.
+	// Returns transport.ErrRepositoryNotFound if the repository does not
+	// exist.
+	Load(host, path string) (storer.Storer, error)


why not a Endpoint entity?

This is independent of the transport being used, we could pass an endpoint though and get only the path from it.

alcortesm · 2016-12-23T09:27:23Z

cli/go-git/main.go

+	Verbose bool `short:"v" description:"Activates the verbose mode"`
+}
+
+func (c *cmd) print(format string, a ...interface{}) {


Is this dead code?

Yes. Removed.

alcortesm · 2016-12-23T10:07:29Z

cli/go-git/main.go

+	fmt.Printf(format, a...)
+}
+
+func resolvePath(path string) (string, error) {


I think this can be done calling filepath.Abs(path)

alcortesm · 2016-12-23T10:27:00Z

plumbing/transport/file/server_test.go

+		}
+
+		outChan <- string(b)
+		outDoneChan <- nil


The common use of a done channel is: you send true (or an empty struct) to it when you are done.

But here we are using done channel differently: you send errors to them when ever you find and error AND ALSO you send nil to it when you are done.

It looks like we are mixing the error reporting of a function running inside a gorutine with the notification that it has finish. It reminds me of the style use in programming languages where functions return only one value and errors has to be codified along with valid values (read returning -1 on error).

This is very uncommon in Go, but I don't know enough about the problem to say it is wrong. I had the same doubts whenever I run a command over ssh from Go. I'm pretty sure there is a better solution but I have not had time to find it. Can we give this a second though? is it worth it?

We should at least change the name of the channel, and maybe have more channels to signal different things and use select to read from them.

I've moved this code to a function, which should be more legible. Also changed the error channel to always return a value (either error or nil), just as if it was a regular error return value.

alcortesm · 2016-12-23T10:32:05Z

plumbing/transport/server/loader.go

+	return filesystem.NewStorage(fs)
+}
+
+type MapLoader map[transport.Endpoint]storer.Storer


I don't understand this file. This public value is missing comments.

It looks as if Load returns the corresponding storer for a given endpoint. So there seems to be a relation between storer and endpoints somewhere. This fact should be explained somewhere.

Is this relation bijective?

I don't think Load is a good name for a function that returns a codomain.

Then we have MapLoader, where we can establish or own relations between endpoints and storers by just updating a map. That looks fine, useful and easy to understand.

Then we also have fsLoader, where we cannot establish relations between endpoints and storer ourselves, but are established instead based on some property of the billy filesystem and a small part of and endpoint (its path). It would be very helpful to explain why all this is usefull or why only the path is used.

Maybe I am just too discconected from this to understand it.

I've added further comments to explain each behaviour.

A Loader loads a repository (actually, a storer.Storer, which is a low-level representation of a repository) given an endpoint.

The fsLoader, as documented in NewFilesystemLoader, loads repositories from a file system. That is, it gets an endpoint's path, looks it up in the base file system, and if it's a repository, creates a storer for it. The DefaultLoader is a file system loader using the native OS filesystem.

MapLoader uses a lookup map to return storers that were already created by the user. This is useful for testing (e.g. registering a memory repository with a given path), but also for actual production applications, such as in-process pushes (i.e. source and destination repositories are both loaded in the same go-git process).

The relation in MapLoader is not bijective.

With respect to the name, feel free to suggest any alternative. Load takes an endpoint and it tries to build a storer for it if the repository exists.

I don't think I can provide a better name for Load right now.

I don't know what problems are you trying to solve with this loader thing. The description of the PR is not enough to understand why a Loader is needed and how we were doing that before having a loader. I think I am too disconnected from the project to give my opinion.

From my point of view: I don't understand some of the names here, they are too generic:

'storer.Storer': of what?, you say it is actually a repository in the previous answer but the name does not give any hints about it. By its definition it is anything capable of holding encoded objects and references (which looks as a classical git repository on disk to me, this is, a packfile + references).

'server.Loader': of what?, it does not actually loads a server as its name implies, but apparently it is a database capable of returning the encoded objects and references of a repository from its endpoint. So is this some kind of constructor or factory of Storers? f it is so, maybe it should be closer to the storer package instead of in the server package. There is also a comment still says it looks at an (optional) host and path, that may be outdated. It looks as a resolver to me: given the name of a resource, returns the resource.

I don't think any of these is wrong, it is just that I am very lost and I have not found a description of the problem and how we have decided to solve it, so it is very difficult to understand your solution and think about it.

Maybe we can talk today about it.

Please, feel free to dismiss my review and merge the PR if none of these make sense to you. We can talk about it after merging.

As discussed before, we'll proceed with current naming. We agreed that it's correct that Loader is in the server package, since there are no uses out of it. We can revisit this if a use case comes up for it.

With respect to naming of both Loader and Storer, the discussion is still open, but we'll not stop the PR.

as discussed AFK

* server: add generic server implementation (transport-independent), both for git-upload-pack and git-receive-pack. * server: move internal functions to internal/common. * cli: add git-receive-pack and git-upload-pack implementations. * format/packfile: add UpdateObjectStorage function, extracted from Remote. * transport: implement tranport RPC-like, only with git-upload-pack and git-receive-pack methods. Client renamed to Transport. * storer: add storer.Storer interface. * protocol/packp: add UploadPackResponse constructor with packfile. * protocol/packp: fix UploadPackResponse encoding, add tests. * protocol/packp/capability: implement All.

1.2.1

smola requested a review from mcuadros December 19, 2016 11:51

smola added this to the v4 milestone Dec 19, 2016

smola changed the title ~~[WIP] server: add git server implementation~~ server: add git server implementation Dec 20, 2016

smola added 3 commits December 20, 2016 10:00

add storer.Storer interface.

64e4b34

server: move internal functions to internal/common.

6d68b52

smola requested a review from alcortesm December 20, 2016 09:17

mcuadros suggested changes Dec 20, 2016

View reviewed changes

smola added 7 commits December 20, 2016 10:27

server: clarify TODOs.

2050ad9

server: move code to functions.

386cd18

requested changes

8a967a7

server: move repo loading code to server.Loader.

640233f

capability: implement All.

4e0406d

add cli

59a993a

packp: add UploadPackResponse constructor with packfile.

bde1e3e

changes

8a00430

mcuadros suggested changes Dec 20, 2016

View reviewed changes

smola added 2 commits December 21, 2016 10:03

fix UploadPackResponse encoding, add tests.

dc30b0a

implement tranport RPC-like

231e386

mcuadros approved these changes Dec 22, 2016

View reviewed changes

alcortesm previously requested changes Dec 23, 2016

View reviewed changes

requested changes

03438c5

smola merged commit 133b97e into src-d:master Jan 4, 2017

smola deleted the server branch January 4, 2017 10:18

smola mentioned this pull request Jan 16, 2017

feat(receive-pack) read-only #60

Closed

gsalingu-ovhus pushed a commit to gsalingu-ovhus/go-git that referenced this pull request Mar 28, 2019

Merge pull request src-d#190 from camathieu/1.2.1

8e55244

1.2.1


		var DefaultServer = NewServer(server.DefaultHandler)

		type Server struct {

server: add git server implementation #190

server: add git server implementation #190

Uh oh!

Conversation

smola commented Dec 19, 2016

Uh oh!

smola commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 76.09% (diff: 59.87%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcortesm Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov-io commented Dec 20, 2016 •

edited

Loading

alcortesm Jan 3, 2017 •

edited

Loading

alcortesm Jan 3, 2017 •

edited

Loading