clients/http: better error handling

Author: mcuadros

clients/common/common.go

@@ -16,9 +16,10 @@ import (
 )
 
 var (
-	ErrNotFound           = errors.New("repository not found")
-	ErrEmptyGitUploadPack = errors.New("empty git-upload-pack given")
-	ErrInvalidAuthMethod  = errors.New("invalid auth method")
+	ErrRepositoryNotFound    = errors.New("repository not found")
+	ErrAuthorizationRequired = errors.New("authorization required")
+	ErrEmptyGitUploadPack    = errors.New("empty git-upload-pack given")
+	ErrInvalidAuthMethod     = errors.New("invalid auth method")
 )
 
 const GitUploadPackServiceName = "git-upload-pack"

clients/http/common.go

@@ -48,11 +48,14 @@ func NewHTTPError(r *http.Response) error {
 		return nil
 	}
 
-	err := &HTTPError{r}
-	if r.StatusCode == 404 || r.StatusCode == 401 {
-		return core.NewPermanentError(common.ErrNotFound)
+	switch r.StatusCode {
+	case 401:
+		return common.ErrAuthorizationRequired
+	case 404:
+		return common.ErrRepositoryNotFound
 	}
 
+	err := &HTTPError{r}
 	return core.NewUnexpectedError(err)
 }
 

clients/http/common_test.go

@@ -28,11 +28,11 @@ func (s *SuiteCommon) TestNewHTTPError200(c *C) {
 }
 
 func (s *SuiteCommon) TestNewHTTPError401(c *C) {
-	s.testNewHTTPError(c, 401, "permanent client error.*not found.*")
+	s.testNewHTTPError(c, 401, "authorization required")
 }
 
 func (s *SuiteCommon) TestNewHTTPError404(c *C) {
-	s.testNewHTTPError(c, 404, "permanent client error.*not found.*")
+	s.testNewHTTPError(c, 404, "repository not found")
 }
 
 func (s *SuiteCommon) TestNewHTTPError40x(c *C) {

clients/http/git_upload_pack.go

@@ -1,6 +1,7 @@
 package http
 
 import (
+	"bufio"
 	"fmt"
 	"io"
 	"net/http"
@@ -66,11 +67,20 @@ func (s *GitUploadPackService) Fetch(r *common.GitUploadPackRequest) (io.ReadClo
 		return nil, err
 	}
 
-	if err := s.discardResponseInfo(res.Body); err != nil {
+	reader := newBufferedReadCloser(res.Body)
+	if _, err := reader.Peek(1); err != nil {
+		if err == io.ErrUnexpectedEOF {
+			return nil, common.ErrEmptyGitUploadPack
+		}
+
+		return nil, err
+	}
+
+	if err := s.discardResponseInfo(reader); err != nil {
 		return nil, err
 	}
 
-	return res.Body, nil
+	return reader, nil
 }
 
 func (s *GitUploadPackService) discardResponseInfo(r io.Reader) error {
@@ -138,3 +148,16 @@ func (s *GitUploadPackService) applyAuthToRequest(req *http.Request) {
 func (s *GitUploadPackService) Disconnect() (err error) {
 	return nil
 }
+
+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()
+}

clients/http/git_upload_pack_test.go

@@ -41,6 +41,26 @@ func (s *RemoteSuite) TestConnectWithAuthWrongType(c *C) {
 	c.Assert(r.ConnectWithAuth(&mockAuth{}), Equals, common.ErrInvalidAuthMethod)
 }
 
+func (s *RemoteSuite) TestInfoEmpty(c *C) {
+	endpoint, _ := common.NewEndpoint("https://github.com/git-fixture/empty")
+	r := NewGitUploadPackService(endpoint)
+	c.Assert(r.Connect(), IsNil)
+
+	info, err := r.Info()
+	c.Assert(err, Equals, common.ErrAuthorizationRequired)
+	c.Assert(info, IsNil)
+}
+
+func (s *RemoteSuite) TestInfoNotExists(c *C) {
+	endpoint, _ := common.NewEndpoint("https://github.com/git-fixture/not-exists")
+	r := NewGitUploadPackService(endpoint)
+	c.Assert(r.Connect(), IsNil)
+
+	info, err := r.Info()
+	c.Assert(err, Equals, common.ErrAuthorizationRequired)
+	c.Assert(info, IsNil)
+}
+
 func (s *RemoteSuite) TestDefaultBranch(c *C) {
 	r := NewGitUploadPackService(s.Endpoint)
 	c.Assert(r.Connect(), IsNil)
@@ -74,6 +94,19 @@ func (s *RemoteSuite) TestFetch(c *C) {
 	c.Assert(b, HasLen, 85374)
 }
 
+func (s *RemoteSuite) TestFetchNoChanges(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
+
+	req := &common.GitUploadPackRequest{}
+	req.Want(core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
+	req.Have(core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
+
+	reader, err := r.Fetch(req)
+	c.Assert(err, Equals, common.ErrEmptyGitUploadPack)
+	c.Assert(reader, IsNil)
+}
+
 func (s *RemoteSuite) TestFetchMulti(c *C) {
 	r := NewGitUploadPackService(s.Endpoint)
 	c.Assert(r.Connect(), IsNil)

clients/ssh/git_upload_pack.go

@@ -152,6 +152,7 @@ func (s *GitUploadPackService) Fetch(r *common.GitUploadPackRequest) (rc io.Read
 	if err != nil {
 		return nil, err
 	}
+
 	defer func() {
 		// the session can be closed by the other endpoint,
 		// therefore we must ignore a close error.
@@ -168,20 +169,19 @@ func (s *GitUploadPackService) Fetch(r *common.GitUploadPackRequest) (rc io.Read
 		return nil, err
 	}
 
+	if err := session.Start("git-upload-pack " + s.vcs.FullName + ".git"); err != nil {
+		return nil, err
+	}
+
 	go func() {
 		fmt.Fprintln(si, r.String())
 		err = si.Close()
 	}()
 
-	err = session.Start("git-upload-pack " + s.vcs.FullName + ".git")
-	if err != nil {
-		return nil, err
-	}
 	// TODO: investigate this *ExitError type (command fails or
 	// doesn't complete successfully), as it is happenning all
 	// the time, but everything seems to work fine.
-	err = session.Wait()
-	if err != nil {
+	if err := session.Wait(); err != nil {
 		if _, ok := err.(*ssh.ExitError); !ok {
 			return nil, err
 		}
@@ -205,6 +205,7 @@ func (s *GitUploadPackService) Fetch(r *common.GitUploadPackRequest) (rc io.Read
 	if err != nil {
 		return nil, err
 	}
+
 	buf := bytes.NewBuffer(data)
 	return ioutil.NopCloser(buf), nil
 }