From 92e7a27137b900d494e70ed5247776cf7b19e3d1 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Fri, 17 Apr 2020 14:10:54 +0000 Subject: [PATCH] pre-push: find named remote for URL if possible Some users use tools like JGit, which use URLs instead of named remotes when invoking the pre-push hook. This is problematic because we use remote-tracking branches to optimize our ref walk and when a user is using a URL, we can't apply that optimization since we don't know which named remote is being used. However, if we have a named remote with only one URL, we can apply this optimization by looking up the remote name and using that as our remote instead. We do this only in the pre-push hook, since this is the major time that we need to walk the history. Add some unit and integration tests for this and also update one of the existing integration tests which was relying on the previous behavior by switching it to use an equivalent URL instead of the actual remote URL. --- commands/command_pre_push.go | 3 +- git/git.go | 48 ++++++++++++++++++++ git/git_test.go | 87 ++++++++++++++++++++++++++++++++++++ t/t-pre-push.sh | 32 ++++++++++--- 4 files changed, 164 insertions(+), 6 deletions(-) diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 6b9f9170..0983888c 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -47,7 +47,8 @@ func prePushCommand(cmd *cobra.Command, args []string) { requireGitVersion() // Remote is first arg - if err := cfg.SetValidPushRemote(args[0]); err != nil { + remote, _ := git.MapRemoteURL(args[0], true) + if err := cfg.SetValidPushRemote(remote); err != nil { Exit("Invalid remote name %q: %s", args[0], err) } diff --git a/git/git.go b/git/git.go index 700ef7da..1453072f 100644 --- a/git/git.go +++ b/git/git.go @@ -353,6 +353,54 @@ func RemoteList() ([]string, error) { return ret, nil } +func RemoteURLs(push bool) (map[string][]string, error) { + cmd := gitNoLFS("remote", "-v") + + outp, err := cmd.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("failed to call git remote -v: %v", err) + } + cmd.Start() + defer cmd.Wait() + + scanner := bufio.NewScanner(outp) + + text := "(fetch)" + if push { + text = "(push)" + } + ret := make(map[string][]string) + for scanner.Scan() { + // [remote, urlpair-text] + pair := strings.Split(strings.TrimSpace(scanner.Text()), "\t") + if len(pair) != 2 { + continue + } + // [url, "(fetch)" | "(push)"] + urlpair := strings.Split(pair[1], " ") + if len(urlpair) != 2 || urlpair[1] != text { + continue + } + ret[pair[0]] = append(ret[pair[0]], urlpair[0]) + } + + return ret, nil +} + +func MapRemoteURL(url string, push bool) (string, bool) { + urls, err := RemoteURLs(push) + if err != nil { + return url, false + } + + for name, remotes := range urls { + if len(remotes) == 1 && url == remotes[0] { + return name, true + } + } + return url, false +} + // Refs returns all of the local and remote branches and tags for the current // repository. Other refs (HEAD, refs/stash, git notes) are ignored. func LocalRefs() ([]*Ref, error) { diff --git a/git/git_test.go b/git/git_test.go index 866b8b7d..a8db9228 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -702,3 +702,90 @@ func TestRefTypeKnownPrefixes(t *testing.T) { assert.Equal(t, expected.Ok, ok) } } + +func TestRemoteURLs(t *testing.T) { + repo := test.NewRepo(t) + repo.Pushd() + defer func() { + repo.Popd() + repo.Cleanup() + }() + cfg := repo.GitConfig() + cfg.SetLocal("remote.foo.url", "https://github.com/git-lfs/git-lfs.git") + cfg.SetLocal("remote.bar.url", "https://github.com/git-lfs/wildmatch.git") + cfg.SetLocal("remote.bar.pushurl", "https://github.com/git-lfs/pktline.git") + + expected := make(map[string][]string) + expected["foo"] = []string{"https://github.com/git-lfs/git-lfs.git"} + expected["bar"] = []string{"https://github.com/git-lfs/wildmatch.git"} + actual, err := RemoteURLs(false) + assert.Nil(t, err) + assert.Equal(t, expected, actual) + + expected["bar"] = []string{"https://github.com/git-lfs/pktline.git"} + actual, err = RemoteURLs(true) + assert.Nil(t, err) + assert.Equal(t, expected, actual) +} + +func TestMapRemoteURL(t *testing.T) { + repo := test.NewRepo(t) + repo.Pushd() + defer func() { + repo.Popd() + repo.Cleanup() + }() + cfg := repo.GitConfig() + cfg.SetLocal("remote.foo.url", "https://github.com/git-lfs/git-lfs.git") + cfg.SetLocal("remote.bar.url", "https://github.com/git-lfs/wildmatch.git") + cfg.SetLocal("remote.bar.pushurl", "https://github.com/git-lfs/pktline.git") + + tests := []struct { + url string + push bool + match bool + val string + }{ + { + "https://github.com/git-lfs/git-lfs.git", + false, + true, + "foo", + }, + { + "https://github.com/git-lfs/git-lfs.git", + true, + true, + "foo", + }, + { + "https://github.com/git-lfs/wildmatch.git", + false, + true, + "bar", + }, + { + "https://github.com/git-lfs/pktline.git", + true, + true, + "bar", + }, + { + "https://github.com/git-lfs/pktline.git", + false, + false, + "https://github.com/git-lfs/pktline.git", + }, + { + "https://github.com/git/git.git", + true, + false, + "https://github.com/git/git.git", + }, + } + for _, test := range tests { + val, ok := MapRemoteURL(test.url, test.push) + assert.Equal(t, ok, test.match) + assert.Equal(t, val, test.val) + } +} diff --git a/t/t-pre-push.sh b/t/t-pre-push.sh index 0f439e64..fc63eed7 100755 --- a/t/t-pre-push.sh +++ b/t/t-pre-push.sh @@ -1181,6 +1181,28 @@ begin_test "pre-push with pushDefault and explicit remote" ) end_test +begin_test "pre-push uses optimization if remote URL matches" +( + set -e + reponame="pre-push-remote-url-optimization" + setup_remote_repo "$reponame" + clone_repo "$reponame" "$reponame" + + endpoint=$(git config remote.origin.url) + contents_oid=$(calc_oid 'hi\n') + git config "lfs.$endpoint.locksverify" false + git lfs track "*.dat" + echo "hi" > a.dat + git add .gitattributes a.dat + git commit -m "add a.dat" + + refute_server_object "$reponame" $contents_oid "refs/heads/master" + + GIT_TRACE=1 GIT_TRANSFER_TRACE=1 git push "$endpoint" master 2>&1 | tee push.log + grep 'rev-list.*--not --remotes=origin' push.log +) +end_test + begin_test "pre-push does not traverse Git objects server has" ( set -e @@ -1198,10 +1220,10 @@ begin_test "pre-push does not traverse Git objects server has" refute_server_object "$reponame" $contents_oid "refs/heads/master" - # We use a URL instead of a named remote so that we can't make use of the - # optimization that ignores objects we already have in remote tracking - # branches. - GIT_TRACE=1 GIT_TRANSFER_TRACE=1 git push "$endpoint" master 2>&1 | tee push.log + # We use a different URL instead of a named remote or the remote URL so that + # we can't make use of the optimization that ignores objects we already have + # in remote tracking branches. + GIT_TRACE=1 GIT_TRANSFER_TRACE=1 git push "$endpoint.git" master 2>&1 | tee push.log assert_server_object "$reponame" $contents_oid "refs/heads/master" @@ -1212,7 +1234,7 @@ begin_test "pre-push does not traverse Git objects server has" refute_server_object "$reponame" $contents2_oid "refs/heads/master" - GIT_TRACE=1 GIT_TRANSFER_TRACE=1 git push "$endpoint" master 2>&1 | tee push.log + GIT_TRACE=1 GIT_TRANSFER_TRACE=1 git push "$endpoint.git" master 2>&1 | tee push.log assert_server_object "$reponame" $contents2_oid "refs/heads/master"