Merge pull request #418 from smola/ssh-issue-310

fix push on git and ssh

Author: smola

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
 }

plumbing/transport/git/common.go

@@ -90,12 +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())
+	}
 
-// Wait no-op function, required by the interface
-func (c *command) Wait() error {
-	return nil
+	return fmt.Sprintf("%s %s%chost=%s%c", cmd, ep.Path(), 0, host, 0)
 }
 
 // Close closes the TCP connection and connection.

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)
+	}
+}

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)
 }
 
@@ -263,10 +256,14 @@ 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.
-		return nil, s.Command.Wait()
+		return nil, s.Command.Close()
 	}
 
 	report := packp.NewReportStatus()
@@ -278,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 {
@@ -302,7 +299,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()
@@ -329,10 +326,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."
+	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 {
@@ -352,6 +351,14 @@ func isRepoNotFoundError(s string) bool {
 		return true
 	}
 
+	if strings.HasPrefix(s, gitProtocolNoSuchErr) {
+		return true
+	}
+
+	if strings.HasPrefix(s, gitProtocolAccessDeniedErr) {
+		return true
+	}
+
 	return false
 }
 
@@ -403,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()
-}

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) {
@@ -270,7 +271,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,14 +285,15 @@ 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)
 }
 
 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 +308,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)
 }

remote.go

@@ -76,6 +76,8 @@ func (r *Remote) Push(o *PushOptions) (err error) {
 		return err
 	}
 
+	defer ioutil.CheckClose(s, &err)
+
 	ar, err := s.AdvertisedReferences()
 	if err != nil {
 		return err