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, 65 insertions, 131 deletions

diff --git a/plumbing/transport/http/common.go b/plumbing/transport/http/common.go
index 038c469..a1b085b 100644
--- a/plumbing/transport/http/common.go
+++ b/plumbing/transport/http/common.go
@@ -9,33 +9,38 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing/transport"
 )
 
-type Client struct {
+type client struct {
 	c *http.Client
 }
 
+// DefaultClient is the default HTTP client, which uses `http.DefaultClient`.
 var DefaultClient = NewClient(nil)
 
 // NewClient creates a new client with a custom net/http client.
 // See `InstallProtocol` to install and override default http client.
 // Unless a properly initialized client is given, it will fall back into
 // `http.DefaultClient`.
+//
+// Note that for HTTP client cannot distinguist between private repositories and
+// unexistent repositories on GitHub. So it returns `ErrAuthorizationRequired`
+// for both.
 func NewClient(c *http.Client) transport.Client {
 	if c == nil {
-		return &Client{http.DefaultClient}
+		return &client{http.DefaultClient}
 	}
 
-	return &Client{
+	return &client{
 		c: c,
 	}
 }
 
-func (c *Client) NewFetchPackSession(ep transport.Endpoint) (
+func (c *client) NewFetchPackSession(ep transport.Endpoint) (
 	transport.FetchPackSession, error) {
 
 	return newFetchPackSession(c.c, ep), nil
 }
 
-func (c *Client) NewSendPackSession(ep transport.Endpoint) (
+func (c *client) NewSendPackSession(ep transport.Endpoint) (
 	transport.SendPackSession, error) {
 
 	return newSendPackSession(c.c, ep), nil
diff --git a/plumbing/transport/http/common_test.go b/plumbing/transport/http/common_test.go
index 1d09fba..432bd07 100644
--- a/plumbing/transport/http/common_test.go
+++ b/plumbing/transport/http/common_test.go
@@ -20,16 +20,20 @@ var _ = Suite(&ClientSuite{})
 
 func (s *ClientSuite) SetUpSuite(c *C) {
 	var err error
-	s.Endpoint, err = transport.NewEndpoint("https://github.com/git-fixtures/basic")
+	s.Endpoint, err = transport.NewEndpoint(
+		"https://github.com/git-fixtures/basic",
+	)
 	c.Assert(err, IsNil)
 }
 
 func (s *FetchPackSuite) TestNewClient(c *C) {
-	roundTripper := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}
-	client := &http.Client{Transport: roundTripper}
-	r := NewClient(client).(*Client)
-
-	c.Assert(r.c, Equals, client)
+	roundTripper := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+	}
+	cl := &http.Client{Transport: roundTripper}
+	r, ok := NewClient(cl).(*client)
+	c.Assert(ok, Equals, true)
+	c.Assert(r.c, Equals, cl)
 }
 
 func (s *ClientSuite) TestNewBasicAuth(c *C) {
@@ -54,7 +58,8 @@ func (s *ClientSuite) TestNewErrNotFound(c *C) {
 }
 
 func (s *ClientSuite) TestNewHTTPError40x(c *C) {
-	s.testNewHTTPError(c, http.StatusPaymentRequired, "unexpected client error.*")
+	s.testNewHTTPError(c, http.StatusPaymentRequired,
+		"unexpected client error.*")
 }
 
 func (s *ClientSuite) testNewHTTPError(c *C, code int, msg string) {
diff --git a/plumbing/transport/http/fetch_pack.go b/plumbing/transport/http/fetch_pack.go
index 0c32672..0becb7b 100644
--- a/plumbing/transport/http/fetch_pack.go
+++ b/plumbing/transport/http/fetch_pack.go
@@ -1,7 +1,6 @@
 package http
 
 import (
-	"bufio"
 	"bytes"
 	"fmt"
 	"io"
@@ -9,12 +8,15 @@ import (
 	"strings"
 
 	"gopkg.in/src-d/go-git.v4/plumbing"
+	"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/transport"
+	"gopkg.in/src-d/go-git.v4/utils/ioutil"
 )
 
 type fetchPackSession struct {
 	*session
+	advRefsRun bool
 }
 
 func newFetchPackSession(c *http.Client,
@@ -31,6 +33,11 @@ func newFetchPackSession(c *http.Client,
 
 func (s *fetchPackSession) AdvertisedReferences() (*transport.UploadPackInfo,
 	error) {
+	if s.advRefsRun {
+		return nil, transport.ErrAdvertistedReferencesAlreadyCalled
+	}
+
+	defer func() { s.advRefsRun = true }()
 
 	url := fmt.Sprintf(
 		"%s/info/refs?service=%s",
@@ -50,15 +57,28 @@ func (s *fetchPackSession) AdvertisedReferences() (*transport.UploadPackInfo,
 	}
 
 	defer res.Body.Close()
+
 	if res.StatusCode == http.StatusUnauthorized {
 		return nil, transport.ErrAuthorizationRequired
 	}
 
 	i := transport.NewUploadPackInfo()
-	return i, i.Decode(res.Body)
+	if err := i.Decode(res.Body); err != nil {
+		if err == advrefs.ErrEmpty {
+			err = transport.ErrEmptyRemoteRepository
+		}
+
+		return nil, err
+	}
+
+	return i, nil
 }
 
 func (s *fetchPackSession) FetchPack(r *transport.UploadPackRequest) (io.ReadCloser, error) {
+	if r.IsEmpty() {
+		return nil, transport.ErrEmptyUploadPackRequest
+	}
+
 	url := fmt.Sprintf(
 		"%s/%s",
 		s.endpoint.String(), transport.UploadPackServiceName,
@@ -69,20 +89,21 @@ func (s *fetchPackSession) FetchPack(r *transport.UploadPackRequest) (io.ReadClo
 		return nil, err
 	}
 
-	reader := newBufferedReadCloser(res.Body)
-	if _, err := reader.Peek(1); err != nil {
-		if err == io.ErrUnexpectedEOF {
-			return nil, transport.ErrEmptyUploadPackRequest
-		}
+	reader, err := ioutil.NonEmptyReader(res.Body)
+	if err == ioutil.ErrEmptyReader || err == io.ErrUnexpectedEOF {
+		return nil, transport.ErrEmptyUploadPackRequest
+	}
 
+	if err != nil {
 		return nil, err
 	}
 
-	if err := discardResponseInfo(reader); err != nil {
+	rc := ioutil.NewReadCloser(reader, res.Body)
+	if err := discardResponseInfo(rc); err != nil {
 		return nil, err
 	}
 
-	return reader, nil
+	return rc, nil
 }
 
 // Close does nothing.
@@ -140,16 +161,3 @@ func (s *fetchPackSession) applyHeadersToRequest(req *http.Request, content *str
 		req.Header.Add("Content-Length", string(content.Len()))
 	}
 }
-
-type bufferedReadCloser struct {
-	*bufio.Reader
-	closer io.Closer
-}
-
-func newBufferedReadCloser(r io.ReadCloser) *bufferedReadCloser {
-	return &bufferedReadCloser{bufio.NewReader(r), r}
-}
-
-func (r *bufferedReadCloser) Close() error {
-	return r.closer.Close()
-}
diff --git a/plumbing/transport/http/fetch_pack_test.go b/plumbing/transport/http/fetch_pack_test.go
index 5ec9991..c7666c8 100644
--- a/plumbing/transport/http/fetch_pack_test.go
+++ b/plumbing/transport/http/fetch_pack_test.go
@@ -1,122 +1,38 @@
 package http
 
 import (
-	"fmt"
-	"io/ioutil"
-
-	"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) {
-	fmt.Println("SetUpSuite\n")
-	var err error
-	s.Endpoint, err = transport.NewEndpoint("https://github.com/git-fixtures/basic")
-	c.Assert(err, IsNil)
-}
+	s.FetchPackSuite.Client = DefaultClient
 
-func (s *FetchPackSuite) TestInfoEmpty(c *C) {
-	endpoint, _ := transport.NewEndpoint("https://github.com/git-fixture/empty")
-	r, err := DefaultClient.NewFetchPackSession(endpoint)
+	ep, err := transport.NewEndpoint("https://github.com/git-fixtures/basic.git")
 	c.Assert(err, IsNil)
-	info, err := r.AdvertisedReferences()
-	c.Assert(err, Equals, transport.ErrAuthorizationRequired)
-	c.Assert(info, IsNil)
-}
+	s.FetchPackSuite.Endpoint = ep
 
-//TODO: Test this error with HTTP BasicAuth too.
-func (s *FetchPackSuite) TestInfoNotExists(c *C) {
-	endpoint, _ := transport.NewEndpoint("https://github.com/git-fixture/not-exists")
-	r, err := DefaultClient.NewFetchPackSession(endpoint)
+	ep, err = transport.NewEndpoint("https://github.com/git-fixtures/empty.git")
 	c.Assert(err, IsNil)
-	info, err := r.AdvertisedReferences()
-	c.Assert(err, Equals, transport.ErrAuthorizationRequired)
-	c.Assert(info, IsNil)
-}
+	s.FetchPackSuite.EmptyEndpoint = ep
 
-func (s *FetchPackSuite) TestDefaultBranch(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
+	ep, err = transport.NewEndpoint("https://github.com/git-fixtures/non-existent.git")
 	c.Assert(err, IsNil)
-	info, err := r.AdvertisedReferences()
-	c.Assert(err, IsNil)
-	c.Assert(info.Capabilities.SymbolicReference("HEAD"), Equals, "refs/heads/master")
-}
-
-func (s *FetchPackSuite) TestCapabilities(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-	info, err := r.AdvertisedReferences()
-	c.Assert(err, IsNil)
-	c.Assert(info.Capabilities.Get("agent").Values, HasLen, 1)
+	s.FetchPackSuite.NonExistentEndpoint = ep
 }
 
-func (s *FetchPackSuite) TestFullFetchPack(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
+func (s *FetchPackSuite) TestInfoNotExists(c *C) {
+	r, err := s.Client.NewFetchPackSession(s.NonExistentEndpoint)
 	c.Assert(err, IsNil)
-
 	info, err := r.AdvertisedReferences()
-	c.Assert(err, IsNil)
-	c.Assert(info, NotNil)
-
-	req := &transport.UploadPackRequest{}
-	req.Want(plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-
-	reader, err := r.FetchPack(req)
-	c.Assert(err, IsNil)
-
-	b, err := ioutil.ReadAll(reader)
-	c.Assert(err, IsNil)
-	c.Assert(b, HasLen, 85374)
-}
-
-func (s *FetchPackSuite) TestFetchPack(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-
-	req := &transport.UploadPackRequest{}
-	req.Want(plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-
-	reader, err := r.FetchPack(req)
-	c.Assert(err, IsNil)
-
-	b, err := ioutil.ReadAll(reader)
-	c.Assert(err, IsNil)
-	c.Assert(b, HasLen, 85374)
-}
-
-func (s *FetchPackSuite) TestFetchPackNoChanges(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	c.Assert(err, IsNil)
-
-	req := &transport.UploadPackRequest{}
-	req.Want(plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-	req.Have(plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-
-	reader, err := r.FetchPack(req)
-	c.Assert(err, Equals, transport.ErrEmptyUploadPackRequest)
-	c.Assert(reader, IsNil)
-}
-
-func (s *FetchPackSuite) TestFetchPackMulti(c *C) {
-	r, err := DefaultClient.NewFetchPackSession(s.Endpoint)
-	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)
-
-	b, err := ioutil.ReadAll(reader)
-	c.Assert(err, IsNil)
-	c.Assert(b, HasLen, 85585)
+	c.Assert(err, Equals, transport.ErrAuthorizationRequired)
+	c.Assert(info, IsNil)
 }