From 7908196d194b4536b60ccf13914f5722ab97cda4 Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Fri, 9 Jun 2017 12:13:53 +0200 Subject: ensure receive-pack session is closed on push. * at low level, ReceivePack must close its stream to the server to signal it has finished. * remote.go: Close() must be called on session. --- plumbing/transport/internal/common/common.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'plumbing/transport') diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go index f67ae29..7001d05 100644 --- a/plumbing/transport/internal/common/common.go +++ b/plumbing/transport/internal/common/common.go @@ -263,6 +263,10 @@ func (s *session) ReceivePack(req *packp.ReferenceUpdateRequest) (*packp.ReportS return nil, err } + if err := s.Stdin.Close(); err != nil { + return nil, err + } + if !req.Capabilities.Supports(capability.ReportStatus) { // If we have neither report-status or sideband, we can only // check return value error. @@ -302,7 +306,7 @@ func (s *session) finish() error { func (s *session) Close() error { if err := s.finish(); err != nil { _ = s.Command.Close() - return nil + return err } return s.Command.Close() -- cgit From 8dee150a302e98c26b92133a80d6b43e4cb31a87 Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Mon, 12 Jun 2017 11:25:39 +0200 Subject: transport/git: ensure port is added to host parameter --- plumbing/transport/git/common.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'plumbing/transport') diff --git a/plumbing/transport/git/common.go b/plumbing/transport/git/common.go index 753f125..6776b69 100644 --- a/plumbing/transport/git/common.go +++ b/plumbing/transport/git/common.go @@ -90,7 +90,12 @@ func (c *command) StdoutPipe() (io.Reader, error) { } func endpointToCommand(cmd string, ep transport.Endpoint) string { - return fmt.Sprintf("%s %s%chost=%s%c", cmd, ep.Path(), 0, ep.Host(), 0) + host := ep.Host() + if ep.Port() != DefaultPort { + host = fmt.Sprintf("%s:%d", ep.Host(), ep.Port()) + } + + return fmt.Sprintf("%s %s%chost=%s%c", cmd, ep.Path(), 0, host, 0) } // Wait no-op function, required by the interface -- cgit From 20637689e489758b36d46e2409e20eb4fa703e73 Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Mon, 12 Jun 2017 11:26:27 +0200 Subject: plumbing/transport: detect git protocol "no such repository" error --- plumbing/transport/internal/common/common.go | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'plumbing/transport') diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go index 7001d05..41be776 100644 --- a/plumbing/transport/internal/common/common.go +++ b/plumbing/transport/internal/common/common.go @@ -337,6 +337,7 @@ var ( bitbucketRepoNotFoundErr = "conq: repository does not exist." localRepoNotFoundErr = "does not appear to be a git repository" gitProtocolNotFoundErr = "ERR \n Repository not found." + gitProtocolNoSuchErr = "ERR no such repository" ) func isRepoNotFoundError(s string) bool { @@ -356,6 +357,10 @@ func isRepoNotFoundError(s string) bool { return true } + if strings.HasPrefix(s, gitProtocolNoSuchErr) { + return true + } + return false } -- cgit From 1deb0fa5a1e8ed605d78717db288d5a8685e2a5d Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Mon, 12 Jun 2017 11:27:21 +0200 Subject: transport/git: add git-receive-pack tests --- plumbing/transport/git/receive_pack_test.go | 140 ++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 plumbing/transport/git/receive_pack_test.go (limited to 'plumbing/transport') diff --git a/plumbing/transport/git/receive_pack_test.go b/plumbing/transport/git/receive_pack_test.go new file mode 100644 index 0000000..d73966e --- /dev/null +++ b/plumbing/transport/git/receive_pack_test.go @@ -0,0 +1,140 @@ +package git + +import ( + "fmt" + "io" + "io/ioutil" + "net" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/src-d/go-git-fixtures" + "gopkg.in/src-d/go-git.v4/plumbing/transport" + "gopkg.in/src-d/go-git.v4/plumbing/transport/test" + + . "gopkg.in/check.v1" +) + +type ReceivePackSuite struct { + test.ReceivePackSuite + fixtures.Suite + + base string + daemon *exec.Cmd +} + +var _ = Suite(&ReceivePackSuite{}) + +func (s *ReceivePackSuite) SetUpTest(c *C) { + s.ReceivePackSuite.Client = DefaultClient + + port, err := freePort() + c.Assert(err, IsNil) + + base, err := ioutil.TempDir(os.TempDir(), "go-git-daemon-test") + c.Assert(err, IsNil) + s.base = base + + host := fmt.Sprintf("localhost_%d", port) + interpolatedBase := filepath.Join(base, host) + err = os.MkdirAll(interpolatedBase, 0755) + c.Assert(err, IsNil) + + dotgit := fixtures.Basic().One().DotGit().Base() + prepareRepo(c, dotgit) + err = os.Rename(dotgit, filepath.Join(interpolatedBase, "basic.git")) + c.Assert(err, IsNil) + + ep, err := transport.NewEndpoint(fmt.Sprintf("git://localhost:%d/basic.git", port)) + c.Assert(err, IsNil) + s.ReceivePackSuite.Endpoint = ep + + dotgit = fixtures.ByTag("empty").One().DotGit().Base() + prepareRepo(c, dotgit) + err = os.Rename(dotgit, filepath.Join(interpolatedBase, "empty.git")) + c.Assert(err, IsNil) + + ep, err = transport.NewEndpoint(fmt.Sprintf("git://localhost:%d/empty.git", port)) + c.Assert(err, IsNil) + s.ReceivePackSuite.EmptyEndpoint = ep + + ep, err = transport.NewEndpoint(fmt.Sprintf("git://localhost:%d/non-existent.git", port)) + c.Assert(err, IsNil) + s.ReceivePackSuite.NonExistentEndpoint = ep + + s.daemon = exec.Command( + "git", + "daemon", + fmt.Sprintf("--base-path=%s", base), + "--export-all", + "--enable=receive-pack", + "--reuseaddr", + fmt.Sprintf("--port=%d", port), + // Use interpolated paths to validate that clients are specifying + // host and port properly. + // Note that some git versions (e.g. v2.11.0) had a bug that prevented + // the use of repository paths containing colons (:), so we use + // underscore (_) instead of colon in the interpolation. + // See https://github.com/git/git/commit/fe050334074c5132d01e1df2c1b9a82c9b8d394c + fmt.Sprintf("--interpolated-path=%s/%%H_%%P%%D", base), + // Unless max-connections is limited to 1, a git-receive-pack + // might not be seen by a subsequent operation. + "--max-connections=1", + // Whitelist required for interpolated paths. + fmt.Sprintf("%s/%s", interpolatedBase, "basic.git"), + fmt.Sprintf("%s/%s", interpolatedBase, "empty.git"), + ) + + // Environment must be inherited in order to acknowledge GIT_EXEC_PATH if set. + s.daemon.Env = os.Environ() + + err = s.daemon.Start() + c.Assert(err, IsNil) + + // Connections might be refused if we start sending request too early. + time.Sleep(time.Millisecond * 500) +} + +func (s *ReceivePackSuite) TearDownTest(c *C) { + err := s.daemon.Process.Signal(os.Interrupt) + c.Assert(err, IsNil) + _ = s.daemon.Wait() + err = os.RemoveAll(s.base) + c.Assert(err, IsNil) +} + +func freePort() (int, error) { + addr, err := net.ResolveTCPAddr("tcp", "localhost:0") + if err != nil { + return 0, err + } + + l, err := net.ListenTCP("tcp", addr) + if err != nil { + return 0, err + } + + return l.Addr().(*net.TCPAddr).Port, l.Close() +} + +const bareConfig = `[core] +repositoryformatversion = 0 +filemode = true +bare = true` + +func prepareRepo(c *C, path string) { + // git-receive-pack refuses to update refs/heads/master on non-bare repo + // so we ensure bare repo config. + config := filepath.Join(path, "config") + if _, err := os.Stat(config); err == nil { + f, err := os.OpenFile(config, os.O_TRUNC|os.O_WRONLY, 0) + c.Assert(err, IsNil) + content := strings.NewReader(bareConfig) + _, err = io.Copy(f, content) + c.Assert(err, IsNil) + c.Assert(f.Close(), IsNil) + } +} -- cgit From 9ebf0e651465fc304405d0253f837c1b5e929c24 Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Mon, 12 Jun 2017 13:51:01 +0200 Subject: plumbing/transport: detect "access denied error" "ERR access denied or repository not exported:" is now detected as transport.ErrRepositoryNotFound, since that's what git-daemon returns when --informative-errors is not used. --- plumbing/transport/internal/common/common.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'plumbing/transport') diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go index 41be776..d947d28 100644 --- a/plumbing/transport/internal/common/common.go +++ b/plumbing/transport/internal/common/common.go @@ -333,11 +333,12 @@ func (s *session) checkNotFoundError() error { } var ( - githubRepoNotFoundErr = "ERROR: Repository not found." - bitbucketRepoNotFoundErr = "conq: repository does not exist." - localRepoNotFoundErr = "does not appear to be a git repository" - gitProtocolNotFoundErr = "ERR \n Repository not found." - gitProtocolNoSuchErr = "ERR no such repository" + githubRepoNotFoundErr = "ERROR: Repository not found." + bitbucketRepoNotFoundErr = "conq: repository does not exist." + localRepoNotFoundErr = "does not appear to be a git repository" + gitProtocolNotFoundErr = "ERR \n Repository not found." + gitProtocolNoSuchErr = "ERR no such repository" + gitProtocolAccessDeniedErr = "ERR access denied" ) func isRepoNotFoundError(s string) bool { @@ -361,6 +362,10 @@ func isRepoNotFoundError(s string) bool { return true } + if strings.HasPrefix(s, gitProtocolAccessDeniedErr) { + return true + } + return false } -- cgit From 4b2d9b42bf18556bc51434e7221927c681bce746 Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Mon, 12 Jun 2017 15:29:55 +0200 Subject: fix ReceivePackSuite.TestSendPackAddDeleteReference --- plumbing/transport/test/receive_pack.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'plumbing/transport') diff --git a/plumbing/transport/test/receive_pack.go b/plumbing/transport/test/receive_pack.go index f4be8c8..066e684 100644 --- a/plumbing/transport/test/receive_pack.go +++ b/plumbing/transport/test/receive_pack.go @@ -270,7 +270,6 @@ func (s *ReceivePackSuite) TestSendPackAddDeleteReference(c *C) { func (s *ReceivePackSuite) testSendPackAddReference(c *C) { r, err := s.Client.NewReceivePackSession(s.Endpoint, s.EmptyAuth) c.Assert(err, IsNil) - defer func() { c.Assert(r.Close(), IsNil) }() fixture := fixtures.Basic().ByTag("packfile").One() @@ -285,6 +284,8 @@ func (s *ReceivePackSuite) testSendPackAddReference(c *C) { req.Capabilities.Set(capability.ReportStatus) } + c.Assert(r.Close(), IsNil) + s.receivePack(c, s.Endpoint, req, nil, false) s.checkRemoteReference(c, s.Endpoint, "refs/heads/newbranch", fixture.Head) } @@ -292,7 +293,6 @@ func (s *ReceivePackSuite) testSendPackAddReference(c *C) { func (s *ReceivePackSuite) testSendPackDeleteReference(c *C) { r, err := s.Client.NewReceivePackSession(s.Endpoint, s.EmptyAuth) c.Assert(err, IsNil) - defer func() { c.Assert(r.Close(), IsNil) }() fixture := fixtures.Basic().ByTag("packfile").One() @@ -307,6 +307,8 @@ func (s *ReceivePackSuite) testSendPackDeleteReference(c *C) { req.Capabilities.Set(capability.ReportStatus) } + c.Assert(r.Close(), IsNil) + s.receivePack(c, s.Endpoint, req, nil, false) s.checkRemoteReference(c, s.Endpoint, "refs/heads/newbranch", plumbing.ZeroHash) } -- cgit From cbbd2a9ad8ee990c79bd847b0df2823e2449ea4e Mon Sep 17 00:00:00 2001 From: "Santiago M. Mola" Date: Tue, 13 Jun 2017 14:23:15 +0200 Subject: transport/internal: remove Wait function, use Close directly --- plumbing/transport/file/client.go | 17 ++++++++++++----- plumbing/transport/git/common.go | 5 ----- plumbing/transport/internal/common/common.go | 26 +++++--------------------- plumbing/transport/test/receive_pack.go | 3 ++- 4 files changed, 19 insertions(+), 32 deletions(-) (limited to 'plumbing/transport') diff --git a/plumbing/transport/file/client.go b/plumbing/transport/file/client.go index d2a57d0..a199b01 100644 --- a/plumbing/transport/file/client.go +++ b/plumbing/transport/file/client.go @@ -3,6 +3,7 @@ package file import ( "io" + "os" "os/exec" "gopkg.in/src-d/go-git.v4/plumbing/transport" @@ -71,10 +72,16 @@ func (c *command) Close() error { return nil } - return c.cmd.Process.Kill() -} - -func (c *command) Wait() error { defer func() { c.closed = true }() - return c.cmd.Wait() + err := c.cmd.Wait() + if _, ok := err.(*os.PathError); ok { + return nil + } + + // When a repository does not exist, the command exits with code 128. + if _, ok := err.(*exec.ExitError); ok { + return nil + } + + return err } diff --git a/plumbing/transport/git/common.go b/plumbing/transport/git/common.go index 6776b69..fcd02f8 100644 --- a/plumbing/transport/git/common.go +++ b/plumbing/transport/git/common.go @@ -98,11 +98,6 @@ func endpointToCommand(cmd string, ep transport.Endpoint) string { return fmt.Sprintf("%s %s%chost=%s%c", cmd, ep.Path(), 0, host, 0) } -// Wait no-op function, required by the interface -func (c *command) Wait() error { - return nil -} - // Close closes the TCP connection and connection. func (c *command) Close() error { if !c.connected { diff --git a/plumbing/transport/internal/common/common.go b/plumbing/transport/internal/common/common.go index d947d28..c1e1518 100644 --- a/plumbing/transport/internal/common/common.go +++ b/plumbing/transport/internal/common/common.go @@ -59,14 +59,8 @@ type Command interface { // Start starts the specified command. It does not wait for it to // complete. Start() error - // Wait waits for the command to exit. It must have been started by - // Start. The returned error is nil if the command runs, has no - // problems copying stdin, stdout, and stderr, and exits with a zero - // exit status. - Wait() error // Close closes the command and releases any resources used by it. It - // can be called to forcibly finish the command without calling to Wait - // or to release resources after calling Wait. + // will block until the command exits. Close() error } @@ -178,6 +172,7 @@ func (s *session) handleAdvRefDecodeError(err error) error { // If repository is not found, we get empty stdout and server writes an // error to stderr. if err == packp.ErrEmptyInput { + s.finished = true if err := s.checkNotFoundError(); err != nil { return err } @@ -246,9 +241,7 @@ func (s *session) UploadPack(req *packp.UploadPackRequest) (*packp.UploadPackRes return nil, err } - wc := &waitCloser{s.Command} - rc := ioutil.NewReadCloser(r, wc) - + rc := ioutil.NewReadCloser(r, s.Command) return DecodeUploadPackResponse(rc, req) } @@ -270,7 +263,7 @@ func (s *session) ReceivePack(req *packp.ReferenceUpdateRequest) (*packp.ReportS if !req.Capabilities.Supports(capability.ReportStatus) { // If we have neither report-status or sideband, we can only // check return value error. - return nil, s.Command.Wait() + return nil, s.Command.Close() } report := packp.NewReportStatus() @@ -282,7 +275,7 @@ func (s *session) ReceivePack(req *packp.ReferenceUpdateRequest) (*packp.ReportS return report, err } - return report, s.Command.Wait() + return report, s.Command.Close() } func (s *session) finish() error { @@ -417,12 +410,3 @@ func DecodeUploadPackResponse(r io.ReadCloser, req *packp.UploadPackRequest) ( return res, nil } - -type waitCloser struct { - Command Command -} - -// Close waits until the command exits and returns error, if any. -func (c *waitCloser) Close() error { - return c.Command.Wait() -} diff --git a/plumbing/transport/test/receive_pack.go b/plumbing/transport/test/receive_pack.go index 066e684..bb1c58a 100644 --- a/plumbing/transport/test/receive_pack.go +++ b/plumbing/transport/test/receive_pack.go @@ -39,10 +39,10 @@ func (s *ReceivePackSuite) TestAdvertisedReferencesEmpty(c *C) { func (s *ReceivePackSuite) TestAdvertisedReferencesNotExists(c *C) { r, err := s.Client.NewReceivePackSession(s.NonExistentEndpoint, s.EmptyAuth) c.Assert(err, IsNil) - defer func() { c.Assert(r.Close(), IsNil) }() ar, err := r.AdvertisedReferences() c.Assert(err, Equals, transport.ErrRepositoryNotFound) c.Assert(ar, IsNil) + c.Assert(r.Close(), IsNil) r, err = s.Client.NewReceivePackSession(s.NonExistentEndpoint, s.EmptyAuth) c.Assert(err, IsNil) @@ -54,6 +54,7 @@ func (s *ReceivePackSuite) TestAdvertisedReferencesNotExists(c *C) { writer, err := r.ReceivePack(req) c.Assert(err, Equals, transport.ErrRepositoryNotFound) c.Assert(writer, IsNil) + c.Assert(r.Close(), IsNil) } func (s *ReceivePackSuite) TestCallAdvertisedReferenceTwice(c *C) { -- cgit