plumbing/transport: add common tests and fixes. (#136)

* plumbing/transport: add common tests and fixes.

* add common test suite for different transport implementations.
* fix different behaviour on error handling for ssh and http.
  fixes issue #123.
* support detecting unexisting repositories with SSH + GitHub/Bitbucket
  (apparently, there is no standard for all SSH servers).
* remove ssh.NewClient (only DefaultClient makes sense at the moment).
* make ssh.Client and http.Client private.
* utils/ioutil: utilities to work with io interfaces.

* * transport: test actual objects fetched, not just packfile size.

* * fix doc typo.

* * improve UploadPackRequest.IsEmpty

4 files changed, 83 insertions, 104 deletions

diff --git a/plumbing/transport/ssh/common.go b/plumbing/transport/ssh/common.go
index 6f0f3d4..c327c41 100644
--- a/plumbing/transport/ssh/common.go
+++ b/plumbing/transport/ssh/common.go
@@ -11,32 +11,22 @@ import (
 	"golang.org/x/crypto/ssh"
 )
 
-// New errors introduced by this package.
 var (
-	ErrAdvertistedReferencesAlreadyCalled = errors.New("cannot call AdvertisedReference twice")
-	ErrAlreadyConnected                   = errors.New("ssh session already created")
-	ErrAuthRequired                       = errors.New("cannot connect: auth required")
-	ErrNotConnected                       = errors.New("not connected")
-	ErrUploadPackAnswerFormat             = errors.New("git-upload-pack bad answer format")
-	ErrUnsupportedVCS                     = errors.New("only git is supported")
-	ErrUnsupportedRepo                    = errors.New("only github.com is supported")
+	errAlreadyConnected = errors.New("ssh session already created")
 )
 
-type Client struct{}
+type client struct{}
 
-var DefaultClient = NewClient()
+// DefaultClient is the default SSH client.
+var DefaultClient = &client{}
 
-func NewClient() transport.Client {
-	return &Client{}
-}
-
-func (c *Client) NewFetchPackSession(ep transport.Endpoint) (
+func (c *client) NewFetchPackSession(ep transport.Endpoint) (
 	transport.FetchPackSession, error) {
 
 	return newFetchPackSession(ep)
 }
 
-func (c *Client) NewSendPackSession(ep transport.Endpoint) (
+func (c *client) NewSendPackSession(ep transport.Endpoint) (
 	transport.SendPackSession, error) {
 
 	return newSendPackSession(ep)
@@ -49,6 +39,7 @@ type session struct {
 	session     *ssh.Session
 	stdin       io.WriteCloser
 	stdout      io.Reader
+	stderr      io.Reader
 	sessionDone chan error
 	auth        AuthMethod
 }
@@ -70,6 +61,11 @@ func (s *session) Close() error {
 	}
 
 	s.connected = false
+
+	//XXX: If did read the full packfile, then the session might be already
+	//     closed.
+	_ = s.session.Close()
+
 	return s.client.Close()
 }
 
@@ -79,7 +75,7 @@ func (s *session) Close() error {
 // environment var.
 func (s *session) connect() error {
 	if s.connected {
-		return ErrAlreadyConnected
+		return errAlreadyConnected
 	}
 
 	if err := s.setAuthFromEndpoint(); err != nil {
@@ -138,6 +134,11 @@ func (s *session) openSSHSession() error {
 		return fmt.Errorf("cannot pipe remote stdout: %s", err)
 	}
 
+	s.stderr, err = s.session.StderrPipe()
+	if err != nil {
+		return fmt.Errorf("cannot pipe remote stderr: %s", err)
+	}
+
 	return nil
 }
 
@@ -149,3 +150,20 @@ func (s *session) runCommand(cmd string) chan error {
 
 	return done
 }
+
+const (
+	githubRepoNotFoundErr    = "ERROR: Repository not found."
+	bitbucketRepoNotFoundErr = "conq: repository does not exist."
+)
+
+func isRepoNotFoundError(s string) bool {
+	if strings.HasPrefix(s, githubRepoNotFoundErr) {
+		return true
+	}
+
+	if strings.HasPrefix(s, bitbucketRepoNotFoundErr) {
+		return true
+	}
+
+	return false
+}
diff --git a/plumbing/transport/ssh/common_test.go b/plumbing/transport/ssh/common_test.go
index ac4d03e..da99148 100644
--- a/plumbing/transport/ssh/common_test.go
+++ b/plumbing/transport/ssh/common_test.go
@@ -7,11 +7,3 @@ import (
 )
 
 func Test(t *testing.T) { TestingT(t) }
-
-type ClientSuite struct{}
-
-var _ = Suite(&ClientSuite{})
-
-func (s *ClientSuite) TestNewClient(c *C) {
-	c.Assert(DefaultClient, DeepEquals, NewClient())
-}
diff --git a/plumbing/transport/ssh/fetch_pack.go b/plumbing/transport/ssh/fetch_pack.go
index bda4edf..b43160a 100644
--- a/plumbing/transport/ssh/fetch_pack.go
+++ b/plumbing/transport/ssh/fetch_pack.go
@@ -2,13 +2,16 @@
 package ssh
 
 import (
+	"bufio"
 	"bytes"
 	"fmt"
 	"io"
 
+	"gopkg.in/src-d/go-git.v4/plumbing/format/packp/advrefs"
 	"gopkg.in/src-d/go-git.v4/plumbing/format/packp/pktline"
 	"gopkg.in/src-d/go-git.v4/plumbing/format/packp/ulreq"
 	"gopkg.in/src-d/go-git.v4/plumbing/transport"
+	"gopkg.in/src-d/go-git.v4/utils/ioutil"
 
 	"golang.org/x/crypto/ssh"
 )
@@ -35,17 +38,35 @@ func newFetchPackSession(ep transport.Endpoint) (*fetchPackSession, error) {
 
 func (s *fetchPackSession) AdvertisedReferences() (*transport.UploadPackInfo, error) {
 	if s.advRefsRun {
-		return nil, ErrAdvertistedReferencesAlreadyCalled
+		return nil, transport.ErrAdvertistedReferencesAlreadyCalled
 	}
 
+	defer func() { s.advRefsRun = true }()
+
 	if err := s.ensureRunCommand(); err != nil {
 		return nil, err
 	}
 
-	defer func() { s.advRefsRun = true }()
-
 	i := transport.NewUploadPackInfo()
-	return i, i.Decode(s.stdout)
+	if err := i.Decode(s.stdout); err != nil {
+		if err != advrefs.ErrEmpty {
+			return nil, err
+		}
+
+		_ = s.stdin.Close()
+		scan := bufio.NewScanner(s.stderr)
+		if !scan.Scan() {
+			return nil, transport.ErrEmptyRemoteRepository
+		}
+
+		if isRepoNotFoundError(string(scan.Bytes())) {
+			return nil, transport.ErrRepositoryNotFound
+		}
+
+		return nil, err
+	}
+
+	return i, nil
 }
 
 // FetchPack returns a packfile for a given upload request.
@@ -53,6 +74,10 @@ func (s *fetchPackSession) AdvertisedReferences() (*transport.UploadPackInfo, er
 func (s *fetchPackSession) FetchPack(req *transport.UploadPackRequest) (
 	io.ReadCloser, error) {
 
+	if req.IsEmpty() {
+		return nil, transport.ErrEmptyUploadPackRequest
+	}
+
 	if !s.advRefsRun {
 		if _, err := s.AdvertisedReferences(); err != nil {
 			return nil, err
@@ -63,11 +88,19 @@ func (s *fetchPackSession) FetchPack(req *transport.UploadPackRequest) (
 		return nil, err
 	}
 
-	return &fetchSession{
+	fs := &fetchSession{
 		Reader:  s.stdout,
 		session: s.session.session,
 		done:    s.done,
-	}, nil
+	}
+
+	r, err := ioutil.NonEmptyReader(fs)
+	if err == ioutil.ErrEmptyReader {
+		_ = fs.Close()
+		return nil, transport.ErrEmptyUploadPackRequest
+	}
+
+	return ioutil.NewReadCloser(r, fs), nil
 }
 
 func (s *fetchPackSession) ensureRunCommand() error {
diff --git a/plumbing/transport/ssh/fetch_pack_test.go b/plumbing/transport/ssh/fetch_pack_test.go
index 3d62e57..a0321b3 100644
--- a/plumbing/transport/ssh/fetch_pack_test.go
+++ b/plumbing/transport/ssh/fetch_pack_test.go
@@ -1,100 +1,36 @@
 package ssh
 
 import (
-	"io/ioutil"
 	"os"
 
-	"gopkg.in/src-d/go-git.v4/plumbing"
 	"gopkg.in/src-d/go-git.v4/plumbing/transport"
+	"gopkg.in/src-d/go-git.v4/plumbing/transport/test"
 
 	. "gopkg.in/check.v1"
 )
 
 type FetchPackSuite struct {
-	Endpoint transport.Endpoint
+	test.FetchPackSuite
 }
 
 var _ = Suite(&FetchPackSuite{})
 
 func (s *FetchPackSuite) SetUpSuite(c *C) {
-	var err error
-	s.Endpoint, err = transport.NewEndpoint("git@github.com:git-fixtures/basic.git")
-	c.Assert(err, IsNil)
-
 	if os.Getenv("SSH_AUTH_SOCK") == "" {
 		c.Skip("SSH_AUTH_SOCK is not set")
 	}
-}
-
-func (s *FetchPackSuite) TestDefaultBranch(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(r.Close(), IsNil) }()
 
-	info, err := r.AdvertisedReferences()
-	c.Assert(err, IsNil)
-	c.Assert(info.Capabilities.SymbolicReference("HEAD"), Equals, "refs/heads/master")
-}
+	s.FetchPackSuite.Client = DefaultClient
 
-func (s *FetchPackSuite) TestCapabilities(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
+	ep, err := transport.NewEndpoint("git@github.com:git-fixtures/basic.git")
 	c.Assert(err, IsNil)
-	defer func() { c.Assert(r.Close(), IsNil) }()
+	s.FetchPackSuite.Endpoint = ep
 
-	info, err := r.AdvertisedReferences()
+	ep, err = transport.NewEndpoint("git@github.com:git-fixtures/empty.git")
 	c.Assert(err, IsNil)
-	c.Assert(info.Capabilities.Get("agent").Values, HasLen, 1)
-}
-
-func (s *FetchPackSuite) TestFullFetchPack(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(r.Close(), IsNil) }()
-
-	_, err = r.AdvertisedReferences()
-	c.Assert(err, IsNil)
-
-	req := &transport.UploadPackRequest{}
-	req.Want(plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-	req.Want(plumbing.NewHash("e8d3ffab552895c19b9fcf7aa264d277cde33881"))
-	reader, err := r.FetchPack(req)
-	c.Assert(err, IsNil)
-
-	defer func() { c.Assert(reader.Close(), IsNil) }()
+	s.FetchPackSuite.EmptyEndpoint = ep
 
-	b, err := ioutil.ReadAll(reader)
+	ep, err = transport.NewEndpoint("git@github.com:git-fixtures/non-existent.git")
 	c.Assert(err, IsNil)
-	c.Check(len(b), Equals, 85585)
-}
-
-func (s *FetchPackSuite) TestFetchPack(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(r.Close(), IsNil) }()
-
-	req := &transport.UploadPackRequest{}
-	req.Want(plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-	req.Want(plumbing.NewHash("e8d3ffab552895c19b9fcf7aa264d277cde33881"))
-	reader, err := r.FetchPack(req)
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(reader.Close(), IsNil) }()
-
-	b, err := ioutil.ReadAll(reader)
-	c.Assert(err, IsNil)
-	c.Check(len(b), Equals, 85585)
-}
-
-func (s *FetchPackSuite) TestFetchError(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(r.Close(), IsNil) }()
-
-	req := &transport.UploadPackRequest{}
-	req.Want(plumbing.NewHash("1111111111111111111111111111111111111111"))
-
-	reader, err := r.FetchPack(req)
-	c.Assert(err, IsNil)
-
-	err = reader.Close()
-	c.Assert(err, Not(IsNil))
+	s.FetchPackSuite.NonExistentEndpoint = ep
 }