Add configurable http client factory (fixes #120) (#121)

* new http client factory ready to install/override default http(s)
 * mv GitUploadPackServiceFactory to clients.common pkg
 * rename http.HTTPError to http.Err
 * rename http.HTTPAuthMethod to http.AuthMethod
 * add doc and examples/ usage
 * general improvements:
   - update install link in readme to v4 (example are already pointing v4)
   - fix indentation in package doc (styling for godoc.org)
   - use http.Status constants instead of integers
   - close leaked response body
   - rm named returns which stutter in doc
   - fix one format string
   - rm unnecessary if checks
   - documentation fixes

7 files changed, 72 insertions, 47 deletions

diff --git a/plumbing/client/common.go b/plumbing/client/common.go
index 6a99339..1524753 100644
--- a/plumbing/client/common.go
+++ b/plumbing/client/common.go
@@ -1,4 +1,4 @@
-// Package clients includes the implementation for diferent transport protocols
+// Package clients includes the implementation for different transport protocols
 //
 // go-git needs the packfile and the refs of the repo. The
 // `NewGitUploadPackService` function returns an object that allows to
@@ -21,17 +21,15 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing/client/ssh"
 )
 
-type GitUploadPackServiceFactory func(common.Endpoint) common.GitUploadPackService
-
 // Protocols are the protocols supported by default.
-var Protocols = map[string]GitUploadPackServiceFactory{
+var Protocols = map[string]common.GitUploadPackServiceFactory{
 	"http":  http.NewGitUploadPackService,
 	"https": http.NewGitUploadPackService,
 	"ssh":   ssh.NewGitUploadPackService,
 }
 
 // InstallProtocol adds or modifies an existing protocol.
-func InstallProtocol(scheme string, f GitUploadPackServiceFactory) {
+func InstallProtocol(scheme string, f common.GitUploadPackServiceFactory) {
 	Protocols[scheme] = f
 }
 
diff --git a/plumbing/client/common/common.go b/plumbing/client/common/common.go
index 97f78c4..b2d52e8 100644
--- a/plumbing/client/common/common.go
+++ b/plumbing/client/common/common.go
@@ -36,6 +36,9 @@ type GitUploadPackService interface {
 	Disconnect() error
 }
 
+// GitUploadPackServiceFactory is capable of instantiating GitUploadPackService with given endpoint
+type GitUploadPackServiceFactory func(Endpoint) GitUploadPackService
+
 type AuthMethod interface {
 	Name() string
 	String() string
diff --git a/plumbing/client/http/common.go b/plumbing/client/http/common.go
index 4c07876..2447995 100644
--- a/plumbing/client/http/common.go
+++ b/plumbing/client/http/common.go
@@ -9,8 +9,8 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing/client/common"
 )
 
-// HTTPAuthMethod concrete implementation of common.AuthMethod for HTTP services
-type HTTPAuthMethod interface {
+// AuthMethod is concrete implementation of common.AuthMethod for HTTP services
+type AuthMethod interface {
 	common.AuthMethod
 	setAuth(r *http.Request)
 }
@@ -29,7 +29,7 @@ func (a *BasicAuth) setAuth(r *http.Request) {
 	r.SetBasicAuth(a.username, a.password)
 }
 
-// Name name of the auth
+// Name is name of the auth
 func (a *BasicAuth) Name() string {
 	return "http-basic-auth"
 }
@@ -43,34 +43,33 @@ func (a *BasicAuth) String() string {
 	return fmt.Sprintf("%s - %s:%s", a.Name(), a.username, masked)
 }
 
-// HTTPError a dedicated error to return errors bases on status codes
-type HTTPError struct {
+// Err is a dedicated error to return errors based on status code
+type Err struct {
 	Response *http.Response
 }
 
-// NewHTTPError returns a new HTTPError based on a http response
-func NewHTTPError(r *http.Response) error {
-	if r.StatusCode >= 200 && r.StatusCode < 300 {
+// NewErr returns a new Err based on a http response
+func NewErr(r *http.Response) error {
+	if r.StatusCode >= http.StatusOK && r.StatusCode < http.StatusMultipleChoices {
 		return nil
 	}
 
 	switch r.StatusCode {
-	case 401:
+	case http.StatusUnauthorized:
 		return common.ErrAuthorizationRequired
-	case 404:
+	case http.StatusNotFound:
 		return common.ErrRepositoryNotFound
 	}
 
-	err := &HTTPError{r}
-	return plumbing.NewUnexpectedError(err)
+	return plumbing.NewUnexpectedError(&Err{r})
 }
 
 // StatusCode returns the status code of the response
-func (e *HTTPError) StatusCode() int {
+func (e *Err) StatusCode() int {
 	return e.Response.StatusCode
 }
 
-func (e *HTTPError) Error() string {
+func (e *Err) Error() string {
 	return fmt.Sprintf("unexpected requesting %q status code: %d",
 		e.Response.Request.URL, e.Response.StatusCode,
 	)
diff --git a/plumbing/client/http/common_test.go b/plumbing/client/http/common_test.go
index 287897d..7503d84 100644
--- a/plumbing/client/http/common_test.go
+++ b/plumbing/client/http/common_test.go
@@ -20,23 +20,22 @@ func (s *SuiteCommon) TestNewBasicAuth(c *C) {
 	c.Assert(a.String(), Equals, "http-basic-auth - foo:*******")
 }
 
-func (s *SuiteCommon) TestNewHTTPError200(c *C) {
-	res := &http.Response{StatusCode: 200}
-	res.StatusCode = 200
-	err := NewHTTPError(res)
+func (s *SuiteCommon) TestNewErrOK(c *C) {
+	res := &http.Response{StatusCode: http.StatusOK}
+	err := NewErr(res)
 	c.Assert(err, IsNil)
 }
 
-func (s *SuiteCommon) TestNewHTTPError401(c *C) {
-	s.testNewHTTPError(c, 401, "authorization required")
+func (s *SuiteCommon) TestNewErrUnauthorized(c *C) {
+	s.testNewHTTPError(c, http.StatusUnauthorized, "authorization required")
 }
 
-func (s *SuiteCommon) TestNewHTTPError404(c *C) {
-	s.testNewHTTPError(c, 404, "repository not found")
+func (s *SuiteCommon) TestNewErrNotFound(c *C) {
+	s.testNewHTTPError(c, http.StatusNotFound, "repository not found")
 }
 
 func (s *SuiteCommon) TestNewHTTPError40x(c *C) {
-	s.testNewHTTPError(c, 402, "unexpected client error.*")
+	s.testNewHTTPError(c, http.StatusPaymentRequired, "unexpected client error.*")
 }
 
 func (s *SuiteCommon) testNewHTTPError(c *C, code int, msg string) {
@@ -46,7 +45,7 @@ func (s *SuiteCommon) testNewHTTPError(c *C, code int, msg string) {
 		Request:    req,
 	}
 
-	err := NewHTTPError(res)
+	err := NewErr(res)
 	c.Assert(err, NotNil)
 	c.Assert(err, ErrorMatches, msg)
 }
diff --git a/plumbing/client/http/git_upload_pack.go b/plumbing/client/http/git_upload_pack.go
index c1f4a0b..1ecf299 100644
--- a/plumbing/client/http/git_upload_pack.go
+++ b/plumbing/client/http/git_upload_pack.go
@@ -13,26 +13,41 @@ import (
 	"gopkg.in/src-d/go-git.v4/plumbing/format/packp/pktline"
 )
 
-// GitUploadPackService git-upoad-pack service over HTTP
+// GitUploadPackService git-upload-pack service over HTTP
 type GitUploadPackService struct {
 	client   *http.Client
 	endpoint common.Endpoint
-	auth     HTTPAuthMethod
+	auth     AuthMethod
 }
 
 // NewGitUploadPackService connects to a git-upload-pack service over HTTP, the
 // auth is extracted from the URL, or can be provided using the SetAuth method
 func NewGitUploadPackService(endpoint common.Endpoint) common.GitUploadPackService {
+	return newGitUploadPackService(endpoint, http.DefaultClient)
+}
+
+// NewGitUploadPackServiceFactory creates a http client factory with a customizable client
+// See `InstallProtocol` to install and override default http client.
+// Unless a properly initialized client is given, it will fall back into `http.DefaultClient`.
+func NewGitUploadPackServiceFactory(client *http.Client) common.GitUploadPackServiceFactory {
+	return func(endpoint common.Endpoint) common.GitUploadPackService {
+		return newGitUploadPackService(endpoint, client)
+	}
+}
+
+func newGitUploadPackService(endpoint common.Endpoint, client *http.Client) common.GitUploadPackService {
+	if client == nil {
+		client = http.DefaultClient
+	}
 	s := &GitUploadPackService{
-		client:   http.DefaultClient,
+		client:   client,
 		endpoint: endpoint,
 	}
-
 	s.setBasicAuthFromEndpoint()
 	return s
 }
 
-// Connect has not any effect, is here just for meet the interface
+// Connect has not any effect, is here to satisfy interface
 func (s *GitUploadPackService) Connect() error {
 	return nil
 }
@@ -54,7 +69,7 @@ func (s *GitUploadPackService) setBasicAuthFromEndpoint() {
 
 // SetAuth sets the AuthMethod
 func (s *GitUploadPackService) SetAuth(auth common.AuthMethod) error {
-	httpAuth, ok := auth.(HTTPAuthMethod)
+	httpAuth, ok := auth.(AuthMethod)
 	if !ok {
 		return common.ErrInvalidAuthMethod
 	}
@@ -139,7 +154,8 @@ func (s *GitUploadPackService) doRequest(method, url string, content *strings.Re
 		return nil, plumbing.NewUnexpectedError(err)
 	}
 
-	if err := NewHTTPError(res); err != nil {
+	if err := NewErr(res); err != nil {
+		_ = res.Body.Close()
 		return nil, err
 	}
 
@@ -168,7 +184,7 @@ func (s *GitUploadPackService) applyAuthToRequest(req *http.Request) {
 }
 
 // Disconnect do nothing
-func (s *GitUploadPackService) Disconnect() (err error) {
+func (s *GitUploadPackService) Disconnect() error {
 	return nil
 }
 
diff --git a/plumbing/client/http/git_upload_pack_test.go b/plumbing/client/http/git_upload_pack_test.go
index a50dbdf..8010cea 100644
--- a/plumbing/client/http/git_upload_pack_test.go
+++ b/plumbing/client/http/git_upload_pack_test.go
@@ -1,7 +1,9 @@
 package http
 
 import (
+	"crypto/tls"
 	"io/ioutil"
+	"net/http"
 
 	. "gopkg.in/check.v1"
 	"gopkg.in/src-d/go-git.v4/plumbing"
@@ -30,6 +32,18 @@ func (s *RemoteSuite) TestNewGitUploadPackServiceAuth(c *C) {
 	c.Assert(auth.String(), Equals, "http-basic-auth - foo:*******")
 }
 
+func (s *RemoteSuite) TestNewGitUploadPackServiceFactory(c *C) {
+	e, err := common.NewEndpoint("https://foo:bar@github.com/git-fixtures/basic")
+	c.Assert(err, IsNil)
+
+	roundTripper := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}
+	client := &http.Client{Transport: roundTripper}
+	r := NewGitUploadPackServiceFactory(client)(e).(*GitUploadPackService)
+
+	c.Assert(r.auth.String(), Equals, "http-basic-auth - foo:*******")
+	c.Assert(r.client.Transport, Equals, roundTripper)
+}
+
 func (s *RemoteSuite) TestConnect(c *C) {
 	r := NewGitUploadPackService(s.Endpoint)
 	c.Assert(r.Connect(), IsNil)
diff --git a/plumbing/client/ssh/git_upload_pack.go b/plumbing/client/ssh/git_upload_pack.go
index e2b73fd..db7fa93 100644
--- a/plumbing/client/ssh/git_upload_pack.go
+++ b/plumbing/client/ssh/git_upload_pack.go
@@ -84,11 +84,7 @@ func (s *GitUploadPackService) setAuthFromEndpoint() error {
 
 	var err error
 	s.auth, err = NewSSHAgentAuth(u)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return err
 }
 
 // SetAuth sets the AuthMethod
@@ -105,7 +101,7 @@ func (s *GitUploadPackService) SetAuth(auth common.AuthMethod) error {
 // Info returns the GitUploadPackInfo of the repository. The client must be
 // connected with the repository (using the ConnectWithAuth() method) before
 // using this method.
-func (s *GitUploadPackService) Info() (i *common.GitUploadPackInfo, err error) {
+func (s *GitUploadPackService) Info() (*common.GitUploadPackInfo, error) {
 	if !s.connected {
 		return nil, ErrNotConnected
 	}
@@ -125,12 +121,12 @@ func (s *GitUploadPackService) Info() (i *common.GitUploadPackInfo, err error) {
 		return nil, err
 	}
 
-	i = common.NewGitUploadPackInfo()
+	i := common.NewGitUploadPackInfo()
 	return i, i.Decode(bytes.NewReader(out))
 }
 
 // Disconnect the SSH client.
-func (s *GitUploadPackService) Disconnect() (err error) {
+func (s *GitUploadPackService) Disconnect() error {
 	if !s.connected {
 		return ErrNotConnected
 	}
@@ -142,7 +138,7 @@ func (s *GitUploadPackService) Disconnect() (err error) {
 // SSH session on a connected GitUploadPackService, sends the given
 // upload request to the server and returns a reader for the received
 // packfile.  Closing the returned reader will close the SSH session.
-func (s *GitUploadPackService) Fetch(req *common.GitUploadPackRequest) (rc io.ReadCloser, err error) {
+func (s *GitUploadPackService) Fetch(req *common.GitUploadPackRequest) (io.ReadCloser, error) {
 	if !s.connected {
 		return nil, ErrNotConnected
 	}
@@ -243,7 +239,7 @@ func sendHaves(w io.Writer, req *common.GitUploadPackRequest) error {
 	e := pktline.NewEncoder(w)
 	for _, have := range req.Haves {
 		if err := e.Encodef("have %s\n", have); err != nil {
-			return fmt.Errorf("sending haves for %q: err ", have, err)
+			return fmt.Errorf("sending haves for %q: %s", have, err)
 		}
 	}