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..a77020fe0 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" @@ -91,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 @@ -117,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)) } @@ -238,6 +230,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,46 +246,48 @@ 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 hasLocalBranch { - if err := git.CheckoutLocal(branch); err != nil { - return err - } - } else { - if err := git.CheckoutRemote(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 { + 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 + } + } else { + if err := git.MergeFastForward("FETCH_HEAD"); err != nil { + return err + } + } + } else { + if hasLocalBranch { + if err := git.UpdateBranch(branch, "FETCH_HEAD"); err != nil { + return err + } + } else { + if err := git.CreateBranch(branch, "FETCH_HEAD", fmt.Sprintf("%s/%s", remote, branch)); 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..d2127e66c 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,49 @@ 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,14 +203,10 @@ 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() 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() @@ -253,32 +217,47 @@ func Test_SyncRun(t *testing.T) { { name: "sync local repo with parent and local changes", tty: true, - opts: &SyncOptions{}, - gitStubs: func(mgc *mockGitClient) { - mgc.On("IsDirty").Return(true, nil).Once() - }, - wantErr: true, - errMsg: "can't sync because there are local changes, please commit or stash them", - }, - { - name: "sync local repo with parent not on default branch", - 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("trunk", nil).Once() + mgc.On("IsDirty").Return(true, nil).Once() + }, + wantErr: true, + 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", + tty: true, + 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("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("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 +449,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 { @@ -487,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 {