From 66ad6ad7d06d517a1dd69daccdd3ad0ac56bfe87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 9 Aug 2021 22:10:52 +0200 Subject: [PATCH 1/4] Avoid `git checkout` during `gh repo sync` - If the local branch already exists, use `git update-ref` - If it needs to be created, use `git branch `, but don't switch to the new branch Bonus fixes - Enables operation while on detached HEAD - Enables operation even when the current remote doesn't track all branches in the remote repo (uses FETCH_HEAD instead of the `/` syntax) --- pkg/cmd/repo/sync/git.go | 23 +++++--- pkg/cmd/repo/sync/mocks.go | 8 +-- pkg/cmd/repo/sync/sync.go | 45 +++++++------- pkg/cmd/repo/sync/sync_test.go | 103 +++++++++++++-------------------- 4 files changed, 79 insertions(+), 100 deletions(-) diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index c328c7a85..9976a923b 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -9,9 +9,9 @@ import ( type gitClient interface { BranchRemote(string) (string, error) - CheckoutLocal(string) error - CheckoutRemote(string, string) error CurrentBranch() (string, error) + UpdateBranch(string, string) error + CreateBranch(string, string, string) error Fetch(string, string) error HasLocalBranch(string) bool IsAncestor(string, string) (bool, error) @@ -36,18 +36,23 @@ func (g *gitExecuter) BranchRemote(branch string) (string, error) { return parts[0], nil } -func (g *gitExecuter) CheckoutLocal(branch string) error { - args := []string{"checkout", branch} - cmd, err := git.GitCommand(args...) +func (g *gitExecuter) UpdateBranch(branch, ref string) error { + cmd, err := git.GitCommand("update-ref", fmt.Sprintf("refs/heads/%s", branch), ref) if err != nil { return err } return cmd.Run() } -func (g *gitExecuter) CheckoutRemote(remote, branch string) error { - args := []string{"checkout", "--track", fmt.Sprintf("%s/%s", remote, branch)} - cmd, err := git.GitCommand(args...) +func (g *gitExecuter) CreateBranch(branch, ref, upstream string) error { + cmd, err := git.GitCommand("branch", branch, ref) + if err != nil { + return err + } + if err := cmd.Run(); err != nil { + return err + } + cmd, err = git.GitCommand("branch", "--set-upstream-to", upstream, branch) if err != nil { return err } @@ -97,7 +102,7 @@ func (g *gitExecuter) IsDirty() (bool, error) { } func (g *gitExecuter) MergeFastForward(ref string) error { - args := []string{"merge", "--ff-only", ref} + args := []string{"merge", "--ff-only", "--quiet", ref} cmd, err := git.GitCommand(args...) if err != nil { return err diff --git a/pkg/cmd/repo/sync/mocks.go b/pkg/cmd/repo/sync/mocks.go index 7b83157a5..718a89054 100644 --- a/pkg/cmd/repo/sync/mocks.go +++ b/pkg/cmd/repo/sync/mocks.go @@ -13,13 +13,13 @@ func (g *mockGitClient) BranchRemote(a string) (string, error) { return args.String(0), args.Error(1) } -func (g *mockGitClient) CheckoutLocal(a string) error { - args := g.Called(a) +func (g *mockGitClient) UpdateBranch(b, r string) error { + args := g.Called(b, r) return args.Error(0) } -func (g *mockGitClient) CheckoutRemote(a, b string) error { - args := g.Called(a, b) +func (g *mockGitClient) CreateBranch(b, r, u string) error { + args := g.Called(b, r, u) return args.Error(0) } diff --git a/pkg/cmd/repo/sync/sync.go b/pkg/cmd/repo/sync/sync.go index beb113887..68ae12707 100644 --- a/pkg/cmd/repo/sync/sync.go +++ b/pkg/cmd/repo/sync/sync.go @@ -9,6 +9,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" "github.com/cli/cli/context" + gitpkg "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -238,6 +239,7 @@ var mismatchRemotesError = errors.New("branch remote does not match specified so func executeLocalRepoSync(srcRepo ghrepo.Interface, remote string, opts *SyncOptions) error { git := opts.Git branch := opts.Branch + useForce := opts.Force if err := git.Fetch(remote, fmt.Sprintf("refs/heads/%s", branch)); err != nil { return err @@ -253,47 +255,44 @@ func executeLocalRepoSync(srcRepo ghrepo.Interface, remote string, opts *SyncOpt return mismatchRemotesError } - fastForward, err := git.IsAncestor(branch, fmt.Sprintf("%s/%s", remote, branch)) + fastForward, err := git.IsAncestor(branch, "FETCH_HEAD") if err != nil { return err } - if !fastForward && !opts.Force { + if !fastForward && !useForce { return divergingError } + if fastForward && useForce { + useForce = false + } } - startBranch, err := git.CurrentBranch() - if err != nil { + currentBranch, err := git.CurrentBranch() + if err != nil && !errors.Is(err, gitpkg.ErrNotOnAnyBranch) { return err } - if startBranch != branch { + if currentBranch == branch { + if useForce { + if err := git.ResetHard("FETCH_HEAD"); err != nil { + return err + } + } else { + if err := git.MergeFastForward("FETCH_HEAD"); err != nil { + return err + } + } + } else { if hasLocalBranch { - if err := git.CheckoutLocal(branch); err != nil { + if err := git.UpdateBranch(branch, "FETCH_HEAD"); err != nil { return err } } else { - if err := git.CheckoutRemote(remote, branch); err != nil { + if err := git.CreateBranch(branch, "FETCH_HEAD", fmt.Sprintf("%s/%s", remote, branch)); err != nil { return err } } } - if hasLocalBranch { - if opts.Force { - if err := git.ResetHard(fmt.Sprintf("refs/remotes/%s/%s", remote, branch)); err != nil { - return err - } - } else { - if err := git.MergeFastForward(fmt.Sprintf("refs/remotes/%s/%s", remote, branch)); err != nil { - return err - } - } - } - if startBranch != branch { - if err := git.CheckoutLocal(startBranch); err != nil { - return err - } - } return nil } diff --git a/pkg/cmd/repo/sync/sync_test.go b/pkg/cmd/repo/sync/sync_test.go index cfbd1c281..f396ef878 100644 --- a/pkg/cmd/repo/sync/sync_test.go +++ b/pkg/cmd/repo/sync/sync_test.go @@ -126,29 +126,26 @@ func Test_SyncRun(t *testing.T) { mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() - mgc.On("IsAncestor", "trunk", "origin/trunk").Return(true, nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(true, nil).Once() mgc.On("CurrentBranch").Return("trunk", nil).Once() - mgc.On("MergeFastForward", "refs/remotes/origin/trunk").Return(nil).Once() + mgc.On("MergeFastForward", "FETCH_HEAD").Return(nil).Once() }, wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to local repository\n", }, { name: "sync local repo with parent - notty", tty: false, - opts: &SyncOptions{}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + opts: &SyncOptions{ + Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() - mgc.On("IsAncestor", "trunk", "origin/trunk").Return(true, nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(true, nil).Once() mgc.On("CurrentBranch").Return("trunk", nil).Once() - mgc.On("MergeFastForward", "refs/remotes/origin/trunk").Return(nil).Once() + mgc.On("MergeFastForward", "FETCH_HEAD").Return(nil).Once() }, wantStdout: "", }, @@ -156,78 +153,50 @@ func Test_SyncRun(t *testing.T) { name: "sync local repo with specified source repo", tty: true, opts: &SyncOptions{ + Branch: "trunk", SrcArg: "OWNER2/REPO2", }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) - }, gitStubs: func(mgc *mockGitClient) { mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "upstream", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("upstream", nil).Once() - mgc.On("IsAncestor", "trunk", "upstream/trunk").Return(true, nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(true, nil).Once() mgc.On("CurrentBranch").Return("trunk", nil).Once() - mgc.On("MergeFastForward", "refs/remotes/upstream/trunk").Return(nil).Once() + mgc.On("MergeFastForward", "FETCH_HEAD").Return(nil).Once() }, wantStdout: "✓ Synced the \"trunk\" branch from OWNER2/REPO2 to local repository\n", }, - { - name: "sync local repo with parent and specified branch", - tty: true, - opts: &SyncOptions{ - Branch: "test", - }, - gitStubs: func(mgc *mockGitClient) { - mgc.On("IsDirty").Return(false, nil).Once() - mgc.On("Fetch", "origin", "refs/heads/test").Return(nil).Once() - mgc.On("HasLocalBranch", "test").Return(true).Once() - mgc.On("BranchRemote", "test").Return("origin", nil).Once() - mgc.On("IsAncestor", "test", "origin/test").Return(true, nil).Once() - mgc.On("CurrentBranch").Return("test", nil).Once() - mgc.On("MergeFastForward", "refs/remotes/origin/test").Return(nil).Once() - }, - wantStdout: "✓ Synced the \"test\" branch from OWNER/REPO to local repository\n", - }, { name: "sync local repo with parent and force specified", tty: true, opts: &SyncOptions{ - Force: true, - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + Branch: "trunk", + Force: true, }, gitStubs: func(mgc *mockGitClient) { mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() - mgc.On("IsAncestor", "trunk", "origin/trunk").Return(false, nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(false, nil).Once() mgc.On("CurrentBranch").Return("trunk", nil).Once() - mgc.On("ResetHard", "refs/remotes/origin/trunk").Return(nil).Once() + mgc.On("ResetHard", "FETCH_HEAD").Return(nil).Once() }, wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to local repository\n", }, { name: "sync local repo with parent and not fast forward merge", tty: true, - opts: &SyncOptions{}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + opts: &SyncOptions{ + Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() - mgc.On("IsAncestor", "trunk", "origin/trunk").Return(false, nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(false, nil).Once() }, wantErr: true, errMsg: "can't sync because there are diverging changes; use `--force` to overwrite the destination branch", @@ -235,11 +204,8 @@ func Test_SyncRun(t *testing.T) { { name: "sync local repo with parent and mismatching branch remotes", tty: true, - opts: &SyncOptions{}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + opts: &SyncOptions{ + Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { mgc.On("IsDirty").Return(false, nil).Once() @@ -261,24 +227,34 @@ func Test_SyncRun(t *testing.T) { errMsg: "can't sync because there are local changes, please commit or stash them", }, { - name: "sync local repo with parent not on default branch", + name: "sync local repo with parent - existing branch, non-current", tty: true, - opts: &SyncOptions{}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`)) + opts: &SyncOptions{ + Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() - mgc.On("IsAncestor", "trunk", "origin/trunk").Return(true, nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(true, nil).Once() mgc.On("CurrentBranch").Return("test", nil).Once() - mgc.On("CheckoutLocal", "trunk").Return(nil).Once() - mgc.On("MergeFastForward", "refs/remotes/origin/trunk").Return(nil).Once() - mgc.On("CheckoutLocal", "test").Return(nil).Once() + mgc.On("UpdateBranch", "trunk", "FETCH_HEAD").Return(nil).Once() + }, + wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to local repository\n", + }, + { + name: "sync local repo with parent - create new branch", + tty: true, + opts: &SyncOptions{ + Branch: "trunk", + }, + gitStubs: func(mgc *mockGitClient) { + mgc.On("IsDirty").Return(false, nil).Once() + mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() + mgc.On("HasLocalBranch", "trunk").Return(false).Once() + mgc.On("CurrentBranch").Return("test", nil).Once() + mgc.On("CreateBranch", "trunk", "FETCH_HEAD", "origin/trunk").Return(nil).Once() }, wantStdout: "✓ Synced the \"trunk\" branch from OWNER/REPO to local repository\n", }, @@ -470,9 +446,8 @@ func Test_SyncRun(t *testing.T) { return tt.remotes, nil } - tt.opts.Git = newMockGitClient(t, tt.gitStubs) - t.Run(tt.name, func(t *testing.T) { + tt.opts.Git = newMockGitClient(t, tt.gitStubs) defer reg.Verify(t) err := syncRun(tt.opts) if tt.wantErr { From 0f1ab13b9ef0c2c7ee4cd9b204f3af06278d9adf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 10 Aug 2021 14:29:23 +0200 Subject: [PATCH 2/4] Only check if working copy is dirty when syncing current branch In other cases, we don't have to abort the operation since it can proceed without being affected by the working copy at all. --- pkg/cmd/repo/sync/sync.go | 25 +++++++++++++------------ pkg/cmd/repo/sync/sync_test.go | 15 +++++++++------ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/repo/sync/sync.go b/pkg/cmd/repo/sync/sync.go index 68ae12707..0749b5c77 100644 --- a/pkg/cmd/repo/sync/sync.go +++ b/pkg/cmd/repo/sync/sync.go @@ -92,24 +92,20 @@ func syncRun(opts *SyncOptions) error { } func syncLocalRepo(opts *SyncOptions) error { - var err error var srcRepo ghrepo.Interface - dirtyRepo, err := opts.Git.IsDirty() - if err != nil { - return err - } - if dirtyRepo { - return fmt.Errorf("can't sync because there are local changes, please commit or stash them") - } - if opts.SrcArg != "" { + var err error srcRepo, err = ghrepo.FromFullName(opts.SrcArg) + if err != nil { + return err + } } else { + var err error srcRepo, err = opts.BaseRepo() - } - if err != nil { - return err + if err != nil { + return err + } } // Find remote that matches the srcRepo @@ -273,6 +269,11 @@ func executeLocalRepoSync(srcRepo ghrepo.Interface, remote string, opts *SyncOpt return err } if currentBranch == branch { + if isDirty, err := git.IsDirty(); err == nil && isDirty { + return fmt.Errorf("can't sync because there are local changes; please stash them before trying again") + } else if err != nil { + return err + } if useForce { if err := git.ResetHard("FETCH_HEAD"); err != nil { return err diff --git a/pkg/cmd/repo/sync/sync_test.go b/pkg/cmd/repo/sync/sync_test.go index f396ef878..9fa88b469 100644 --- a/pkg/cmd/repo/sync/sync_test.go +++ b/pkg/cmd/repo/sync/sync_test.go @@ -192,7 +192,6 @@ func Test_SyncRun(t *testing.T) { Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { - mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() @@ -208,7 +207,6 @@ func Test_SyncRun(t *testing.T) { Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { - mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("upstream", nil).Once() @@ -219,12 +217,19 @@ func Test_SyncRun(t *testing.T) { { name: "sync local repo with parent and local changes", tty: true, - opts: &SyncOptions{}, + opts: &SyncOptions{ + Branch: "trunk", + }, gitStubs: func(mgc *mockGitClient) { + mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() + mgc.On("HasLocalBranch", "trunk").Return(true).Once() + mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() + mgc.On("IsAncestor", "trunk", "FETCH_HEAD").Return(true, nil).Once() + mgc.On("CurrentBranch").Return("trunk", nil).Once() mgc.On("IsDirty").Return(true, nil).Once() }, wantErr: true, - errMsg: "can't sync because there are local changes, please commit or stash them", + errMsg: "can't sync because there are local changes; please stash them before trying again", }, { name: "sync local repo with parent - existing branch, non-current", @@ -233,7 +238,6 @@ func Test_SyncRun(t *testing.T) { Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { - mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(true).Once() mgc.On("BranchRemote", "trunk").Return("origin", nil).Once() @@ -250,7 +254,6 @@ func Test_SyncRun(t *testing.T) { Branch: "trunk", }, gitStubs: func(mgc *mockGitClient) { - mgc.On("IsDirty").Return(false, nil).Once() mgc.On("Fetch", "origin", "refs/heads/trunk").Return(nil).Once() mgc.On("HasLocalBranch", "trunk").Return(false).Once() mgc.On("CurrentBranch").Return("test", nil).Once() From 6136a39ed63ae903ea17eb347a1bae5e0474054e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 10 Aug 2021 14:30:36 +0200 Subject: [PATCH 3/4] Use `remotes.FindByRepo()` --- pkg/cmd/repo/sync/sync.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/repo/sync/sync.go b/pkg/cmd/repo/sync/sync.go index 0749b5c77..a77020fe0 100644 --- a/pkg/cmd/repo/sync/sync.go +++ b/pkg/cmd/repo/sync/sync.go @@ -114,14 +114,9 @@ func syncLocalRepo(opts *SyncOptions) error { if err != nil { return err } - for _, r := range remotes { - if r.RepoName() == srcRepo.RepoName() && - r.RepoOwner() == srcRepo.RepoOwner() && - r.RepoHost() == srcRepo.RepoHost() { - remote = r.Name - } - } - if remote == "" { + if r, err := remotes.FindByRepo(srcRepo.RepoOwner(), srcRepo.RepoName()); err == nil { + remote = r.Name + } else { return fmt.Errorf("can't find corresponding remote for %s", ghrepo.FullName(srcRepo)) } From f4bded30f85965689b4b8d98b21db6170e0d32dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 10 Aug 2021 14:30:55 +0200 Subject: [PATCH 4/4] Mark test helper --- pkg/cmd/repo/sync/sync_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/repo/sync/sync_test.go b/pkg/cmd/repo/sync/sync_test.go index 9fa88b469..d2127e66c 100644 --- a/pkg/cmd/repo/sync/sync_test.go +++ b/pkg/cmd/repo/sync/sync_test.go @@ -465,9 +465,11 @@ func Test_SyncRun(t *testing.T) { } func newMockGitClient(t *testing.T, config func(*mockGitClient)) *mockGitClient { + t.Helper() m := &mockGitClient{} m.Test(t) t.Cleanup(func() { + t.Helper() m.AssertExpectations(t) }) if config != nil {