From c8b16f965c4ce8b8c7987f92bb3738cdf57a1a22 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Thu, 26 Oct 2017 13:47:08 -0600
Subject: [PATCH 01/15] lfsapi: move the cred helper init code to DoWithAuth()

---
 lfsapi/auth.go   | 9 ++++++++-
 lfsapi/lfsapi.go | 6 ------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/lfsapi/auth.go b/lfsapi/auth.go
index b64ff662..2488d5d2 100644
--- a/lfsapi/auth.go
+++ b/lfsapi/auth.go
@@ -23,7 +23,14 @@ var (
 func (c *Client) DoWithAuth(remote string, req *http.Request) (*http.Response, error) {
 	credHelper := c.Credentials
 	if credHelper == nil {
-		credHelper = defaultCredentialHelper
+		var err error
+		credHelper, err = getCredentialHelper(c.osEnv, c.gitEnv)
+		if err != nil {
+			tracerx.Printf("error getting credential helper: %s", err)
+		}
+		if credHelper == nil {
+			credHelper = defaultCredentialHelper
+		}
 	}
 
 	netrcFinder := c.Netrc
diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go
index 898391dd..74d786a6 100644
--- a/lfsapi/lfsapi.go
+++ b/lfsapi/lfsapi.go
@@ -71,11 +71,6 @@ func NewClient(ctx Context) (*Client, error) {
 		return nil, errors.Wrap(err, fmt.Sprintf("bad netrc file %s", netrcfile))
 	}
 
-	creds, err := getCredentialHelper(osEnv, gitEnv)
-	if err != nil {
-		return nil, errors.Wrap(err, "cannot find credential helper(s)")
-	}
-
 	var sshResolver SSHResolver = &sshAuthClient{os: osEnv}
 	if gitEnv.Bool("lfs.cachecredentials", true) {
 		sshResolver = withSSHCache(sshResolver)
@@ -83,7 +78,6 @@ func NewClient(ctx Context) (*Client, error) {
 
 	c := &Client{
 		Endpoints:           NewEndpointFinder(ctx),
-		Credentials:         creds,
 		SSH:                 sshResolver,
 		Netrc:               netrc,
 		DialTimeout:         gitEnv.Int("lfs.dialtimeout", 0),

From 28ff509488c18bad51f407e5db506ec1cd213e01 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Thu, 26 Oct 2017 16:09:37 -0600
Subject: [PATCH 02/15] lfsapi: return null cred helper if cred helper isn't
 used for a req

---
 lfsapi/auth.go      |  82 ++++++++++++++----------------------
 lfsapi/auth_test.go |  10 +++--
 lfsapi/creds.go     | 100 +++++++++++++++++++++-----------------------
 3 files changed, 85 insertions(+), 107 deletions(-)

diff --git a/lfsapi/auth.go b/lfsapi/auth.go
index 2488d5d2..d86b2a69 100644
--- a/lfsapi/auth.go
+++ b/lfsapi/auth.go
@@ -21,29 +21,7 @@ var (
 )
 
 func (c *Client) DoWithAuth(remote string, req *http.Request) (*http.Response, error) {
-	credHelper := c.Credentials
-	if credHelper == nil {
-		var err error
-		credHelper, err = getCredentialHelper(c.osEnv, c.gitEnv)
-		if err != nil {
-			tracerx.Printf("error getting credential helper: %s", err)
-		}
-		if credHelper == nil {
-			credHelper = defaultCredentialHelper
-		}
-	}
-
-	netrcFinder := c.Netrc
-	if netrcFinder == nil {
-		netrcFinder = defaultNetrcFinder
-	}
-
-	ef := c.Endpoints
-	if ef == nil {
-		ef = defaultEndpointFinder
-	}
-
-	apiEndpoint, access, creds, credsURL, err := getCreds(credHelper, netrcFinder, ef, remote, req)
+	apiEndpoint, access, credHelper, credsURL, creds, err := c.getCreds(remote, req)
 	if err != nil {
 		return nil, err
 	}
@@ -101,32 +79,48 @@ func (c *Client) doWithCreds(req *http.Request, credHelper CredentialHelper, cre
 // 3. The Git Remote URL, which should be something like "https://git.com/repo.git"
 //    This URL is used for the Git Credential Helper. This way existing https
 //    Git remote credentials can be re-used for LFS.
-func getCreds(credHelper CredentialHelper, netrcFinder NetrcFinder, ef EndpointFinder, remote string, req *http.Request) (Endpoint, Access, Creds, *url.URL, error) {
+func (c *Client) getCreds(remote string, req *http.Request) (Endpoint, Access, CredentialHelper, *url.URL, Creds, error) {
+	ef := c.Endpoints
+	if ef == nil {
+		ef = defaultEndpointFinder
+	}
+
+	netrcFinder := c.Netrc
+	if netrcFinder == nil {
+		netrcFinder = defaultNetrcFinder
+	}
+
 	operation := getReqOperation(req)
 	apiEndpoint := ef.Endpoint(operation, remote)
 	access := ef.AccessFor(apiEndpoint.Url)
 
 	if access != NTLMAccess {
 		if requestHasAuth(req) || setAuthFromNetrc(netrcFinder, req) || access == NoneAccess {
-			return apiEndpoint, access, nil, nil, nil
+			return apiEndpoint, access, nullCreds, nil, nil, nil
 		}
 
 		credsURL, err := getCredURLForAPI(ef, operation, remote, apiEndpoint, req)
 		if err != nil {
-			return apiEndpoint, access, nil, nil, errors.Wrap(err, "creds")
+			return apiEndpoint, access, nullCreds, nil, nil, errors.Wrap(err, "creds")
 		}
 
 		if credsURL == nil {
-			return apiEndpoint, access, nil, nil, nil
+			return apiEndpoint, access, nullCreds, nil, nil, nil
 		}
 
-		creds, err := fillGitCreds(credHelper, ef, req, credsURL)
-		return apiEndpoint, access, creds, credsURL, err
+		credHelper, creds, err := c.getGitCreds(ef, req, credsURL)
+		if err == nil {
+			tracerx.Printf("Filled credentials for %s", credsURL)
+			setRequestAuth(req, creds["username"], creds["password"])
+		}
+		return apiEndpoint, access, credHelper, credsURL, creds, err
 	}
 
+	// NTLM ONLY
+
 	credsURL, err := url.Parse(apiEndpoint.Url)
 	if err != nil {
-		return apiEndpoint, access, nil, nil, errors.Wrap(err, "creds")
+		return apiEndpoint, access, nullCreds, nil, nil, errors.Wrap(err, "creds")
 	}
 
 	if netrcMachine := getAuthFromNetrc(netrcFinder, req); netrcMachine != nil {
@@ -138,20 +132,16 @@ func getCreds(credHelper CredentialHelper, netrcFinder NetrcFinder, ef EndpointF
 			"source":   "netrc",
 		}
 
-		return apiEndpoint, access, creds, credsURL, nil
+		return apiEndpoint, access, nullCreds, credsURL, creds, nil
 	}
 
-	creds, err := getGitCreds(credHelper, ef, req, credsURL)
-	return apiEndpoint, access, creds, credsURL, err
+	// NTLM uses creds to create the session
+	credHelper, creds, err := c.getGitCreds(ef, req, credsURL)
+	return apiEndpoint, access, credHelper, credsURL, creds, err
 }
 
-func getGitCreds(credHelper CredentialHelper, ef EndpointFinder, req *http.Request, u *url.URL) (Creds, error) {
-	path := strings.TrimPrefix(u.Path, "/")
-	input := Creds{"protocol": u.Scheme, "host": u.Host, "path": path}
-	if u.User != nil && u.User.Username() != "" {
-		input["username"] = u.User.Username()
-	}
-
+func (c *Client) getGitCreds(ef EndpointFinder, req *http.Request, u *url.URL) (CredentialHelper, Creds, error) {
+	credHelper, input := c.getCredentialHelper(u)
 	creds, err := credHelper.Fill(input)
 	if creds == nil || len(creds) < 1 {
 		errmsg := fmt.Sprintf("Git credentials for %s not found", u)
@@ -163,17 +153,7 @@ func getGitCreds(credHelper CredentialHelper, ef EndpointFinder, req *http.Reque
 		err = errors.New(errmsg)
 	}
 
-	return creds, err
-}
-
-func fillGitCreds(credHelper CredentialHelper, ef EndpointFinder, req *http.Request, u *url.URL) (Creds, error) {
-	creds, err := getGitCreds(credHelper, ef, req, u)
-	if err == nil {
-		tracerx.Printf("Filled credentials for %s", u)
-		setRequestAuth(req, creds["username"], creds["password"])
-	}
-
-	return creds, err
+	return credHelper, creds, err
 }
 
 func getAuthFromNetrc(netrcFinder NetrcFinder, req *http.Request) *netrc.Machine {
diff --git a/lfsapi/auth_test.go b/lfsapi/auth_test.go
index daedcdc1..e55e45e0 100644
--- a/lfsapi/auth_test.go
+++ b/lfsapi/auth_test.go
@@ -517,8 +517,10 @@ func TestGetCreds(t *testing.T) {
 		},
 	}
 
-	credHelper := &fakeCredentialFiller{}
-	netrcFinder := &fakeNetrc{}
+	client := &Client{
+		Credentials: &fakeCredentialFiller{},
+		Netrc:       &fakeNetrc{},
+	}
 	for desc, test := range tests {
 		t.Log(desc)
 		req, err := http.NewRequest(test.Method, test.Href, nil)
@@ -531,8 +533,8 @@ func TestGetCreds(t *testing.T) {
 			req.Header.Set(key, value)
 		}
 
-		ef := NewEndpointFinder(NewContext(nil, nil, test.Config))
-		endpoint, access, creds, credsURL, err := getCreds(credHelper, netrcFinder, ef, test.Remote, req)
+		client.Endpoints = NewEndpointFinder(NewContext(nil, nil, test.Config))
+		endpoint, access, _, credsURL, creds, err := client.getCreds(test.Remote, req)
 		if !assert.Nil(t, err) {
 			continue
 		}
diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index 187043b1..745e5af7 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -8,86 +8,61 @@ import (
 	"strings"
 	"sync"
 
-	"github.com/git-lfs/git-lfs/config"
 	"github.com/git-lfs/git-lfs/errors"
 	"github.com/rubyist/tracerx"
 )
 
-// credsConfig supplies configuration options pertaining to the authorization
-// process in package lfsapi.
-type credsConfig struct {
-	// AskPass is a string containing an executable name as well as a
-	// program arguments.
-	//
-	// See: https://git-scm.com/docs/gitcredentials#_requesting_credentials
-	// for more.
-	AskPass string `os:"GIT_ASKPASS" git:"core.askpass" os:"SSH_ASKPASS"`
-	// Helper is a string defining the credential helper that Git should use.
-	Helper string `git:"credential.helper"`
-	// Cached is a boolean determining whether or not to enable the
-	// credential cacher.
-	Cached bool
-	// SkipPrompt is a boolean determining whether or not to prompt the user
-	// for a password.
-	SkipPrompt bool `os:"GIT_TERMINAL_PROMPT"`
-}
-
 // getCredentialHelper parses a 'credsConfig' from the git and OS environments,
 // returning the appropriate CredentialHelper to authenticate requests with.
 //
 // It returns an error if any configuration was invalid, or otherwise
 // un-useable.
-func getCredentialHelper(osEnv, gitEnv config.Environment) (CredentialHelper, error) {
-	ccfg, err := getCredentialConfig(osEnv, gitEnv)
-	if err != nil {
-		return nil, err
+func (c *Client) getCredentialHelper(u *url.URL) (CredentialHelper, Creds) {
+	path := strings.TrimPrefix(u.Path, "/")
+	input := Creds{"protocol": u.Scheme, "host": u.Host, "path": path}
+	if u.User != nil && u.User.Username() != "" {
+		input["username"] = u.User.Username()
 	}
 
+	if c.Credentials != nil {
+		return c.Credentials, input
+	}
+
+	askpass, ok := c.osEnv.Get("GIT_ASKPASS")
+	if !ok {
+		askpass, ok = c.gitEnv.Get("core.askpass")
+	}
+	if !ok {
+		askpass, ok = c.osEnv.Get("SSH_ASKPASS")
+	}
+	helper, _ := c.gitEnv.Get("credential.helper")
+	cached := c.gitEnv.Bool("lfs.cachecredentials", true)
+	skipPrompt := c.osEnv.Bool("GIT_TERMINAL_PROMPT", false)
+
 	var hs []CredentialHelper
-	if len(ccfg.Helper) == 0 && len(ccfg.AskPass) > 0 {
+	if len(helper) == 0 && len(askpass) > 0 {
 		hs = append(hs, &AskPassCredentialHelper{
-			Program: ccfg.AskPass,
+			Program: askpass,
 		})
 	}
 
 	var h CredentialHelper
 	h = &commandCredentialHelper{
-		SkipPrompt: ccfg.SkipPrompt,
+		SkipPrompt: skipPrompt,
 	}
 
-	if ccfg.Cached {
+	if cached {
 		h = withCredentialCache(h)
 	}
 	hs = append(hs, h)
 
 	switch len(hs) {
 	case 0:
-		return nil, nil
+		return defaultCredentialHelper, input
 	case 1:
-		return hs[0], nil
+		return hs[0], input
 	}
-	return CredentialHelpers(hs), nil
-}
-
-// getCredentialConfig parses a *credsConfig given the OS and Git
-// configurations.
-func getCredentialConfig(o, g config.Environment) (*credsConfig, error) {
-	askpass, ok := o.Get("GIT_ASKPASS")
-	if !ok {
-		askpass, ok = g.Get("core.askpass")
-	}
-	if !ok {
-		askpass, ok = o.Get("SSH_ASKPASS")
-	}
-	helper, _ := g.Get("credential.helper")
-	what := &credsConfig{
-		AskPass:    askpass,
-		Helper:     helper,
-		Cached:     g.Bool("lfs.cachecredentials", true),
-		SkipPrompt: o.Bool("GIT_TERMINAL_PROMPT", false),
-	}
-
-	return what, nil
+	return CredentialHelpers(hs), input
 }
 
 // CredentialHelpers is a []CredentialHelper that iterates through each
@@ -332,6 +307,8 @@ func (h *commandCredentialHelper) Reject(creds Creds) error {
 }
 
 func (h *commandCredentialHelper) Approve(creds Creds) error {
+	tracerx.Printf("creds: git credential approve (%q, %q, %q)",
+		creds["protocol"], creds["host"], creds["path"])
 	_, err := h.exec("approve", creds)
 	return err
 }
@@ -383,3 +360,22 @@ func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, e
 
 	return creds, nil
 }
+
+type nullCredentialHelper struct{}
+
+var (
+	nullCredError = errors.New("No credential helper configured")
+	nullCreds     = &nullCredentialHelper{}
+)
+
+func (h *nullCredentialHelper) Fill(input Creds) (Creds, error) {
+	return nil, nullCredError
+}
+
+func (h *nullCredentialHelper) Approve(creds Creds) error {
+	return nil
+}
+
+func (h *nullCredentialHelper) Reject(creds Creds) error {
+	return nil
+}

From b1a43072543f15e30b094539ac2795c299f20e95 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Thu, 26 Oct 2017 16:29:43 -0600
Subject: [PATCH 03/15] lfsapi: only send 'path' to cred helpers if
 credential.useHttpPath is on

---
 lfsapi/auth_test.go | 41 ++++++++++++++++++++++++++++-------------
 lfsapi/creds.go     |  6 ++++--
 lfsapi/ntlm_test.go |  1 -
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/lfsapi/auth_test.go b/lfsapi/auth_test.go
index e55e45e0..349b4563 100644
--- a/lfsapi/auth_test.go
+++ b/lfsapi/auth_test.go
@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/git-lfs/git-lfs/errors"
+	"github.com/git-lfs/git-lfs/git"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -93,7 +94,6 @@ func TestDoWithAuthApprove(t *testing.T) {
 	assert.True(t, creds.IsApproved(Creds(map[string]string{
 		"username": "user",
 		"password": "pass",
-		"path":     "repo/lfs",
 		"protocol": "http",
 		"host":     srv.Listener.Addr().String(),
 	})))
@@ -264,6 +264,28 @@ func TestGetCreds(t *testing.T) {
 				"lfs.url": "https://git-server.com/repo/lfs",
 				"lfs.https://git-server.com/repo/lfs.access": "basic",
 			},
+			Expected: getCredsExpected{
+				Access:        BasicAccess,
+				Endpoint:      "https://git-server.com/repo/lfs",
+				Authorization: basicAuth("git-server.com", "monkey"),
+				CredsURL:      "https://git-server.com/repo/lfs",
+				Creds: map[string]string{
+					"protocol": "https",
+					"host":     "git-server.com",
+					"username": "git-server.com",
+					"password": "monkey",
+				},
+			},
+		},
+		"basic access with usehttppath": getCredsTest{
+			Remote: "origin",
+			Method: "GET",
+			Href:   "https://git-server.com/repo/lfs/locks",
+			Config: map[string]string{
+				"lfs.url": "https://git-server.com/repo/lfs",
+				"lfs.https://git-server.com/repo/lfs.access": "basic",
+				"credential.usehttppath":                     "true",
+			},
 			Expected: getCredsExpected{
 				Access:        BasicAccess,
 				Endpoint:      "https://git-server.com/repo/lfs",
@@ -295,7 +317,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "git-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs",
 				},
 			},
 		},
@@ -369,7 +390,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "user",
 					"password": "monkey",
-					"path":     "repo/lfs",
 				},
 			},
 		},
@@ -392,7 +412,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "git-server.com",
 					"password": "monkey",
-					"path":     "repo",
 				},
 			},
 		},
@@ -443,7 +462,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com",
 					"username": "git-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs/locks",
 				},
 			},
 		},
@@ -465,7 +483,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "lfs-server.com",
 					"username": "lfs-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs/locks",
 				},
 			},
 		},
@@ -487,7 +504,6 @@ func TestGetCreds(t *testing.T) {
 					"host":     "git-server.com:8080",
 					"username": "git-server.com:8080",
 					"password": "monkey",
-					"path":     "repo/lfs/locks",
 				},
 			},
 		},
@@ -509,7 +525,6 @@ func TestGetCreds(t *testing.T) {
 				Creds: map[string]string{
 					"host":     "git-server.com",
 					"password": "monkey",
-					"path":     "repo/lfs",
 					"protocol": "https",
 					"username": "git-server.com",
 				},
@@ -517,10 +532,6 @@ func TestGetCreds(t *testing.T) {
 		},
 	}
 
-	client := &Client{
-		Credentials: &fakeCredentialFiller{},
-		Netrc:       &fakeNetrc{},
-	}
 	for desc, test := range tests {
 		t.Log(desc)
 		req, err := http.NewRequest(test.Method, test.Href, nil)
@@ -533,7 +544,11 @@ func TestGetCreds(t *testing.T) {
 			req.Header.Set(key, value)
 		}
 
-		client.Endpoints = NewEndpointFinder(NewContext(nil, nil, test.Config))
+		ctx := NewContext(git.NewConfig("", ""), nil, test.Config)
+		client, _ := NewClient(ctx)
+		client.Credentials = &fakeCredentialFiller{}
+		client.Netrc = &fakeNetrc{}
+		client.Endpoints = NewEndpointFinder(ctx)
 		endpoint, access, _, credsURL, creds, err := client.getCreds(test.Remote, req)
 		if !assert.Nil(t, err) {
 			continue
diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index 745e5af7..86f1d5c8 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -18,11 +18,13 @@ import (
 // It returns an error if any configuration was invalid, or otherwise
 // un-useable.
 func (c *Client) getCredentialHelper(u *url.URL) (CredentialHelper, Creds) {
-	path := strings.TrimPrefix(u.Path, "/")
-	input := Creds{"protocol": u.Scheme, "host": u.Host, "path": path}
+	input := Creds{"protocol": u.Scheme, "host": u.Host}
 	if u.User != nil && u.User.Username() != "" {
 		input["username"] = u.User.Username()
 	}
+	if c.gitEnv.Bool("credential.usehttppath", false) {
+		input["path"] = strings.TrimPrefix(u.Path, "/")
+	}
 
 	if c.Credentials != nil {
 		return c.Credentials, input
diff --git a/lfsapi/ntlm_test.go b/lfsapi/ntlm_test.go
index fa776173..ab9eba94 100644
--- a/lfsapi/ntlm_test.go
+++ b/lfsapi/ntlm_test.go
@@ -90,7 +90,6 @@ func TestNTLMAuth(t *testing.T) {
 	creds := Creds{
 		"protocol": srvURL.Scheme,
 		"host":     srvURL.Host,
-		"path":     "ntlm",
 		"username": "ntlmdomain\\ntlmuser",
 		"password": "ntlmpass",
 	}

From 58d06f043775a56b93191340ef14b6e66d2db32c Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Thu, 26 Oct 2017 18:17:45 -0600
Subject: [PATCH 04/15] lfsapi: introduce new credential cache and
 CredentialHelperSet

---
 lfsapi/creds.go      | 154 ++++++++++++++++++++++++++
 lfsapi/creds_test.go | 258 +++++++++++++++++++++++++++++++++++++++++++
 lfsapi/lfsapi.go     |   1 -
 3 files changed, 412 insertions(+), 1 deletion(-)

diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index 86f1d5c8..eaf79186 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -363,6 +363,160 @@ func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, e
 	return creds, nil
 }
 
+type credentialCacher2 struct {
+	creds map[string]Creds
+	mu    sync.Mutex
+}
+
+func newCredentialCacher() *credentialCacher2 {
+	return &credentialCacher2{creds: make(map[string]Creds)}
+}
+
+func (c credentialCacher2) Fill(what Creds) (Creds, error) {
+	key := credCacheKey(what)
+	c.mu.Lock()
+	cached, ok := c.creds[key]
+	c.mu.Unlock()
+
+	if ok {
+		return cached, nil
+	}
+
+	return nil, credHelperNoOp
+}
+
+func (c credentialCacher2) Approve(what Creds) error {
+	key := credCacheKey(what)
+
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	if _, ok := c.creds[key]; ok {
+		return nil
+	}
+
+	c.creds[key] = what
+	return credHelperNoOp
+}
+
+func (c credentialCacher2) Reject(what Creds) error {
+	key := credCacheKey(what)
+	c.mu.Lock()
+	delete(c.creds, key)
+	c.mu.Unlock()
+	return credHelperNoOp
+}
+
+// CredentialHelperSet is a []CredentialHelper that iterates through each
+// credential helper to fill, reject, or approve credentials.
+type CredentialHelperSet struct {
+	helpers        []CredentialHelper
+	skippedHelpers map[int]bool
+	mu             sync.Mutex
+}
+
+func NewCredentialHelpers(helpers []CredentialHelper) CredentialHelper {
+	return &CredentialHelperSet{
+		helpers:        helpers,
+		skippedHelpers: make(map[int]bool),
+	}
+}
+
+var credHelperNoOp = errors.New("no-op!")
+
+// Fill implements CredentialHelper.Fill by asking each CredentialHelper in
+// order to fill the credentials.
+//
+// If a fill was successful, it is returned immediately, and no other
+// `CredentialHelper`s are consulted. If any CredentialHelper returns an error,
+// it is returned immediately.
+func (s CredentialHelperSet) Fill(what Creds) (Creds, error) {
+	errs := make([]string, 0, len(s.helpers))
+	for i, h := range s.helpers {
+		if s.skipped(i) {
+			continue
+		}
+
+		creds, err := h.Fill(what)
+		if err != nil {
+			if err != credHelperNoOp {
+				s.skip(i)
+				tracerx.Printf("credential fill error: %s", err)
+				errs = append(errs, err.Error())
+			}
+			continue
+		}
+
+		if creds != nil {
+			return creds, nil
+		}
+	}
+
+	if len(errs) > 0 {
+		return nil, errors.New("credential fill errors:\n" + strings.Join(errs, "\n"))
+	}
+
+	return nil, nil
+}
+
+// Reject implements CredentialHelper.Reject and rejects the given Creds "what"
+// amongst all knonw CredentialHelpers. If any `CredentialHelper`s returned a
+// non-nil error, no further `CredentialHelper`s are notified, so as to prevent
+// inconsistent state.
+func (s CredentialHelperSet) Reject(what Creds) error {
+	for i, h := range s.helpers {
+		if s.skipped(i) {
+			continue
+		}
+
+		if err := h.Reject(what); err != credHelperNoOp {
+			return err
+		}
+	}
+
+	return errors.New("no valid credential helpers to reject")
+}
+
+// Approve implements CredentialHelper.Approve and approves the given Creds
+// "what" amongst all known CredentialHelpers. If any `CredentialHelper`s
+// returned a non-nil error, no further `CredentialHelper`s are notified, so as
+// to prevent inconsistent state.
+func (s CredentialHelperSet) Approve(what Creds) error {
+	skipped := make(map[int]bool)
+	for i, h := range s.helpers {
+		if s.skipped(i) {
+			skipped[i] = true
+			continue
+		}
+
+		if err := h.Approve(what); err != credHelperNoOp {
+			if err != nil && i > 0 { // clear any cached approvals
+				for j := 0; j < i; j++ {
+					if !skipped[j] {
+						s.helpers[j].Reject(what)
+					}
+				}
+			}
+			return err
+		}
+	}
+
+	return errors.New("no valid credential helpers to approve")
+}
+
+func (s CredentialHelperSet) skip(i int) {
+	s.mu.Lock()
+	s.skippedHelpers[i] = true
+	s.mu.Unlock()
+}
+
+func (s CredentialHelperSet) skipped(i int) bool {
+	s.mu.Lock()
+	skipped := s.skippedHelpers[i]
+	s.mu.Unlock()
+	return skipped
+}
+
 type nullCredentialHelper struct{}
 
 var (
diff --git a/lfsapi/creds_test.go b/lfsapi/creds_test.go
index b0bc01ce..a74cbac2 100644
--- a/lfsapi/creds_test.go
+++ b/lfsapi/creds_test.go
@@ -8,6 +8,264 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+type testCredHelper struct {
+	fillErr    error
+	approveErr error
+	rejectErr  error
+	fill       []Creds
+	approve    []Creds
+	reject     []Creds
+}
+
+func newTestCredHelper() *testCredHelper {
+	return &testCredHelper{
+		fill:    make([]Creds, 0),
+		approve: make([]Creds, 0),
+		reject:  make([]Creds, 0),
+	}
+}
+
+func (h *testCredHelper) Fill(input Creds) (Creds, error) {
+	h.fill = append(h.fill, input)
+	return input, h.fillErr
+}
+
+func (h *testCredHelper) Approve(creds Creds) error {
+	h.approve = append(h.approve, creds)
+	return h.approveErr
+}
+
+func (h *testCredHelper) Reject(creds Creds) error {
+	h.reject = append(h.reject, creds)
+	return h.rejectErr
+}
+
+func TestCredHelperSetNoErrors(t *testing.T) {
+	cache := newCredentialCacher()
+	helper1 := newTestCredHelper()
+	helper2 := newTestCredHelper()
+	helpers := NewCredentialHelpers([]CredentialHelper{cache, helper1, helper2})
+	creds := Creds{"protocol": "https", "host": "example.com"}
+
+	out, err := helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+
+	// calling Fill() with empty cache
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 2, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+
+	credsWithPass := Creds{"protocol": "https", "host": "example.com", "username": "foo", "password": "bar"}
+	assert.Nil(t, helpers.Approve(credsWithPass))
+	assert.Equal(t, 1, len(helper1.approve))
+	assert.Equal(t, 0, len(helper2.approve))
+
+	// calling Approve() again is cached
+	assert.Nil(t, helpers.Approve(credsWithPass))
+	assert.Equal(t, 1, len(helper1.approve))
+	assert.Equal(t, 0, len(helper2.approve))
+
+	// access cache
+	for i := 0; i < 3; i++ {
+		out, err = helpers.Fill(creds)
+		assert.Nil(t, err)
+		assert.Equal(t, credsWithPass, out)
+		assert.Equal(t, 2, len(helper1.fill))
+		assert.Equal(t, 0, len(helper2.fill))
+	}
+
+	assert.Nil(t, helpers.Reject(creds))
+	assert.Equal(t, 1, len(helper1.reject))
+	assert.Equal(t, 0, len(helper2.reject))
+
+	// Reject() is never cached
+	assert.Nil(t, helpers.Reject(creds))
+	assert.Equal(t, 2, len(helper1.reject))
+	assert.Equal(t, 0, len(helper2.reject))
+
+	// calling Fill() with empty cache
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 3, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+}
+
+func TestCredHelperSetFillError(t *testing.T) {
+	cache := newCredentialCacher()
+	helper1 := newTestCredHelper()
+	helper2 := newTestCredHelper()
+	helpers := NewCredentialHelpers([]CredentialHelper{cache, helper1, helper2})
+	creds := Creds{"protocol": "https", "host": "example.com"}
+
+	helper1.fillErr = errors.New("boom")
+	out, err := helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 1, len(helper2.fill))
+
+	assert.Nil(t, helpers.Approve(creds))
+	assert.Equal(t, 0, len(helper1.approve))
+	assert.Equal(t, 1, len(helper2.approve))
+
+	// Fill() with cache
+	for i := 0; i < 3; i++ {
+		out, err = helpers.Fill(creds)
+		assert.Nil(t, err)
+		assert.Equal(t, creds, out)
+		assert.Equal(t, 1, len(helper1.fill))
+		assert.Equal(t, 1, len(helper2.fill))
+	}
+
+	assert.Nil(t, helpers.Reject(creds))
+	assert.Equal(t, 0, len(helper1.reject))
+	assert.Equal(t, 1, len(helper2.reject))
+
+	// Fill() with empty cache
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill)) // still skipped
+	assert.Equal(t, 2, len(helper2.fill))
+}
+
+func TestCredHelperSetApproveError(t *testing.T) {
+	cache := newCredentialCacher()
+	helper1 := newTestCredHelper()
+	helper2 := newTestCredHelper()
+	helpers := NewCredentialHelpers([]CredentialHelper{cache, helper1, helper2})
+	creds := Creds{"protocol": "https", "host": "example.com"}
+
+	approveErr := errors.New("boom")
+	helper1.approveErr = approveErr
+	out, err := helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+
+	assert.Equal(t, approveErr, helpers.Approve(creds))
+	assert.Equal(t, 1, len(helper1.approve))
+	assert.Equal(t, 0, len(helper2.approve))
+
+	// cache is never set
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 2, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+
+	assert.Nil(t, helpers.Reject(creds))
+	assert.Equal(t, 1, len(helper1.reject))
+	assert.Equal(t, 0, len(helper2.reject))
+}
+
+func TestCredHelperSetFillAndApproveError(t *testing.T) {
+	cache := newCredentialCacher()
+	helper1 := newTestCredHelper()
+	helper2 := newTestCredHelper()
+	helpers := NewCredentialHelpers([]CredentialHelper{cache, helper1, helper2})
+	creds := Creds{"protocol": "https", "host": "example.com"}
+
+	credErr := errors.New("boom")
+	helper1.fillErr = credErr
+	helper2.approveErr = credErr
+
+	out, err := helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 1, len(helper2.fill))
+
+	assert.Equal(t, credErr, helpers.Approve(creds))
+	assert.Equal(t, 0, len(helper1.approve)) // skipped
+	assert.Equal(t, 0, len(helper1.reject))  // skipped
+	assert.Equal(t, 1, len(helper2.approve))
+
+	// never approved, so cache is empty
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill)) // still skipped
+	assert.Equal(t, 2, len(helper2.fill))
+}
+
+func TestCredHelperSetRejectError(t *testing.T) {
+	cache := newCredentialCacher()
+	helper1 := newTestCredHelper()
+	helper2 := newTestCredHelper()
+	helpers := NewCredentialHelpers([]CredentialHelper{cache, helper1, helper2})
+	creds := Creds{"protocol": "https", "host": "example.com"}
+
+	rejectErr := errors.New("boom")
+	helper1.rejectErr = rejectErr
+	out, err := helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+
+	assert.Nil(t, helpers.Approve(creds))
+	assert.Equal(t, 1, len(helper1.approve))
+	assert.Equal(t, 0, len(helper2.approve))
+
+	// Fill() with cache
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+
+	assert.Equal(t, rejectErr, helpers.Reject(creds))
+	assert.Equal(t, 1, len(helper1.reject))
+	assert.Equal(t, 0, len(helper2.reject))
+
+	// failed Reject() still clears cache
+	out, err = helpers.Fill(creds)
+	assert.Nil(t, err)
+	assert.Equal(t, creds, out)
+	assert.Equal(t, 2, len(helper1.fill))
+	assert.Equal(t, 0, len(helper2.fill))
+}
+
+func TestCredHelperSetAllFillErrors(t *testing.T) {
+	cache := newCredentialCacher()
+	helper1 := newTestCredHelper()
+	helper2 := newTestCredHelper()
+	helpers := NewCredentialHelpers([]CredentialHelper{cache, helper1, helper2})
+	creds := Creds{"protocol": "https", "host": "example.com"}
+
+	helper1.fillErr = errors.New("boom 1")
+	helper2.fillErr = errors.New("boom 2")
+	out, err := helpers.Fill(creds)
+	if assert.NotNil(t, err) {
+		assert.Equal(t, "credential fill errors:\nboom 1\nboom 2", err.Error())
+	}
+	assert.Nil(t, out)
+	assert.Equal(t, 1, len(helper1.fill))
+	assert.Equal(t, 1, len(helper2.fill))
+
+	err = helpers.Approve(creds)
+	if assert.NotNil(t, err) {
+		assert.Equal(t, "no valid credential helpers to approve", err.Error())
+	}
+	assert.Equal(t, 0, len(helper1.approve))
+	assert.Equal(t, 0, len(helper2.approve))
+
+	err = helpers.Reject(creds)
+	if assert.NotNil(t, err) {
+		assert.Equal(t, "no valid credential helpers to reject", err.Error())
+	}
+	assert.Equal(t, 0, len(helper1.reject))
+	assert.Equal(t, 0, len(helper2.reject))
+}
+
 // test that cache satisfies Fill() without looking at creds
 func TestCredsCacheFillFromCache(t *testing.T) {
 	creds := newFakeCreds()
diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go
index 74d786a6..426809b5 100644
--- a/lfsapi/lfsapi.go
+++ b/lfsapi/lfsapi.go
@@ -47,7 +47,6 @@ type Client struct {
 
 	LoggingStats bool // DEPRECATED
 
-	// only used for per-host ssl certs
 	gitEnv config.Environment
 	osEnv  config.Environment
 	uc     *config.URLConfig

From f060f0dc16a16418d54541d1b0296c3bb8f644e7 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Thu, 26 Oct 2017 18:34:33 -0600
Subject: [PATCH 05/15] lfsapi: replace old credential cache and
 CredentialHelpers types

---
 lfsapi/creds.go      | 232 +++++++++-----------------------------
 lfsapi/creds_test.go | 260 -------------------------------------------
 lfsapi/lfsapi.go     |  38 +++++--
 3 files changed, 83 insertions(+), 447 deletions(-)

diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index eaf79186..ec817c81 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -12,6 +12,27 @@ import (
 	"github.com/rubyist/tracerx"
 )
 
+type CredentialHelper interface {
+	Fill(Creds) (Creds, error)
+	Reject(Creds) error
+	Approve(Creds) error
+}
+
+type Creds map[string]string
+
+func bufferCreds(c Creds) *bytes.Buffer {
+	buf := new(bytes.Buffer)
+
+	for k, v := range c {
+		buf.Write([]byte(k))
+		buf.Write([]byte("="))
+		buf.Write([]byte(v))
+		buf.Write([]byte("\n"))
+	}
+
+	return buf
+}
+
 // getCredentialHelper parses a 'credsConfig' from the git and OS environments,
 // returning the appropriate CredentialHelper to authenticate requests with.
 //
@@ -30,94 +51,18 @@ func (c *Client) getCredentialHelper(u *url.URL) (CredentialHelper, Creds) {
 		return c.Credentials, input
 	}
 
-	askpass, ok := c.osEnv.Get("GIT_ASKPASS")
-	if !ok {
-		askpass, ok = c.gitEnv.Get("core.askpass")
+	helpers := make([]CredentialHelper, 0, 3)
+	if c.cachingCredHelper != nil {
+		helpers = append(helpers, c.cachingCredHelper)
 	}
-	if !ok {
-		askpass, ok = c.osEnv.Get("SSH_ASKPASS")
-	}
-	helper, _ := c.gitEnv.Get("credential.helper")
-	cached := c.gitEnv.Bool("lfs.cachecredentials", true)
-	skipPrompt := c.osEnv.Bool("GIT_TERMINAL_PROMPT", false)
-
-	var hs []CredentialHelper
-	if len(helper) == 0 && len(askpass) > 0 {
-		hs = append(hs, &AskPassCredentialHelper{
-			Program: askpass,
-		})
-	}
-
-	var h CredentialHelper
-	h = &commandCredentialHelper{
-		SkipPrompt: skipPrompt,
-	}
-
-	if cached {
-		h = withCredentialCache(h)
-	}
-	hs = append(hs, h)
-
-	switch len(hs) {
-	case 0:
-		return defaultCredentialHelper, input
-	case 1:
-		return hs[0], input
-	}
-	return CredentialHelpers(hs), input
-}
-
-// CredentialHelpers is a []CredentialHelper that iterates through each
-// credential helper to fill, reject, or approve credentials.
-type CredentialHelpers []CredentialHelper
-
-// Fill implements CredentialHelper.Fill by asking each CredentialHelper in
-// order to fill the credentials.
-//
-// If a fill was successful, it is returned immediately, and no other
-// `CredentialHelper`s are consulted. If any CredentialHelper returns an error,
-// it is returned immediately.
-func (h CredentialHelpers) Fill(what Creds) (Creds, error) {
-	for _, c := range h {
-		creds, err := c.Fill(what)
-		if err != nil {
-			return nil, err
-		}
-
-		if creds != nil {
-			return creds, nil
+	if c.askpassCredHelper != nil {
+		helper, _ := c.gitEnv.Get("credential.helper")
+		if len(helper) == 0 {
+			helpers = append(helpers, c.askpassCredHelper)
 		}
 	}
 
-	return nil, nil
-}
-
-// Reject implements CredentialHelper.Reject and rejects the given Creds "what"
-// amongst all knonw CredentialHelpers. If any `CredentialHelper`s returned a
-// non-nil error, no further `CredentialHelper`s are notified, so as to prevent
-// inconsistent state.
-func (h CredentialHelpers) Reject(what Creds) error {
-	for _, c := range h {
-		if err := c.Reject(what); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-// Approve implements CredentialHelper.Approve and approves the given Creds
-// "what" amongst all known CredentialHelpers. If any `CredentialHelper`s
-// returned a non-nil error, no further `CredentialHelper`s are notified, so as
-// to prevent inconsistent state.
-func (h CredentialHelpers) Approve(what Creds) error {
-	for _, c := range h {
-		if err := c.Approve(what); err != nil {
-			return err
-		}
-	}
-
-	return nil
+	return NewCredentialHelpers(append(helpers, c.commandCredHelper)), input
 }
 
 // AskPassCredentialHelper implements the CredentialHelper type for GIT_ASKPASS
@@ -211,88 +156,6 @@ func (a *AskPassCredentialHelper) args(prompt string) []string {
 	return []string{prompt}
 }
 
-type CredentialHelper interface {
-	Fill(Creds) (Creds, error)
-	Reject(Creds) error
-	Approve(Creds) error
-}
-
-type Creds map[string]string
-
-func bufferCreds(c Creds) *bytes.Buffer {
-	buf := new(bytes.Buffer)
-
-	for k, v := range c {
-		buf.Write([]byte(k))
-		buf.Write([]byte("="))
-		buf.Write([]byte(v))
-		buf.Write([]byte("\n"))
-	}
-
-	return buf
-}
-
-func withCredentialCache(helper CredentialHelper) CredentialHelper {
-	return &credentialCacher{
-		cmu:    new(sync.Mutex),
-		creds:  make(map[string]Creds),
-		helper: helper,
-	}
-}
-
-type credentialCacher struct {
-	// cmu guards creds
-	cmu    *sync.Mutex
-	creds  map[string]Creds
-	helper CredentialHelper
-}
-
-func credCacheKey(creds Creds) string {
-	parts := []string{
-		creds["protocol"],
-		creds["host"],
-		creds["path"],
-	}
-	return strings.Join(parts, "//")
-}
-
-func (c *credentialCacher) Fill(creds Creds) (Creds, error) {
-	key := credCacheKey(creds)
-
-	c.cmu.Lock()
-	defer c.cmu.Unlock()
-
-	if cache, ok := c.creds[key]; ok {
-		tracerx.Printf("creds: git credential cache (%q, %q, %q)",
-			creds["protocol"], creds["host"], creds["path"])
-		return cache, nil
-	}
-
-	creds, err := c.helper.Fill(creds)
-	if err == nil && len(creds["username"]) > 0 && len(creds["password"]) > 0 {
-		c.creds[key] = creds
-	}
-	return creds, err
-}
-
-func (c *credentialCacher) Reject(creds Creds) error {
-	c.cmu.Lock()
-	defer c.cmu.Unlock()
-
-	delete(c.creds, credCacheKey(creds))
-	return c.helper.Reject(creds)
-}
-
-func (c *credentialCacher) Approve(creds Creds) error {
-	err := c.helper.Approve(creds)
-	if err == nil {
-		c.cmu.Lock()
-		c.creds[credCacheKey(creds)] = creds
-		c.cmu.Unlock()
-	}
-	return err
-}
-
 type commandCredentialHelper struct {
 	SkipPrompt bool
 }
@@ -363,16 +226,25 @@ func (h *commandCredentialHelper) exec(subcommand string, input Creds) (Creds, e
 	return creds, nil
 }
 
-type credentialCacher2 struct {
+type credentialCacher struct {
 	creds map[string]Creds
 	mu    sync.Mutex
 }
 
-func newCredentialCacher() *credentialCacher2 {
-	return &credentialCacher2{creds: make(map[string]Creds)}
+func newCredentialCacher() *credentialCacher {
+	return &credentialCacher{creds: make(map[string]Creds)}
 }
 
-func (c credentialCacher2) Fill(what Creds) (Creds, error) {
+func credCacheKey(creds Creds) string {
+	parts := []string{
+		creds["protocol"],
+		creds["host"],
+		creds["path"],
+	}
+	return strings.Join(parts, "//")
+}
+
+func (c credentialCacher) Fill(what Creds) (Creds, error) {
 	key := credCacheKey(what)
 	c.mu.Lock()
 	cached, ok := c.creds[key]
@@ -385,7 +257,7 @@ func (c credentialCacher2) Fill(what Creds) (Creds, error) {
 	return nil, credHelperNoOp
 }
 
-func (c credentialCacher2) Approve(what Creds) error {
+func (c credentialCacher) Approve(what Creds) error {
 	key := credCacheKey(what)
 
 	c.mu.Lock()
@@ -399,7 +271,7 @@ func (c credentialCacher2) Approve(what Creds) error {
 	return credHelperNoOp
 }
 
-func (c credentialCacher2) Reject(what Creds) error {
+func (c credentialCacher) Reject(what Creds) error {
 	key := credCacheKey(what)
 	c.mu.Lock()
 	delete(c.creds, key)
@@ -407,16 +279,16 @@ func (c credentialCacher2) Reject(what Creds) error {
 	return credHelperNoOp
 }
 
-// CredentialHelperSet is a []CredentialHelper that iterates through each
+// CredentialHelpers is a []CredentialHelper that iterates through each
 // credential helper to fill, reject, or approve credentials.
-type CredentialHelperSet struct {
+type CredentialHelpers struct {
 	helpers        []CredentialHelper
 	skippedHelpers map[int]bool
 	mu             sync.Mutex
 }
 
 func NewCredentialHelpers(helpers []CredentialHelper) CredentialHelper {
-	return &CredentialHelperSet{
+	return &CredentialHelpers{
 		helpers:        helpers,
 		skippedHelpers: make(map[int]bool),
 	}
@@ -430,7 +302,7 @@ var credHelperNoOp = errors.New("no-op!")
 // If a fill was successful, it is returned immediately, and no other
 // `CredentialHelper`s are consulted. If any CredentialHelper returns an error,
 // it is returned immediately.
-func (s CredentialHelperSet) Fill(what Creds) (Creds, error) {
+func (s CredentialHelpers) Fill(what Creds) (Creds, error) {
 	errs := make([]string, 0, len(s.helpers))
 	for i, h := range s.helpers {
 		if s.skipped(i) {
@@ -463,7 +335,7 @@ func (s CredentialHelperSet) Fill(what Creds) (Creds, error) {
 // amongst all knonw CredentialHelpers. If any `CredentialHelper`s returned a
 // non-nil error, no further `CredentialHelper`s are notified, so as to prevent
 // inconsistent state.
-func (s CredentialHelperSet) Reject(what Creds) error {
+func (s CredentialHelpers) Reject(what Creds) error {
 	for i, h := range s.helpers {
 		if s.skipped(i) {
 			continue
@@ -481,7 +353,7 @@ func (s CredentialHelperSet) Reject(what Creds) error {
 // "what" amongst all known CredentialHelpers. If any `CredentialHelper`s
 // returned a non-nil error, no further `CredentialHelper`s are notified, so as
 // to prevent inconsistent state.
-func (s CredentialHelperSet) Approve(what Creds) error {
+func (s CredentialHelpers) Approve(what Creds) error {
 	skipped := make(map[int]bool)
 	for i, h := range s.helpers {
 		if s.skipped(i) {
@@ -504,13 +376,13 @@ func (s CredentialHelperSet) Approve(what Creds) error {
 	return errors.New("no valid credential helpers to approve")
 }
 
-func (s CredentialHelperSet) skip(i int) {
+func (s CredentialHelpers) skip(i int) {
 	s.mu.Lock()
 	s.skippedHelpers[i] = true
 	s.mu.Unlock()
 }
 
-func (s CredentialHelperSet) skipped(i int) bool {
+func (s CredentialHelpers) skipped(i int) bool {
 	s.mu.Lock()
 	skipped := s.skippedHelpers[i]
 	s.mu.Unlock()
diff --git a/lfsapi/creds_test.go b/lfsapi/creds_test.go
index a74cbac2..4e78e066 100644
--- a/lfsapi/creds_test.go
+++ b/lfsapi/creds_test.go
@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 type testCredHelper struct {
@@ -265,262 +264,3 @@ func TestCredHelperSetAllFillErrors(t *testing.T) {
 	assert.Equal(t, 0, len(helper1.reject))
 	assert.Equal(t, 0, len(helper2.reject))
 }
-
-// test that cache satisfies Fill() without looking at creds
-func TestCredsCacheFillFromCache(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(creds).(*credentialCacher)
-	cache.creds["http//lfs.test//foo/bar"] = Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	}
-
-	filled, err := cache.Fill(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-	})
-	assert.Nil(t, err)
-	require.NotNil(t, filled)
-	assert.Equal(t, "u", filled["username"])
-	assert.Equal(t, "p", filled["password"])
-
-	assert.Equal(t, 1, len(cache.creds))
-	cached, ok := cache.creds["http//lfs.test//foo/bar"]
-	assert.True(t, ok)
-	assert.Equal(t, "u", cached["username"])
-	assert.Equal(t, "p", cached["password"])
-}
-
-// test that cache caches Fill() value from creds
-func TestCredsCacheFillFromValidHelperFill(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(creds).(*credentialCacher)
-
-	creds.list = append(creds.list, Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	})
-
-	assert.Equal(t, 0, len(cache.creds))
-
-	filled, err := cache.Fill(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-	})
-	assert.Nil(t, err)
-	require.NotNil(t, filled)
-	assert.Equal(t, "u", filled["username"])
-	assert.Equal(t, "p", filled["password"])
-
-	assert.Equal(t, 1, len(cache.creds))
-	cached, ok := cache.creds["http//lfs.test//foo/bar"]
-	assert.True(t, ok)
-	assert.Equal(t, "u", cached["username"])
-	assert.Equal(t, "p", cached["password"])
-
-	creds.list = make([]Creds, 0)
-	filled2, err := cache.Fill(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-	})
-	assert.Nil(t, err)
-	require.NotNil(t, filled2)
-	assert.Equal(t, "u", filled2["username"])
-	assert.Equal(t, "p", filled2["password"])
-}
-
-// test that cache ignores Fill() value from creds with missing username+password
-func TestCredsCacheFillFromInvalidHelperFill(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(creds).(*credentialCacher)
-
-	creds.list = append(creds.list, Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "no-password",
-	})
-
-	assert.Equal(t, 0, len(cache.creds))
-
-	filled, err := cache.Fill(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	})
-	assert.Nil(t, err)
-	require.NotNil(t, filled)
-	assert.Equal(t, "no-password", filled["username"])
-	assert.Equal(t, "", filled["password"])
-
-	assert.Equal(t, 0, len(cache.creds))
-}
-
-// test that cache ignores Fill() value from creds with error
-func TestCredsCacheFillFromErroringHelperFill(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(&erroringCreds{creds}).(*credentialCacher)
-
-	creds.list = append(creds.list, Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	})
-
-	assert.Equal(t, 0, len(cache.creds))
-
-	filled, err := cache.Fill(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-	})
-	assert.NotNil(t, err)
-	require.NotNil(t, filled)
-	assert.Equal(t, "u", filled["username"])
-	assert.Equal(t, "p", filled["password"])
-
-	assert.Equal(t, 0, len(cache.creds))
-}
-
-func TestCredsCacheRejectWithoutError(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(creds).(*credentialCacher)
-
-	cache.creds["http//lfs.test//foo/bar"] = Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	}
-
-	err := cache.Reject(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-	})
-	assert.Nil(t, err)
-	assert.Equal(t, 0, len(cache.creds))
-}
-
-func TestCredsCacheRejectWithError(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(&erroringCreds{creds}).(*credentialCacher)
-
-	cache.creds["http//lfs.test//foo/bar"] = Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	}
-
-	err := cache.Reject(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-	})
-	assert.NotNil(t, err)
-	assert.Equal(t, 0, len(cache.creds))
-}
-
-func TestCredsCacheApproveWithoutError(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(creds).(*credentialCacher)
-
-	assert.Equal(t, 0, len(cache.creds))
-
-	err := cache.Approve(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "U",
-		"password": "P",
-	})
-	assert.Nil(t, err)
-	assert.Equal(t, 1, len(cache.creds))
-	cached, ok := cache.creds["http//lfs.test//foo/bar"]
-	assert.True(t, ok)
-	assert.Equal(t, "U", cached["username"])
-	assert.Equal(t, "P", cached["password"])
-}
-
-func TestCredsCacheApproveWithError(t *testing.T) {
-	creds := newFakeCreds()
-	cache := withCredentialCache(&erroringCreds{creds}).(*credentialCacher)
-
-	assert.Equal(t, 0, len(cache.creds))
-
-	err := cache.Approve(Creds{
-		"protocol": "http",
-		"host":     "lfs.test",
-		"path":     "foo/bar",
-		"username": "u",
-		"password": "p",
-	})
-	assert.NotNil(t, err)
-	assert.Equal(t, 0, len(cache.creds))
-}
-
-func newFakeCreds() *fakeCreds {
-	return &fakeCreds{list: make([]Creds, 0)}
-}
-
-type erroringCreds struct {
-	helper CredentialHelper
-}
-
-func (e *erroringCreds) Fill(creds Creds) (Creds, error) {
-	c, _ := e.helper.Fill(creds)
-	return c, errors.New("fill error")
-}
-
-func (e *erroringCreds) Reject(creds Creds) error {
-	e.helper.Reject(creds)
-	return errors.New("reject error")
-}
-
-func (e *erroringCreds) Approve(creds Creds) error {
-	e.helper.Approve(creds)
-	return errors.New("approve error")
-}
-
-type fakeCreds struct {
-	list []Creds
-}
-
-func credsMatch(c1, c2 Creds) bool {
-	return c1["protocol"] == c2["protocol"] &&
-		c1["host"] == c2["host"] &&
-		c1["path"] == c2["path"]
-}
-
-func (f *fakeCreds) Fill(creds Creds) (Creds, error) {
-	for _, saved := range f.list {
-		if credsMatch(creds, saved) {
-			return saved, nil
-		}
-	}
-	return creds, nil
-}
-
-func (f *fakeCreds) Reject(creds Creds) error {
-	return nil
-}
-
-func (f *fakeCreds) Approve(creds Creds) error {
-	return nil
-}
diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go
index 426809b5..411a8203 100644
--- a/lfsapi/lfsapi.go
+++ b/lfsapi/lfsapi.go
@@ -47,9 +47,12 @@ type Client struct {
 
 	LoggingStats bool // DEPRECATED
 
-	gitEnv config.Environment
-	osEnv  config.Environment
-	uc     *config.URLConfig
+	commandCredHelper *commandCredentialHelper
+	askpassCredHelper *AskPassCredentialHelper
+	cachingCredHelper *credentialCacher
+	gitEnv            config.Environment
+	osEnv             config.Environment
+	uc                *config.URLConfig
 }
 
 type Context interface {
@@ -70,8 +73,9 @@ func NewClient(ctx Context) (*Client, error) {
 		return nil, errors.Wrap(err, fmt.Sprintf("bad netrc file %s", netrcfile))
 	}
 
+	cacheCreds := gitEnv.Bool("lfs.cachecredentials", true)
 	var sshResolver SSHResolver = &sshAuthClient{os: osEnv}
-	if gitEnv.Bool("lfs.cachecredentials", true) {
+	if cacheCreds {
 		sshResolver = withSSHCache(sshResolver)
 	}
 
@@ -86,9 +90,29 @@ func NewClient(ctx Context) (*Client, error) {
 		SkipSSLVerify:       !gitEnv.Bool("http.sslverify", true) || osEnv.Bool("GIT_SSL_NO_VERIFY", false),
 		Verbose:             osEnv.Bool("GIT_CURL_VERBOSE", false),
 		DebuggingVerbose:    osEnv.Bool("LFS_DEBUG_HTTP", false),
-		gitEnv:              gitEnv,
-		osEnv:               osEnv,
-		uc:                  config.NewURLConfig(gitEnv),
+		commandCredHelper: &commandCredentialHelper{
+			SkipPrompt: osEnv.Bool("GIT_TERMINAL_PROMPT", false),
+		},
+		gitEnv: gitEnv,
+		osEnv:  osEnv,
+		uc:     config.NewURLConfig(gitEnv),
+	}
+
+	askpass, ok := osEnv.Get("GIT_ASKPASS")
+	if !ok {
+		askpass, ok = gitEnv.Get("core.askpass")
+	}
+	if !ok {
+		askpass, _ = osEnv.Get("SSH_ASKPASS")
+	}
+	if len(askpass) > 0 {
+		c.askpassCredHelper = &AskPassCredentialHelper{
+			Program: askpass,
+		}
+	}
+
+	if cacheCreds {
+		c.cachingCredHelper = newCredentialCacher()
 	}
 
 	return c, nil

From 9b43b3aafd6fb9dd79979386597f71c0e90a409d Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Thu, 26 Oct 2017 18:36:08 -0600
Subject: [PATCH 06/15] lfsapi: add tracer message when pulling from credential
 cache

---
 lfsapi/creds.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index ec817c81..d955a2d2 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -251,6 +251,8 @@ func (c credentialCacher) Fill(what Creds) (Creds, error) {
 	c.mu.Unlock()
 
 	if ok {
+		tracerx.Printf("creds: git credential cache (%q, %q, %q)",
+			what["protocol"], what["host"], what["path"])
 		return cached, nil
 	}
 

From 68b1fded436f56904ddb4591c4e94666e19abb64 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 09:39:04 -0600
Subject: [PATCH 07/15] lfsapi: change method receivers to pointers (the types
 are structs, not maps)

---
 lfsapi/creds.go | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index d955a2d2..d54a8a89 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -244,7 +244,7 @@ func credCacheKey(creds Creds) string {
 	return strings.Join(parts, "//")
 }
 
-func (c credentialCacher) Fill(what Creds) (Creds, error) {
+func (c *credentialCacher) Fill(what Creds) (Creds, error) {
 	key := credCacheKey(what)
 	c.mu.Lock()
 	cached, ok := c.creds[key]
@@ -259,7 +259,7 @@ func (c credentialCacher) Fill(what Creds) (Creds, error) {
 	return nil, credHelperNoOp
 }
 
-func (c credentialCacher) Approve(what Creds) error {
+func (c *credentialCacher) Approve(what Creds) error {
 	key := credCacheKey(what)
 
 	c.mu.Lock()
@@ -273,7 +273,7 @@ func (c credentialCacher) Approve(what Creds) error {
 	return credHelperNoOp
 }
 
-func (c credentialCacher) Reject(what Creds) error {
+func (c *credentialCacher) Reject(what Creds) error {
 	key := credCacheKey(what)
 	c.mu.Lock()
 	delete(c.creds, key)
@@ -304,7 +304,7 @@ var credHelperNoOp = errors.New("no-op!")
 // If a fill was successful, it is returned immediately, and no other
 // `CredentialHelper`s are consulted. If any CredentialHelper returns an error,
 // it is returned immediately.
-func (s CredentialHelpers) Fill(what Creds) (Creds, error) {
+func (s *CredentialHelpers) Fill(what Creds) (Creds, error) {
 	errs := make([]string, 0, len(s.helpers))
 	for i, h := range s.helpers {
 		if s.skipped(i) {
@@ -337,7 +337,7 @@ func (s CredentialHelpers) Fill(what Creds) (Creds, error) {
 // amongst all knonw CredentialHelpers. If any `CredentialHelper`s returned a
 // non-nil error, no further `CredentialHelper`s are notified, so as to prevent
 // inconsistent state.
-func (s CredentialHelpers) Reject(what Creds) error {
+func (s *CredentialHelpers) Reject(what Creds) error {
 	for i, h := range s.helpers {
 		if s.skipped(i) {
 			continue
@@ -355,7 +355,7 @@ func (s CredentialHelpers) Reject(what Creds) error {
 // "what" amongst all known CredentialHelpers. If any `CredentialHelper`s
 // returned a non-nil error, no further `CredentialHelper`s are notified, so as
 // to prevent inconsistent state.
-func (s CredentialHelpers) Approve(what Creds) error {
+func (s *CredentialHelpers) Approve(what Creds) error {
 	skipped := make(map[int]bool)
 	for i, h := range s.helpers {
 		if s.skipped(i) {
@@ -378,13 +378,13 @@ func (s CredentialHelpers) Approve(what Creds) error {
 	return errors.New("no valid credential helpers to approve")
 }
 
-func (s CredentialHelpers) skip(i int) {
+func (s *CredentialHelpers) skip(i int) {
 	s.mu.Lock()
 	s.skippedHelpers[i] = true
 	s.mu.Unlock()
 }
 
-func (s CredentialHelpers) skipped(i int) bool {
+func (s *CredentialHelpers) skipped(i int) bool {
 	s.mu.Lock()
 	skipped := s.skippedHelpers[i]
 	s.mu.Unlock()

From 3fbf57ca2bfd120c42542320ecf6531be44ccecd Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 09:51:45 -0600
Subject: [PATCH 08/15] git: remove low value debug message

---
 git/filter_process_scanner.go | 2 --
 1 file changed, 2 deletions(-)

diff --git a/git/filter_process_scanner.go b/git/filter_process_scanner.go
index f56059a3..8af20c7c 100644
--- a/git/filter_process_scanner.go
+++ b/git/filter_process_scanner.go
@@ -172,8 +172,6 @@ func (o *FilterProcessScanner) Err() error { return o.err }
 // will read the body of the request. Since the body is _not_ offset, one
 // request should be read in its entirety before consuming the next request.
 func (o *FilterProcessScanner) readRequest() (*Request, error) {
-	tracerx.Printf("Read filter-process request.")
-
 	requestList, err := o.pl.readPacketList()
 	if err != nil {
 		return nil, err

From d340a739d97abdadc3182c8612ff1221f53c689c Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 09:57:08 -0600
Subject: [PATCH 09/15] test: test credential changes

---
 test/test-credentials-no-prompt.sh | 22 ++++++++++++++++++++
 test/test-credentials.sh           | 32 ++++++++++++++++++++----------
 test/test-expired.sh               |  1 -
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/test/test-credentials-no-prompt.sh b/test/test-credentials-no-prompt.sh
index 3e85248e..e5564d41 100755
--- a/test/test-credentials-no-prompt.sh
+++ b/test/test-credentials-no-prompt.sh
@@ -28,3 +28,25 @@ begin_test "attempt private access without credential helper"
     grep "Git credentials for $GITSERVER/$reponame not found" push.log
 )
 end_test
+
+begin_test "askpass: push with bad askpass"
+(
+  set -e
+
+  reponame="askpass-with-git-environ"
+  setup_remote_repo "$reponame"
+  clone_repo "$reponame" "$reponame"
+
+  git lfs track "*.dat"
+  echo "hello" > a.dat
+
+  git add .gitattributes a.dat
+  git commit -m "initial commit"
+
+  git config "credential.helper" ""
+  GIT_TERMINAL_PROMPT=0 GIT_ASKPASS="lfs-askpass-2" SSH_ASKPASS="dont-call-me" GIT_TRACE=1 git push origin master 2>&1 | tee push.log
+  grep "filling with GIT_ASKPASS" push.log                     # attempt askpass
+  grep 'credential fill error: exec: "lfs-askpass-2"' push.log # askpass fails
+  grep "creds: git credential fill" push.log                   # attempt git credential
+)
+end_test
diff --git a/test/test-credentials.sh b/test/test-credentials.sh
index dab525dd..3acbe79f 100755
--- a/test/test-credentials.sh
+++ b/test/test-credentials.sh
@@ -8,7 +8,7 @@ begin_test "credentials without useHttpPath, with bad path password"
 (
   set -e
 
-  reponame="$(basename "$0" ".sh")"
+  reponame="no-httppath-bad-password"
   setup_remote_repo "$reponame"
 
   printf "path:wrong" > "$CREDSDIR/127.0.0.1--$reponame"
@@ -20,16 +20,18 @@ begin_test "credentials without useHttpPath, with bad path password"
   git lfs track "*.dat" 2>&1 | tee track.log
   grep "Tracking \"\*.dat\"" track.log
 
-  contents="a"
-  contents_oid=$(calc_oid "$contents")
-
-  printf "$contents" > a.dat
+  printf "a" > a.dat
   git add a.dat
   git add .gitattributes
   git commit -m "add a.dat"
 
-  git push origin without-path 2>&1 | tee push.log
+  GIT_TRACE=1 git push origin without-path 2>&1 | tee push.log
   grep "(1 of 1 files)" push.log
+
+  echo "approvals:"
+  [ "1" -eq "$(cat push.log | grep "creds: git credential approve" | wc -l)" ]
+  echo "fills:"
+  [ "1" -eq "$(cat push.log | grep "creds: git credential fill" | wc -l)" ]
 )
 end_test
 
@@ -37,7 +39,7 @@ begin_test "credentials with useHttpPath, with wrong password"
 (
   set -e
 
-  reponame="$(basename "$0" ".sh")"
+  reponame="httppath-bad-password"
   setup_remote_repo "$reponame"
 
   printf "path:wrong" > "$CREDSDIR/127.0.0.1--$reponame"
@@ -56,8 +58,12 @@ begin_test "credentials with useHttpPath, with wrong password"
   git add .gitattributes
   git commit -m "add a.dat"
 
-  git push origin with-path-wrong-pass 2>&1 | tee push.log
+  GIT_TRACE=1 git push origin with-path-wrong-pass 2>&1 | tee push.log
   [ "0" = "$(grep -c "(1 of 1 files)" push.log)" ]
+  echo "approvals:"
+  [ "0" -eq "$(cat push.log | grep "creds: git credential approve" | wc -l)" ]
+  echo "fills:"
+  [ "2" -eq "$(cat push.log | grep "creds: git credential fill" | wc -l)" ]
 )
 end_test
 
@@ -86,8 +92,12 @@ begin_test "credentials with useHttpPath, with correct password"
   git add .gitattributes
   git commit -m "add b.dat"
 
-  git push origin with-path-correct-pass 2>&1 | tee push.log
+  GIT_TRACE=1 git push origin with-path-correct-pass 2>&1 | tee push.log
   grep "(1 of 1 files)" push.log
+  echo "approvals:"
+  [ "1" -eq "$(cat push.log | grep "creds: git credential approve" | wc -l)" ]
+  echo "fills:"
+  [ "1" -eq "$(cat push.log | grep "creds: git credential fill" | wc -l)" ]
 )
 end_test
 
@@ -175,8 +185,10 @@ begin_test "credentials from netrc"
   git add .gitattributes a.dat
   git commit -m "add a.dat"
 
-  git lfs push netrc master 2>&1 | tee push.log
+  GIT_TRACE=1 git lfs push netrc master 2>&1 | tee push.log
   grep "(1 of 1 files)" push.log
+  echo "any git credential calls:"
+  [ "0" -eq "$(cat push.log | grep "git credential" | wc -l)" ]
 )
 end_test
 
diff --git a/test/test-expired.sh b/test/test-expired.sh
index beba72f7..5639442c 100755
--- a/test/test-expired.sh
+++ b/test/test-expired.sh
@@ -47,7 +47,6 @@ for typ in "${expiration_types[@]}"; do
 
     sshurl="${GITSERVER/http:\/\//ssh://git@}/$reponame"
     git config lfs.url "$sshurl"
-    git config lfs.cachecredentials "true"
 
     contents="contents"
     contents_oid="$(calc_oid "$contents")"

From 5f3ad17bd4441d7c1b3700dc74ae9f2c7a289a57 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 10:28:48 -0600
Subject: [PATCH 10/15] test: fix bad-askpass test

---
 test/test-credentials-no-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/test-credentials-no-prompt.sh b/test/test-credentials-no-prompt.sh
index e5564d41..a9fd287c 100755
--- a/test/test-credentials-no-prompt.sh
+++ b/test/test-credentials-no-prompt.sh
@@ -33,7 +33,7 @@ begin_test "askpass: push with bad askpass"
 (
   set -e
 
-  reponame="askpass-with-git-environ"
+  reponame="askpass-with-bad-askpass"
   setup_remote_repo "$reponame"
   clone_repo "$reponame" "$reponame"
 

From d9e1ce2fe67597fc4094543fa512d64ae9e10987 Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 10:29:14 -0600
Subject: [PATCH 11/15] config: introduce static Bool() and Int() helpers

---
 config/environment.go | 50 +++++++++++++++++++++++++------------------
 config/url_config.go  |  5 +++++
 lfsapi/lfsapi.go      | 30 ++++----------------------
 3 files changed, 38 insertions(+), 47 deletions(-)

diff --git a/config/environment.go b/config/environment.go
index 172bcf92..cf955584 100644
--- a/config/environment.go
+++ b/config/environment.go
@@ -65,13 +65,39 @@ func (e *environment) GetAll(key string) []string {
 	return e.Fetcher.GetAll(key)
 }
 
-func (e *environment) Bool(key string, def bool) (val bool) {
+func (e *environment) Bool(key string, def bool) bool {
 	s, _ := e.Fetcher.Get(key)
-	if len(s) == 0 {
+	return Bool(s, def)
+}
+
+func (e *environment) Int(key string, def int) int {
+	s, _ := e.Fetcher.Get(key)
+	return Int(s, def)
+}
+
+func (e *environment) All() map[string][]string {
+	return e.Fetcher.All()
+}
+
+func Int(value string, def int) int {
+	if len(value) == 0 {
 		return def
 	}
 
-	switch strings.ToLower(s) {
+	i, err := strconv.Atoi(value)
+	if err != nil {
+		return def
+	}
+
+	return i
+}
+
+func Bool(value string, def bool) bool {
+	if len(value) == 0 {
+		return def
+	}
+
+	switch strings.ToLower(value) {
 	case "true", "1", "on", "yes", "t":
 		return true
 	case "false", "0", "off", "no", "f":
@@ -80,21 +106,3 @@ func (e *environment) Bool(key string, def bool) (val bool) {
 		return false
 	}
 }
-
-func (e *environment) Int(key string, def int) (val int) {
-	s, _ := e.Fetcher.Get(key)
-	if len(s) == 0 {
-		return def
-	}
-
-	i, err := strconv.Atoi(s)
-	if err != nil {
-		return def
-	}
-
-	return i
-}
-
-func (e *environment) All() map[string][]string {
-	return e.Fetcher.All()
-}
diff --git a/config/url_config.go b/config/url_config.go
index 835ac8cb..21663a79 100644
--- a/config/url_config.go
+++ b/config/url_config.go
@@ -50,6 +50,11 @@ func (c *URLConfig) GetAll(prefix, rawurl, key string) []string {
 	return c.git.GetAll(strings.Join([]string{prefix, key}, "."))
 }
 
+func (c *URLConfig) Bool(prefix, rawurl, key string, def bool) bool {
+	s, _ := c.Get(prefix, rawurl, key)
+	return Bool(s, def)
+}
+
 func (c *URLConfig) getAll(prefix, rawurl, key string) []string {
 	hosts, paths := c.hostsAndPaths(rawurl)
 
diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go
index 411a8203..db20dab8 100644
--- a/lfsapi/lfsapi.go
+++ b/lfsapi/lfsapi.go
@@ -6,8 +6,6 @@ import (
 	"io"
 	"net/http"
 	"regexp"
-	"strconv"
-	"strings"
 	"sync"
 
 	"github.com/ThomsonReutersEikon/go-ntlm/ntlm"
@@ -208,34 +206,14 @@ func (e testEnv) GetAll(key string) []string {
 	return make([]string, 0)
 }
 
-func (e testEnv) Int(key string, def int) (val int) {
+func (e testEnv) Int(key string, def int) int {
 	s, _ := e.Get(key)
-	if len(s) == 0 {
-		return def
-	}
-
-	i, err := strconv.Atoi(s)
-	if err != nil {
-		return def
-	}
-
-	return i
+	return config.Int(s, def)
 }
 
-func (e testEnv) Bool(key string, def bool) (val bool) {
+func (e testEnv) Bool(key string, def bool) bool {
 	s, _ := e.Get(key)
-	if len(s) == 0 {
-		return def
-	}
-
-	switch strings.ToLower(s) {
-	case "true", "1", "on", "yes", "t":
-		return true
-	case "false", "0", "off", "no", "f":
-		return false
-	default:
-		return false
-	}
+	return config.Bool(s, def)
 }
 
 func (e testEnv) All() map[string][]string {

From 1b50834d1029f53661c7bfb6ef36706d162b8e7c Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 10:29:34 -0600
Subject: [PATCH 12/15] lfsapi: look for credential.<url>.* values

---
 lfsapi/creds.go          |  5 +--
 test/test-credentials.sh | 67 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index d54a8a89..00c59470 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -39,11 +39,12 @@ func bufferCreds(c Creds) *bytes.Buffer {
 // It returns an error if any configuration was invalid, or otherwise
 // un-useable.
 func (c *Client) getCredentialHelper(u *url.URL) (CredentialHelper, Creds) {
+	rawurl := fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path)
 	input := Creds{"protocol": u.Scheme, "host": u.Host}
 	if u.User != nil && u.User.Username() != "" {
 		input["username"] = u.User.Username()
 	}
-	if c.gitEnv.Bool("credential.usehttppath", false) {
+	if c.uc.Bool("credential", rawurl, "usehttppath", false) {
 		input["path"] = strings.TrimPrefix(u.Path, "/")
 	}
 
@@ -56,7 +57,7 @@ func (c *Client) getCredentialHelper(u *url.URL) (CredentialHelper, Creds) {
 		helpers = append(helpers, c.cachingCredHelper)
 	}
 	if c.askpassCredHelper != nil {
-		helper, _ := c.gitEnv.Get("credential.helper")
+		helper, _ := c.uc.Get("credential", rawurl, "helper")
 		if len(helper) == 0 {
 			helpers = append(helpers, c.askpassCredHelper)
 		}
diff --git a/test/test-credentials.sh b/test/test-credentials.sh
index 3acbe79f..fcef870f 100755
--- a/test/test-credentials.sh
+++ b/test/test-credentials.sh
@@ -4,6 +4,31 @@
 
 ensure_git_version_isnt $VERSION_LOWER "2.3.0"
 
+begin_test "credentails with url-specific helper skips askpass"
+(
+  set -e
+
+  reponame="url-specific-helper"
+  setup_remote_repo "$reponame"
+
+  clone_repo "$reponame" "$reponame"
+  git config credential.useHttpPath false
+  git config credential.helper ""
+  git config credential.$GITSERVER.helper "lfstest"
+
+  git lfs track "*.dat"
+  echo "hello" > a.dat
+
+  git add .gitattributes a.dat
+  git commit -m "initial commit"
+
+  # askpass is skipped
+  GIT_ASKPASS="lfs-bad-cmd" GIT_TRACE=1 git push origin master 2>&1 | tee push.log
+
+  [ "0" -eq "$(grep "filling with GIT_ASKPASS" push.log | wc -l)" ]
+)
+end_test
+
 begin_test "credentials without useHttpPath, with bad path password"
 (
   set -e
@@ -32,6 +57,43 @@ begin_test "credentials without useHttpPath, with bad path password"
   [ "1" -eq "$(cat push.log | grep "creds: git credential approve" | wc -l)" ]
   echo "fills:"
   [ "1" -eq "$(cat push.log | grep "creds: git credential fill" | wc -l)" ]
+
+  echo "credential calls have no path:"
+  credcalls="$(grep "creds: git credential" push.log)"
+  [ "0" -eq "$(echo "$credcalls" | grep "no-httppath-bad-password" | wc -l)" ]
+  expected="$(echo "$credcalls" | wc -l)"
+  [ "$expected" -eq "$(printf "$credcalls" | grep '", "")' | wc -l)" ]
+)
+end_test
+
+begin_test "credentials with url-specific useHttpPath, with bad path password"
+(
+  set -e
+
+  reponame="url-specific-httppath-bad-password"
+  setup_remote_repo "$reponame"
+
+  printf "path:wrong" > "$CREDSDIR/127.0.0.1--$reponame"
+
+  clone_repo "$reponame" with-url-specific-path
+  git config credential.$GITSERVER.useHttpPath false
+  git checkout -b without-path
+
+  git lfs track "*.dat" 2>&1 | tee track.log
+  grep "Tracking \"\*.dat\"" track.log
+
+  printf "a" > a.dat
+  git add a.dat
+  git add .gitattributes
+  git commit -m "add a.dat"
+
+  GIT_TRACE=1 git push origin without-path 2>&1 | tee push.log
+  grep "(1 of 1 files)" push.log
+
+  echo "approvals:"
+  [ "1" -eq "$(cat push.log | grep "creds: git credential approve" | wc -l)" ]
+  echo "fills:"
+  [ "1" -eq "$(cat push.log | grep "creds: git credential fill" | wc -l)" ]
 )
 end_test
 
@@ -98,6 +160,11 @@ begin_test "credentials with useHttpPath, with correct password"
   [ "1" -eq "$(cat push.log | grep "creds: git credential approve" | wc -l)" ]
   echo "fills:"
   [ "1" -eq "$(cat push.log | grep "creds: git credential fill" | wc -l)" ]
+  echo "credential calls have path:"
+  credcalls="$(grep "creds: git credential" push.log)"
+  [ "0" -eq "$(echo "$credcalls" | grep '", "")' | wc -l)" ]
+  expected="$(echo "$credcalls" | wc -l)"
+  [ "$expected" -eq "$(printf "$credcalls" | grep "test-credentials" | wc -l)" ]
 )
 end_test
 

From efc9e8820eac7c7ee0a918a3d5b2611711b412bc Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 10:42:17 -0600
Subject: [PATCH 13/15] lfsapi: add test case for credential.URL.usehttppath

---
 lfsapi/auth_test.go | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lfsapi/auth_test.go b/lfsapi/auth_test.go
index 349b4563..2a922115 100644
--- a/lfsapi/auth_test.go
+++ b/lfsapi/auth_test.go
@@ -300,6 +300,29 @@ func TestGetCreds(t *testing.T) {
 				},
 			},
 		},
+		"basic access with url-specific usehttppath": getCredsTest{
+			Remote: "origin",
+			Method: "GET",
+			Href:   "https://git-server.com/repo/lfs/locks",
+			Config: map[string]string{
+				"lfs.url": "https://git-server.com/repo/lfs",
+				"lfs.https://git-server.com/repo/lfs.access":    "basic",
+				"credential.https://git-server.com.usehttppath": "true",
+			},
+			Expected: getCredsExpected{
+				Access:        BasicAccess,
+				Endpoint:      "https://git-server.com/repo/lfs",
+				Authorization: basicAuth("git-server.com", "monkey"),
+				CredsURL:      "https://git-server.com/repo/lfs",
+				Creds: map[string]string{
+					"protocol": "https",
+					"host":     "git-server.com",
+					"username": "git-server.com",
+					"password": "monkey",
+					"path":     "repo/lfs",
+				},
+			},
+		},
 		"ntlm": getCredsTest{
 			Remote: "origin",
 			Method: "GET",

From c18cd21affe46fc24394343e6f45be3e9c0a55fe Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 14:10:46 -0600
Subject: [PATCH 14/15] add some godocs

---
 config/environment.go | 19 +++++++++++++++++++
 lfsapi/auth.go        |  3 +++
 lfsapi/client.go      |  2 ++
 lfsapi/creds.go       | 32 ++++++++++++++++++++++++--------
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/config/environment.go b/config/environment.go
index cf955584..73b8e513 100644
--- a/config/environment.go
+++ b/config/environment.go
@@ -79,6 +79,15 @@ func (e *environment) All() map[string][]string {
 	return e.Fetcher.All()
 }
 
+// Int returns the int value associated with the given value, or the value
+// "def", if the value is blank.
+//
+// To convert from a the string value attached to a given key,
+// `strconv.Atoi(val)` is called. If `Atoi` returned a non-nil error,
+// then the value "def" will be returned instead.
+//
+// Otherwise, if the value was converted `string -> int` successfully,
+// then it will be returned wholesale.
 func Int(value string, def int) int {
 	if len(value) == 0 {
 		return def
@@ -92,6 +101,16 @@ func Int(value string, def int) int {
 	return i
 }
 
+// Bool returns the boolean state associated with the given value, or the
+// value "def", if the value is blank.
+//
+// The "boolean state associated with a given key" is defined as the
+// case-insensitive string comparison with the following:
+//
+// 1) true if...
+//   "true", "1", "on", "yes", or "t"
+// 2) false if...
+//   "false", "0", "off", "no", "f", or otherwise.
 func Bool(value string, def bool) bool {
 	if len(value) == 0 {
 		return def
diff --git a/lfsapi/auth.go b/lfsapi/auth.go
index d86b2a69..92c9aec6 100644
--- a/lfsapi/auth.go
+++ b/lfsapi/auth.go
@@ -20,6 +20,9 @@ var (
 	defaultEndpointFinder   = NewEndpointFinder(nil)
 )
 
+// DoWithAuth sends an HTTP request to get an HTTP response. It attempts to add
+// authentication from netrc or git's credential helpers if necessary,
+// supporting basic and ntlm authentication.
 func (c *Client) DoWithAuth(remote string, req *http.Request) (*http.Response, error) {
 	apiEndpoint, access, credHelper, credsURL, creds, err := c.getCreds(remote, req)
 	if err != nil {
diff --git a/lfsapi/client.go b/lfsapi/client.go
index 00968f92..4d9ffa16 100644
--- a/lfsapi/client.go
+++ b/lfsapi/client.go
@@ -79,6 +79,8 @@ func joinURL(prefix, suffix string) string {
 	return prefix + slash + suffix
 }
 
+// Do sends an HTTP request to get an HTTP response. It wraps net/http, adding
+// extra headers, redirection handling, and error reporting.
 func (c *Client) Do(req *http.Request) (*http.Response, error) {
 	req.Header = c.extraHeadersFor(req)
 	req.Header.Set("User-Agent", UserAgent)
diff --git a/lfsapi/creds.go b/lfsapi/creds.go
index 00c59470..4c113b49 100644
--- a/lfsapi/creds.go
+++ b/lfsapi/creds.go
@@ -12,12 +12,17 @@ import (
 	"github.com/rubyist/tracerx"
 )
 
+// CredentialHelper is an interface used by the lfsapi Client to interact with
+// the 'git credential' command: https://git-scm.com/docs/gitcredentials
+// Other implementations include ASKPASS support, and an in-memory cache.
 type CredentialHelper interface {
 	Fill(Creds) (Creds, error)
 	Reject(Creds) error
 	Approve(Creds) error
 }
 
+// Creds represents a set of key/value pairs that are passed to 'git credential'
+// as input.
 type Creds map[string]string
 
 func bufferCreds(c Creds) *bytes.Buffer {
@@ -282,14 +287,23 @@ func (c *credentialCacher) Reject(what Creds) error {
 	return credHelperNoOp
 }
 
+// CredentialHelpers iterates through a slice of CredentialHelper objects
 // CredentialHelpers is a []CredentialHelper that iterates through each
-// credential helper to fill, reject, or approve credentials.
+// credential helper to fill, reject, or approve credentials. Typically, the
+// first success returns immediately. Errors are reported to tracerx, unless
+// all credential helpers return errors. Any erroring credential helpers are
+// skipped for future calls.
+//
+// A CredentialHelper can return a credHelperNoOp error, signaling that the
+// CredentialHelpers should try the next one.
 type CredentialHelpers struct {
 	helpers        []CredentialHelper
 	skippedHelpers map[int]bool
 	mu             sync.Mutex
 }
 
+// NewCredentialHelpers initializes a new CredentialHelpers from the given
+// slice of CredentialHelper instances.
 func NewCredentialHelpers(helpers []CredentialHelper) CredentialHelper {
 	return &CredentialHelpers{
 		helpers:        helpers,
@@ -304,7 +318,10 @@ var credHelperNoOp = errors.New("no-op!")
 //
 // If a fill was successful, it is returned immediately, and no other
 // `CredentialHelper`s are consulted. If any CredentialHelper returns an error,
-// it is returned immediately.
+// it is reported to tracerx, and the next one is attempted. If they all error,
+// then a collection of all the error messages is returned. Erroring credential
+// helpers are added to the skip list, and never attempted again for the
+// lifetime of the current Git LFS command.
 func (s *CredentialHelpers) Fill(what Creds) (Creds, error) {
 	errs := make([]string, 0, len(s.helpers))
 	for i, h := range s.helpers {
@@ -335,9 +352,7 @@ func (s *CredentialHelpers) Fill(what Creds) (Creds, error) {
 }
 
 // Reject implements CredentialHelper.Reject and rejects the given Creds "what"
-// amongst all knonw CredentialHelpers. If any `CredentialHelper`s returned a
-// non-nil error, no further `CredentialHelper`s are notified, so as to prevent
-// inconsistent state.
+// with the first successful attempt.
 func (s *CredentialHelpers) Reject(what Creds) error {
 	for i, h := range s.helpers {
 		if s.skipped(i) {
@@ -353,9 +368,10 @@ func (s *CredentialHelpers) Reject(what Creds) error {
 }
 
 // Approve implements CredentialHelper.Approve and approves the given Creds
-// "what" amongst all known CredentialHelpers. If any `CredentialHelper`s
-// returned a non-nil error, no further `CredentialHelper`s are notified, so as
-// to prevent inconsistent state.
+// "what" with the first successful CredentialHelper. If an error occurrs,
+// it calls Reject() with the same Creds and returns the error immediately. This
+// ensures a caching credential helper removes the cache, since the Erroring
+// CredentialHelper never successfully saved it.
 func (s *CredentialHelpers) Approve(what Creds) error {
 	skipped := make(map[int]bool)
 	for i, h := range s.helpers {

From d039e3791aca6100a0e0944cc7f46d1f4d0f047b Mon Sep 17 00:00:00 2001
From: rick olson <technoweenie@gmail.com>
Date: Fri, 27 Oct 2017 14:26:02 -0600
Subject: [PATCH 15/15] lfs: PointerScanner is nil after error, so don't close

---
 lfs/gitscanner_catfilebatch.go | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lfs/gitscanner_catfilebatch.go b/lfs/gitscanner_catfilebatch.go
index fa785df3..4b14c38e 100644
--- a/lfs/gitscanner_catfilebatch.go
+++ b/lfs/gitscanner_catfilebatch.go
@@ -19,8 +19,6 @@ import (
 func runCatFileBatch(pointerCh chan *WrappedPointer, lockableCh chan string, lockableSet *lockableNameSet, revs *StringChannelWrapper, errCh chan error) error {
 	scanner, err := NewPointerScanner()
 	if err != nil {
-		scanner.Close()
-
 		return err
 	}