From 0f97041639b55bc4631145e2053a47a1eb8cdef0 Mon Sep 17 00:00:00 2001 From: Máximo Cuadros Date: Thu, 25 Aug 2016 15:10:40 +0200 Subject: clients/http: better error handling --- clients/common/common.go | 7 ++++--- clients/http/common.go | 9 ++++++--- clients/http/common_test.go | 4 ++-- clients/http/git_upload_pack.go | 27 +++++++++++++++++++++++++-- clients/http/git_upload_pack_test.go | 33 +++++++++++++++++++++++++++++++++ clients/ssh/git_upload_pack.go | 13 +++++++------ 6 files changed, 77 insertions(+), 16 deletions(-) (limited to 'clients') diff --git a/clients/common/common.go b/clients/common/common.go index d128f3f..e1140ec 100644 --- a/clients/common/common.go +++ b/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" diff --git a/clients/http/common.go b/clients/http/common.go index a4b86bd..703208b 100644 --- a/clients/http/common.go +++ b/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) } diff --git a/clients/http/common_test.go b/clients/http/common_test.go index d3e2c9d..287897d 100644 --- a/clients/http/common_test.go +++ b/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) { diff --git a/clients/http/git_upload_pack.go b/clients/http/git_upload_pack.go index 79aec3f..bca4534 100644 --- a/clients/http/git_upload_pack.go +++ b/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() +} diff --git a/clients/http/git_upload_pack_test.go b/clients/http/git_upload_pack_test.go index e252ced..e344a49 100644 --- a/clients/http/git_upload_pack_test.go +++ b/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) diff --git a/clients/ssh/git_upload_pack.go b/clients/ssh/git_upload_pack.go index 03b289a..eb5926e 100644 --- a/clients/ssh/git_upload_pack.go +++ b/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 } -- cgit