From c8b16f965c4ce8b8c7987f92bb3738cdf57a1a22 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 26 Oct 2017 13:47:08 -0600 Subject: [PATCH 01/71] 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 Date: Thu, 26 Oct 2017 16:09:37 -0600 Subject: [PATCH 02/71] 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 Date: Thu, 26 Oct 2017 16:29:43 -0600 Subject: [PATCH 03/71] 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 Date: Thu, 26 Oct 2017 18:17:45 -0600 Subject: [PATCH 04/71] 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 Date: Thu, 26 Oct 2017 18:34:33 -0600 Subject: [PATCH 05/71] 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 Date: Thu, 26 Oct 2017 18:36:08 -0600 Subject: [PATCH 06/71] 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 Date: Fri, 27 Oct 2017 09:39:04 -0600 Subject: [PATCH 07/71] 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 Date: Fri, 27 Oct 2017 09:51:45 -0600 Subject: [PATCH 08/71] 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 Date: Fri, 27 Oct 2017 09:57:08 -0600 Subject: [PATCH 09/71] 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 Date: Fri, 27 Oct 2017 10:28:48 -0600 Subject: [PATCH 10/71] 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 Date: Fri, 27 Oct 2017 10:29:14 -0600 Subject: [PATCH 11/71] 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 Date: Fri, 27 Oct 2017 10:29:34 -0600 Subject: [PATCH 12/71] lfsapi: look for credential..* 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 Date: Fri, 27 Oct 2017 10:42:17 -0600 Subject: [PATCH 13/71] 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 Date: Fri, 27 Oct 2017 14:10:46 -0600 Subject: [PATCH 14/71] 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 19c7787617aca18c387b26c75cd11ddf64865b7b Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 15:00:07 -0600 Subject: [PATCH 15/71] config: make (*Configuration) CurrentRemote private, add Remote() and SetRemote() --- commands/command_clone.go | 2 +- commands/command_env.go | 2 +- commands/command_fetch.go | 16 ++++++++-------- commands/command_filter_process.go | 4 ++-- commands/command_lock.go | 2 +- commands/command_locks.go | 2 +- commands/command_post_checkout.go | 2 +- commands/command_post_commit.go | 2 +- commands/command_post_merge.go | 2 +- commands/command_pull.go | 2 +- commands/command_track.go | 2 +- commands/command_unlock.go | 2 +- commands/uploader.go | 2 +- config/config.go | 14 +++++++++++--- lfs/lfs.go | 4 ++-- 15 files changed, 34 insertions(+), 26 deletions(-) diff --git a/commands/command_clone.go b/commands/command_clone.go index 80536261..a301f23c 100644 --- a/commands/command_clone.go +++ b/commands/command_clone.go @@ -86,7 +86,7 @@ func cloneCommand(cmd *cobra.Command, args []string) { filter := buildFilepathFilter(cfg, includeArg, excludeArg) if cloneFlags.NoCheckout || cloneFlags.Bare { // If --no-checkout or --bare then we shouldn't check out, just fetch instead - cfg.CurrentRemote = remote + cfg.SetRemote(remote) fetchRef(ref.Name, filter) } else { pull(remote, filter) diff --git a/commands/command_env.go b/commands/command_env.go index cc1090ec..e0818009 100644 --- a/commands/command_env.go +++ b/commands/command_env.go @@ -9,7 +9,7 @@ import ( func envCommand(cmd *cobra.Command, args []string) { config.ShowConfigWarnings = true - endpoint := getAPIClient().Endpoints.Endpoint("download", cfg.CurrentRemote) + endpoint := getAPIClient().Endpoints.Endpoint("download", cfg.Remote()) gitV, err := git.Version() if err != nil { diff --git a/commands/command_fetch.go b/commands/command_fetch.go index 1026f42e..94d19583 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -42,9 +42,9 @@ func fetchCommand(cmd *cobra.Command, args []string) { if err := git.ValidateRemote(args[0]); err != nil { Exit("Invalid remote name %q", args[0]) } - cfg.CurrentRemote = args[0] + cfg.SetRemote(args[0]) } else { - cfg.CurrentRemote = "" + cfg.SetRemote("") } if len(args) > 1 { @@ -104,7 +104,7 @@ func fetchCommand(cmd *cobra.Command, args []string) { if !success { c := getAPIClient() - e := c.Endpoints.Endpoint("download", cfg.CurrentRemote) + e := c.Endpoints.Endpoint("download", cfg.Remote()) Exit("error: failed to fetch some objects from '%s'", e.Url) } } @@ -184,7 +184,7 @@ func fetchRecent(fetchconf lfs.FetchPruneConfig, alreadyFetchedRefs []*git.Ref, if fetchconf.FetchRecentRefsDays > 0 { Print("Fetching recent branches within %v days", fetchconf.FetchRecentRefsDays) refsSince := time.Now().AddDate(0, 0, -fetchconf.FetchRecentRefsDays) - refs, err := git.RecentBranches(refsSince, fetchconf.FetchRecentRefsIncludeRemotes, cfg.CurrentRemote) + refs, err := git.RecentBranches(refsSince, fetchconf.FetchRecentRefsIncludeRemotes, cfg.Remote()) if err != nil { Panic(err, "Could not scan for recent refs") } @@ -269,19 +269,19 @@ func scanAll() []*lfs.WrappedPointer { // Returns true if all completed with no errors, false if errors were written to stderr/log func fetchAndReportToChan(allpointers []*lfs.WrappedPointer, filter *filepathfilter.Filter, out chan<- *lfs.WrappedPointer) bool { // Lazily initialize the current remote. - if len(cfg.CurrentRemote) == 0 { + if len(cfg.Remote()) == 0 { // Actively find the default remote, don't just assume origin defaultRemote, err := cfg.GitConfig().DefaultRemote() if err != nil { Exit("No default remote") } - cfg.CurrentRemote = defaultRemote + cfg.SetRemote(defaultRemote) } ready, pointers, meter := readyAndMissingPointers(allpointers, filter) q := newDownloadQueue( - getTransferManifestOperationRemote("download", cfg.CurrentRemote), - cfg.CurrentRemote, tq.WithProgress(meter), + getTransferManifestOperationRemote("download", cfg.Remote()), + cfg.Remote(), tq.WithProgress(meter), ) if out != nil { diff --git a/commands/command_filter_process.go b/commands/command_filter_process.go index 56554fa2..16373bf9 100644 --- a/commands/command_filter_process.go +++ b/commands/command_filter_process.go @@ -69,8 +69,8 @@ func filterCommand(cmd *cobra.Command, args []string) { if supportsDelay { q = tq.NewTransferQueue( tq.Download, - getTransferManifestOperationRemote("download", cfg.CurrentRemote), - cfg.CurrentRemote, + getTransferManifestOperationRemote("download", cfg.Remote()), + cfg.Remote(), ) go infiniteTransferBuffer(q, available) } diff --git a/commands/command_lock.go b/commands/command_lock.go index 51a2d619..5326d4ac 100644 --- a/commands/command_lock.go +++ b/commands/command_lock.go @@ -90,7 +90,7 @@ func lockPath(file string) (string, error) { func init() { RegisterCommand("lock", lockCommand, func(cmd *cobra.Command) { - cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.CurrentRemote, lockRemoteHelp) + cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.Remote(), lockRemoteHelp) cmd.Flags().BoolVarP(&locksCmdFlags.JSON, "json", "", false, "print output in json") }) } diff --git a/commands/command_locks.go b/commands/command_locks.go index ed49d143..0544b9a1 100644 --- a/commands/command_locks.go +++ b/commands/command_locks.go @@ -109,7 +109,7 @@ func (l *locksFlags) Filters() (map[string]string, error) { func init() { RegisterCommand("locks", locksCommand, func(cmd *cobra.Command) { - cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.CurrentRemote, lockRemoteHelp) + cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.Remote(), lockRemoteHelp) cmd.Flags().StringVarP(&locksCmdFlags.Path, "path", "p", "", "filter locks results matching a particular path") cmd.Flags().StringVarP(&locksCmdFlags.Id, "id", "i", "", "filter locks results matching a particular ID") cmd.Flags().IntVarP(&locksCmdFlags.Limit, "limit", "l", 0, "optional limit for number of results to return") diff --git a/commands/command_post_checkout.go b/commands/command_post_checkout.go index 4fd772fd..40617817 100644 --- a/commands/command_post_checkout.go +++ b/commands/command_post_checkout.go @@ -32,7 +32,7 @@ func postCheckoutCommand(cmd *cobra.Command, args []string) { requireGitVersion() - lockClient := newLockClient(cfg.CurrentRemote) + lockClient := newLockClient(cfg.Remote()) // Skip this hook if no lockable patterns have been configured if len(lockClient.GetLockablePatterns()) == 0 { diff --git a/commands/command_post_commit.go b/commands/command_post_commit.go index 0ea2c731..79014249 100644 --- a/commands/command_post_commit.go +++ b/commands/command_post_commit.go @@ -24,7 +24,7 @@ func postCommitCommand(cmd *cobra.Command, args []string) { requireGitVersion() - lockClient := newLockClient(cfg.CurrentRemote) + lockClient := newLockClient(cfg.Remote()) // Skip this hook if no lockable patterns have been configured if len(lockClient.GetLockablePatterns()) == 0 { diff --git a/commands/command_post_merge.go b/commands/command_post_merge.go index 485a1614..afc4398d 100644 --- a/commands/command_post_merge.go +++ b/commands/command_post_merge.go @@ -24,7 +24,7 @@ func postMergeCommand(cmd *cobra.Command, args []string) { requireGitVersion() - lockClient := newLockClient(cfg.CurrentRemote) + lockClient := newLockClient(cfg.Remote()) // Skip this hook if no lockable patterns have been configured if len(lockClient.GetLockablePatterns()) == 0 { diff --git a/commands/command_pull.go b/commands/command_pull.go index 2a5ee179..a8c5f1d7 100644 --- a/commands/command_pull.go +++ b/commands/command_pull.go @@ -40,7 +40,7 @@ func pullCommand(cmd *cobra.Command, args []string) { } func pull(remote string, filter *filepathfilter.Filter) { - cfg.CurrentRemote = remote + cfg.SetRemote(remote) ref, err := git.CurrentRef() if err != nil { Panic(err, "Could not pull") diff --git a/commands/command_track.go b/commands/command_track.go index 057fb635..9a2c8fa9 100644 --- a/commands/command_track.go +++ b/commands/command_track.go @@ -205,7 +205,7 @@ ArgsLoop: } // now flip read-only mode based on lockable / not lockable changes - lockClient := newLockClient(cfg.CurrentRemote) + lockClient := newLockClient(cfg.Remote()) err = lockClient.FixFileWriteFlagsInDir(relpath, readOnlyPatterns, writeablePatterns) if err != nil { LoggedError(err, "Error changing lockable file permissions: %s", err) diff --git a/commands/command_unlock.go b/commands/command_unlock.go index 0ca2e382..a9f83374 100644 --- a/commands/command_unlock.go +++ b/commands/command_unlock.go @@ -131,7 +131,7 @@ func unlockAbortIfFileModifiedById(id string, lockClient *locking.Client) { func init() { RegisterCommand("unlock", unlockCommand, func(cmd *cobra.Command) { - cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.CurrentRemote, lockRemoteHelp) + cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.Remote(), lockRemoteHelp) cmd.Flags().StringVarP(&unlockCmdFlags.Id, "id", "i", "", "unlock a lock by its ID") cmd.Flags().BoolVarP(&unlockCmdFlags.Force, "force", "f", false, "forcibly break another user's lock(s)") cmd.Flags().BoolVarP(&locksCmdFlags.JSON, "json", "", false, "print output in json") diff --git a/commands/uploader.go b/commands/uploader.go index 060d3cc0..ee7a81d6 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -91,7 +91,7 @@ const ( ) func newUploadContext(remote string, dryRun bool) *uploadContext { - cfg.CurrentRemote = remote + cfg.SetRemote(remote) ctx := &uploadContext{ Remote: remote, diff --git a/config/config.go b/config/config.go index 4cb45197..95545536 100644 --- a/config/config.go +++ b/config/config.go @@ -32,7 +32,7 @@ type Configuration struct { // configuration. Git Environment - CurrentRemote string + currentRemote string // gitConfig can fetch or modify the current Git config and track the Git // version. @@ -54,7 +54,7 @@ func New() *Configuration { func NewIn(workdir, gitdir string) *Configuration { gitConf := git.NewConfig(workdir, gitdir) c := &Configuration{ - CurrentRemote: defaultRemote, + currentRemote: defaultRemote, Os: EnvironmentOf(NewOsFetcher()), gitConfig: gitConf, } @@ -106,7 +106,7 @@ type Values struct { // This method should only be used during testing. func NewFrom(v Values) *Configuration { c := &Configuration{ - CurrentRemote: defaultRemote, + currentRemote: defaultRemote, Os: EnvironmentOf(mapFetcher(v.Os)), gitConfig: git.NewConfig("", ""), } @@ -151,6 +151,14 @@ func (c *Configuration) FetchExcludePaths() []string { return tools.CleanPaths(patterns, ",") } +func (c *Configuration) Remote() string { + return c.currentRemote +} + +func (c *Configuration) SetRemote(name string) { + c.currentRemote = name +} + func (c *Configuration) Remotes() []string { c.loadGitConfig() return c.remotes diff --git a/lfs/lfs.go b/lfs/lfs.go index 05cc2a2a..4279acbd 100644 --- a/lfs/lfs.go +++ b/lfs/lfs.go @@ -25,8 +25,8 @@ func Environ(cfg *config.Configuration, manifest *tq.Manifest) []string { panic(err.Error()) } - download := api.Endpoints.AccessFor(api.Endpoints.Endpoint("download", cfg.CurrentRemote).Url) - upload := api.Endpoints.AccessFor(api.Endpoints.Endpoint("upload", cfg.CurrentRemote).Url) + download := api.Endpoints.AccessFor(api.Endpoints.Endpoint("download", cfg.Remote()).Url) + upload := api.Endpoints.AccessFor(api.Endpoints.Endpoint("upload", cfg.Remote()).Url) dltransfers := manifest.GetDownloadAdapterNames() sort.Strings(dltransfers) From 6eed720b61f6e45505e40989874acdb890d5471d Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 15:07:32 -0600 Subject: [PATCH 16/71] commands: remove any cfg.Remote() calls from Command flags --- commands/command_lock.go | 8 ++++++-- commands/command_locks.go | 8 ++++++-- commands/command_post_checkout.go | 2 +- commands/command_post_commit.go | 2 +- commands/command_post_merge.go | 2 +- commands/command_track.go | 2 +- commands/command_unlock.go | 8 ++++++-- commands/commands.go | 4 ++-- commands/uploader.go | 12 ++++++------ 9 files changed, 30 insertions(+), 18 deletions(-) diff --git a/commands/command_lock.go b/commands/command_lock.go index 5326d4ac..6043633d 100644 --- a/commands/command_lock.go +++ b/commands/command_lock.go @@ -28,7 +28,11 @@ func lockCommand(cmd *cobra.Command, args []string) { Exit(err.Error()) } - lockClient := newLockClient(lockRemote) + if len(lockRemote) > 0 { + cfg.SetRemote(lockRemote) + } + + lockClient := newLockClient() defer lockClient.Close() lock, err := lockClient.LockFile(path) @@ -90,7 +94,7 @@ func lockPath(file string) (string, error) { func init() { RegisterCommand("lock", lockCommand, func(cmd *cobra.Command) { - cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.Remote(), lockRemoteHelp) + cmd.Flags().StringVarP(&lockRemote, "remote", "r", "", lockRemoteHelp) cmd.Flags().BoolVarP(&locksCmdFlags.JSON, "json", "", false, "print output in json") }) } diff --git a/commands/command_locks.go b/commands/command_locks.go index 0544b9a1..fa3d48ef 100644 --- a/commands/command_locks.go +++ b/commands/command_locks.go @@ -22,7 +22,11 @@ func locksCommand(cmd *cobra.Command, args []string) { Exit("Error building filters: %v", err) } - lockClient := newLockClient(lockRemote) + if len(lockRemote) > 0 { + cfg.SetRemote(lockRemote) + } + + lockClient := newLockClient() defer lockClient.Close() locks, err := lockClient.SearchLocks(filters, locksCmdFlags.Limit, locksCmdFlags.Local) @@ -109,7 +113,7 @@ func (l *locksFlags) Filters() (map[string]string, error) { func init() { RegisterCommand("locks", locksCommand, func(cmd *cobra.Command) { - cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.Remote(), lockRemoteHelp) + cmd.Flags().StringVarP(&lockRemote, "remote", "r", "", lockRemoteHelp) cmd.Flags().StringVarP(&locksCmdFlags.Path, "path", "p", "", "filter locks results matching a particular path") cmd.Flags().StringVarP(&locksCmdFlags.Id, "id", "i", "", "filter locks results matching a particular ID") cmd.Flags().IntVarP(&locksCmdFlags.Limit, "limit", "l", 0, "optional limit for number of results to return") diff --git a/commands/command_post_checkout.go b/commands/command_post_checkout.go index 40617817..0057d6cc 100644 --- a/commands/command_post_checkout.go +++ b/commands/command_post_checkout.go @@ -32,7 +32,7 @@ func postCheckoutCommand(cmd *cobra.Command, args []string) { requireGitVersion() - lockClient := newLockClient(cfg.Remote()) + lockClient := newLockClient() // Skip this hook if no lockable patterns have been configured if len(lockClient.GetLockablePatterns()) == 0 { diff --git a/commands/command_post_commit.go b/commands/command_post_commit.go index 79014249..64d567fe 100644 --- a/commands/command_post_commit.go +++ b/commands/command_post_commit.go @@ -24,7 +24,7 @@ func postCommitCommand(cmd *cobra.Command, args []string) { requireGitVersion() - lockClient := newLockClient(cfg.Remote()) + lockClient := newLockClient() // Skip this hook if no lockable patterns have been configured if len(lockClient.GetLockablePatterns()) == 0 { diff --git a/commands/command_post_merge.go b/commands/command_post_merge.go index afc4398d..03d4392b 100644 --- a/commands/command_post_merge.go +++ b/commands/command_post_merge.go @@ -24,7 +24,7 @@ func postMergeCommand(cmd *cobra.Command, args []string) { requireGitVersion() - lockClient := newLockClient(cfg.Remote()) + lockClient := newLockClient() // Skip this hook if no lockable patterns have been configured if len(lockClient.GetLockablePatterns()) == 0 { diff --git a/commands/command_track.go b/commands/command_track.go index 9a2c8fa9..3472f9c3 100644 --- a/commands/command_track.go +++ b/commands/command_track.go @@ -205,7 +205,7 @@ ArgsLoop: } // now flip read-only mode based on lockable / not lockable changes - lockClient := newLockClient(cfg.Remote()) + lockClient := newLockClient() err = lockClient.FixFileWriteFlagsInDir(relpath, readOnlyPatterns, writeablePatterns) if err != nil { LoggedError(err, "Error changing lockable file permissions: %s", err) diff --git a/commands/command_unlock.go b/commands/command_unlock.go index a9f83374..3334bf69 100644 --- a/commands/command_unlock.go +++ b/commands/command_unlock.go @@ -35,7 +35,11 @@ func unlockCommand(cmd *cobra.Command, args []string) { Exit(unlockUsage) } - lockClient := newLockClient(lockRemote) + if len(lockRemote) > 0 { + cfg.SetRemote(lockRemote) + } + + lockClient := newLockClient() defer lockClient.Close() if hasPath { @@ -131,7 +135,7 @@ func unlockAbortIfFileModifiedById(id string, lockClient *locking.Client) { func init() { RegisterCommand("unlock", unlockCommand, func(cmd *cobra.Command) { - cmd.Flags().StringVarP(&lockRemote, "remote", "r", cfg.Remote(), lockRemoteHelp) + cmd.Flags().StringVarP(&lockRemote, "remote", "r", "", lockRemoteHelp) cmd.Flags().StringVarP(&unlockCmdFlags.Id, "id", "i", "", "unlock a lock by its ID") cmd.Flags().BoolVarP(&unlockCmdFlags.Force, "force", "f", false, "forcibly break another user's lock(s)") cmd.Flags().BoolVarP(&locksCmdFlags.JSON, "json", "", false, "print output in json") diff --git a/commands/commands.go b/commands/commands.go index 887b817f..74e9ac45 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -90,8 +90,8 @@ func closeAPIClient() error { return apiClient.Close() } -func newLockClient(remote string) *locking.Client { - lockClient, err := locking.NewClient(remote, getAPIClient()) +func newLockClient() *locking.Client { + lockClient, err := locking.NewClient(cfg.Remote(), getAPIClient()) if err == nil { os.MkdirAll(cfg.LFSStorageDir(), 0755) err = lockClient.SetupFileCache(cfg.LFSStorageDir()) diff --git a/commands/uploader.go b/commands/uploader.go index ee7a81d6..603b7de2 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -116,7 +116,7 @@ func newUploadContext(remote string, dryRun bool) *uploadContext { return ctx } - ourLocks, theirLocks, verifyState := verifyLocks(remote) + ourLocks, theirLocks, verifyState := verifyLocks() ctx.lockVerifyState = verifyState for _, l := range theirLocks { ctx.theirLocks[l.Path] = l @@ -128,14 +128,14 @@ func newUploadContext(remote string, dryRun bool) *uploadContext { return ctx } -func verifyLocks(remote string) (ours, theirs []locking.Lock, st verifyState) { - endpoint := getAPIClient().Endpoints.Endpoint("upload", remote) +func verifyLocks() (ours, theirs []locking.Lock, st verifyState) { + endpoint := getAPIClient().Endpoints.Endpoint("upload", cfg.Remote()) state := getVerifyStateFor(endpoint) if state == verifyStateDisabled { return } - lockClient := newLockClient(remote) + lockClient := newLockClient() ours, theirs, err := lockClient.VerifiableLocks(0) if err != nil { @@ -149,7 +149,7 @@ func verifyLocks(remote string) (ours, theirs []locking.Lock, st verifyState) { Exit("ERROR: Authentication error: %s", err) } } else { - Print("Remote %q does not support the LFS locking API. Consider disabling it with:", remote) + Print("Remote %q does not support the LFS locking API. Consider disabling it with:", cfg.Remote()) Print(" $ git config lfs.%s.locksverify false", endpoint.Url) if state == verifyStateEnabled { ExitWithError(err) @@ -157,7 +157,7 @@ func verifyLocks(remote string) (ours, theirs []locking.Lock, st verifyState) { } } } else if state == verifyStateUnknown { - Print("Locking support detected on remote %q. Consider enabling it with:", remote) + Print("Locking support detected on remote %q. Consider enabling it with:", cfg.Remote()) Print(" $ git config lfs.%s.locksverify true", endpoint.Url) } From 0f4c34055c145889386de81b861764f79c64c5aa Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 15:11:54 -0600 Subject: [PATCH 17/71] config: lazily set default remote --- config/config.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/config/config.go b/config/config.go index 95545536..3d2b58b8 100644 --- a/config/config.go +++ b/config/config.go @@ -32,7 +32,7 @@ type Configuration struct { // configuration. Git Environment - currentRemote string + currentRemote *string // gitConfig can fetch or modify the current Git config and track the Git // version. @@ -54,9 +54,8 @@ func New() *Configuration { func NewIn(workdir, gitdir string) *Configuration { gitConf := git.NewConfig(workdir, gitdir) c := &Configuration{ - currentRemote: defaultRemote, - Os: EnvironmentOf(NewOsFetcher()), - gitConfig: gitConf, + Os: EnvironmentOf(NewOsFetcher()), + gitConfig: gitConf, } if len(gitConf.WorkDir) > 0 { @@ -106,9 +105,8 @@ type Values struct { // This method should only be used during testing. func NewFrom(v Values) *Configuration { c := &Configuration{ - currentRemote: defaultRemote, - Os: EnvironmentOf(mapFetcher(v.Os)), - gitConfig: git.NewConfig("", ""), + Os: EnvironmentOf(mapFetcher(v.Os)), + gitConfig: git.NewConfig("", ""), } c.Git = &delayedEnvironment{ callback: func() Environment { @@ -152,11 +150,16 @@ func (c *Configuration) FetchExcludePaths() []string { } func (c *Configuration) Remote() string { - return c.currentRemote + c.loadingGit.Lock() + defer c.loadingGit.Unlock() + if c.currentRemote == nil { + c.currentRemote = &defaultRemote + } + return *c.currentRemote } func (c *Configuration) SetRemote(name string) { - c.currentRemote = name + c.currentRemote = &name } func (c *Configuration) Remotes() []string { From fb0598536d3b60082d0c6df7d1bc2814c1709d9b Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 15:31:46 -0600 Subject: [PATCH 18/71] lfs: only show default Endpoint env if default remote is origin --- commands/command_env.go | 14 ++++++++------ config/config.go | 4 ++++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/commands/command_env.go b/commands/command_env.go index e0818009..d4633189 100644 --- a/commands/command_env.go +++ b/commands/command_env.go @@ -9,7 +9,6 @@ import ( func envCommand(cmd *cobra.Command, args []string) { config.ShowConfigWarnings = true - endpoint := getAPIClient().Endpoints.Endpoint("download", cfg.Remote()) gitV, err := git.Version() if err != nil { @@ -20,11 +19,14 @@ func envCommand(cmd *cobra.Command, args []string) { Print(gitV) Print("") - if len(endpoint.Url) > 0 { - access := getAPIClient().Endpoints.AccessFor(endpoint.Url) - Print("Endpoint=%s (auth=%s)", endpoint.Url, access) - if len(endpoint.SshUserAndHost) > 0 { - Print(" SSH=%s:%s", endpoint.SshUserAndHost, endpoint.SshPath) + if cfg.IsDefaultRemote() { + endpoint := getAPIClient().Endpoints.Endpoint("download", cfg.Remote()) + if len(endpoint.Url) > 0 { + access := getAPIClient().Endpoints.AccessFor(endpoint.Url) + Print("Endpoint=%s (auth=%s)", endpoint.Url, access) + if len(endpoint.SshUserAndHost) > 0 { + Print(" SSH=%s:%s", endpoint.SshUserAndHost, endpoint.SshPath) + } } } diff --git a/config/config.go b/config/config.go index 3d2b58b8..51cdadd0 100644 --- a/config/config.go +++ b/config/config.go @@ -149,6 +149,10 @@ func (c *Configuration) FetchExcludePaths() []string { return tools.CleanPaths(patterns, ",") } +func (c *Configuration) IsDefaultRemote() bool { + return c.Remote() == defaultRemote +} + func (c *Configuration) Remote() string { c.loadingGit.Lock() defer c.loadingGit.Unlock() From 77ebffe6f1ed6283978306555375a68f8eba787b Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 15:32:01 -0600 Subject: [PATCH 19/71] config: lazily load Remote() from git.DefaultRemote() --- config/config.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index 51cdadd0..f855c121 100644 --- a/config/config.go +++ b/config/config.go @@ -157,7 +157,15 @@ func (c *Configuration) Remote() string { c.loadingGit.Lock() defer c.loadingGit.Unlock() if c.currentRemote == nil { - c.currentRemote = &defaultRemote + r, err := c.GitConfig().DefaultRemote() + if err == nil && len(r) > 0 { + c.currentRemote = &r + } else { + if err != nil { + tracerx.Printf("Error getting default remote: %+v", err) + } + c.currentRemote = &defaultRemote + } } return *c.currentRemote } From 079c21b6d4952afd3e8039e13ef13bb9b9b670a3 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 16:05:22 -0600 Subject: [PATCH 20/71] commands: fetch now defaults to origin remote --- commands/command_fetch.go | 12 ------------ test/test-fetch.sh | 6 ------ 2 files changed, 18 deletions(-) diff --git a/commands/command_fetch.go b/commands/command_fetch.go index 94d19583..e131c9b5 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -43,8 +43,6 @@ func fetchCommand(cmd *cobra.Command, args []string) { Exit("Invalid remote name %q", args[0]) } cfg.SetRemote(args[0]) - } else { - cfg.SetRemote("") } if len(args) > 1 { @@ -268,16 +266,6 @@ func scanAll() []*lfs.WrappedPointer { // Fetch and report completion of each OID to a channel (optional, pass nil to skip) // Returns true if all completed with no errors, false if errors were written to stderr/log func fetchAndReportToChan(allpointers []*lfs.WrappedPointer, filter *filepathfilter.Filter, out chan<- *lfs.WrappedPointer) bool { - // Lazily initialize the current remote. - if len(cfg.Remote()) == 0 { - // Actively find the default remote, don't just assume origin - defaultRemote, err := cfg.GitConfig().DefaultRemote() - if err != nil { - Exit("No default remote") - } - cfg.SetRemote(defaultRemote) - } - ready, pointers, meter := readyAndMissingPointers(allpointers, filter) q := newDownloadQueue( getTransferManifestOperationRemote("download", cfg.Remote()), diff --git a/test/test-fetch.sh b/test/test-fetch.sh index f17f15b3..a8fa2a04 100755 --- a/test/test-fetch.sh +++ b/test/test-fetch.sh @@ -440,12 +440,6 @@ begin_test "fetch with no origin remote" # and no origin, but only 1 remote, should pick the only one as default git lfs fetch assert_local_object "$contents_oid" 1 - - # delete again, now add a second remote, also non-origin - rm -rf .git/lfs - git remote add something2 "$GITSERVER/$reponame" - git lfs fetch 2>&1 | grep "No default remote" - refute_local_object "$contents_oid" ) end_test From abbabdbe0977e9575c5d80fb8842b28bb00f5e99 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 16:11:16 -0600 Subject: [PATCH 21/71] commands: teach pull() to get remote from cfg.Remote() --- commands/command_clone.go | 9 ++------- commands/command_pull.go | 16 ++++------------ 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/commands/command_clone.go b/commands/command_clone.go index a301f23c..a2db0ce9 100644 --- a/commands/command_clone.go +++ b/commands/command_clone.go @@ -72,13 +72,9 @@ func cloneCommand(cmd *cobra.Command, args []string) { requireInRepo() - // Now just call pull with default args // Support --origin option to clone - var remote string if len(cloneFlags.Origin) > 0 { - remote = cloneFlags.Origin - } else { - remote = "origin" + cfg.SetRemote(cloneFlags.Origin) } if ref, err := git.CurrentRef(); err == nil { @@ -86,10 +82,9 @@ func cloneCommand(cmd *cobra.Command, args []string) { filter := buildFilepathFilter(cfg, includeArg, excludeArg) if cloneFlags.NoCheckout || cloneFlags.Bare { // If --no-checkout or --bare then we shouldn't check out, just fetch instead - cfg.SetRemote(remote) fetchRef(ref.Name, filter) } else { - pull(remote, filter) + pull(filter) err := postCloneSubmodules(args) if err != nil { Exit("Error performing 'git lfs pull' for submodules: %v", err) diff --git a/commands/command_pull.go b/commands/command_pull.go index a8c5f1d7..5b2d49d8 100644 --- a/commands/command_pull.go +++ b/commands/command_pull.go @@ -18,29 +18,20 @@ func pullCommand(cmd *cobra.Command, args []string) { requireGitVersion() requireInRepo() - var remote string if len(args) > 0 { // Remote is first arg if err := git.ValidateRemote(args[0]); err != nil { Panic(err, fmt.Sprintf("Invalid remote name '%v'", args[0])) } - remote = args[0] - } else { - // Actively find the default remote, don't just assume origin - defaultRemote, err := cfg.GitConfig().DefaultRemote() - if err != nil { - Panic(err, "No default remote") - } - remote = defaultRemote + cfg.SetRemote(args[0]) } includeArg, excludeArg := getIncludeExcludeArgs(cmd) filter := buildFilepathFilter(cfg, includeArg, excludeArg) - pull(remote, filter) + pull(filter) } -func pull(remote string, filter *filepathfilter.Filter) { - cfg.SetRemote(remote) +func pull(filter *filepathfilter.Filter) { ref, err := git.CurrentRef() if err != nil { Panic(err, "Could not pull") @@ -48,6 +39,7 @@ func pull(remote string, filter *filepathfilter.Filter) { pointers := newPointerMap() meter := progress.NewMeter(progress.WithOSEnv(cfg.Os)) + remote := cfg.Remote() singleCheckout := newSingleCheckout(cfg.Git, remote) q := newDownloadQueue(singleCheckout.Manifest(), remote, tq.WithProgress(meter)) gitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) { From 29930e5ed5110edeea23c242346cd9c5fc5d9e7b Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 16:14:26 -0600 Subject: [PATCH 22/71] commands: teach newUploadContext() to use cfg.Remote() --- commands/command_pre_push.go | 3 ++- commands/command_push.go | 4 +++- commands/uploader.go | 8 +++----- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 83d9babd..fd551eba 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -49,8 +49,9 @@ func prePushCommand(cmd *cobra.Command, args []string) { if err := git.ValidateRemote(args[0]); err != nil { Exit("Invalid remote name %q", args[0]) } + cfg.SetRemote(args[0]) - ctx := newUploadContext(args[0], prePushDryRun) + ctx := newUploadContext(prePushDryRun) gitscanner, err := ctx.buildGitScanner() if err != nil { diff --git a/commands/command_push.go b/commands/command_push.go index 838e6223..1a1d89e8 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -117,7 +117,9 @@ func pushCommand(cmd *cobra.Command, args []string) { Exit("Invalid remote name %q", args[0]) } - ctx := newUploadContext(args[0], pushDryRun) + cfg.SetRemote(args[0]) + + ctx := newUploadContext(pushDryRun) if pushObjectIDs { if len(args) < 2 { diff --git a/commands/uploader.go b/commands/uploader.go index 603b7de2..bc824f09 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -90,12 +90,9 @@ const ( verifyStateDisabled ) -func newUploadContext(remote string, dryRun bool) *uploadContext { - cfg.SetRemote(remote) - +func newUploadContext(dryRun bool) *uploadContext { ctx := &uploadContext{ - Remote: remote, - Manifest: getTransferManifestOperationRemote("upload", remote), + Remote: cfg.Remote(), DryRun: dryRun, uploadedOids: tools.NewStringSet(), gitfilter: lfs.NewGitFilter(cfg), @@ -105,6 +102,7 @@ func newUploadContext(remote string, dryRun bool) *uploadContext { allowMissing: cfg.Git.Bool("lfs.allowincompletepush", true), } + ctx.Manifest = getTransferManifestOperationRemote("upload", ctx.Remote) ctx.meter = buildProgressMeter(ctx.DryRun) ctx.tq = newUploadQueue(ctx.Manifest, ctx.Remote, tq.WithProgress(ctx.meter), tq.DryRun(ctx.DryRun)) ctx.committerName, ctx.committerEmail = cfg.CurrentCommitter() From 88f0982002c4c0f6e7424f5337df98f2be4c4333 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 16:20:38 -0600 Subject: [PATCH 23/71] config: add (*Configuration) SetValidRemote(), wrapping git.ValidateRemote() --- commands/command_fetch.go | 5 ++--- commands/command_pre_push.go | 6 ++---- commands/command_pull.go | 5 ++--- commands/command_push.go | 6 ++---- config/config.go | 8 ++++++++ 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/commands/command_fetch.go b/commands/command_fetch.go index e131c9b5..4826146d 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -39,10 +39,9 @@ func fetchCommand(cmd *cobra.Command, args []string) { if len(args) > 0 { // Remote is first arg - if err := git.ValidateRemote(args[0]); err != nil { - Exit("Invalid remote name %q", args[0]) + if err := cfg.SetValidRemote(args[0]); err != nil { + Exit("Invalid remote name %q: %s", args[0], err) } - cfg.SetRemote(args[0]) } if len(args) > 1 { diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index fd551eba..18214375 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -5,7 +5,6 @@ import ( "os" "strings" - "github.com/git-lfs/git-lfs/git" "github.com/rubyist/tracerx" "github.com/spf13/cobra" ) @@ -46,10 +45,9 @@ func prePushCommand(cmd *cobra.Command, args []string) { requireGitVersion() // Remote is first arg - if err := git.ValidateRemote(args[0]); err != nil { - Exit("Invalid remote name %q", args[0]) + if err := cfg.SetValidRemote(args[0]); err != nil { + Exit("Invalid remote name %q: %s", args[0], err) } - cfg.SetRemote(args[0]) ctx := newUploadContext(prePushDryRun) diff --git a/commands/command_pull.go b/commands/command_pull.go index 5b2d49d8..ae261b11 100644 --- a/commands/command_pull.go +++ b/commands/command_pull.go @@ -20,10 +20,9 @@ func pullCommand(cmd *cobra.Command, args []string) { if len(args) > 0 { // Remote is first arg - if err := git.ValidateRemote(args[0]); err != nil { - Panic(err, fmt.Sprintf("Invalid remote name '%v'", args[0])) + if err := cfg.SetValidRemote(args[0]); err != nil { + Exit("Invalid remote name %q: %s", args[0], err) } - cfg.SetRemote(args[0]) } includeArg, excludeArg := getIncludeExcludeArgs(cmd) diff --git a/commands/command_push.go b/commands/command_push.go index 1a1d89e8..511a3758 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -113,12 +113,10 @@ func pushCommand(cmd *cobra.Command, args []string) { requireGitVersion() // Remote is first arg - if err := git.ValidateRemote(args[0]); err != nil { - Exit("Invalid remote name %q", args[0]) + if err := cfg.SetValidRemote(args[0]); err != nil { + Exit("Invalid remote name %q: %s", args[0], err) } - cfg.SetRemote(args[0]) - ctx := newUploadContext(pushDryRun) if pushObjectIDs { diff --git a/config/config.go b/config/config.go index f855c121..bca192d3 100644 --- a/config/config.go +++ b/config/config.go @@ -170,6 +170,14 @@ func (c *Configuration) Remote() string { return *c.currentRemote } +func (c *Configuration) SetValidRemote(name string) error { + if err := git.ValidateRemote(name); err != nil { + return err + } + c.SetRemote(name) + return nil +} + func (c *Configuration) SetRemote(name string) { c.currentRemote = &name } From 573f7b570ad5870704681274022808591a029bab Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 16:38:49 -0600 Subject: [PATCH 24/71] config: re-implement DefaultRemote() in *Configuration with the Git env --- config/config.go | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/config/config.go b/config/config.go index bca192d3..45e90e58 100644 --- a/config/config.go +++ b/config/config.go @@ -38,6 +38,7 @@ type Configuration struct { // version. gitConfig *git.Configuration + ref *git.Ref fs *fs.Filesystem gitDir *string workDir string @@ -149,21 +150,45 @@ func (c *Configuration) FetchExcludePaths() []string { return tools.CleanPaths(patterns, ",") } +func (c *Configuration) CurrentRef() *git.Ref { + c.loading.Lock() + defer c.loading.Unlock() + if c.ref == nil { + r, err := git.CurrentRef() + if err != nil { + tracerx.Printf("Error loading current ref: %s", err) + c.ref = &git.Ref{} + } else { + c.ref = r + } + } + return c.ref +} + func (c *Configuration) IsDefaultRemote() bool { return c.Remote() == defaultRemote } func (c *Configuration) Remote() string { - c.loadingGit.Lock() - defer c.loadingGit.Unlock() + ref := c.CurrentRef() + + c.loading.Lock() + defer c.loading.Unlock() + if c.currentRemote == nil { - r, err := c.GitConfig().DefaultRemote() - if err == nil && len(r) > 0 { - c.currentRemote = &r + if len(ref.Name) == 0 { + c.currentRemote = &defaultRemote + return defaultRemote + } + + if remote, ok := c.Git.Get(fmt.Sprintf("branch.%s.remote", ref.Name)); ok { + // try tracking remote + c.currentRemote = &remote + } else if remotes := c.Remotes(); len(remotes) == 1 { + // use only remote if there is only 1 + c.currentRemote = &remotes[0] } else { - if err != nil { - tracerx.Printf("Error getting default remote: %+v", err) - } + // fall back to default :( c.currentRemote = &defaultRemote } } From 2c1a44df0005b2683a5074c3545e11dce077f065 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 16:42:49 -0600 Subject: [PATCH 25/71] git: remove some dead code --- config/config.go | 5 +++++ git/git.go | 46 ---------------------------------------------- git/git_test.go | 4 ---- 3 files changed, 5 insertions(+), 50 deletions(-) diff --git a/config/config.go b/config/config.go index 45e90e58..607760ff 100644 --- a/config/config.go +++ b/config/config.go @@ -169,6 +169,11 @@ func (c *Configuration) IsDefaultRemote() bool { return c.Remote() == defaultRemote } +// Remote returns the default remote based on: +// 1. The currently tracked remote branch, if present +// 2. Any other SINGLE remote defined in .git/config +// 3. Use "origin" as a fallback. +// Results are cached after the first hit. func (c *Configuration) Remote() string { ref := c.CurrentRef() diff --git a/git/git.go b/git/git.go index 983a0834..38b3ae01 100644 --- a/git/git.go +++ b/git/git.go @@ -242,19 +242,6 @@ func (c *Configuration) CurrentRemoteRef() (*Ref, error) { return ResolveRef(remoteref) } -// RemoteForCurrentBranch returns the name of the remote that the current branch is tracking -func (c *Configuration) RemoteForCurrentBranch() (string, error) { - ref, err := CurrentRef() - if err != nil { - return "", err - } - remote := c.RemoteForBranch(ref.Name) - if remote == "" { - return "", fmt.Errorf("remote not found for branch %q", ref.Name) - } - return remote, nil -} - // RemoteRefForCurrentBranch returns the full remote ref (refs/remotes/{remote}/{remotebranch}) // that the current branch is tracking. func (c *Configuration) RemoteRefNameForCurrentBranch() (string, error) { @@ -423,39 +410,6 @@ func ValidateRemoteURL(remote string) error { } } -// DefaultRemote returns the default remote based on: -// 1. The currently tracked remote branch, if present -// 2. "origin", if defined -// 3. Any other SINGLE remote defined in .git/config -// Returns an error if all of these fail, i.e. no tracked remote branch, no -// "origin", and either no remotes defined or 2+ non-"origin" remotes -func (c *Configuration) DefaultRemote() (string, error) { - tracked, err := c.RemoteForCurrentBranch() - if err == nil { - return tracked, nil - } - - // Otherwise, check what remotes are defined - remotes, err := RemoteList() - if err != nil { - return "", err - } - switch len(remotes) { - case 0: - return "", errors.New("No remotes defined") - case 1: // always use a single remote whether it's origin or otherwise - return remotes[0], nil - default: - for _, remote := range remotes { - // Use origin if present - if remote == "origin" { - return remote, nil - } - } - } - return "", errors.New("Unable to pick default remote, too ambiguous") -} - func UpdateIndexFromStdin() *subprocess.Cmd { return git("update-index", "-q", "--refresh", "--stdin") } diff --git a/git/git_test.go b/git/git_test.go index e2e9a8d3..0ec125a7 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -71,10 +71,6 @@ func TestCurrentRefAndCurrentRemoteRef(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "refs/remotes/origin/someremotebranch", refname) - remote, err := gitConf.RemoteForCurrentBranch() - assert.Nil(t, err) - assert.Equal(t, "origin", remote) - ref, err = ResolveRef(outputs[2].Sha) assert.Nil(t, err) assert.Equal(t, &Ref{outputs[2].Sha, RefTypeOther, outputs[2].Sha}, ref) From bee3e755e1cc2206c342344ea98bd29bb51c3235 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 27 Oct 2017 17:01:25 -0600 Subject: [PATCH 26/71] test: add happy path test for non-origin remote --- test/test-happy-path.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/test-happy-path.sh b/test/test-happy-path.sh index 1a74c77b..84adf85b 100755 --- a/test/test-happy-path.sh +++ b/test/test-happy-path.sh @@ -64,6 +64,35 @@ begin_test "happy path" ) end_test +begin_test "happy path on non-origin remote" +( + set -e + + reponame="happy-without-origin" + setup_remote_repo "$reponame" + + clone_repo "$reponame" repo-without-origin + git lfs track "*.dat" + git add .gitattributes + git commit -m "track" + git push origin master + + clone_repo "$reponame" clone-without-origin + git remote rename origin happy-path + + cd ../repo-without-origin + echo "a" > a.dat + git add a.dat + git commit -m "boom" + git push origin master + + cd ../clone-without-origin + echo "remotes:" + git remote + git pull happy-path master +) +end_test + begin_test "clears local temp objects" ( set -e From 1477d3f32efc1e6aa9dd4906dfcb9bde39e531ca Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 30 Oct 2017 12:18:55 -0600 Subject: [PATCH 27/71] add some ci debug messages --- config/config.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/config.go b/config/config.go index 607760ff..31ea4899 100644 --- a/config/config.go +++ b/config/config.go @@ -181,19 +181,24 @@ func (c *Configuration) Remote() string { defer c.loading.Unlock() if c.currentRemote == nil { + tracerx.Printf("REMOTE: %+v", ref) if len(ref.Name) == 0 { + tracerx.Printf("REMOTE: empty") c.currentRemote = &defaultRemote return defaultRemote } if remote, ok := c.Git.Get(fmt.Sprintf("branch.%s.remote", ref.Name)); ok { + tracerx.Printf("REMOTE: branch.*.remote") // try tracking remote c.currentRemote = &remote } else if remotes := c.Remotes(); len(remotes) == 1 { + tracerx.Printf("REMOTE: first remote: %+v", remotes) // use only remote if there is only 1 c.currentRemote = &remotes[0] } else { // fall back to default :( + tracerx.Printf("REMOTE: default") c.currentRemote = &defaultRemote } } From 52bc20f3d70392108aa83efdfe3612c610e06724 Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 30 Oct 2017 13:42:41 -0600 Subject: [PATCH 28/71] more debug info --- config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index 31ea4899..3508bc13 100644 --- a/config/config.go +++ b/config/config.go @@ -189,7 +189,7 @@ func (c *Configuration) Remote() string { } if remote, ok := c.Git.Get(fmt.Sprintf("branch.%s.remote", ref.Name)); ok { - tracerx.Printf("REMOTE: branch.*.remote") + tracerx.Printf("REMOTE: branch.*.remote %q (%v)", remote, ok) // try tracking remote c.currentRemote = &remote } else if remotes := c.Remotes(); len(remotes) == 1 { From af85d8605149d5a5c1c8f65ca1a1863c7da78a46 Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 30 Oct 2017 13:56:02 -0600 Subject: [PATCH 29/71] lfs: teach gitfilter to pass remote to transferqueue for downloads --- lfs/gitfilter_smudge.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lfs/gitfilter_smudge.go b/lfs/gitfilter_smudge.go index bb2c4cde..e3c82f61 100644 --- a/lfs/gitfilter_smudge.go +++ b/lfs/gitfilter_smudge.go @@ -81,7 +81,8 @@ func (f *GitFilter) downloadFile(writer io.Writer, ptr *Pointer, workingfile, me // // Either way, forward it into the *tq.TransferQueue so that updates are // sent over correctly. - q := tq.NewTransferQueue(tq.Download, manifest, "", tq.WithProgressCallback(cb)) + + q := tq.NewTransferQueue(tq.Download, manifest, f.cfg.Remote(), tq.WithProgressCallback(cb)) q.Add(filepath.Base(workingfile), mediafile, ptr.Oid, ptr.Size) q.Wait() From 114fb482d9b84d0f6c88778217aaed8711526a4f Mon Sep 17 00:00:00 2001 From: rick olson Date: Mon, 30 Oct 2017 14:05:46 -0600 Subject: [PATCH 30/71] remove debug messages --- config/config.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/config/config.go b/config/config.go index 3508bc13..607760ff 100644 --- a/config/config.go +++ b/config/config.go @@ -181,24 +181,19 @@ func (c *Configuration) Remote() string { defer c.loading.Unlock() if c.currentRemote == nil { - tracerx.Printf("REMOTE: %+v", ref) if len(ref.Name) == 0 { - tracerx.Printf("REMOTE: empty") c.currentRemote = &defaultRemote return defaultRemote } if remote, ok := c.Git.Get(fmt.Sprintf("branch.%s.remote", ref.Name)); ok { - tracerx.Printf("REMOTE: branch.*.remote %q (%v)", remote, ok) // try tracking remote c.currentRemote = &remote } else if remotes := c.Remotes(); len(remotes) == 1 { - tracerx.Printf("REMOTE: first remote: %+v", remotes) // use only remote if there is only 1 c.currentRemote = &remotes[0] } else { // fall back to default :( - tracerx.Printf("REMOTE: default") c.currentRemote = &defaultRemote } } From c3573c727dc91a8ac11af9219ffb138ddb98d55e Mon Sep 17 00:00:00 2001 From: rick olson Date: Tue, 31 Oct 2017 11:23:37 -0600 Subject: [PATCH 31/71] config,locking,commands: send remote ref name to lock verify api calls --- commands/uploader.go | 2 +- config/config.go | 33 +++++++++++++++++ locking/api.go | 2 ++ locking/locks.go | 24 ++++--------- locking/locks_test.go | 7 ++-- test/cmd/lfstest-gitserver.go | 13 +++++++ test/test-pre-push.sh | 67 +++++++++++++++++++++++++++++++++++ 7 files changed, 126 insertions(+), 22 deletions(-) diff --git a/commands/uploader.go b/commands/uploader.go index bc824f09..66770841 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -135,7 +135,7 @@ func verifyLocks() (ours, theirs []locking.Lock, st verifyState) { lockClient := newLockClient() - ours, theirs, err := lockClient.VerifiableLocks(0) + ours, theirs, err := lockClient.VerifiableLocks(cfg.RemoteRefName(), 0) if err != nil { if errors.IsNotImplementedError(err) { disableFor(endpoint) diff --git a/config/config.go b/config/config.go index 607760ff..ed910947 100644 --- a/config/config.go +++ b/config/config.go @@ -39,6 +39,7 @@ type Configuration struct { gitConfig *git.Configuration ref *git.Ref + remoteRef *git.Ref fs *fs.Filesystem gitDir *string workDir string @@ -165,6 +166,38 @@ func (c *Configuration) CurrentRef() *git.Ref { return c.ref } +func (c *Configuration) RemoteRef() *git.Ref { + r := c.CurrentRef() + + c.loading.Lock() + defer c.loading.Unlock() + + if c.remoteRef != nil { + return c.remoteRef + } + + if r != nil { + merge, _ := c.Git.Get(fmt.Sprintf("branch.%s.merge", r.Name)) + if strings.HasPrefix(merge, "refs/heads/") { + c.remoteRef = &git.Ref{ + Name: merge[11:], + Type: git.RefTypeRemoteBranch, + } + } else { + c.remoteRef = r + } + } + + return c.remoteRef +} + +func (c *Configuration) RemoteRefName() string { + if r := c.RemoteRef(); r != nil { + return r.Name + } + return "" +} + func (c *Configuration) IsDefaultRemote() bool { return c.Remote() == defaultRemote } diff --git a/locking/api.go b/locking/api.go index 12f6d1a0..2d6e13e1 100644 --- a/locking/api.go +++ b/locking/api.go @@ -192,6 +192,8 @@ func (c *lockClient) Search(remote string, searchReq *lockSearchRequest) (*lockL // lockVerifiableRequest encapsulates the request sent to the server when the // client would like a list of locks to verify a Git push. type lockVerifiableRequest struct { + Ref string `json:"ref"` + // Cursor is an optional field used to tell the server which lock was // seen last, if scanning through multiple pages of results. // diff --git a/locking/locks.go b/locking/locks.go index feb0a537..293dd1bb 100644 --- a/locking/locks.go +++ b/locking/locks.go @@ -206,13 +206,16 @@ func (c *Client) SearchLocks(filter map[string]string, limit int, localOnly bool } } -func (c *Client) VerifiableLocks(limit int) (ourLocks, theirLocks []Lock, err error) { +func (c *Client) VerifiableLocks(ref string, limit int) (ourLocks, theirLocks []Lock, err error) { ourLocks = make([]Lock, 0, limit) theirLocks = make([]Lock, 0, limit) body := &lockVerifiableRequest{ + Ref: ref, Limit: limit, } + c.cache.Clear() + for { list, res, err := c.client.SearchVerifiable(c.Remote, body) if res != nil { @@ -236,6 +239,7 @@ func (c *Client) VerifiableLocks(limit int) (ourLocks, theirLocks []Lock, err er } for _, l := range list.Ours { + c.cache.Add(l) ourLocks = append(ourLocks, l) if limit > 0 && (len(ourLocks)+len(theirLocks)) >= limit { return ourLocks, theirLocks, nil @@ -243,6 +247,7 @@ func (c *Client) VerifiableLocks(limit int) (ourLocks, theirLocks []Lock, err er } for _, l := range list.Theirs { + c.cache.Add(l) theirLocks = append(theirLocks, l) if limit > 0 && (len(ourLocks)+len(theirLocks)) >= limit { return ourLocks, theirLocks, nil @@ -349,23 +354,6 @@ func (c *Client) lockIdFromPath(path string) (string, error) { } } -// Fetch locked files for the current user and cache them locally -// This can be used to sync up locked files when moving machines -func (c *Client) refreshLockCache() error { - ourLocks, _, err := c.VerifiableLocks(0) - if err != nil { - return err - } - - // We're going to overwrite the entire local cache - c.cache.Clear() - for _, l := range ourLocks { - c.cache.Add(l) - } - - return nil -} - // IsFileLockedByCurrentCommitter returns whether a file is locked by the // current user, as cached locally func (c *Client) IsFileLockedByCurrentCommitter(path string) bool { diff --git a/locking/locks_test.go b/locking/locks_test.go index 0e1b6603..3eaa5c97 100644 --- a/locking/locks_test.go +++ b/locking/locks_test.go @@ -64,8 +64,7 @@ func TestRefreshCache(t *testing.T) { assert.Nil(t, err) assert.Empty(t, locks) - // Should load from test data, just Fred's - err = client.refreshLockCache() + _, _, err = client.VerifiableLocks("", 100) assert.Nil(t, err) locks, err = client.SearchLocks(nil, 0, true) @@ -79,6 +78,8 @@ func TestRefreshCache(t *testing.T) { Lock{Path: "folder/test1.dat", Id: "101", Owner: &User{Name: "Fred"}, LockedAt: zeroTime}, Lock{Path: "folder/test2.dat", Id: "102", Owner: &User{Name: "Fred"}, LockedAt: zeroTime}, Lock{Path: "root.dat", Id: "103", Owner: &User{Name: "Fred"}, LockedAt: zeroTime}, + Lock{Path: "other/test1.dat", Id: "199", Owner: &User{Name: "Charles"}, LockedAt: zeroTime}, + Lock{Path: "folder/test3.dat", Id: "99", Owner: &User{Name: "Alice"}, LockedAt: zeroTime}, }, locks) } @@ -129,7 +130,7 @@ func TestGetVerifiableLocks(t *testing.T) { client, err := NewClient("", lfsclient) assert.Nil(t, err) - ourLocks, theirLocks, err := client.VerifiableLocks(0) + ourLocks, theirLocks, err := client.VerifiableLocks("", 0) assert.Nil(t, err) // Need to include zero time in structure for equal to work diff --git a/test/cmd/lfstest-gitserver.go b/test/cmd/lfstest-gitserver.go index 7bb58ec2..ab440d15 100644 --- a/test/cmd/lfstest-gitserver.go +++ b/test/cmd/lfstest-gitserver.go @@ -882,6 +882,7 @@ type LockList struct { } type VerifiableLockRequest struct { + Ref string `json:"ref,omitempty"` Cursor string `json:"cursor,omitempty"` Limit int `json:"limit,omitempty"` } @@ -1089,6 +1090,18 @@ func locksHandler(w http.ResponseWriter, r *http.Request, repo string) { return } + if strings.HasSuffix(repo, "ref-required") { + parts := strings.Split(repo, "-") + lenParts := len(parts) + if lenParts > 3 && parts[lenParts-3] != reqBody.Ref { + w.WriteHeader(403) + enc.Encode(struct { + Message string `json:"message"` + }{fmt.Sprintf("Expected ref %q, got %q", parts[lenParts-3], reqBody.Ref)}) + return + } + } + ll := &VerifiableLockList{} locks, nextCursor, err := getFilteredLocks(repo, "", reqBody.Cursor, diff --git a/test/test-pre-push.sh b/test/test-pre-push.sh index 6816cca7..2435b100 100755 --- a/test/test-pre-push.sh +++ b/test/test-pre-push.sh @@ -777,6 +777,73 @@ begin_test "pre-push disable locks verify on partial url" ) end_test +begin_test "pre-push locks verify 403 with good ref" +( + set -e + + reponame="lock-verify-master-ref-required" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + contents="example" + contents_oid="$(calc_oid "$contents")" + printf "$contents" > a.dat + git lfs track "*.dat" + git add .gitattributes a.dat + git commit --message "initial commit" + + git config "lfs.$GITSERVER/$reponame.git.locksverify" true + git push origin master 2>&1 | tee push.log + + assert_server_object "$reponame" "$contents_oid" +) +end_test + +begin_test "pre-push locks verify 403 with good tracked ref" +( + set -e + + reponame="lock-verify-tracked-ref-required" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + contents="example" + contents_oid="$(calc_oid "$contents")" + printf "$contents" > a.dat + git lfs track "*.dat" + git add .gitattributes a.dat + git commit --message "initial commit" + + git config branch.master.merge refs/heads/tracked + git config "lfs.$GITSERVER/$reponame.git.locksverify" true + git push origin master 2>&1 | tee push.log + + assert_server_object "$reponame" "$contents_oid" +) +end_test + +begin_test "pre-push locks verify 403 with bad ref" +( + set -e + + reponame="lock-verify-other-ref-required" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + contents="example" + contents_oid="$(calc_oid "$contents")" + printf "$contents" > a.dat + git lfs track "*.dat" + git add .gitattributes a.dat + git commit --message "initial commit" + + git config "lfs.$GITSERVER/$reponame.git.locksverify" true + git push origin master 2>&1 | tee push.log + grep "failed to push some refs" push.log + refute_server_object "$reponame" "$contents_oid" +) +end_test + begin_test "pre-push locks verify 5xx with verification unset" ( set -e From b447e269f12c5df12430fd9cce88cfb3ae631610 Mon Sep 17 00:00:00 2001 From: rick olson Date: Tue, 31 Oct 2017 15:05:42 -0600 Subject: [PATCH 32/71] git: add ParseRef() --- config/config.go | 7 ++----- git/git.go | 25 +++++++++++++++++++++++++ git/git_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/config/config.go b/config/config.go index ed910947..2ce9a16b 100644 --- a/config/config.go +++ b/config/config.go @@ -178,11 +178,8 @@ func (c *Configuration) RemoteRef() *git.Ref { if r != nil { merge, _ := c.Git.Get(fmt.Sprintf("branch.%s.merge", r.Name)) - if strings.HasPrefix(merge, "refs/heads/") { - c.remoteRef = &git.Ref{ - Name: merge[11:], - Type: git.RefTypeRemoteBranch, - } + if len(merge) > 0 { + c.remoteRef = git.ParseRef(merge, "") } else { c.remoteRef = r } diff --git a/git/git.go b/git/git.go index 38b3ae01..9676bbb0 100644 --- a/git/git.go +++ b/git/git.go @@ -63,6 +63,31 @@ func (t RefType) Prefix() (string, bool) { } } +func ParseRef(absRef, sha string) *Ref { + r := &Ref{Sha: sha} + if strings.HasPrefix(absRef, "refs/heads/") { + r.Name = absRef[11:] + r.Type = RefTypeLocalBranch + } else if strings.HasPrefix(absRef, "refs/tags/") { + r.Name = absRef[10:] + r.Type = RefTypeLocalTag + } else if strings.HasPrefix(absRef, "refs/remotes/tags/") { + r.Name = absRef[18:] + r.Type = RefTypeRemoteTag + } else if strings.HasPrefix(absRef, "refs/remotes/") { + r.Name = absRef[13:] + r.Type = RefTypeRemoteBranch + } else { + r.Name = absRef + if absRef == "HEAD" { + r.Type = RefTypeHEAD + } else { + r.Type = RefTypeOther + } + } + return r +} + // A git reference (branch, tag etc) type Ref struct { Name string diff --git a/git/git_test.go b/git/git_test.go index 0ec125a7..16fd895b 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -13,6 +13,32 @@ import ( "github.com/stretchr/testify/assert" ) +func TestParseRefs(t *testing.T) { + tests := map[string]RefType{ + "refs/heads": RefTypeLocalBranch, + "refs/tags": RefTypeLocalTag, + "refs/remotes/tags": RefTypeRemoteTag, + "refs/remotes": RefTypeRemoteBranch, + } + + for prefix, expectedType := range tests { + r := ParseRef(prefix+"/branch", "abc123") + assert.Equal(t, "abc123", r.Sha, "prefix: "+prefix) + assert.Equal(t, "branch", r.Name, "prefix: "+prefix) + assert.Equal(t, expectedType, r.Type, "prefix: "+prefix) + } + + r := ParseRef("refs/foo/branch", "abc123") + assert.Equal(t, "abc123", r.Sha, "prefix: refs/foo") + assert.Equal(t, "refs/foo/branch", r.Name, "prefix: refs/foo") + assert.Equal(t, RefTypeOther, r.Type, "prefix: refs/foo") + + r = ParseRef("HEAD", "abc123") + assert.Equal(t, "abc123", r.Sha, "prefix: HEAD") + assert.Equal(t, "HEAD", r.Name, "prefix: HEAD") + assert.Equal(t, RefTypeHEAD, r.Type, "prefix: HEAD") +} + func TestCurrentRefAndCurrentRemoteRef(t *testing.T) { repo := test.NewRepo(t) repo.Pushd() From 44e27fbc3eadc0df1cfe82a299de9ced9961c47a Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Wed, 1 Nov 2017 09:58:15 -0400 Subject: [PATCH 33/71] attribs: fix a comment typo --- git/attribs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/attribs.go b/git/attribs.go index 44419328..63a2379c 100644 --- a/git/attribs.go +++ b/git/attribs.go @@ -36,7 +36,7 @@ func (s *AttributeSource) String() string { // GetAttributePaths returns a list of entries in .gitattributes which are // configured with the filter=lfs attribute -// workingDIr is the root of the working copy +// workingDir is the root of the working copy // gitDir is the root of the git repo func GetAttributePaths(workingDir, gitDir string) []AttributePath { paths := make([]AttributePath, 0) From 1d4c8a6d7b1e11d749fc71eeeb6fffd1b93a010b Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Wed, 1 Nov 2017 09:58:18 -0400 Subject: [PATCH 34/71] attribs: trim whitespace and ignore comments The gitattributes(5) docs says that trailing and leading whitespace is ignored as well as line starting with `#`. See #2413. --- git/attribs.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git/attribs.go b/git/attribs.go index 63a2379c..067bf42d 100644 --- a/git/attribs.go +++ b/git/attribs.go @@ -56,7 +56,11 @@ func GetAttributePaths(workingDir, gitDir string) []AttributePath { scanner.Split(le.ScanLines) for scanner.Scan() { - line := scanner.Text() + line := strings.TrimSpace(scanner.Text()) + + if strings.HasPrefix(line, "#") { + continue + } // Check for filter=lfs (signifying that LFS is tracking // this file) or "lockable", which indicates that the From 30d5ce83e5f32a28eba4be5dc436d4f44ff7ae5f Mon Sep 17 00:00:00 2001 From: rick olson Date: Wed, 1 Nov 2017 17:35:43 -0600 Subject: [PATCH 35/71] commands: extract lockVerifier from uploadContext --- commands/command_pre_push.go | 25 ++-- commands/command_push.go | 2 +- commands/lockverifier.go | 232 +++++++++++++++++++++++++++++++++++ commands/uploader.go | 212 ++++++++------------------------ commands/uploader_test.go | 7 +- test/test-pre-push.sh | 6 +- test/test-push.sh | 9 +- 7 files changed, 300 insertions(+), 193 deletions(-) create mode 100644 commands/lockverifier.go diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 18214375..2ca034f5 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -5,6 +5,7 @@ import ( "os" "strings" + "github.com/git-lfs/git-lfs/git" "github.com/rubyist/tracerx" "github.com/spf13/cobra" ) @@ -69,13 +70,13 @@ func prePushCommand(cmd *cobra.Command, args []string) { tracerx.Printf("pre-push: %s", line) - left, _ := decodeRefs(line) - if left == prePushDeleteBranch { + left, right := decodeRefs(line) + if left.Sha == prePushDeleteBranch { continue } - if err := uploadLeftOrAll(gitscanner, ctx, left); err != nil { - Print("Error scanning for Git LFS files in %q", left) + if err := uploadLeftOrAll(gitscanner, ctx, left, right); err != nil { + Print("Error scanning for Git LFS files in %+v", left) ExitWithError(err) } } @@ -85,19 +86,15 @@ func prePushCommand(cmd *cobra.Command, args []string) { // decodeRefs pulls the sha1s out of the line read from the pre-push // hook's stdin. -func decodeRefs(input string) (string, string) { +func decodeRefs(input string) (*git.Ref, *git.Ref) { refs := strings.Split(strings.TrimSpace(input), " ") - var left, right string - - if len(refs) > 1 { - left = refs[1] + for len(refs) < 4 { + refs = append(refs, "") } - if len(refs) > 3 { - right = "^" + refs[3] - } - - return left, right + leftRef := git.ParseRef(refs[0], refs[1]) + rightRef := git.ParseRef(refs[2], refs[3]) + return leftRef, rightRef } func init() { diff --git a/commands/command_push.go b/commands/command_push.go index 511a3758..dce88bcf 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -35,7 +35,7 @@ func uploadsBetweenRefAndRemote(ctx *uploadContext, refnames []string) { } for _, ref := range refs { - if err = uploadLeftOrAll(gitscanner, ctx, ref.Name); err != nil { + if err = uploadLeftOrAll(gitscanner, ctx, ref, nil); err != nil { Print("Error scanning for Git LFS files in the %q ref", ref.Name) ExitWithError(err) } diff --git a/commands/lockverifier.go b/commands/lockverifier.go new file mode 100644 index 00000000..79b3f7b4 --- /dev/null +++ b/commands/lockverifier.go @@ -0,0 +1,232 @@ +package commands + +import ( + "fmt" + "sort" + "strconv" + "strings" + + "github.com/git-lfs/git-lfs/config" + "github.com/git-lfs/git-lfs/errors" + "github.com/git-lfs/git-lfs/lfsapi" + "github.com/git-lfs/git-lfs/locking" + "github.com/git-lfs/git-lfs/tq" +) + +type verifyState byte + +const ( + verifyStateUnknown verifyState = iota + verifyStateEnabled + verifyStateDisabled +) + +// lockVerifier verifies locked files before updating one or more refs. +type lockVerifier struct { + endpoint lfsapi.Endpoint + verifyState verifyState + verifiedRefs map[string]bool + + // all existing locks + ourLocks map[string]*refLock + theirLocks map[string]*refLock + + // locks from ourLocks that have been modified + ownedLocks []*refLock + + // locks from theirLocks that have been modified + unownedLocks []*refLock +} + +func (lv *lockVerifier) Verify(ref string) { + if lv.verifyState == verifyStateDisabled || lv.verifiedRefs[ref] { + return + } + + lockClient := newLockClient() + ours, theirs, err := lockClient.VerifiableLocks(ref, 0) + if err != nil { + if errors.IsNotImplementedError(err) { + disableFor(lv.endpoint.Url) + } else if lv.verifyState == verifyStateUnknown || lv.verifyState == verifyStateEnabled { + if errors.IsAuthError(err) { + if lv.verifyState == verifyStateUnknown { + Error("WARNING: Authentication error: %s", err) + } else if lv.verifyState == verifyStateEnabled { + Exit("ERROR: Authentication error: %s", err) + } + } else { + Print("Remote %q does not support the LFS locking API. Consider disabling it with:", cfg.Remote()) + Print(" $ git config lfs.%s.locksverify false", lv.endpoint.Url) + if lv.verifyState == verifyStateEnabled { + ExitWithError(err) + } + } + } + } else if lv.verifyState == verifyStateUnknown { + Print("Locking support detected on remote %q. Consider enabling it with:", cfg.Remote()) + Print(" $ git config lfs.%s.locksverify true", lv.endpoint.Url) + } + + lv.addLocks(ref, ours, lv.ourLocks) + lv.addLocks(ref, theirs, lv.theirLocks) + lv.verifiedRefs[ref] = true +} + +func (lv *lockVerifier) addLocks(ref string, locks []locking.Lock, set map[string]*refLock) { + for _, l := range locks { + if rl, ok := set[l.Path]; ok { + if err := rl.Add(ref, l); err != nil { + Error("WARNING: error adding %q lock for ref %q: %+v", l.Path, ref, err) + } + } else { + set[l.Path] = lv.newRefLocks(ref, l) + } + } +} + +// Determines if a filename is lockable. Implements lfs.GitScannerSet +func (lv *lockVerifier) Contains(name string) bool { + if lv == nil { + return false + } + _, ok := lv.theirLocks[name] + return ok +} + +func (lv *lockVerifier) LockedByThem(name string) bool { + if lock, ok := lv.theirLocks[name]; ok { + lv.unownedLocks = append(lv.unownedLocks, lock) + return true + } + return false +} + +func (lv *lockVerifier) LockedByUs(name string) bool { + if lock, ok := lv.ourLocks[name]; ok { + lv.ownedLocks = append(lv.ownedLocks, lock) + return true + } + return false +} + +func (lv *lockVerifier) UnownedLocks() []*refLock { + return lv.unownedLocks +} + +func (lv *lockVerifier) HasUnownedLocks() bool { + return len(lv.unownedLocks) > 0 +} + +func (lv *lockVerifier) OwnedLocks() []*refLock { + return lv.ownedLocks +} + +func (lv *lockVerifier) HasOwnedLocks() bool { + return len(lv.ownedLocks) > 0 +} + +func (lv *lockVerifier) Enabled() bool { + return lv.verifyState == verifyStateEnabled +} + +func (lv *lockVerifier) newRefLocks(ref string, l locking.Lock) *refLock { + return &refLock{ + allRefs: lv.verifiedRefs, + path: l.Path, + refs: map[string]locking.Lock{ref: l}, + } +} + +func newLockVerifier(m *tq.Manifest) *lockVerifier { + lv := &lockVerifier{ + endpoint: getAPIClient().Endpoints.Endpoint("upload", cfg.Remote()), + verifiedRefs: make(map[string]bool), + ourLocks: make(map[string]*refLock), + theirLocks: make(map[string]*refLock), + } + + // Do not check locks for standalone transfer, because there is no LFS + // server to ask. + if m.IsStandaloneTransfer() { + lv.verifyState = verifyStateDisabled + } else { + lv.verifyState = getVerifyStateFor(lv.endpoint.Url) + } + + return lv +} + +// refLock represents a unique locked file path, potentially across multiple +// refs. It tracks each individual lock in case different users locked the +// same path across multiple refs. +type refLock struct { + path string + allRefs map[string]bool + refs map[string]locking.Lock +} + +// Path returns the locked path. +func (r *refLock) Path() string { + return r.path +} + +// Owners returns the list of owners that locked this file, including what +// specific refs the files were locked in. If a user locked a file on all refs, +// don't bother listing them. +// +// Example: technoweenie, bob (refs: foo) +func (r *refLock) Owners() string { + users := make(map[string][]string, len(r.refs)) + for ref, lock := range r.refs { + u := lock.Owner.Name + if _, ok := users[u]; !ok { + users[u] = make([]string, 0, len(r.refs)) + } + users[u] = append(users[u], ref) + } + + owners := make([]string, 0, len(users)) + for name, refs := range users { + seenRefCount := 0 + for _, ref := range refs { + if r.allRefs[ref] { + seenRefCount++ + } + } + if seenRefCount == len(r.allRefs) { // lock is included in all refs, so don't list them + owners = append(owners, name) + continue + } + + sort.Strings(refs) + owners = append(owners, fmt.Sprintf("%s (refs: %s)", name, strings.Join(refs, ", "))) + } + sort.Strings(owners) + return strings.Join(owners, ", ") +} + +func (r *refLock) Add(ref string, l locking.Lock) error { + r.refs[ref] = l + return nil +} + +// getVerifyStateFor returns whether or not lock verification is enabled for the +// given url. If no state has been explicitly set, an "unknown" state will be +// returned instead. +func getVerifyStateFor(rawurl string) verifyState { + uc := config.NewURLConfig(cfg.Git) + + v, ok := uc.Get("lfs", rawurl, "locksverify") + if !ok { + if supportsLockingAPI(rawurl) { + return verifyStateEnabled + } + return verifyStateUnknown + } + + if enabled, _ := strconv.ParseBool(v); enabled { + return verifyStateEnabled + } + return verifyStateDisabled +} diff --git a/commands/uploader.go b/commands/uploader.go index 66770841..e275f027 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -5,28 +5,38 @@ import ( "net/url" "os" "path/filepath" - "strconv" "strings" "sync" - "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/errors" + "github.com/git-lfs/git-lfs/git" "github.com/git-lfs/git-lfs/lfs" - "github.com/git-lfs/git-lfs/lfsapi" - "github.com/git-lfs/git-lfs/locking" "github.com/git-lfs/git-lfs/progress" "github.com/git-lfs/git-lfs/tools" "github.com/git-lfs/git-lfs/tq" "github.com/rubyist/tracerx" ) -func uploadLeftOrAll(g *lfs.GitScanner, ctx *uploadContext, ref string) error { +func uploadLeftOrAll(g *lfs.GitScanner, ctx *uploadContext, left, right *git.Ref) error { + leftName := left.Name + if len(left.Sha) > 0 { + leftName = left.Sha + } + if pushAll { - if err := g.ScanRefWithDeleted(ref, nil); err != nil { + if err := g.ScanRefWithDeleted(leftName, nil); err != nil { return err } } else { - if err := g.ScanLeftToRemote(ref, nil); err != nil { + if right == nil { + if merge, ok := cfg.Git.Get(fmt.Sprintf("branch.%s.merge", left.Name)); ok { + right = git.ParseRef(merge, "") + } else { + right = &git.Ref{Name: left.Name} + } + } + tracerx.Printf("DEBUG LEFT to RIGHT: %+v => %+v", left, right) + if err := g.ScanLeftToRemote(leftName, nil); err != nil { return err } } @@ -46,18 +56,7 @@ type uploadContext struct { committerName string committerEmail string - trackedLocksMu *sync.Mutex - - // ALL verifiable locks - lockVerifyState verifyState - ourLocks map[string]locking.Lock - theirLocks map[string]locking.Lock - - // locks from ourLocks that were modified in this push - ownedLocks []locking.Lock - - // locks from theirLocks that were modified in this push - unownedLocks []locking.Lock + lockVerifier *lockVerifier // allowMissing specifies whether pushes containing missing/corrupt // pointers should allow pushing Git blobs @@ -68,100 +67,28 @@ type uploadContext struct { errMu sync.Mutex } -// Determines if a filename is lockable. Serves as a wrapper around theirLocks -// that implements GitScannerSet. -type gitScannerLockables struct { - m map[string]locking.Lock -} - -func (l *gitScannerLockables) Contains(name string) bool { - if l == nil { - return false - } - _, ok := l.m[name] - return ok -} - -type verifyState byte - -const ( - verifyStateUnknown verifyState = iota - verifyStateEnabled - verifyStateDisabled -) - func newUploadContext(dryRun bool) *uploadContext { + remote := cfg.Remote() + manifest := getTransferManifestOperationRemote("upload", remote) ctx := &uploadContext{ - Remote: cfg.Remote(), - DryRun: dryRun, - uploadedOids: tools.NewStringSet(), - gitfilter: lfs.NewGitFilter(cfg), - ourLocks: make(map[string]locking.Lock), - theirLocks: make(map[string]locking.Lock), - trackedLocksMu: new(sync.Mutex), - allowMissing: cfg.Git.Bool("lfs.allowincompletepush", true), + Remote: remote, + Manifest: manifest, + DryRun: dryRun, + uploadedOids: tools.NewStringSet(), + gitfilter: lfs.NewGitFilter(cfg), + lockVerifier: newLockVerifier(manifest), + allowMissing: cfg.Git.Bool("lfs.allowincompletepush", true), } - ctx.Manifest = getTransferManifestOperationRemote("upload", ctx.Remote) ctx.meter = buildProgressMeter(ctx.DryRun) ctx.tq = newUploadQueue(ctx.Manifest, ctx.Remote, tq.WithProgress(ctx.meter), tq.DryRun(ctx.DryRun)) ctx.committerName, ctx.committerEmail = cfg.CurrentCommitter() - // Do not check locks for standalone transfer, because there is no LFS - // server to ask. - if ctx.Manifest.IsStandaloneTransfer() { - ctx.lockVerifyState = verifyStateDisabled - return ctx - } - - ourLocks, theirLocks, verifyState := verifyLocks() - ctx.lockVerifyState = verifyState - for _, l := range theirLocks { - ctx.theirLocks[l.Path] = l - } - for _, l := range ourLocks { - ctx.ourLocks[l.Path] = l - } + ctx.lockVerifier.Verify(cfg.RemoteRefName()) return ctx } -func verifyLocks() (ours, theirs []locking.Lock, st verifyState) { - endpoint := getAPIClient().Endpoints.Endpoint("upload", cfg.Remote()) - state := getVerifyStateFor(endpoint) - if state == verifyStateDisabled { - return - } - - lockClient := newLockClient() - - ours, theirs, err := lockClient.VerifiableLocks(cfg.RemoteRefName(), 0) - if err != nil { - if errors.IsNotImplementedError(err) { - disableFor(endpoint) - } else if state == verifyStateUnknown || state == verifyStateEnabled { - if errors.IsAuthError(err) { - if state == verifyStateUnknown { - Error("WARNING: Authentication error: %s", err) - } else if state == verifyStateEnabled { - Exit("ERROR: Authentication error: %s", err) - } - } else { - Print("Remote %q does not support the LFS locking API. Consider disabling it with:", cfg.Remote()) - Print(" $ git config lfs.%s.locksverify false", endpoint.Url) - if state == verifyStateEnabled { - ExitWithError(err) - } - } - } - } else if state == verifyStateUnknown { - Print("Locking support detected on remote %q. Consider enabling it with:", cfg.Remote()) - Print(" $ git config lfs.%s.locksverify true", endpoint.Url) - } - - return ours, theirs, state -} - func (c *uploadContext) scannerError() error { c.errMu.Lock() defer c.errMu.Unlock() @@ -189,15 +116,8 @@ func (c *uploadContext) buildGitScanner() (*lfs.GitScanner, error) { } }) - gitscanner.FoundLockable = func(name string) { - if lock, ok := c.theirLocks[name]; ok { - c.trackedLocksMu.Lock() - c.unownedLocks = append(c.unownedLocks, lock) - c.trackedLocksMu.Unlock() - } - } - - gitscanner.PotentialLockables = &gitScannerLockables{m: c.theirLocks} + gitscanner.FoundLockable = func(n string) { c.lockVerifier.LockedByThem(n) } + gitscanner.PotentialLockables = c.lockVerifier return gitscanner, gitscanner.RemoteForPush(c.Remote) } @@ -237,11 +157,7 @@ func (c *uploadContext) prepareUpload(unfiltered ...*lfs.WrappedPointer) (*tq.Tr // current committer. var canUpload bool = true - if lock, ok := c.theirLocks[p.Name]; ok { - c.trackedLocksMu.Lock() - c.unownedLocks = append(c.unownedLocks, lock) - c.trackedLocksMu.Unlock() - + if c.lockVerifier.LockedByThem(p.Name) { // If the verification state is enabled, this failed // locks verification means that the push should fail. // @@ -250,14 +166,10 @@ func (c *uploadContext) prepareUpload(unfiltered ...*lfs.WrappedPointer) (*tq.Tr // // If the state is undefined, the verification error is // sent as a warning and the user can upload. - canUpload = c.lockVerifyState != verifyStateEnabled + canUpload = !c.lockVerifier.Enabled() } - if lock, ok := c.ourLocks[p.Name]; ok { - c.trackedLocksMu.Lock() - c.ownedLocks = append(c.ownedLocks, lock) - c.trackedLocksMu.Unlock() - } + c.lockVerifier.LockedByUs(p.Name) if canUpload { // estimate in meter early (even if it's not going into @@ -346,25 +258,23 @@ func (c *uploadContext) Await() { os.Exit(2) } - c.trackedLocksMu.Lock() - if ul := len(c.unownedLocks); ul > 0 { - Print("Unable to push %d locked file(s):", ul) - for _, unowned := range c.unownedLocks { - Print("* %s - %s", unowned.Path, unowned.Owner) + if c.lockVerifier.HasUnownedLocks() { + Print("Unable to push locked files:") + for _, unowned := range c.lockVerifier.UnownedLocks() { + Print("* %s - %s", unowned.Path(), unowned.Owners()) } - if c.lockVerifyState == verifyStateEnabled { + if c.lockVerifier.Enabled() { Exit("ERROR: Cannot update locked files.") } else { Error("WARNING: The above files would have halted this push.") } - } else if len(c.ownedLocks) > 0 { - Print("Consider unlocking your own locked file(s): (`git lfs unlock `)") - for _, owned := range c.ownedLocks { - Print("* %s", owned.Path) + } else if c.lockVerifier.HasOwnedLocks() { + Print("Consider unlocking your own locked files: (`git lfs unlock `)") + for _, owned := range c.lockVerifier.OwnedLocks() { + Print("* %s", owned.Path()) } } - c.trackedLocksMu.Unlock() } var ( @@ -438,33 +348,13 @@ func (c *uploadContext) ensureFile(smudgePath, cleanPath string) error { return nil } -// getVerifyStateFor returns whether or not lock verification is enabled for the -// given "endpoint". If no state has been explicitly set, an "unknown" state -// will be returned instead. -func getVerifyStateFor(endpoint lfsapi.Endpoint) verifyState { - uc := config.NewURLConfig(cfg.Git) - - v, ok := uc.Get("lfs", endpoint.Url, "locksverify") - if !ok { - if supportsLockingAPI(endpoint) { - return verifyStateEnabled - } - return verifyStateUnknown - } - - if enabled, _ := strconv.ParseBool(v); enabled { - return verifyStateEnabled - } - return verifyStateDisabled -} - -// supportsLockingAPI returns whether or not a given lfsapi.Endpoint "e" -// is known to support the LFS locking API by whether or not its hostname is -// included in the list above. -func supportsLockingAPI(e lfsapi.Endpoint) bool { - u, err := url.Parse(e.Url) +// supportsLockingAPI returns whether or not a given url is known to support +// the LFS locking API by whether or not its hostname is included in the list +// above. +func supportsLockingAPI(rawurl string) bool { + u, err := url.Parse(rawurl) if err != nil { - tracerx.Printf("commands: unable to parse %q to determine locking support: %v", e.Url, err) + tracerx.Printf("commands: unable to parse %q to determine locking support: %v", rawurl, err) return false } @@ -480,10 +370,10 @@ func supportsLockingAPI(e lfsapi.Endpoint) bool { // disableFor disables lock verification for the given lfsapi.Endpoint, // "endpoint". -func disableFor(endpoint lfsapi.Endpoint) error { - tracerx.Printf("commands: disabling lock verification for %q", endpoint.Url) +func disableFor(rawurl string) error { + tracerx.Printf("commands: disabling lock verification for %q", rawurl) - key := strings.Join([]string{"lfs", endpoint.Url, "locksverify"}, ".") + key := strings.Join([]string{"lfs", rawurl, "locksverify"}, ".") _, err := cfg.SetGitLocalKey(key, "false") return err diff --git a/commands/uploader_test.go b/commands/uploader_test.go index aae644e1..e5fd0637 100644 --- a/commands/uploader_test.go +++ b/commands/uploader_test.go @@ -3,7 +3,6 @@ package commands import ( "testing" - "github.com/git-lfs/git-lfs/lfsapi" "github.com/stretchr/testify/assert" ) @@ -13,11 +12,7 @@ type LockingSupportTestCase struct { } func (l *LockingSupportTestCase) Assert(t *testing.T) { - ep := lfsapi.Endpoint{ - Url: l.Given, - } - - assert.Equal(t, l.ExpectedToMatch, supportsLockingAPI(ep)) + assert.Equal(t, l.ExpectedToMatch, supportsLockingAPI(l.Given)) } func TestSupportedLockingHosts(t *testing.T) { diff --git a/test/test-pre-push.sh b/test/test-pre-push.sh index 2435b100..4d96c880 100755 --- a/test/test-pre-push.sh +++ b/test/test-pre-push.sh @@ -583,7 +583,7 @@ begin_test "pre-push with our lock" git commit -m "add unauthorized changes" GIT_CURL_VERBOSE=1 git push origin master 2>&1 | tee push.log - grep "Consider unlocking your own locked file(s)" push.log + grep "Consider unlocking your own locked files" push.log grep "* locked.dat" push.log assert_server_lock "$id" @@ -631,7 +631,7 @@ begin_test "pre-push with their lock on lfs file" exit 1 fi - grep "Unable to push 1 locked file(s)" push.log + grep "Unable to push locked files" push.log grep "* locked_theirs.dat - Git LFS Tests" push.log grep "ERROR: Cannot update locked files." push.log @@ -687,7 +687,7 @@ begin_test "pre-push with their lock on non-lfs lockable file" exit 1 fi - grep "Unable to push 2 locked file(s)" push.log + grep "Unable to push locked files" push.log grep "* large_locked_theirs.dat - Git LFS Tests" push.log grep "* tiny_locked_theirs.dat - Git LFS Tests" push.log grep "ERROR: Cannot update locked files." push.log diff --git a/test/test-push.sh b/test/test-push.sh index bbf58f1b..015a6e09 100755 --- a/test/test-push.sh +++ b/test/test-push.sh @@ -485,15 +485,8 @@ begin_test "push ambiguous branch name" # lfs push master, should work git lfs push origin master - # push ambiguous, should fail - set +e + # push ambiguous, does not fail since lfs scans git with sha, not ref name git lfs push origin ambiguous - if [ $? -eq 0 ] - then - exit 1 - fi - set -e - ) end_test From 44832addfe40f82dc3b1051d783bcf4966d6ccbc Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 12:33:57 -0600 Subject: [PATCH 36/71] commands: extract refUpdate type --- commands/command_pre_push.go | 34 ++++++++--- commands/command_push.go | 109 +++++++++++++++++++---------------- commands/refs.go | 53 +++++++++++++++++ commands/uploader.go | 21 ++----- 4 files changed, 140 insertions(+), 77 deletions(-) create mode 100644 commands/refs.go diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 2ca034f5..5971a4b1 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -2,6 +2,7 @@ package commands import ( "bufio" + "io" "os" "strings" @@ -50,20 +51,38 @@ func prePushCommand(cmd *cobra.Command, args []string) { Exit("Invalid remote name %q: %s", args[0], err) } + updates := prePushRefs(os.Stdin) ctx := newUploadContext(prePushDryRun) - gitscanner, err := ctx.buildGitScanner() if err != nil { ExitWithError(err) } defer gitscanner.Close() - // We can be passed multiple lines of refs - scanner := bufio.NewScanner(os.Stdin) + for _, update := range updates { + if err := uploadLeftOrAll(gitscanner, ctx, update); err != nil { + Print("Error scanning for Git LFS files in %+v", update.Left()) + ExitWithError(err) + } + } + ctx.Await() +} + +// prePushRefs parses commit information that the pre-push git hook receives: +// +// +// +// Each line describes a proposed update of the remote ref at the remote sha to +// the local sha. Multiple updates can be received on multiple lines (such as +// from 'git push --all'). These updates are typically received over STDIN. +func prePushRefs(r io.Reader) []*refUpdate { + scanner := bufio.NewScanner(r) + refs := make([]*refUpdate, 0, 1) + + // We can be passed multiple lines of refs for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) - if len(line) == 0 { continue } @@ -75,13 +94,10 @@ func prePushCommand(cmd *cobra.Command, args []string) { continue } - if err := uploadLeftOrAll(gitscanner, ctx, left, right); err != nil { - Print("Error scanning for Git LFS files in %+v", left) - ExitWithError(err) - } + refs = append(refs, newRefUpdate(cfg.Git, left, right)) } - ctx.Await() + return refs } // decodeRefs pulls the sha1s out of the line read from the pre-push diff --git a/commands/command_push.go b/commands/command_push.go index dce88bcf..c3f71fde 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -19,6 +19,46 @@ var ( // shares some global vars and functions with command_pre_push.go ) +// pushCommand pushes local objects to a Git LFS server. It takes two +// arguments: +// +// ` ` +// +// Remote must be a remote name, not a URL +// +// pushCommand calculates the git objects to send by comparing the range +// of commits between the local and remote git servers. +func pushCommand(cmd *cobra.Command, args []string) { + if len(args) == 0 { + Print("Specify a remote and a remote branch name (`git lfs push origin master`)") + os.Exit(1) + } + + requireGitVersion() + + // Remote is first arg + if err := cfg.SetValidRemote(args[0]); err != nil { + Exit("Invalid remote name %q: %s", args[0], err) + } + + ctx := newUploadContext(pushDryRun) + if pushObjectIDs { + if len(args) < 2 { + Print("Usage: git lfs push --object-id [lfs-object-id] ...") + return + } + + uploadsWithObjectIDs(ctx, args[1:]) + } else { + if len(args) < 1 { + Print("Usage: git lfs push --dry-run [ref]") + return + } + + uploadsBetweenRefAndRemote(ctx, args[1:]) + } +} + func uploadsBetweenRefAndRemote(ctx *uploadContext, refnames []string) { tracerx.Printf("Upload refs %v to remote %v", refnames, ctx.Remote) @@ -28,15 +68,15 @@ func uploadsBetweenRefAndRemote(ctx *uploadContext, refnames []string) { } defer gitscanner.Close() - refs, err := refsByNames(refnames) + updates, err := lfsPushRefs(refnames, pushAll) if err != nil { Error(err.Error()) Exit("Error getting local refs.") } - for _, ref := range refs { - if err = uploadLeftOrAll(gitscanner, ctx, ref, nil); err != nil { - Print("Error scanning for Git LFS files in the %q ref", ref.Name) + for _, update := range updates { + if err = uploadLeftOrAll(gitscanner, ctx, update); err != nil { + Print("Error scanning for Git LFS files in the %q ref", update.Left().Name) ExitWithError(err) } } @@ -68,14 +108,21 @@ func uploadsWithObjectIDs(ctx *uploadContext, oids []string) { ctx.Await() } -func refsByNames(refnames []string) ([]*git.Ref, error) { +// lfsPushRefs returns valid ref updates from the given ref and --all arguments. +// Either one or more refs can be explicitly specified, or --all indicates all +// local refs are pushed. +func lfsPushRefs(refnames []string, pushAll bool) ([]*refUpdate, error) { localrefs, err := git.LocalRefs() if err != nil { return nil, err } if pushAll && len(refnames) == 0 { - return localrefs, nil + refs := make([]*refUpdate, len(localrefs)) + for i, lr := range localrefs { + refs[i] = newRefUpdate(cfg.Git, lr, nil) + } + return refs, nil } reflookup := make(map[string]*git.Ref, len(localrefs)) @@ -83,59 +130,19 @@ func refsByNames(refnames []string) ([]*git.Ref, error) { reflookup[ref.Name] = ref } - refs := make([]*git.Ref, len(refnames)) + refs := make([]*refUpdate, len(refnames)) for i, name := range refnames { - if ref, ok := reflookup[name]; ok { - refs[i] = ref + if left, ok := reflookup[name]; ok { + refs[i] = newRefUpdate(cfg.Git, left, nil) } else { - refs[i] = &git.Ref{Name: name, Type: git.RefTypeOther, Sha: name} + left := &git.Ref{Name: name, Type: git.RefTypeOther, Sha: name} + refs[i] = newRefUpdate(cfg.Git, left, nil) } } return refs, nil } -// pushCommand pushes local objects to a Git LFS server. It takes two -// arguments: -// -// ` ` -// -// Remote must be a remote name, not a URL -// -// pushCommand calculates the git objects to send by comparing the range -// of commits between the local and remote git servers. -func pushCommand(cmd *cobra.Command, args []string) { - if len(args) == 0 { - Print("Specify a remote and a remote branch name (`git lfs push origin master`)") - os.Exit(1) - } - - requireGitVersion() - - // Remote is first arg - if err := cfg.SetValidRemote(args[0]); err != nil { - Exit("Invalid remote name %q: %s", args[0], err) - } - - ctx := newUploadContext(pushDryRun) - - if pushObjectIDs { - if len(args) < 2 { - Print("Usage: git lfs push --object-id [lfs-object-id] ...") - return - } - - uploadsWithObjectIDs(ctx, args[1:]) - } else { - if len(args) < 1 { - Print("Usage: git lfs push --dry-run [ref]") - return - } - - uploadsBetweenRefAndRemote(ctx, args[1:]) - } -} - func init() { RegisterCommand("push", pushCommand, func(cmd *cobra.Command) { cmd.Flags().BoolVarP(&pushDryRun, "dry-run", "d", false, "Do everything except actually send the updates") diff --git a/commands/refs.go b/commands/refs.go new file mode 100644 index 00000000..9c46c8c6 --- /dev/null +++ b/commands/refs.go @@ -0,0 +1,53 @@ +package commands + +import ( + "fmt" + + "github.com/git-lfs/git-lfs/config" + "github.com/git-lfs/git-lfs/git" +) + +type refUpdate struct { + git config.Environment + left *git.Ref + right *git.Ref +} + +func newRefUpdate(g config.Environment, l, r *git.Ref) *refUpdate { + return &refUpdate{ + git: g, + left: l, + right: r, + } +} + +func (u *refUpdate) Left() *git.Ref { + return u.left +} + +func (u *refUpdate) LeftCommitish() string { + return refCommitish(u.Left()) +} + +func (u *refUpdate) Right() *git.Ref { + if u.right == nil { + l := u.Left() + if merge, ok := u.git.Get(fmt.Sprintf("branch.%s.merge", l.Name)); ok { + u.right = git.ParseRef(merge, "") + } else { + u.right = &git.Ref{Name: l.Name} + } + } + return u.right +} + +func (u *refUpdate) RightCommitish() string { + return refCommitish(u.Right()) +} + +func refCommitish(r *git.Ref) string { + if len(r.Sha) > 0 { + return r.Sha + } + return r.Name +} diff --git a/commands/uploader.go b/commands/uploader.go index e275f027..a7616f28 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -9,7 +9,6 @@ import ( "sync" "github.com/git-lfs/git-lfs/errors" - "github.com/git-lfs/git-lfs/git" "github.com/git-lfs/git-lfs/lfs" "github.com/git-lfs/git-lfs/progress" "github.com/git-lfs/git-lfs/tools" @@ -17,26 +16,14 @@ import ( "github.com/rubyist/tracerx" ) -func uploadLeftOrAll(g *lfs.GitScanner, ctx *uploadContext, left, right *git.Ref) error { - leftName := left.Name - if len(left.Sha) > 0 { - leftName = left.Sha - } - +func uploadLeftOrAll(g *lfs.GitScanner, ctx *uploadContext, update *refUpdate) error { if pushAll { - if err := g.ScanRefWithDeleted(leftName, nil); err != nil { + if err := g.ScanRefWithDeleted(update.LeftCommitish(), nil); err != nil { return err } } else { - if right == nil { - if merge, ok := cfg.Git.Get(fmt.Sprintf("branch.%s.merge", left.Name)); ok { - right = git.ParseRef(merge, "") - } else { - right = &git.Ref{Name: left.Name} - } - } - tracerx.Printf("DEBUG LEFT to RIGHT: %+v => %+v", left, right) - if err := g.ScanLeftToRemote(leftName, nil); err != nil { + tracerx.Printf("DEBUG LEFT to RIGHT: %+v => %+v", update.Left(), update.Right()) + if err := g.ScanLeftToRemote(update.LeftCommitish(), nil); err != nil { return err } } From 8a1b0fb9976e7c1c2db4cd6415cb3e6c236c5937 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 12:55:44 -0600 Subject: [PATCH 37/71] commands: move lock verification for refs --- commands/command_pre_push.go | 4 +++- commands/command_push.go | 2 ++ commands/lockverifier.go | 11 +++++++++++ commands/uploader.go | 3 --- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 5971a4b1..6674a3ae 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -51,7 +51,6 @@ func prePushCommand(cmd *cobra.Command, args []string) { Exit("Invalid remote name %q: %s", args[0], err) } - updates := prePushRefs(os.Stdin) ctx := newUploadContext(prePushDryRun) gitscanner, err := ctx.buildGitScanner() if err != nil { @@ -59,6 +58,9 @@ func prePushCommand(cmd *cobra.Command, args []string) { } defer gitscanner.Close() + updates := prePushRefs(os.Stdin) + verifyLocksForUpdates(ctx.lockVerifier, updates) + for _, update := range updates { if err := uploadLeftOrAll(gitscanner, ctx, update); err != nil { Print("Error scanning for Git LFS files in %+v", update.Left()) diff --git a/commands/command_push.go b/commands/command_push.go index c3f71fde..08028bbf 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -74,6 +74,8 @@ func uploadsBetweenRefAndRemote(ctx *uploadContext, refnames []string) { Exit("Error getting local refs.") } + verifyLocksForUpdates(ctx.lockVerifier, updates) + for _, update := range updates { if err = uploadLeftOrAll(gitscanner, ctx, update); err != nil { Print("Error scanning for Git LFS files in the %q ref", update.Left().Name) diff --git a/commands/lockverifier.go b/commands/lockverifier.go index 79b3f7b4..ea7be73b 100644 --- a/commands/lockverifier.go +++ b/commands/lockverifier.go @@ -11,6 +11,7 @@ import ( "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/locking" "github.com/git-lfs/git-lfs/tq" + "github.com/rubyist/tracerx" ) type verifyState byte @@ -21,6 +22,15 @@ const ( verifyStateDisabled ) +func verifyLocksForUpdates(lv *lockVerifier, updates []*refUpdate) { + lv.Verify(cfg.RemoteRefName()) + /* + for _, update := range updates { + lv.Verify(update.Right().Name) + } + // */ +} + // lockVerifier verifies locked files before updating one or more refs. type lockVerifier struct { endpoint lfsapi.Endpoint @@ -43,6 +53,7 @@ func (lv *lockVerifier) Verify(ref string) { return } + tracerx.Printf("LOCK VERIFY %q", ref) lockClient := newLockClient() ours, theirs, err := lockClient.VerifiableLocks(ref, 0) if err != nil { diff --git a/commands/uploader.go b/commands/uploader.go index a7616f28..ea55240d 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -70,9 +70,6 @@ func newUploadContext(dryRun bool) *uploadContext { ctx.meter = buildProgressMeter(ctx.DryRun) ctx.tq = newUploadQueue(ctx.Manifest, ctx.Remote, tq.WithProgress(ctx.meter), tq.DryRun(ctx.DryRun)) ctx.committerName, ctx.committerEmail = cfg.CurrentCommitter() - - ctx.lockVerifier.Verify(cfg.RemoteRefName()) - return ctx } From 645998d262cd821c6374b23c7a3de70eceda0945 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 13:30:31 -0600 Subject: [PATCH 38/71] commands: support more push.default options --- commands/command_pre_push.go | 2 +- commands/command_push.go | 6 +-- commands/refs.go | 64 +++++++++++++++++++++++++------- commands/refs_test.go | 72 ++++++++++++++++++++++++++++++++++++ test/test-pre-push.sh | 3 +- 5 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 commands/refs_test.go diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 6674a3ae..f0a61e54 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -96,7 +96,7 @@ func prePushRefs(r io.Reader) []*refUpdate { continue } - refs = append(refs, newRefUpdate(cfg.Git, left, right)) + refs = append(refs, newRefUpdate(cfg.Git, cfg.Remote(), left, right)) } return refs diff --git a/commands/command_push.go b/commands/command_push.go index 08028bbf..a3986201 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -122,7 +122,7 @@ func lfsPushRefs(refnames []string, pushAll bool) ([]*refUpdate, error) { if pushAll && len(refnames) == 0 { refs := make([]*refUpdate, len(localrefs)) for i, lr := range localrefs { - refs[i] = newRefUpdate(cfg.Git, lr, nil) + refs[i] = newRefUpdate(cfg.Git, cfg.Remote(), lr, nil) } return refs, nil } @@ -135,10 +135,10 @@ func lfsPushRefs(refnames []string, pushAll bool) ([]*refUpdate, error) { refs := make([]*refUpdate, len(refnames)) for i, name := range refnames { if left, ok := reflookup[name]; ok { - refs[i] = newRefUpdate(cfg.Git, left, nil) + refs[i] = newRefUpdate(cfg.Git, cfg.Remote(), left, nil) } else { left := &git.Ref{Name: name, Type: git.RefTypeOther, Sha: name} - refs[i] = newRefUpdate(cfg.Git, left, nil) + refs[i] = newRefUpdate(cfg.Git, cfg.Remote(), left, nil) } } diff --git a/commands/refs.go b/commands/refs.go index 9c46c8c6..988ef899 100644 --- a/commands/refs.go +++ b/commands/refs.go @@ -5,19 +5,22 @@ import ( "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/git" + "github.com/rubyist/tracerx" ) type refUpdate struct { - git config.Environment - left *git.Ref - right *git.Ref + git config.Environment + remote string + left *git.Ref + right *git.Ref } -func newRefUpdate(g config.Environment, l, r *git.Ref) *refUpdate { +func newRefUpdate(g config.Environment, remote string, l, r *git.Ref) *refUpdate { return &refUpdate{ - git: g, - left: l, - right: r, + git: g, + remote: remote, + left: l, + right: r, } } @@ -31,16 +34,51 @@ func (u *refUpdate) LeftCommitish() string { func (u *refUpdate) Right() *git.Ref { if u.right == nil { - l := u.Left() - if merge, ok := u.git.Get(fmt.Sprintf("branch.%s.merge", l.Name)); ok { - u.right = git.ParseRef(merge, "") - } else { - u.right = &git.Ref{Name: l.Name} - } + u.right = defaultRemoteRef(u.git, u.remote, u.Left()) } return u.right } +// defaultRemoteRef returns the remote ref receiving a push based on the current +// repository config and local ref being pushed. +// +// See push.default rules in https://git-scm.com/docs/git-config +func defaultRemoteRef(g config.Environment, remote string, left *git.Ref) *git.Ref { + pushMode, _ := g.Get("push.default") + switch pushMode { + case "", "simple": + brRemote, _ := g.Get(fmt.Sprintf("branch.%s.remote", left.Name)) + if brRemote == remote { + // in centralized workflow, work like 'upstream' with an added safety to + // refuse to push if the upstream branch’s name is different from the + // local one. + return trackingRef(g, left) + } + + // When pushing to a remote that is different from the remote you normally + // pull from, work as current. + return &git.Ref{Name: left.Name} + case "upstream", "tracking": + // push the current branch back to the branch whose changes are usually + // integrated into the current branch + return trackingRef(g, left) + case "current": + // push the current branch to update a branch with the same name on the + // receiving end. + return &git.Ref{Name: left.Name} + default: + tracerx.Printf("WARNING: %q push mode not supported", pushMode) + return &git.Ref{Name: left.Name} + } +} + +func trackingRef(g config.Environment, left *git.Ref) *git.Ref { + if merge, ok := g.Get(fmt.Sprintf("branch.%s.merge", left.Name)); ok { + return git.ParseRef(merge, "") + } + return &git.Ref{Name: left.Name} +} + func (u *refUpdate) RightCommitish() string { return refCommitish(u.Right()) } diff --git a/commands/refs_test.go b/commands/refs_test.go new file mode 100644 index 00000000..e25fb9c4 --- /dev/null +++ b/commands/refs_test.go @@ -0,0 +1,72 @@ +package commands + +import ( + "testing" + + "github.com/git-lfs/git-lfs/config" + "github.com/git-lfs/git-lfs/git" + "github.com/stretchr/testify/assert" +) + +func TestRefUpdateDefault(t *testing.T) { + pushModes := []string{"simple", ""} + for _, pushMode := range pushModes { + cfg := config.NewFrom(config.Values{ + Git: map[string][]string{ + "push.default": []string{pushMode}, + "branch.left.remote": []string{"ignore"}, + "branch.left.merge": []string{"me"}, + }, + }) + + u := newRefUpdate(cfg.Git, "origin", git.ParseRef("left", ""), nil) + assert.Equal(t, "left", u.Right().Name, "pushmode=%q", pushMode) + } +} + +func TestRefUpdateTrackedDefault(t *testing.T) { + pushModes := []string{"simple", "upstream", "tracking", ""} + for _, pushMode := range pushModes { + cfg := config.NewFrom(config.Values{ + Git: map[string][]string{ + "push.default": []string{pushMode}, + "branch.left.remote": []string{"origin"}, + "branch.left.merge": []string{"tracked"}, + }, + }) + + u := newRefUpdate(cfg.Git, "origin", git.ParseRef("left", ""), nil) + assert.Equal(t, "tracked", u.Right().Name, "pushmode=%s", pushMode) + } +} + +func TestRefUpdateCurrentDefault(t *testing.T) { + cfg := config.NewFrom(config.Values{ + Git: map[string][]string{ + "push.default": []string{"current"}, + "branch.left.remote": []string{"origin"}, + "branch.left.merge": []string{"tracked"}, + }, + }) + + u := newRefUpdate(cfg.Git, "origin", git.ParseRef("left", ""), nil) + assert.Equal(t, "left", u.Right().Name) +} + +func TestRefUpdateExplicitLeftAndRight(t *testing.T) { + u := newRefUpdate(nil, "", git.ParseRef("left", "abc123"), git.ParseRef("right", "def456")) + assert.Equal(t, "left", u.Left().Name) + assert.Equal(t, "abc123", u.Left().Sha) + assert.Equal(t, "abc123", u.LeftCommitish()) + assert.Equal(t, "right", u.Right().Name) + assert.Equal(t, "def456", u.Right().Sha) + assert.Equal(t, "def456", u.RightCommitish()) + + u = newRefUpdate(nil, "", git.ParseRef("left", ""), git.ParseRef("right", "")) + assert.Equal(t, "left", u.Left().Name) + assert.Equal(t, "", u.Left().Sha) + assert.Equal(t, "left", u.LeftCommitish()) + assert.Equal(t, "right", u.Right().Name) + assert.Equal(t, "", u.Right().Sha) + assert.Equal(t, "right", u.RightCommitish()) +} diff --git a/test/test-pre-push.sh b/test/test-pre-push.sh index 4d96c880..f2ff8cca 100755 --- a/test/test-pre-push.sh +++ b/test/test-pre-push.sh @@ -814,9 +814,10 @@ begin_test "pre-push locks verify 403 with good tracked ref" git add .gitattributes a.dat git commit --message "initial commit" + git config push.default upstream git config branch.master.merge refs/heads/tracked git config "lfs.$GITSERVER/$reponame.git.locksverify" true - git push origin master 2>&1 | tee push.log + git push 2>&1 | tee push.log assert_server_object "$reponame" "$contents_oid" ) From 9064b637c2a4557f5498b25e950ead7f9a51d616 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 14:11:45 -0600 Subject: [PATCH 39/71] commands: verify locks on all pushed refs --- commands/lockverifier.go | 11 +++-------- config/config.go | 29 ----------------------------- 2 files changed, 3 insertions(+), 37 deletions(-) diff --git a/commands/lockverifier.go b/commands/lockverifier.go index ea7be73b..062f7fb3 100644 --- a/commands/lockverifier.go +++ b/commands/lockverifier.go @@ -11,7 +11,6 @@ import ( "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/locking" "github.com/git-lfs/git-lfs/tq" - "github.com/rubyist/tracerx" ) type verifyState byte @@ -23,12 +22,9 @@ const ( ) func verifyLocksForUpdates(lv *lockVerifier, updates []*refUpdate) { - lv.Verify(cfg.RemoteRefName()) - /* - for _, update := range updates { - lv.Verify(update.Right().Name) - } - // */ + for _, update := range updates { + lv.Verify(update.Right().Name) + } } // lockVerifier verifies locked files before updating one or more refs. @@ -53,7 +49,6 @@ func (lv *lockVerifier) Verify(ref string) { return } - tracerx.Printf("LOCK VERIFY %q", ref) lockClient := newLockClient() ours, theirs, err := lockClient.VerifiableLocks(ref, 0) if err != nil { diff --git a/config/config.go b/config/config.go index 2ce9a16b..96df6995 100644 --- a/config/config.go +++ b/config/config.go @@ -166,35 +166,6 @@ func (c *Configuration) CurrentRef() *git.Ref { return c.ref } -func (c *Configuration) RemoteRef() *git.Ref { - r := c.CurrentRef() - - c.loading.Lock() - defer c.loading.Unlock() - - if c.remoteRef != nil { - return c.remoteRef - } - - if r != nil { - merge, _ := c.Git.Get(fmt.Sprintf("branch.%s.merge", r.Name)) - if len(merge) > 0 { - c.remoteRef = git.ParseRef(merge, "") - } else { - c.remoteRef = r - } - } - - return c.remoteRef -} - -func (c *Configuration) RemoteRefName() string { - if r := c.RemoteRef(); r != nil { - return r.Name - } - return "" -} - func (c *Configuration) IsDefaultRemote() bool { return c.Remote() == defaultRemote } From fd7694727a71dfd014dd462856db760d18117185 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 14:21:43 -0600 Subject: [PATCH 40/71] test: test verify locks on all pushed refs --- test/test-pre-push.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/test-pre-push.sh b/test/test-pre-push.sh index f2ff8cca..eb18e563 100755 --- a/test/test-pre-push.sh +++ b/test/test-pre-push.sh @@ -823,6 +823,28 @@ begin_test "pre-push locks verify 403 with good tracked ref" ) end_test +begin_test "pre-push locks verify 403 with explicit ref" +( + set -e + + reponame="lock-verify-explicit-ref-required" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + contents="example" + contents_oid="$(calc_oid "$contents")" + printf "$contents" > a.dat + git lfs track "*.dat" + git add .gitattributes a.dat + git commit --message "initial commit" + + git config "lfs.$GITSERVER/$reponame.git.locksverify" true + git push origin master:explicit 2>&1 | tee push.log + + assert_server_object "$reponame" "$contents_oid" +) +end_test + begin_test "pre-push locks verify 403 with bad ref" ( set -e From 62f62d7f9d9eac1625b11b0afaa6335ab8523f27 Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 16:13:59 -0600 Subject: [PATCH 41/71] locking: turn Ref property into a struct to allow for future additions --- locking/api.go | 6 +++++- locking/locks.go | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/locking/api.go b/locking/api.go index 2d6e13e1..1ba55120 100644 --- a/locking/api.go +++ b/locking/api.go @@ -12,6 +12,10 @@ type lockClient struct { *lfsapi.Client } +type lockRef struct { + Name string `json:"name,omitempty"` +} + // LockRequest encapsulates the payload sent across the API when a client would // like to obtain a lock against a particular path on a given remote. type lockRequest struct { @@ -192,7 +196,7 @@ func (c *lockClient) Search(remote string, searchReq *lockSearchRequest) (*lockL // lockVerifiableRequest encapsulates the request sent to the server when the // client would like a list of locks to verify a Git push. type lockVerifiableRequest struct { - Ref string `json:"ref"` + Ref *lockRef `json:"ref"` // Cursor is an optional field used to tell the server which lock was // seen last, if scanning through multiple pages of results. diff --git a/locking/locks.go b/locking/locks.go index 293dd1bb..9f4377b1 100644 --- a/locking/locks.go +++ b/locking/locks.go @@ -210,7 +210,7 @@ func (c *Client) VerifiableLocks(ref string, limit int) (ourLocks, theirLocks [] ourLocks = make([]Lock, 0, limit) theirLocks = make([]Lock, 0, limit) body := &lockVerifiableRequest{ - Ref: ref, + Ref: &lockRef{Name: ref}, Limit: limit, } From 4afdac0065e22728b61799dbc842faabe545b47e Mon Sep 17 00:00:00 2001 From: rick olson Date: Thu, 2 Nov 2017 17:04:25 -0600 Subject: [PATCH 42/71] test: update test api to receive ref object in locks/verify request --- test/cmd/lfstest-gitserver.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/test/cmd/lfstest-gitserver.go b/test/cmd/lfstest-gitserver.go index ab440d15..334a0611 100644 --- a/test/cmd/lfstest-gitserver.go +++ b/test/cmd/lfstest-gitserver.go @@ -881,12 +881,23 @@ type LockList struct { Message string `json:"message,omitempty"` } +type Ref struct { + Name string `json:"name,omitempty"` +} + type VerifiableLockRequest struct { - Ref string `json:"ref,omitempty"` + Ref *Ref `json:"ref,omitempty"` Cursor string `json:"cursor,omitempty"` Limit int `json:"limit,omitempty"` } +func (r *VerifiableLockRequest) RefName() string { + if r.Ref == nil { + return "" + } + return r.Ref.Name +} + type VerifiableLockList struct { Ours []Lock `json:"ours"` Theirs []Lock `json:"theirs"` @@ -1093,11 +1104,11 @@ func locksHandler(w http.ResponseWriter, r *http.Request, repo string) { if strings.HasSuffix(repo, "ref-required") { parts := strings.Split(repo, "-") lenParts := len(parts) - if lenParts > 3 && parts[lenParts-3] != reqBody.Ref { + if lenParts > 3 && parts[lenParts-3] != reqBody.RefName() { w.WriteHeader(403) enc.Encode(struct { Message string `json:"message"` - }{fmt.Sprintf("Expected ref %q, got %q", parts[lenParts-3], reqBody.Ref)}) + }{fmt.Sprintf("Expected ref %q, got %q", parts[lenParts-3], reqBody.RefName())}) return } } From 02fcf099d333fdb5bb98478b2594d2b1b6741b2b Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 3 Nov 2017 10:08:08 -0600 Subject: [PATCH 43/71] commands: changes to make pre-push and push more similar --- commands/command_pre_push.go | 17 ++--------------- commands/command_push.go | 25 +++++++------------------ commands/uploader.go | 21 +++++++++++++++++++-- 3 files changed, 28 insertions(+), 35 deletions(-) diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index f0a61e54..c25e6643 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -52,23 +52,10 @@ func prePushCommand(cmd *cobra.Command, args []string) { } ctx := newUploadContext(prePushDryRun) - gitscanner, err := ctx.buildGitScanner() - if err != nil { + updates := prePushRefs(os.Stdin) + if err := uploadForRefUpdates(ctx, updates, false); err != nil { ExitWithError(err) } - defer gitscanner.Close() - - updates := prePushRefs(os.Stdin) - verifyLocksForUpdates(ctx.lockVerifier, updates) - - for _, update := range updates { - if err := uploadLeftOrAll(gitscanner, ctx, update); err != nil { - Print("Error scanning for Git LFS files in %+v", update.Left()) - ExitWithError(err) - } - } - - ctx.Await() } // prePushRefs parses commit information that the pre-push git hook receives: diff --git a/commands/command_push.go b/commands/command_push.go index a3986201..8a93df67 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -62,32 +62,20 @@ func pushCommand(cmd *cobra.Command, args []string) { func uploadsBetweenRefAndRemote(ctx *uploadContext, refnames []string) { tracerx.Printf("Upload refs %v to remote %v", refnames, ctx.Remote) - gitscanner, err := ctx.buildGitScanner() - if err != nil { - ExitWithError(err) - } - defer gitscanner.Close() - updates, err := lfsPushRefs(refnames, pushAll) if err != nil { Error(err.Error()) Exit("Error getting local refs.") } - verifyLocksForUpdates(ctx.lockVerifier, updates) - - for _, update := range updates { - if err = uploadLeftOrAll(gitscanner, ctx, update); err != nil { - Print("Error scanning for Git LFS files in the %q ref", update.Left().Name) - ExitWithError(err) - } + if err := uploadForRefUpdates(ctx, updates, pushAll); err != nil { + ExitWithError(err) } - - ctx.Await() } func uploadsWithObjectIDs(ctx *uploadContext, oids []string) { - for _, oid := range oids { + pointers := make([]*lfs.WrappedPointer, len(oids)) + for i, oid := range oids { mp, err := ctx.gitfilter.ObjectPath(oid) if err != nil { ExitWithError(errors.Wrap(err, "Unable to find local media path:")) @@ -98,15 +86,16 @@ func uploadsWithObjectIDs(ctx *uploadContext, oids []string) { ExitWithError(errors.Wrap(err, "Unable to stat local media path")) } - uploadPointers(ctx, &lfs.WrappedPointer{ + pointers[i] = &lfs.WrappedPointer{ Name: mp, Pointer: &lfs.Pointer{ Oid: oid, Size: stat.Size(), }, - }) + } } + uploadPointers(ctx, pointers...) ctx.Await() } diff --git a/commands/uploader.go b/commands/uploader.go index ea55240d..8342d3dd 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -16,13 +16,30 @@ import ( "github.com/rubyist/tracerx" ) -func uploadLeftOrAll(g *lfs.GitScanner, ctx *uploadContext, update *refUpdate) error { +func uploadForRefUpdates(ctx *uploadContext, updates []*refUpdate, pushAll bool) error { + gitscanner, err := ctx.buildGitScanner() + if err != nil { + return err + } + defer gitscanner.Close() + + verifyLocksForUpdates(ctx.lockVerifier, updates) + for _, update := range updates { + if err := uploadLeftOrAll(gitscanner, ctx, update, pushAll); err != nil { + return errors.Wrap(err, fmt.Sprintf("ref %s:", update.Left().Name)) + } + } + + ctx.Await() + return nil +} + +func uploadLeftOrAll(g *lfs.GitScanner, ctx *uploadContext, update *refUpdate, pushAll bool) error { if pushAll { if err := g.ScanRefWithDeleted(update.LeftCommitish(), nil); err != nil { return err } } else { - tracerx.Printf("DEBUG LEFT to RIGHT: %+v => %+v", update.Left(), update.Right()) if err := g.ScanLeftToRemote(update.LeftCommitish(), nil); err != nil { return err } From c9311e62c158ccfb6a8507a600faf78330bfe184 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 3 Nov 2017 10:36:12 -0600 Subject: [PATCH 44/71] config: add PushRemote() for checking branch.*.pushRemote and remote.pushDefault first --- commands/command_pre_push.go | 2 +- commands/command_push.go | 6 ++-- commands/commands.go | 2 +- commands/lockverifier.go | 6 ++-- commands/uploader.go | 2 +- config/config.go | 23 ++++++++++++++++ config/config_test.go | 53 ++++++++++++++++++++++++++++++++++++ lfs/lfs.go | 2 +- 8 files changed, 86 insertions(+), 10 deletions(-) diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index c25e6643..a5a0b280 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -83,7 +83,7 @@ func prePushRefs(r io.Reader) []*refUpdate { continue } - refs = append(refs, newRefUpdate(cfg.Git, cfg.Remote(), left, right)) + refs = append(refs, newRefUpdate(cfg.Git, cfg.PushRemote(), left, right)) } return refs diff --git a/commands/command_push.go b/commands/command_push.go index 8a93df67..564287eb 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -111,7 +111,7 @@ func lfsPushRefs(refnames []string, pushAll bool) ([]*refUpdate, error) { if pushAll && len(refnames) == 0 { refs := make([]*refUpdate, len(localrefs)) for i, lr := range localrefs { - refs[i] = newRefUpdate(cfg.Git, cfg.Remote(), lr, nil) + refs[i] = newRefUpdate(cfg.Git, cfg.PushRemote(), lr, nil) } return refs, nil } @@ -124,10 +124,10 @@ func lfsPushRefs(refnames []string, pushAll bool) ([]*refUpdate, error) { refs := make([]*refUpdate, len(refnames)) for i, name := range refnames { if left, ok := reflookup[name]; ok { - refs[i] = newRefUpdate(cfg.Git, cfg.Remote(), left, nil) + refs[i] = newRefUpdate(cfg.Git, cfg.PushRemote(), left, nil) } else { left := &git.Ref{Name: name, Type: git.RefTypeOther, Sha: name} - refs[i] = newRefUpdate(cfg.Git, cfg.Remote(), left, nil) + refs[i] = newRefUpdate(cfg.Git, cfg.PushRemote(), left, nil) } } diff --git a/commands/commands.go b/commands/commands.go index 74e9ac45..35940a85 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -91,7 +91,7 @@ func closeAPIClient() error { } func newLockClient() *locking.Client { - lockClient, err := locking.NewClient(cfg.Remote(), getAPIClient()) + lockClient, err := locking.NewClient(cfg.PushRemote(), getAPIClient()) if err == nil { os.MkdirAll(cfg.LFSStorageDir(), 0755) err = lockClient.SetupFileCache(cfg.LFSStorageDir()) diff --git a/commands/lockverifier.go b/commands/lockverifier.go index 062f7fb3..f1da8ea1 100644 --- a/commands/lockverifier.go +++ b/commands/lockverifier.go @@ -62,7 +62,7 @@ func (lv *lockVerifier) Verify(ref string) { Exit("ERROR: Authentication error: %s", err) } } else { - Print("Remote %q does not support the LFS locking API. Consider disabling it with:", cfg.Remote()) + Print("Remote %q does not support the LFS locking API. Consider disabling it with:", cfg.PushRemote()) Print(" $ git config lfs.%s.locksverify false", lv.endpoint.Url) if lv.verifyState == verifyStateEnabled { ExitWithError(err) @@ -70,7 +70,7 @@ func (lv *lockVerifier) Verify(ref string) { } } } else if lv.verifyState == verifyStateUnknown { - Print("Locking support detected on remote %q. Consider enabling it with:", cfg.Remote()) + Print("Locking support detected on remote %q. Consider enabling it with:", cfg.PushRemote()) Print(" $ git config lfs.%s.locksverify true", lv.endpoint.Url) } @@ -146,7 +146,7 @@ func (lv *lockVerifier) newRefLocks(ref string, l locking.Lock) *refLock { func newLockVerifier(m *tq.Manifest) *lockVerifier { lv := &lockVerifier{ - endpoint: getAPIClient().Endpoints.Endpoint("upload", cfg.Remote()), + endpoint: getAPIClient().Endpoints.Endpoint("upload", cfg.PushRemote()), verifiedRefs: make(map[string]bool), ourLocks: make(map[string]*refLock), theirLocks: make(map[string]*refLock), diff --git a/commands/uploader.go b/commands/uploader.go index 8342d3dd..307268db 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -72,7 +72,7 @@ type uploadContext struct { } func newUploadContext(dryRun bool) *uploadContext { - remote := cfg.Remote() + remote := cfg.PushRemote() manifest := getTransferManifestOperationRemote("upload", remote) ctx := &uploadContext{ Remote: remote, diff --git a/config/config.go b/config/config.go index 96df6995..089565b2 100644 --- a/config/config.go +++ b/config/config.go @@ -33,6 +33,7 @@ type Configuration struct { Git Environment currentRemote *string + pushRemote *string // gitConfig can fetch or modify the current Git config and track the Git // version. @@ -201,6 +202,28 @@ func (c *Configuration) Remote() string { return *c.currentRemote } +func (c *Configuration) PushRemote() string { + ref := c.CurrentRef() + c.loading.Lock() + defer c.loading.Unlock() + + if c.pushRemote == nil { + if remote, ok := c.Git.Get(fmt.Sprintf("branch.%s.pushRemote", ref.Name)); ok { + c.pushRemote = &remote + } else if remote, ok := c.Git.Get("remote.pushDefault"); ok { + c.pushRemote = &remote + } else { + c.loading.Unlock() + remote := c.Remote() + c.loading.Lock() + + c.pushRemote = &remote + } + } + + return *c.pushRemote +} + func (c *Configuration) SetValidRemote(name string) error { if err := git.ValidateRemote(name); err != nil { return err diff --git a/config/config_test.go b/config/config_test.go index c45083ba..66464f0f 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -3,9 +3,62 @@ package config import ( "testing" + "github.com/git-lfs/git-lfs/git" "github.com/stretchr/testify/assert" ) +func TestRemoteDefault(t *testing.T) { + cfg := NewFrom(Values{ + Git: map[string][]string{ + "branch.unused.remote": []string{"a"}, + "branch.unused.pushRemote": []string{"b"}, + }, + }) + assert.Equal(t, "origin", cfg.Remote()) + assert.Equal(t, "origin", cfg.PushRemote()) +} + +func TestRemoteBranchConfig(t *testing.T) { + cfg := NewFrom(Values{ + Git: map[string][]string{ + "branch.master.remote": []string{"a"}, + "branch.other.pushRemote": []string{"b"}, + }, + }) + cfg.ref = &git.Ref{Name: "master"} + + assert.Equal(t, "a", cfg.Remote()) + assert.Equal(t, "a", cfg.PushRemote()) +} + +func TestRemotePushDefault(t *testing.T) { + cfg := NewFrom(Values{ + Git: map[string][]string{ + "branch.master.remote": []string{"a"}, + "remote.pushDefault": []string{"b"}, + "branch.other.pushRemote": []string{"c"}, + }, + }) + cfg.ref = &git.Ref{Name: "master"} + + assert.Equal(t, "a", cfg.Remote()) + assert.Equal(t, "b", cfg.PushRemote()) +} + +func TestRemoteBranchPushDefault(t *testing.T) { + cfg := NewFrom(Values{ + Git: map[string][]string{ + "branch.master.remote": []string{"a"}, + "remote.pushDefault": []string{"b"}, + "branch.master.pushRemote": []string{"c"}, + }, + }) + cfg.ref = &git.Ref{Name: "master"} + + assert.Equal(t, "a", cfg.Remote()) + assert.Equal(t, "c", cfg.PushRemote()) +} + func TestBasicTransfersOnlySetValue(t *testing.T) { cfg := NewFrom(Values{ Git: map[string][]string{ diff --git a/lfs/lfs.go b/lfs/lfs.go index 4279acbd..eae67ca0 100644 --- a/lfs/lfs.go +++ b/lfs/lfs.go @@ -26,7 +26,7 @@ func Environ(cfg *config.Configuration, manifest *tq.Manifest) []string { } download := api.Endpoints.AccessFor(api.Endpoints.Endpoint("download", cfg.Remote()).Url) - upload := api.Endpoints.AccessFor(api.Endpoints.Endpoint("upload", cfg.Remote()).Url) + upload := api.Endpoints.AccessFor(api.Endpoints.Endpoint("upload", cfg.PushRemote()).Url) dltransfers := manifest.GetDownloadAdapterNames() sort.Strings(dltransfers) From 87b53b36411ad878dafef2a36179ec58dfa80481 Mon Sep 17 00:00:00 2001 From: Elliott Sales de Andrade Date: Fri, 3 Nov 2017 23:27:57 -0400 Subject: [PATCH 45/71] Wait longer for test lfs server to start. Also, back off on polling a bit after the first check to reduce any I/O contention that might slow things down even more. Instead of 5 seconds, this will wait up to about 60 seconds for files created by the test server to appear. This allows very slow systems to be able to pass tests. Finally, add a message if the file doesn't appear to make it clear what went wrong. --- test/testhelpers.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/testhelpers.sh b/test/testhelpers.sh index c7fe7660..445f2fb3 100644 --- a/test/testhelpers.sh +++ b/test/testhelpers.sh @@ -266,15 +266,20 @@ size %s wait_for_file() { local filename="$1" n=0 - while [ $n -lt 10 ]; do + wait_time=1 + while [ $n -lt 17 ]; do if [ -s $filename ]; then return 0 fi - sleep 0.5 + sleep $wait_time n=`expr $n + 1` + if [ $wait_time -lt 4 ]; then + wait_time=`expr $wait_time \* 2` + fi done + echo "$filename did not appear after 60 seconds." return 1 } From d7e9960a7d83a27a0b898d87d8bbdeb642016562 Mon Sep 17 00:00:00 2001 From: Lars Bilke Date: Tue, 7 Nov 2017 12:25:03 +0100 Subject: [PATCH 46/71] Added documentation git-lfs-ls-files' */- output. This functionality was introduced in https://github.com/git-lfs/git-lfs/commit/2082b26ce728bb8a960b28bcac9aaecbb6a251c1. --- docs/man/git-lfs-ls-files.1.ronn | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/man/git-lfs-ls-files.1.ronn b/docs/man/git-lfs-ls-files.1.ronn index 3d64d46a..fe4fa746 100644 --- a/docs/man/git-lfs-ls-files.1.ronn +++ b/docs/man/git-lfs-ls-files.1.ronn @@ -9,6 +9,7 @@ git-lfs-ls-files(1) -- Show information about Git LFS files in the index and wor Display paths of Git LFS files that are found in the tree at the given reference. If no reference is given, scan the currently checked-out branch. +An asterisk (*) after the OID indicates a LFS pointer, a minus (-) a full object. ## OPTIONS From 036449e37b3166dafd3f3409c8d770e1b8fb2ce7 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Wed, 1 Nov 2017 10:04:35 -0400 Subject: [PATCH 47/71] testlib: clear GIT_CONFIG and XDG_CONFIG_HOME This prevents someone from running the test suite with a custom `GIT_CONFIG` or `XDG_CONFIG_HOME` setting from redirecting the test's configuration logic. Fixes #2448. --- test/testlib.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/testlib.sh b/test/testlib.sh index 93055bca..ea42fdfb 100644 --- a/test/testlib.sh +++ b/test/testlib.sh @@ -92,6 +92,10 @@ begin_test () { mkdir "$HOME" cp "$TESTHOME/.gitconfig" "$HOME/.gitconfig" + # do not let Git use a different configuration file + unset GIT_CONFIG + unset XDG_CONFIG_HOME + # allow the subshell to exit non-zero without exiting this process set -x +e } From 5427600232727855328b7c40021b5bb20a5424b4 Mon Sep 17 00:00:00 2001 From: rick olson Date: Wed, 8 Nov 2017 14:43:54 -0700 Subject: [PATCH 48/71] test: force integration test runner to wait until all output is posted before bailing --- script/integration.go | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/script/integration.go b/script/integration.go index 68b1beb8..894e062b 100644 --- a/script/integration.go +++ b/script/integration.go @@ -50,17 +50,31 @@ func mainIntegration() { for _, file := range files { tests <- file } + close(tests) + + outputDone := make(chan bool) + go func() { + for { + select { + case out, ok := <-output: + if !ok { + outputDone <- true + return + } + + fmt.Println(out) + } + } + }() - go printOutput(output) for i := 0; i < maxprocs; i++ { wg.Add(1) go worker(tests, output, &wg) } - close(tests) wg.Wait() close(output) - printOutput(output) + <-outputDone if erroring { os.Exit(1) @@ -116,19 +130,6 @@ func sendTestOutput(output chan string, testname string, buf *bytes.Buffer, err } } -func printOutput(output <-chan string) { - for { - select { - case out, ok := <-output: - if !ok { - return - } - - fmt.Println(out) - } - } -} - func worker(tests <-chan string, output chan string, wg *sync.WaitGroup) { defer wg.Done() for { From a35d38305d89af23249dcf5ec7004b8f53986a83 Mon Sep 17 00:00:00 2001 From: rick olson Date: Wed, 8 Nov 2017 15:04:40 -0700 Subject: [PATCH 49/71] locking: send full refspec in locks/verify request --- commands/lockverifier.go | 22 ++++++++++++---------- commands/refs.go | 8 ++++---- commands/refs_test.go | 15 +++++++++------ git/git.go | 25 ++++++++++++------------- git/git_test.go | 13 ------------- locking/locks.go | 4 ++-- locking/locks_test.go | 4 ++-- test/cmd/lfstest-gitserver.go | 6 +++--- test/test-pre-push.sh | 8 ++++---- 9 files changed, 48 insertions(+), 57 deletions(-) diff --git a/commands/lockverifier.go b/commands/lockverifier.go index f1da8ea1..ed6565b9 100644 --- a/commands/lockverifier.go +++ b/commands/lockverifier.go @@ -8,9 +8,11 @@ import ( "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/errors" + "github.com/git-lfs/git-lfs/git" "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/locking" "github.com/git-lfs/git-lfs/tq" + "github.com/rubyist/tracerx" ) type verifyState byte @@ -23,7 +25,7 @@ const ( func verifyLocksForUpdates(lv *lockVerifier, updates []*refUpdate) { for _, update := range updates { - lv.Verify(update.Right().Name) + lv.Verify(update.Right()) } } @@ -44,8 +46,8 @@ type lockVerifier struct { unownedLocks []*refLock } -func (lv *lockVerifier) Verify(ref string) { - if lv.verifyState == verifyStateDisabled || lv.verifiedRefs[ref] { +func (lv *lockVerifier) Verify(ref *git.Ref) { + if lv.verifyState == verifyStateDisabled || lv.verifiedRefs[ref.Refspec()] { return } @@ -76,10 +78,10 @@ func (lv *lockVerifier) Verify(ref string) { lv.addLocks(ref, ours, lv.ourLocks) lv.addLocks(ref, theirs, lv.theirLocks) - lv.verifiedRefs[ref] = true + lv.verifiedRefs[ref.Refspec()] = true } -func (lv *lockVerifier) addLocks(ref string, locks []locking.Lock, set map[string]*refLock) { +func (lv *lockVerifier) addLocks(ref *git.Ref, locks []locking.Lock, set map[string]*refLock) { for _, l := range locks { if rl, ok := set[l.Path]; ok { if err := rl.Add(ref, l); err != nil { @@ -136,11 +138,11 @@ func (lv *lockVerifier) Enabled() bool { return lv.verifyState == verifyStateEnabled } -func (lv *lockVerifier) newRefLocks(ref string, l locking.Lock) *refLock { +func (lv *lockVerifier) newRefLocks(ref *git.Ref, l locking.Lock) *refLock { return &refLock{ allRefs: lv.verifiedRefs, path: l.Path, - refs: map[string]locking.Lock{ref: l}, + refs: map[*git.Ref]locking.Lock{ref: l}, } } @@ -169,7 +171,7 @@ func newLockVerifier(m *tq.Manifest) *lockVerifier { type refLock struct { path string allRefs map[string]bool - refs map[string]locking.Lock + refs map[*git.Ref]locking.Lock } // Path returns the locked path. @@ -189,7 +191,7 @@ func (r *refLock) Owners() string { if _, ok := users[u]; !ok { users[u] = make([]string, 0, len(r.refs)) } - users[u] = append(users[u], ref) + users[u] = append(users[u], ref.Name) } owners := make([]string, 0, len(users)) @@ -212,7 +214,7 @@ func (r *refLock) Owners() string { return strings.Join(owners, ", ") } -func (r *refLock) Add(ref string, l locking.Lock) error { +func (r *refLock) Add(ref *git.Ref, l locking.Lock) error { r.refs[ref] = l return nil } diff --git a/commands/refs.go b/commands/refs.go index 988ef899..bc57fff9 100644 --- a/commands/refs.go +++ b/commands/refs.go @@ -57,7 +57,7 @@ func defaultRemoteRef(g config.Environment, remote string, left *git.Ref) *git.R // When pushing to a remote that is different from the remote you normally // pull from, work as current. - return &git.Ref{Name: left.Name} + return left case "upstream", "tracking": // push the current branch back to the branch whose changes are usually // integrated into the current branch @@ -65,10 +65,10 @@ func defaultRemoteRef(g config.Environment, remote string, left *git.Ref) *git.R case "current": // push the current branch to update a branch with the same name on the // receiving end. - return &git.Ref{Name: left.Name} + return left default: tracerx.Printf("WARNING: %q push mode not supported", pushMode) - return &git.Ref{Name: left.Name} + return left } } @@ -76,7 +76,7 @@ func trackingRef(g config.Environment, left *git.Ref) *git.Ref { if merge, ok := g.Get(fmt.Sprintf("branch.%s.merge", left.Name)); ok { return git.ParseRef(merge, "") } - return &git.Ref{Name: left.Name} + return left } func (u *refUpdate) RightCommitish() string { diff --git a/commands/refs_test.go b/commands/refs_test.go index e25fb9c4..cc98b3d8 100644 --- a/commands/refs_test.go +++ b/commands/refs_test.go @@ -19,8 +19,9 @@ func TestRefUpdateDefault(t *testing.T) { }, }) - u := newRefUpdate(cfg.Git, "origin", git.ParseRef("left", ""), nil) + u := newRefUpdate(cfg.Git, "origin", git.ParseRef("refs/heads/left", ""), nil) assert.Equal(t, "left", u.Right().Name, "pushmode=%q", pushMode) + assert.Equal(t, git.RefTypeLocalBranch, u.Right().Type, "pushmode=%q", pushMode) } } @@ -31,12 +32,13 @@ func TestRefUpdateTrackedDefault(t *testing.T) { Git: map[string][]string{ "push.default": []string{pushMode}, "branch.left.remote": []string{"origin"}, - "branch.left.merge": []string{"tracked"}, + "branch.left.merge": []string{"refs/heads/tracked"}, }, }) - u := newRefUpdate(cfg.Git, "origin", git.ParseRef("left", ""), nil) + u := newRefUpdate(cfg.Git, "origin", git.ParseRef("refs/heads/left", ""), nil) assert.Equal(t, "tracked", u.Right().Name, "pushmode=%s", pushMode) + assert.Equal(t, git.RefTypeLocalBranch, u.Right().Type, "pushmode=%q", pushMode) } } @@ -49,12 +51,13 @@ func TestRefUpdateCurrentDefault(t *testing.T) { }, }) - u := newRefUpdate(cfg.Git, "origin", git.ParseRef("left", ""), nil) + u := newRefUpdate(cfg.Git, "origin", git.ParseRef("refs/heads/left", ""), nil) assert.Equal(t, "left", u.Right().Name) + assert.Equal(t, git.RefTypeLocalBranch, u.Right().Type) } func TestRefUpdateExplicitLeftAndRight(t *testing.T) { - u := newRefUpdate(nil, "", git.ParseRef("left", "abc123"), git.ParseRef("right", "def456")) + u := newRefUpdate(nil, "", git.ParseRef("refs/heads/left", "abc123"), git.ParseRef("refs/heads/right", "def456")) assert.Equal(t, "left", u.Left().Name) assert.Equal(t, "abc123", u.Left().Sha) assert.Equal(t, "abc123", u.LeftCommitish()) @@ -62,7 +65,7 @@ func TestRefUpdateExplicitLeftAndRight(t *testing.T) { assert.Equal(t, "def456", u.Right().Sha) assert.Equal(t, "def456", u.RightCommitish()) - u = newRefUpdate(nil, "", git.ParseRef("left", ""), git.ParseRef("right", "")) + u = newRefUpdate(nil, "", git.ParseRef("refs/heads/left", ""), git.ParseRef("refs/heads/right", "")) assert.Equal(t, "left", u.Left().Name) assert.Equal(t, "", u.Left().Sha) assert.Equal(t, "left", u.LeftCommitish()) diff --git a/git/git.go b/git/git.go index 9676bbb0..5b7d877a 100644 --- a/git/git.go +++ b/git/git.go @@ -54,12 +54,8 @@ func (t RefType) Prefix() (string, bool) { return "refs/tags", true case RefTypeRemoteTag: return "refs/remotes/tags", true - case RefTypeHEAD: - return "", false - case RefTypeOther: - return "", false default: - panic(fmt.Sprintf("git: unknown RefType %d", t)) + return "", false } } @@ -95,6 +91,16 @@ type Ref struct { Sha string } +func (r *Ref) Refspec() string { + if r == nil { + return "" + } + if prefix, ok := r.Type.Prefix(); ok { + return fmt.Sprintf("%s/%s", prefix, r.Name) + } + return r.Name +} + // Some top level information about a commit (only first line of message) type CommitSummary struct { Sha string @@ -373,14 +379,7 @@ func UpdateRef(ref *Ref, to []byte, reason string) error { // reflog entry, if a "reason" was provided). It operates within the given // working directory "wd". It returns an error if any were encountered. func UpdateRefIn(wd string, ref *Ref, to []byte, reason string) error { - var refspec string - if prefix, ok := ref.Type.Prefix(); ok { - refspec = fmt.Sprintf("%s/%s", prefix, ref.Name) - } else { - refspec = ref.Name - } - - args := []string{"update-ref", refspec, hex.EncodeToString(to)} + args := []string{"update-ref", ref.Refspec(), hex.EncodeToString(to)} if len(reason) > 0 { args = append(args, "-m", reason) } diff --git a/git/git_test.go b/git/git_test.go index 16fd895b..eacbb88e 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -585,16 +585,3 @@ func TestRefTypeKnownPrefixes(t *testing.T) { assert.Equal(t, expected.Ok, ok) } } - -func TestRefTypeUnknownPrefix(t *testing.T) { - defer func() { - if err := recover(); err != nil { - assert.Equal(t, "git: unknown RefType -1", err) - } else { - t.Fatal("git: expected panic() from RefType.Prefix()") - } - }() - - unknown := RefType(-1) - unknown.Prefix() -} diff --git a/locking/locks.go b/locking/locks.go index 9f4377b1..31b91425 100644 --- a/locking/locks.go +++ b/locking/locks.go @@ -206,11 +206,11 @@ func (c *Client) SearchLocks(filter map[string]string, limit int, localOnly bool } } -func (c *Client) VerifiableLocks(ref string, limit int) (ourLocks, theirLocks []Lock, err error) { +func (c *Client) VerifiableLocks(ref *git.Ref, limit int) (ourLocks, theirLocks []Lock, err error) { ourLocks = make([]Lock, 0, limit) theirLocks = make([]Lock, 0, limit) body := &lockVerifiableRequest{ - Ref: &lockRef{Name: ref}, + Ref: &lockRef{Name: ref.Refspec()}, Limit: limit, } diff --git a/locking/locks_test.go b/locking/locks_test.go index 3eaa5c97..6cf55ad6 100644 --- a/locking/locks_test.go +++ b/locking/locks_test.go @@ -64,7 +64,7 @@ func TestRefreshCache(t *testing.T) { assert.Nil(t, err) assert.Empty(t, locks) - _, _, err = client.VerifiableLocks("", 100) + _, _, err = client.VerifiableLocks(nil, 100) assert.Nil(t, err) locks, err = client.SearchLocks(nil, 0, true) @@ -130,7 +130,7 @@ func TestGetVerifiableLocks(t *testing.T) { client, err := NewClient("", lfsclient) assert.Nil(t, err) - ourLocks, theirLocks, err := client.VerifiableLocks("", 0) + ourLocks, theirLocks, err := client.VerifiableLocks(nil, 0) assert.Nil(t, err) // Need to include zero time in structure for equal to work diff --git a/test/cmd/lfstest-gitserver.go b/test/cmd/lfstest-gitserver.go index 334a0611..f3aee82e 100644 --- a/test/cmd/lfstest-gitserver.go +++ b/test/cmd/lfstest-gitserver.go @@ -1101,14 +1101,14 @@ func locksHandler(w http.ResponseWriter, r *http.Request, repo string) { return } - if strings.HasSuffix(repo, "ref-required") { + if strings.HasSuffix(repo, "branch-required") { parts := strings.Split(repo, "-") lenParts := len(parts) - if lenParts > 3 && parts[lenParts-3] != reqBody.RefName() { + if lenParts > 3 && "refs/heads/"+parts[lenParts-3] != reqBody.RefName() { w.WriteHeader(403) enc.Encode(struct { Message string `json:"message"` - }{fmt.Sprintf("Expected ref %q, got %q", parts[lenParts-3], reqBody.RefName())}) + }{fmt.Sprintf("Expected ref %q, got %q", "refs/heads/"+parts[lenParts-3], reqBody.RefName())}) return } } diff --git a/test/test-pre-push.sh b/test/test-pre-push.sh index eb18e563..071c34f9 100755 --- a/test/test-pre-push.sh +++ b/test/test-pre-push.sh @@ -781,7 +781,7 @@ begin_test "pre-push locks verify 403 with good ref" ( set -e - reponame="lock-verify-master-ref-required" + reponame="lock-verify-master-branch-required" setup_remote_repo "$reponame" clone_repo "$reponame" "$reponame" @@ -803,7 +803,7 @@ begin_test "pre-push locks verify 403 with good tracked ref" ( set -e - reponame="lock-verify-tracked-ref-required" + reponame="lock-verify-tracked-branch-required" setup_remote_repo "$reponame" clone_repo "$reponame" "$reponame" @@ -827,7 +827,7 @@ begin_test "pre-push locks verify 403 with explicit ref" ( set -e - reponame="lock-verify-explicit-ref-required" + reponame="lock-verify-explicit-branch-required" setup_remote_repo "$reponame" clone_repo "$reponame" "$reponame" @@ -849,7 +849,7 @@ begin_test "pre-push locks verify 403 with bad ref" ( set -e - reponame="lock-verify-other-ref-required" + reponame="lock-verify-other-branch-required" setup_remote_repo "$reponame" clone_repo "$reponame" "$reponame" From b714bbf13605fd3dd48e0da006388eb3a45f9455 Mon Sep 17 00:00:00 2001 From: rick olson Date: Wed, 8 Nov 2017 15:09:03 -0700 Subject: [PATCH 50/71] remove leftover import from bad partial commit --- commands/lockverifier.go | 1 - 1 file changed, 1 deletion(-) diff --git a/commands/lockverifier.go b/commands/lockverifier.go index ed6565b9..80fa8855 100644 --- a/commands/lockverifier.go +++ b/commands/lockverifier.go @@ -12,7 +12,6 @@ import ( "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/locking" "github.com/git-lfs/git-lfs/tq" - "github.com/rubyist/tracerx" ) type verifyState byte From 3e35079630c0db70ecbe50ce5e013f0a42498973 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 16 Nov 2017 10:34:56 +0900 Subject: [PATCH 51/71] Fixes #2713: Add uninstall to the list of commands in man page Change-Id: I61accf58c4ec436d164654c93b342502c9f4222e Signed-off-by: David Pursehouse --- docs/man/git-lfs.1.ronn | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/man/git-lfs.1.ronn b/docs/man/git-lfs.1.ronn index 6d61de99..74a7dd1a 100644 --- a/docs/man/git-lfs.1.ronn +++ b/docs/man/git-lfs.1.ronn @@ -59,6 +59,8 @@ commands and low level ("plumbing") commands. Show the status of Git LFS files in the working tree. * git-lfs-track(1): View or add Git LFS paths to Git attributes. +* git-lfs-uninstall(1): + Uninstall Git LFS by removing hooks and smudge/clean filter configuration. * git-lfs-unlock(1): Remove "locked" setting for a file on the Git LFS server. * git-lfs-untrack(1): From ac0f661310ca675b271e92c17963c286d51f8385 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 16 Nov 2017 10:40:20 +0900 Subject: [PATCH 52/71] Fixes #2714: Add --local option to uninstall man page Change-Id: Ie83973ad299eb4741153bbe94a872e69963a4928 Signed-off-by: David Pursehouse --- docs/man/git-lfs-uninstall.1.ronn | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/man/git-lfs-uninstall.1.ronn b/docs/man/git-lfs-uninstall.1.ronn index c4a7fb1f..b6e5de09 100644 --- a/docs/man/git-lfs-uninstall.1.ronn +++ b/docs/man/git-lfs-uninstall.1.ronn @@ -12,6 +12,12 @@ Perform the following actions to remove the Git LFS configuration: * Remove the "lfs" clean and smudge filters from the global Git config. * Uninstall the Git LFS pre-push hook if run from inside a Git repository. +## OPTIONS + +* --local: + Removes the "lfs" smudge and clean filters from the local repository's git + config, instead of the global git config (~/.gitconfig). + ## SEE ALSO git-lfs-install(1). From e0064950e74397a2a8662133f165a972eb5fe2c1 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 12:04:42 -0800 Subject: [PATCH 53/71] lfsapi/auth: teach DoWithAuth to respect http.extraHeaders --- lfsapi/auth.go | 13 +++++++++++++ test/test-extra-header.sh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/lfsapi/auth.go b/lfsapi/auth.go index 92c9aec6..68b80000 100644 --- a/lfsapi/auth.go +++ b/lfsapi/auth.go @@ -24,6 +24,19 @@ var ( // 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) { + req.Header = c.extraHeadersFor(req) + if len(req.Header.Get("Authorization")) > 0 { + // If the extra headers that were configured contained + // credentials, assume it is preferred to use those over + // contacting the credential helper. Fallback to c.Do. + res, err := c.doWithRedirects(c.httpClient(req.Host), req, nil) + if err != nil { + return res, err + } + + return res, c.handleResponse(res) + } + apiEndpoint, access, credHelper, credsURL, creds, err := c.getCreds(remote, req) if err != nil { return nil, err diff --git a/test/test-extra-header.sh b/test/test-extra-header.sh index 1cac229b..823a7d62 100755 --- a/test/test-extra-header.sh +++ b/test/test-extra-header.sh @@ -25,3 +25,37 @@ begin_test "http..extraHeader" grep "> X-Foo: baz" curl.log ) end_test + +begin_test "http..extraHeader with authorization" +( + set -e + + reponame="requirecreds" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + # See: test/cmd/lfstest-gitserver.go:1176. + user="requirecreds" + pass="pass" + auth="Basic $(echo -n $user:$pass | base64)" + + git config --add "http.extraHeader" "Authorization: $auth" + + git lfs track "*.dat" + printf "contents" > a.dat + git add .gitattributes a.dat + git commit -m "initial commit" + + git push origin master 2>&1 | tee curl.log + if [ "0" -ne "${PIPESTATUS[0]}" ]; then + echo >&2 "expected \`git push origin master\` to succeed, didn't" + exit 1 + fi + + [ "0" -eq "$(grep -c "creds: filling with GIT_ASKPASS" curl.log)" ] + [ "0" -eq "$(grep -c "creds: git credential approve" curl.log)" ] + [ "0" -eq "$(grep -c "creds: git credential cache" curl.log)" ] + [ "0" -eq "$(grep -c "creds: git credential fill" curl.log)" ] + [ "0" -eq "$(grep -c "creds: git credential reject" curl.log)" ] +) +end_test From f3b816ad768b4174e21ce0a2fff756bd06a439c5 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 13:13:17 -0800 Subject: [PATCH 54/71] lfsapi/auth: skip call to c.doWithRedirects with getCreds --- lfsapi/auth.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/lfsapi/auth.go b/lfsapi/auth.go index 68b80000..06559189 100644 --- a/lfsapi/auth.go +++ b/lfsapi/auth.go @@ -25,17 +25,6 @@ var ( // supporting basic and ntlm authentication. func (c *Client) DoWithAuth(remote string, req *http.Request) (*http.Response, error) { req.Header = c.extraHeadersFor(req) - if len(req.Header.Get("Authorization")) > 0 { - // If the extra headers that were configured contained - // credentials, assume it is preferred to use those over - // contacting the credential helper. Fallback to c.Do. - res, err := c.doWithRedirects(c.httpClient(req.Host), req, nil) - if err != nil { - return res, err - } - - return res, c.handleResponse(res) - } apiEndpoint, access, credHelper, credsURL, creds, err := c.getCreds(remote, req) if err != nil { From bda2fe845fd7b1b4917ac266c17cea8ec055261a Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 13:50:40 -0800 Subject: [PATCH 55/71] lfsapi/{auth,client}: introduce do() method to circumvent header altering --- lfsapi/auth.go | 2 +- lfsapi/client.go | 7 +++++++ lfsapi/ntlm.go | 6 +++--- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lfsapi/auth.go b/lfsapi/auth.go index 06559189..d4bcc4ad 100644 --- a/lfsapi/auth.go +++ b/lfsapi/auth.go @@ -61,7 +61,7 @@ func (c *Client) doWithCreds(req *http.Request, credHelper CredentialHelper, cre if access == NTLMAccess { return c.doWithNTLM(req, credHelper, creds, credsURL) } - return c.Do(req) + return c.do(req) } // getCreds fills the authorization header for the given request if possible, diff --git a/lfsapi/client.go b/lfsapi/client.go index 4d9ffa16..534df58c 100644 --- a/lfsapi/client.go +++ b/lfsapi/client.go @@ -85,6 +85,13 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { req.Header = c.extraHeadersFor(req) req.Header.Set("User-Agent", UserAgent) + return c.do(req) +} + +// do performs an *http.Request respecting redirects, and handles the response +// as defined in c.handleResponse. Notably, it does not alter the headers for +// the request argument in any way. +func (c *Client) do(req *http.Request) (*http.Response, error) { res, err := c.doWithRedirects(c.httpClient(req.Host), req, nil) if err != nil { return res, err diff --git a/lfsapi/ntlm.go b/lfsapi/ntlm.go index b180b08a..b3b457b8 100644 --- a/lfsapi/ntlm.go +++ b/lfsapi/ntlm.go @@ -14,7 +14,7 @@ import ( ) func (c *Client) doWithNTLM(req *http.Request, credHelper CredentialHelper, creds Creds, credsURL *url.URL) (*http.Response, error) { - res, err := c.Do(req) + res, err := c.do(req) if err != nil && !errors.IsAuthError(err) { return res, err } @@ -69,7 +69,7 @@ func (c *Client) ntlmReAuth(req *http.Request, credHelper CredentialHelper, cred func (c *Client) ntlmNegotiate(req *http.Request, message string) (*http.Response, []byte, error) { req.Header.Add("Authorization", message) - res, err := c.Do(req) + res, err := c.do(req) if err != nil && !errors.IsAuthError(err) { return res, nil, err } @@ -100,7 +100,7 @@ func (c *Client) ntlmChallenge(req *http.Request, challengeBytes []byte, creds C authMsg := base64.StdEncoding.EncodeToString(authenticate.Bytes()) req.Header.Set("Authorization", "NTLM "+authMsg) - return c.Do(req) + return c.do(req) } func (c *Client) ntlmClientSession(creds Creds) (ntlm.ClientSession, error) { From a2bad11e5d171d572ccdd55af6b84a0c217b7c52 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 14:05:17 -0800 Subject: [PATCH 56/71] lfsapi/client: ensure a consistent User-Agent: header --- lfsapi/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lfsapi/client.go b/lfsapi/client.go index 534df58c..ce20c3ab 100644 --- a/lfsapi/client.go +++ b/lfsapi/client.go @@ -83,7 +83,6 @@ func joinURL(prefix, suffix string) string { // 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) return c.do(req) } @@ -92,6 +91,8 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { // as defined in c.handleResponse. Notably, it does not alter the headers for // the request argument in any way. func (c *Client) do(req *http.Request) (*http.Response, error) { + req.Header.Set("User-Agent", UserAgent) + res, err := c.doWithRedirects(c.httpClient(req.Host), req, nil) if err != nil { return res, err From fcfb1b0e94e4b7223c0ff7c5d251c28bf56692a7 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 15:44:08 -0800 Subject: [PATCH 57/71] git: implement fmt.Stringer on *git.Ref --- git/git.go | 14 ++++++++++++++ git/git_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/git/git.go b/git/git.go index 9676bbb0..913189a5 100644 --- a/git/git.go +++ b/git/git.go @@ -95,6 +95,20 @@ type Ref struct { Sha string } +// String implements fmt.Stringer.String and returns the fully-qualified +// reference name (including remote), i.e., for a remote branch called +// 'my-feature' on remote 'origin', this function will return: +// +// refs/remotes/origin/my-feature +func (r *Ref) String() string { + prefix, ok := r.Type.Prefix() + if ok { + return fmt.Sprintf("%s/%s", prefix, r.Name) + } + + return r.Name +} + // Some top level information about a commit (only first line of message) type CommitSummary struct { Sha string diff --git a/git/git_test.go b/git/git_test.go index 16fd895b..f474afa9 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -13,6 +13,44 @@ import ( "github.com/stretchr/testify/assert" ) +func TestRefString(t *testing.T) { + const sha = "0000000000000000000000000000000000000000" + for s, r := range map[string]*Ref{ + "refs/heads/master": &Ref{ + Name: "master", + Type: RefTypeLocalBranch, + Sha: sha, + }, + "refs/remotes/origin/master": &Ref{ + Name: "origin/master", + Type: RefTypeRemoteBranch, + Sha: sha, + }, + "refs/remotes/tags/v1.0.0": &Ref{ + Name: "v1.0.0", + Type: RefTypeRemoteTag, + Sha: sha, + }, + "refs/tags/v1.0.0": &Ref{ + Name: "v1.0.0", + Type: RefTypeLocalTag, + Sha: sha, + }, + "HEAD": &Ref{ + Name: "HEAD", + Type: RefTypeHEAD, + Sha: sha, + }, + "other": &Ref{ + Name: "other", + Type: RefTypeOther, + Sha: sha, + }, + } { + assert.Equal(t, s, r.String()) + } +} + func TestParseRefs(t *testing.T) { tests := map[string]RefType{ "refs/heads": RefTypeLocalBranch, From 0ddb3cc8b64e670187c1470a3bb9574de5306476 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 15:44:20 -0800 Subject: [PATCH 58/71] commands: use ref.String() to fully-qualify references --- commands/command_migrate.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/commands/command_migrate.go b/commands/command_migrate.go index c15e8185..9917b3a7 100644 --- a/commands/command_migrate.go +++ b/commands/command_migrate.go @@ -1,6 +1,7 @@ package commands import ( + "fmt" "path/filepath" "strings" @@ -122,7 +123,7 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string return nil, nil, err } - include = append(include, ref.Name) + include = append(include, ref.String()) } if hardcore { @@ -142,7 +143,7 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string } for _, ref := range localRefs { - include = append(include, ref.Name) + include = append(include, ref.String()) } } else { // Otherwise, if neither --include-ref= or @@ -154,7 +155,9 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string return nil, nil, err } - exclude = append(exclude, remoteRefs...) + for _, rr := range remoteRefs { + exclude = append(exclude, rr.String()) + } } return include, exclude, nil @@ -163,8 +166,8 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string // getRemoteRefs returns a fully qualified set of references belonging to all // remotes known by the currently checked-out repository, or an error if those // references could not be determined. -func getRemoteRefs(l *log.Logger) ([]string, error) { - var refs []string +func getRemoteRefs(l *log.Logger) ([]*git.Ref, error) { + var refs []*git.Ref remotes, err := git.RemoteList() if err != nil { @@ -183,8 +186,12 @@ func getRemoteRefs(l *log.Logger) ([]string, error) { return nil, err } - for _, ref := range refsForRemote { - refs = append(refs, formatRefName(ref, remote)) + for _, rr := range refsForRemote { + // HACK(@ttaylorr): add remote name to fully-qualify + // references: + rr.Name = fmt.Sprintf("%s/%s", remote, rr.Name) + + refs = append(refs, rr) } } From fd07903ec35d9deaa9ccb111bc430bec124d616b Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 15:45:17 -0800 Subject: [PATCH 59/71] test: remove early 'exit' --- test/test-migrate-import.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/test-migrate-import.sh b/test/test-migrate-import.sh index ccf57fb1..c8c9e24f 100755 --- a/test/test-migrate-import.sh +++ b/test/test-migrate-import.sh @@ -403,8 +403,6 @@ begin_test "migrate import (--everything with --include-ref)" ) end_test -exit 0 - begin_test "migrate import (--everything with --exclude-ref)" ( set -e From 55997fae876a91708e6503d0f4a0d8383191b0fd Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 15:45:30 -0800 Subject: [PATCH 60/71] test: ensure 'migrate import' works with ambiguous references --- test/test-migrate-import.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/test-migrate-import.sh b/test/test-migrate-import.sh index c8c9e24f..4a15da67 100755 --- a/test/test-migrate-import.sh +++ b/test/test-migrate-import.sh @@ -380,6 +380,19 @@ begin_test "migrate import (--everything)" ) end_test +begin_test "migrate import (ambiguous reference)" +( + set -e + + setup_multiple_local_branches + + # Create an ambiguously named reference sharing the name as the SHA-1 of + # "HEAD". + git tag "$sha" + + git lfs migrate import --everything +) +end_test begin_test "migrate import (--everything with args)" ( From e9a97f9e622090e7a56f51781df1bde7d25dfc6f Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 16 Nov 2017 15:45:44 -0800 Subject: [PATCH 61/71] test: ensure 'migrate info' works with ambiguous references --- test/test-migrate-info.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/test-migrate-info.sh b/test/test-migrate-info.sh index 58fef7cf..a3595e97 100755 --- a/test/test-migrate-info.sh +++ b/test/test-migrate-info.sh @@ -307,6 +307,20 @@ begin_test "migrate info (--everything)" ) end_test +begin_test "migrate info (ambiguous reference)" +( + set -e + + setup_multiple_local_branches + + # Create an ambiguously named reference sharing the name as the SHA-1 of + # "HEAD". + git tag "$sha" + + git lfs migrate info --everything +) +end_test + begin_test "migrate info (--everything with args)" ( set -e From 9ae89079be8e9e1c74843d9813d47df9d3d01251 Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 17 Nov 2017 08:42:51 -0700 Subject: [PATCH 62/71] git: rename Ref.String to Ref.Refspec --- commands/command_migrate.go | 6 +++--- git/git.go | 21 +++++++++------------ git/git_test.go | 2 +- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/commands/command_migrate.go b/commands/command_migrate.go index 9917b3a7..e610093e 100644 --- a/commands/command_migrate.go +++ b/commands/command_migrate.go @@ -123,7 +123,7 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string return nil, nil, err } - include = append(include, ref.String()) + include = append(include, ref.Refspec()) } if hardcore { @@ -143,7 +143,7 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string } for _, ref := range localRefs { - include = append(include, ref.String()) + include = append(include, ref.Refspec()) } } else { // Otherwise, if neither --include-ref= or @@ -156,7 +156,7 @@ func includeExcludeRefs(l *log.Logger, args []string) (include, exclude []string } for _, rr := range remoteRefs { - exclude = append(exclude, rr.String()) + exclude = append(exclude, rr.Refspec()) } } diff --git a/git/git.go b/git/git.go index 913189a5..2870a6c7 100644 --- a/git/git.go +++ b/git/git.go @@ -95,12 +95,16 @@ type Ref struct { Sha string } -// String implements fmt.Stringer.String and returns the fully-qualified -// reference name (including remote), i.e., for a remote branch called -// 'my-feature' on remote 'origin', this function will return: +// Refspec returns the fully-qualified reference name (including remote), i.e., +// for a remote branch called 'my-feature' on remote 'origin', this function +// will return: // // refs/remotes/origin/my-feature -func (r *Ref) String() string { +func (r *Ref) Refspec() string { + if r == nil { + return "" + } + prefix, ok := r.Type.Prefix() if ok { return fmt.Sprintf("%s/%s", prefix, r.Name) @@ -387,14 +391,7 @@ func UpdateRef(ref *Ref, to []byte, reason string) error { // reflog entry, if a "reason" was provided). It operates within the given // working directory "wd". It returns an error if any were encountered. func UpdateRefIn(wd string, ref *Ref, to []byte, reason string) error { - var refspec string - if prefix, ok := ref.Type.Prefix(); ok { - refspec = fmt.Sprintf("%s/%s", prefix, ref.Name) - } else { - refspec = ref.Name - } - - args := []string{"update-ref", refspec, hex.EncodeToString(to)} + args := []string{"update-ref", ref.Refspec(), hex.EncodeToString(to)} if len(reason) > 0 { args = append(args, "-m", reason) } diff --git a/git/git_test.go b/git/git_test.go index f474afa9..375551d7 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -47,7 +47,7 @@ func TestRefString(t *testing.T) { Sha: sha, }, } { - assert.Equal(t, s, r.String()) + assert.Equal(t, s, r.Refspec()) } } From b0f3d49e712870b0e8947204a31ce36008caa58d Mon Sep 17 00:00:00 2001 From: rick olson Date: Fri, 17 Nov 2017 08:45:12 -0700 Subject: [PATCH 63/71] use a channel to ensure STDOUT receives all test results before exiting --- script/integration.go | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/script/integration.go b/script/integration.go index 68b1beb8..e67b30a9 100644 --- a/script/integration.go +++ b/script/integration.go @@ -50,17 +50,24 @@ func mainIntegration() { for _, file := range files { tests <- file } + close(tests) + + outputDone := make(chan bool) + go func() { + for out := range output { + fmt.Println(out) + } + outputDone <- true + }() - go printOutput(output) for i := 0; i < maxprocs; i++ { wg.Add(1) go worker(tests, output, &wg) } - close(tests) wg.Wait() close(output) - printOutput(output) + <-outputDone if erroring { os.Exit(1) @@ -116,19 +123,6 @@ func sendTestOutput(output chan string, testname string, buf *bytes.Buffer, err } } -func printOutput(output <-chan string) { - for { - select { - case out, ok := <-output: - if !ok { - return - } - - fmt.Println(out) - } - } -} - func worker(tests <-chan string, output chan string, wg *sync.WaitGroup) { defer wg.Done() for { From b038093f1d8b43d128ff76284acc07f9ae4b339f Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 08:11:17 -0800 Subject: [PATCH 64/71] test: add removed $sha declaration --- test/test-migrate-import.sh | 1 + test/test-migrate-info.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/test/test-migrate-import.sh b/test/test-migrate-import.sh index 4a15da67..3870b2e8 100755 --- a/test/test-migrate-import.sh +++ b/test/test-migrate-import.sh @@ -388,6 +388,7 @@ begin_test "migrate import (ambiguous reference)" # Create an ambiguously named reference sharing the name as the SHA-1 of # "HEAD". + sha="$(git rev-parse HEAD)" git tag "$sha" git lfs migrate import --everything diff --git a/test/test-migrate-info.sh b/test/test-migrate-info.sh index a3595e97..cf763b6d 100755 --- a/test/test-migrate-info.sh +++ b/test/test-migrate-info.sh @@ -315,6 +315,7 @@ begin_test "migrate info (ambiguous reference)" # Create an ambiguously named reference sharing the name as the SHA-1 of # "HEAD". + sha="$(git rev-parse HEAD)" git tag "$sha" git lfs migrate info --everything From 85faf514b85266a08d1a37a12d1f400b1f31523f Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 08:21:22 -0800 Subject: [PATCH 65/71] test: ensure commented attr lines are ignored --- test/test-track.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/test-track.sh b/test/test-track.sh index 03ac091c..65ec3423 100755 --- a/test/test-track.sh +++ b/test/test-track.sh @@ -513,3 +513,30 @@ begin_test "track (\$GIT_LFS_TRACK_NO_INSTALL_HOOKS)" [ ! -f .git/hooks/post-merge ] ) end_test + +begin_test "track (with comments)" +( + set -e + + reponame="track-with=comments" + git init "$reponame" + cd "$reponame" + + echo "*.jpg filter=lfs diff=lfs merge=lfs -text" >> .gitattributes + echo "# *.png filter=lfs diff=lfs merge=lfs -text" >> .gitattributes + echo "*.pdf filter=lfs diff=lfs merge=lfs -text" >> .gitattributes + + git add .gitattributes + git commit -m "initial commit" + + git lfs track 2>&1 | tee track.log + if [ "0" -ne "${PIPESTATUS[0]}" ]; then + echo >&2 "expected \`git lfs track\` command to exit cleanly, didn't" + exit 1 + fi + + [ "1" -eq "$(grep -c "\.jpg" track.log)" ] + [ "1" -eq "$(grep -c "\.pdf" track.log)" ] + [ "0" -eq "$(grep -c "\.png" track.log)" ] +) +end_test From d734c54a2feff07850d37a00f210db75ce7b4ed6 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 08:45:30 -0800 Subject: [PATCH 66/71] commands: add '--assume-current-remote-refs' to avoid ls-remote call --- commands/command_migrate.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/commands/command_migrate.go b/commands/command_migrate.go index c15e8185..96eba137 100644 --- a/commands/command_migrate.go +++ b/commands/command_migrate.go @@ -20,6 +20,11 @@ var ( // in the migration. migrateExcludeRefs []string + // migrateAssumeCurrentRemoteReferences assumes that the client has the + // latest copy of remote references, and thus should not contact the + // remote for a set of updated references. + migrateAssumeCurrentRemoteReferences bool + // migrateEverything indicates the presence of the --everything flag, // and instructs 'git lfs migrate' to migrate all local references. migrateEverything bool @@ -171,14 +176,22 @@ func getRemoteRefs(l *log.Logger) ([]string, error) { return nil, err } - w := l.Waiter("migrate: Fetching remote refs") - if err := git.Fetch(remotes...); err != nil { - return nil, err + if !migrateAssumeCurrentRemoteReferences { + w := l.Waiter("migrate: Fetching remote refs") + if err := git.Fetch(remotes...); err != nil { + return nil, err + } + w.Complete() } - w.Complete() for _, remote := range remotes { - refsForRemote, err := git.RemoteRefs(remote) + var refsForRemote []*git.Ref + if migrateAssumeCurrentRemoteReferences { + refsForRemote, err = git.CachedRemoteRefs(remote) + } else { + refsForRemote, err = git.RemoteRefs(remote) + } + if err != nil { return nil, err } @@ -252,6 +265,7 @@ func init() { cmd.PersistentFlags().StringSliceVar(&migrateIncludeRefs, "include-ref", nil, "An explicit list of refs to include") cmd.PersistentFlags().StringSliceVar(&migrateExcludeRefs, "exclude-ref", nil, "An explicit list of refs to exclude") cmd.PersistentFlags().BoolVar(&migrateEverything, "everything", false, "Migrate all local references") + cmd.PersistentFlags().BoolVar(&migrateAssumeCurrentRemoteReferences, "assume-current-remote-refs", false, "Assume up-to-date remote references.") cmd.AddCommand(importCmd, info) }) From 0c1b405bf8aaf0d6954470fa76da65f3b91c9b27 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 08:45:54 -0800 Subject: [PATCH 67/71] docs: document '--assume-current-remote-ref' --- docs/man/git-lfs-migrate.1.ronn | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/man/git-lfs-migrate.1.ronn b/docs/man/git-lfs-migrate.1.ronn index 82e37b05..9abc2d25 100644 --- a/docs/man/git-lfs-migrate.1.ronn +++ b/docs/man/git-lfs-migrate.1.ronn @@ -27,6 +27,11 @@ git-lfs-migrate(1) - Migrate history to or from git-lfs * `--exclude-ref`=: See [INCLUDE AND EXCLUDE (REFS)]. +* `--assume-current-remote-refs`: + Assumes that the known set of remote references is complete, and should not + be refreshed when determining the set of "un-pushed" commits to migrate. Has + no effect when combined with `--include-ref` or `--exclude-ref`. + * `--everything`: See [INCLUDE AND EXCLUDE (REFS)]. From dfac5ad11af82a7c9517060e66edfc2766de8563 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 08:46:13 -0800 Subject: [PATCH 68/71] test: ensure 'migrate import' works with '--assume-current-remote-refs' --- test/test-migrate-import.sh | 41 +++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/test-migrate-import.sh b/test/test-migrate-import.sh index ccf57fb1..29f650b2 100755 --- a/test/test-migrate-import.sh +++ b/test/test-migrate-import.sh @@ -202,6 +202,47 @@ begin_test "migrate import (given branch, exclude remote refs)" ) end_test +begin_test "migrate import (given ref, --assume-current-remote-refs)" +( + set -e + + setup_single_remote_branch + + md_master_oid="$(calc_oid "$(git cat-file -p "refs/heads/master:a.md")")" + md_remote_oid="$(calc_oid "$(git cat-file -p "refs/remotes/origin/master:a.md")")" + txt_master_oid="$(calc_oid "$(git cat-file -p "refs/heads/master:a.txt")")" + txt_remote_oid="$(calc_oid "$(git cat-file -p "refs/remotes/origin/master:a.txt")")" + + git tag pseudo-remote "$(git rev-parse refs/remotes/origin/master)" + # Remove the refs/remotes/origin/master ref, and instruct 'git lfs migrate' to + # not fetch it. + git update-ref -d refs/remotes/origin/master + + git lfs migrate import --assume-current-remote-refs + + assert_pointer "refs/heads/master" "a.md" "$md_master_oid" "50" + assert_pointer "pseudo-remote" "a.md" "$md_remote_oid" "140" + assert_pointer "refs/heads/master" "a.txt" "$txt_master_oid" "30" + assert_pointer "pseudo-remote" "a.txt" "$txt_remote_oid" "120" + + assert_local_object "$md_master_oid" "50" + assert_local_object "$txt_master_oid" "30" + assert_local_object "$md_remote_oid" "140" + assert_local_object "$txt_remote_oid" "120" + + master="$(git rev-parse refs/heads/master)" + remote="$(git rev-parse pseudo-remote)" + + master_attrs="$(git cat-file -p "$master:.gitattributes")" + remote_attrs="$(git cat-file -p "$remote:.gitattributes")" + + echo "$master_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs" + echo "$master_attrs" | grep -q "*.txt filter=lfs diff=lfs merge=lfs" + echo "$remote_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs" + echo "$remote_attrs" | grep -q "*.txt filter=lfs diff=lfs merge=lfs" +) +end_test + begin_test "migrate import (include/exclude ref)" ( set -e From 3860b5e44335c6256019eb6d050160b14ef49a23 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 08:46:33 -0800 Subject: [PATCH 69/71] test: ensure 'migrate info' works with '--assume-current-remote-refs' --- test/test-migrate-info.sh | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/test-migrate-info.sh b/test/test-migrate-info.sh index 58fef7cf..b6c4ac41 100755 --- a/test/test-migrate-info.sh +++ b/test/test-migrate-info.sh @@ -132,6 +132,33 @@ begin_test "migrate info (given branch, exclude remote refs)" ) end_test +begin_test "migrate info (given ref, --assume-current-remote-refs)" +( + set -e + + setup_single_remote_branch + + original_remote="$(git rev-parse refs/remotes/origin/master)" + original_master="$(git rev-parse refs/heads/master)" + + git tag pseudo-remote "$original_remote" + # Remove the refs/remotes/origin/master ref, and instruct 'git lfs migrate' to + # not fetch it. + git update-ref -d refs/remotes/origin/master + + diff -u <(git lfs migrate info --assume-current-remote-refs 2>&1 | tail -n 2) <(cat <<-EOF + *.md 190 B 2/2 files(s) 100% + *.txt 150 B 2/2 files(s) 100% + EOF) + + migrated_remote="$(git rev-parse pseudo-remote)" + migrated_master="$(git rev-parse refs/heads/master)" + + assert_ref_unmoved "refs/remotes/origin/master" "$original_remote" "$migrated_remote" + assert_ref_unmoved "refs/heads/master" "$original_master" "$migrated_master" +) +end_test + begin_test "migrate info (include/exclude ref)" ( set -e From 73dcf2693809ea4074d4e352041086458f43062b Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Fri, 17 Nov 2017 09:05:01 -0800 Subject: [PATCH 70/71] git: prefer sending revisions over STDIN than arguments --- git/rev_list_scanner.go | 9 ++++---- git/rev_list_scanner_test.go | 42 ++++++++++++++++++++---------------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/git/rev_list_scanner.go b/git/rev_list_scanner.go index 294dbe5a..58f28457 100644 --- a/git/rev_list_scanner.go +++ b/git/rev_list_scanner.go @@ -225,7 +225,7 @@ func NewRevListScanner(include, excluded []string, opt *ScanRefsOptions) (*RevLi // occurred. func revListArgs(include, exclude []string, opt *ScanRefsOptions) (io.Reader, []string, error) { var stdin io.Reader - args := []string{"rev-list"} + args := []string{"rev-list", "--stdin"} if !opt.CommitsOnly { args = append(args, "--objects") } @@ -246,15 +246,16 @@ func revListArgs(include, exclude []string, opt *ScanRefsOptions) (io.Reader, [] args = append(args, "--do-walk") } - args = append(args, includeExcludeShas(include, exclude)...) + stdin = strings.NewReader(strings.Join( + includeExcludeShas(include, exclude), "\n")) case ScanAllMode: args = append(args, "--all") case ScanLeftToRemoteMode: if len(opt.SkippedRefs) == 0 { - args = append(args, includeExcludeShas(include, exclude)...) args = append(args, "--not", "--remotes="+opt.Remote) + stdin = strings.NewReader(strings.Join( + includeExcludeShas(include, exclude), "\n")) } else { - args = append(args, "--stdin") stdin = strings.NewReader(strings.Join( append(includeExcludeShas(include, exclude), opt.SkippedRefs...), "\n"), ) diff --git a/git/rev_list_scanner_test.go b/git/rev_list_scanner_test.go index ab29417c..70bd9985 100644 --- a/git/rev_list_scanner_test.go +++ b/git/rev_list_scanner_test.go @@ -4,13 +4,13 @@ import ( "bufio" "encoding/hex" "errors" + "fmt" "io/ioutil" "strings" "sync/atomic" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) type ArgsTestCase struct { @@ -32,11 +32,7 @@ func (c *ArgsTestCase) Assert(t *testing.T) { assert.Nil(t, err) } - require.Equal(t, len(c.ExpectedArgs), len(args)) - for i := 0; i < len(c.ExpectedArgs); i++ { - assert.Equal(t, c.ExpectedArgs[i], args[i], - "element #%d not equal: wanted %q, got %q", i, c.ExpectedArgs[i], args[i]) - } + assert.EqualValues(t, c.ExpectedArgs, args) if stdin != nil { b, err := ioutil.ReadAll(stdin) @@ -60,34 +56,38 @@ func TestRevListArgs(t *testing.T) { Mode: ScanRefsMode, SkipDeletedBlobs: false, }, - ExpectedArgs: []string{"rev-list", "--objects", "--do-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--do-walk", "--"}, }, "scan refs not deleted, left and right": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, SkipDeletedBlobs: true, }, - ExpectedArgs: []string{"rev-list", "--objects", "--no-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--no-walk", "--"}, }, "scan refs deleted, left only": { Include: []string{s1}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, SkipDeletedBlobs: false, }, - ExpectedArgs: []string{"rev-list", "--objects", "--do-walk", s1, "--"}, + ExpectedStdin: s1, + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--do-walk", "--"}, }, "scan refs not deleted, left only": { Include: []string{s1}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, SkipDeletedBlobs: true, }, - ExpectedArgs: []string{"rev-list", "--objects", "--no-walk", s1, "--"}, + ExpectedStdin: s1, + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--no-walk", "--"}, }, "scan all": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ Mode: ScanAllMode, }, - ExpectedArgs: []string{"rev-list", "--objects", "--all", "--"}, + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--all", "--"}, }, "scan left to remote, no skipped refs": { Include: []string{s1}, Opt: &ScanRefsOptions{ @@ -95,7 +95,8 @@ func TestRevListArgs(t *testing.T) { Remote: "origin", SkippedRefs: []string{}, }, - ExpectedArgs: []string{"rev-list", "--objects", s1, "--not", "--remotes=origin", "--"}, + ExpectedStdin: s1, + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--not", "--remotes=origin", "--"}, }, "scan left to remote, skipped refs": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ @@ -103,7 +104,7 @@ func TestRevListArgs(t *testing.T) { Remote: "origin", SkippedRefs: []string{"a", "b", "c"}, }, - ExpectedArgs: []string{"rev-list", "--objects", "--stdin", "--"}, + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--"}, ExpectedStdin: s1 + "\n^" + s2 + "\na\nb\nc", }, "scan unknown type": { @@ -117,35 +118,40 @@ func TestRevListArgs(t *testing.T) { Mode: ScanRefsMode, Order: DateRevListOrder, }, - ExpectedArgs: []string{"rev-list", "--objects", "--date-order", "--do-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--date-order", "--do-walk", "--"}, }, "scan author date order": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, Order: AuthorDateRevListOrder, }, - ExpectedArgs: []string{"rev-list", "--objects", "--author-date-order", "--do-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--author-date-order", "--do-walk", "--"}, }, "scan topo order": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, Order: TopoRevListOrder, }, - ExpectedArgs: []string{"rev-list", "--objects", "--topo-order", "--do-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--topo-order", "--do-walk", "--"}, }, "scan commits only": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, CommitsOnly: true, }, - ExpectedArgs: []string{"rev-list", "--do-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--do-walk", "--"}, }, "scan reverse": { Include: []string{s1}, Exclude: []string{s2}, Opt: &ScanRefsOptions{ Mode: ScanRefsMode, Reverse: true, }, - ExpectedArgs: []string{"rev-list", "--objects", "--reverse", "--do-walk", s1, "^" + s2, "--"}, + ExpectedStdin: fmt.Sprintf("%s\n^%s", s1, s2), + ExpectedArgs: []string{"rev-list", "--stdin", "--objects", "--reverse", "--do-walk", "--"}, }, } { t.Run(desc, c.Assert) From e57c1b38aa5c472e1b6a74cb1faa38a6dc7cb6d1 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Mon, 20 Nov 2017 09:48:50 -0800 Subject: [PATCH 71/71] commands,docs,test: rename to '--skip-fetch' --- commands/command_migrate.go | 14 +++++++------- docs/man/git-lfs-migrate.1.ronn | 2 +- test/test-migrate-import.sh | 4 ++-- test/test-migrate-info.sh | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/commands/command_migrate.go b/commands/command_migrate.go index 96eba137..53e2c9f0 100644 --- a/commands/command_migrate.go +++ b/commands/command_migrate.go @@ -20,10 +20,10 @@ var ( // in the migration. migrateExcludeRefs []string - // migrateAssumeCurrentRemoteReferences assumes that the client has the - // latest copy of remote references, and thus should not contact the - // remote for a set of updated references. - migrateAssumeCurrentRemoteReferences bool + // migrateSkipFetch assumes that the client has the latest copy of + // remote references, and thus should not contact the remote for a set + // of updated references. + migrateSkipFetch bool // migrateEverything indicates the presence of the --everything flag, // and instructs 'git lfs migrate' to migrate all local references. @@ -176,7 +176,7 @@ func getRemoteRefs(l *log.Logger) ([]string, error) { return nil, err } - if !migrateAssumeCurrentRemoteReferences { + if !migrateSkipFetch { w := l.Waiter("migrate: Fetching remote refs") if err := git.Fetch(remotes...); err != nil { return nil, err @@ -186,7 +186,7 @@ func getRemoteRefs(l *log.Logger) ([]string, error) { for _, remote := range remotes { var refsForRemote []*git.Ref - if migrateAssumeCurrentRemoteReferences { + if migrateSkipFetch { refsForRemote, err = git.CachedRemoteRefs(remote) } else { refsForRemote, err = git.RemoteRefs(remote) @@ -265,7 +265,7 @@ func init() { cmd.PersistentFlags().StringSliceVar(&migrateIncludeRefs, "include-ref", nil, "An explicit list of refs to include") cmd.PersistentFlags().StringSliceVar(&migrateExcludeRefs, "exclude-ref", nil, "An explicit list of refs to exclude") cmd.PersistentFlags().BoolVar(&migrateEverything, "everything", false, "Migrate all local references") - cmd.PersistentFlags().BoolVar(&migrateAssumeCurrentRemoteReferences, "assume-current-remote-refs", false, "Assume up-to-date remote references.") + cmd.PersistentFlags().BoolVar(&migrateSkipFetch, "skip-fetch", false, "Assume up-to-date remote references.") cmd.AddCommand(importCmd, info) }) diff --git a/docs/man/git-lfs-migrate.1.ronn b/docs/man/git-lfs-migrate.1.ronn index 9abc2d25..1d7ca703 100644 --- a/docs/man/git-lfs-migrate.1.ronn +++ b/docs/man/git-lfs-migrate.1.ronn @@ -27,7 +27,7 @@ git-lfs-migrate(1) - Migrate history to or from git-lfs * `--exclude-ref`=: See [INCLUDE AND EXCLUDE (REFS)]. -* `--assume-current-remote-refs`: +* `--skip-fetch`: Assumes that the known set of remote references is complete, and should not be refreshed when determining the set of "un-pushed" commits to migrate. Has no effect when combined with `--include-ref` or `--exclude-ref`. diff --git a/test/test-migrate-import.sh b/test/test-migrate-import.sh index 29f650b2..dad70f93 100755 --- a/test/test-migrate-import.sh +++ b/test/test-migrate-import.sh @@ -202,7 +202,7 @@ begin_test "migrate import (given branch, exclude remote refs)" ) end_test -begin_test "migrate import (given ref, --assume-current-remote-refs)" +begin_test "migrate import (given ref, --skip-fetch)" ( set -e @@ -218,7 +218,7 @@ begin_test "migrate import (given ref, --assume-current-remote-refs)" # not fetch it. git update-ref -d refs/remotes/origin/master - git lfs migrate import --assume-current-remote-refs + git lfs migrate import --skip-fetch assert_pointer "refs/heads/master" "a.md" "$md_master_oid" "50" assert_pointer "pseudo-remote" "a.md" "$md_remote_oid" "140" diff --git a/test/test-migrate-info.sh b/test/test-migrate-info.sh index b6c4ac41..f948d47a 100755 --- a/test/test-migrate-info.sh +++ b/test/test-migrate-info.sh @@ -132,7 +132,7 @@ begin_test "migrate info (given branch, exclude remote refs)" ) end_test -begin_test "migrate info (given ref, --assume-current-remote-refs)" +begin_test "migrate info (given ref, --skip-fetch)" ( set -e @@ -146,7 +146,7 @@ begin_test "migrate info (given ref, --assume-current-remote-refs)" # not fetch it. git update-ref -d refs/remotes/origin/master - diff -u <(git lfs migrate info --assume-current-remote-refs 2>&1 | tail -n 2) <(cat <<-EOF + diff -u <(git lfs migrate info --skip-fetch 2>&1 | tail -n 2) <(cat <<-EOF *.md 190 B 2/2 files(s) 100% *.txt 150 B 2/2 files(s) 100% EOF)