STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #4102 from cli/sync-no-checkout

Browse source 
Repo sync optimizations
This commit is contained in:
Mislav Marohnić 2021-08-12 15:33:32 +02:00 • committed by GitHub
commit e567ce00cd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 117 additions and 137 deletions
Download patch file Download diff file Expand all files Collapse all files

23
pkg/cmd/repo/sync/git.go
View file

@ -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

8
pkg/cmd/repo/sync/mocks.go
View file

@ -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)
}

91
pkg/cmd/repo/sync/sync.go
View file

@ -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

132
pkg/cmd/repo/sync/sync_test.go
View file

@ -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 {