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

Address PR comments

Browse source
This commit is contained in:
Sam Coe 2021-06-23 13:45:15 -07:00
parent 0e838052a4
commit 8219710551
No known key found for this signature in database
GPG key ID: 8E322C20F811D086
2 changed files with 119 additions and 143 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -6,28 +6,25 @@ import (
"net/http"
"regexp"
"github.com/AlecAivazis/survey/v2"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/api"
"github.com/cli/cli/context"
"github.com/cli/cli/internal/ghrepo"
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/iostreams"
"github.com/cli/cli/pkg/prompt"
"github.com/spf13/cobra"
)
type SyncOptions struct {
HttpClient func() (*http.Client, error)
IO *iostreams.IOStreams
BaseRepo func() (ghrepo.Interface, error)
Remotes func() (context.Remotes, error)
Git gitClient
DestArg string
SrcArg string
Branch string
Force bool
SkipConfirm bool
HttpClient func() (*http.Client, error)
IO *iostreams.IOStreams
BaseRepo func() (ghrepo.Interface, error)
Remotes func() (context.Remotes, error)
Git gitClient
DestArg string
SrcArg string
Branch string
Force bool
}
func NewCmdSync(f *cmdutil.Factory, runF func(*SyncOptions) error) *cobra.Command {
@ -42,13 +39,21 @@ func NewCmdSync(f *cmdutil.Factory, runF func(*SyncOptions) error) *cobra.Comman
cmd := &cobra.Command{
Use: "sync [<destination-repository>]",
Short: "Sync a repository",
Long: heredoc.Doc(`
Sync destination repository from source repository.
Long: heredoc.Docf(`
Sync destination repository from source repository. Syncing will take a branch
on the source repository and merge it into the branch of the same name on the
destination repository. A fast forward merge will be used execept when the
%[1]s--force%[1]s flag is specified, then the two branches will by synced using
a hard reset.
Without an argument, the local repository is selected as the destination repository.
By default the source repository is the parent of the destination repository.
The source repository can be overridden with the --source flag.
`),
By default the source repository is the parent of the destination repository,
this can be overridden with the %[1]s--source%[1]s flag.
The source repository default branch is selected automatically, but can be
overridden with the %[1]s--branch%[1]s flag.
`, "`"),
Example: heredoc.Doc(`
# Sync local repository from remote parent
$ gh repo sync
@ -67,9 +72,6 @@ func NewCmdSync(f *cmdutil.Factory, runF func(*SyncOptions) error) *cobra.Comman
if len(args) > 0 {
opts.DestArg = args[0]
}
if !opts.IO.CanPrompt() && !opts.SkipConfirm {
return &cmdutil.FlagError{Err: errors.New("`--confirm` required when not running interactively")}
}
if runF != nil {
return runF(&opts)
}
@ -80,46 +82,91 @@ func NewCmdSync(f *cmdutil.Factory, runF func(*SyncOptions) error) *cobra.Comman
cmd.Flags().StringVarP(&opts.SrcArg, "source", "s", "", "Source repository")
cmd.Flags().StringVarP(&opts.Branch, "branch", "b", "", "Branch to sync")
cmd.Flags().BoolVarP(&opts.Force, "force", "", false, "Discard destination repository changes")
cmd.Flags().BoolVarP(&opts.SkipConfirm, "confirm", "y", false, "Skip the confirmation prompt")
return cmd
}
func syncRun(opts *SyncOptions) error {
if opts.DestArg == "" {
return syncLocalRepo(opts)
} else {
return syncRemoteRepo(opts)
}
}
func syncLocalRepo(opts *SyncOptions) error {
var err error
var srcRepo ghrepo.Interface
if opts.SrcArg != "" {
srcRepo, err = ghrepo.FromFullName(opts.SrcArg)
} else {
srcRepo, err = opts.BaseRepo()
}
if err != nil {
return err
}
if opts.Branch == "" {
httpClient, err := opts.HttpClient()
if err != nil {
return err
}
apiClient := api.NewClientFromHTTP(httpClient)
opts.IO.StartProgressIndicator()
opts.Branch, err = api.RepoDefaultBranch(apiClient, srcRepo)
opts.IO.StopProgressIndicator()
if err != nil {
return err
}
}
opts.IO.StartProgressIndicator()
err = executeLocalRepoSync(srcRepo, opts)
opts.IO.StopProgressIndicator()
if err != nil {
if errors.Is(err, divergingError) {
return fmt.Errorf("can't sync because there are diverging changes, you can use `--force` to overwrite the changes")
}
if errors.Is(err, dirtyRepoError) {
return fmt.Errorf("can't sync because there are local changes, please commit or stash them")
}
return err
}
if opts.IO.IsStdoutTTY() {
cs := opts.IO.ColorScheme()
srcStr := fmt.Sprintf("%s:%s", ghrepo.FullName(srcRepo), opts.Branch)
destStr := fmt.Sprintf(".:%s", opts.Branch)
success := fmt.Sprintf("Synced %s from %s\n", cs.Bold(destStr), cs.Bold(srcStr))
fmt.Fprintf(opts.IO.Out, "%s %s", cs.SuccessIcon(), success)
}
return nil
}
func syncRemoteRepo(opts *SyncOptions) error {
httpClient, err := opts.HttpClient()
if err != nil {
return err
}
apiClient := api.NewClientFromHTTP(httpClient)
var local bool
var destRepo, srcRepo ghrepo.Interface
if opts.DestArg == "" {
local = true
destRepo, err = opts.BaseRepo()
if err != nil {
return err
}
} else {
destRepo, err = ghrepo.FromFullName(opts.DestArg)
if err != nil {
return err
}
destRepo, err = ghrepo.FromFullName(opts.DestArg)
if err != nil {
return err
}
if opts.SrcArg == "" {
if local {
srcRepo = destRepo
} else {
opts.IO.StartProgressIndicator()
srcRepo, err = api.RepoParent(apiClient, destRepo)
opts.IO.StopProgressIndicator()
if err != nil {
return err
}
if srcRepo == nil {
return fmt.Errorf("can't determine source repo for %s because repo is not fork", ghrepo.FullName(destRepo))
}
opts.IO.StartProgressIndicator()
srcRepo, err = api.RepoParent(apiClient, destRepo)
opts.IO.StopProgressIndicator()
if err != nil {
return err
}
if srcRepo == nil {
return fmt.Errorf("can't determine source repo for %s because repo is not fork", ghrepo.FullName(destRepo))
}
} else {
srcRepo, err = ghrepo.FromFullName(opts.SrcArg)
@ -128,7 +175,7 @@ func syncRun(opts *SyncOptions) error {
}
}
if !local && destRepo.RepoHost() != srcRepo.RepoHost() {
if destRepo.RepoHost() != srcRepo.RepoHost() {
return fmt.Errorf("can't sync repos from different hosts")
}
@ -141,52 +188,31 @@ func syncRun(opts *SyncOptions) error {
}
}
srcStr := fmt.Sprintf("%s:%s", ghrepo.FullName(srcRepo), opts.Branch)
destStr := fmt.Sprintf("%s:%s", ghrepo.FullName(destRepo), opts.Branch)
if local {
destStr = fmt.Sprintf(".:%s", opts.Branch)
}
cs := opts.IO.ColorScheme()
if !opts.SkipConfirm && opts.IO.CanPrompt() {
if opts.Force {
fmt.Fprintf(opts.IO.ErrOut, "%s Using --force will cause diverging commits on %s to be discarded\n", cs.WarningIcon(), destStr)
}
var confirmed bool
confirmQuestion := &survey.Confirm{
Message: fmt.Sprintf("Sync %s from %s?", destStr, srcStr),
Default: false,
}
err := prompt.SurveyAskOne(confirmQuestion, &confirmed)
if err != nil {
return err
}
if !confirmed {
return cmdutil.CancelError
}
}
opts.IO.StartProgressIndicator()
if local {
err = syncLocalRepo(srcRepo, opts)
} else {
err = syncRemoteRepo(apiClient, destRepo, srcRepo, opts)
}
err = executeRemoteRepoSync(apiClient, destRepo, srcRepo, opts)
opts.IO.StopProgressIndicator()
if err != nil {
if errors.Is(err, divergingError) {
return fmt.Errorf("can't sync because there are diverging changes, you can use `--force` to overwrite the changes")
}
return err
}
if opts.IO.IsStdoutTTY() {
success := cs.Bold(fmt.Sprintf("Synced %s from %s\n", destStr, srcStr))
fmt.Fprintf(opts.IO.Out, "%s %s", cs.SuccessIconWithColor(cs.GreenBold), success)
cs := opts.IO.ColorScheme()
srcStr := fmt.Sprintf("%s:%s", ghrepo.FullName(srcRepo), opts.Branch)
destStr := fmt.Sprintf("%s:%s", ghrepo.FullName(destRepo), opts.Branch)
success := fmt.Sprintf("Synced %s from %s\n", cs.Bold(destStr), cs.Bold(srcStr))
fmt.Fprintf(opts.IO.Out, "%s %s", cs.SuccessIcon(), success)
}
return nil
}
func syncLocalRepo(srcRepo ghrepo.Interface, opts *SyncOptions) error {
var divergingError = errors.New("diverging commits")
var dirtyRepoError = errors.New("dirty repo")
func executeLocalRepoSync(srcRepo ghrepo.Interface, opts *SyncOptions) error {
// Remotes precedence by name
// 1. upstream
// 2. github
@ -213,7 +239,7 @@ func syncLocalRepo(srcRepo ghrepo.Interface, opts *SyncOptions) error {
}
if !fastForward && !opts.Force {
return fmt.Errorf("can't sync because there are diverging commits, you can use `--force` to overwrite the commits on .:%s", branch)
return divergingError
}
}
@ -222,7 +248,7 @@ func syncLocalRepo(srcRepo ghrepo.Interface, opts *SyncOptions) error {
return err
}
if dirtyRepo {
return fmt.Errorf("can't sync because there are local changes, please commit or stash them then try again")
return dirtyRepoError
}
startBranch, err := git.CurrentBranch()
@ -258,7 +284,7 @@ func syncLocalRepo(srcRepo ghrepo.Interface, opts *SyncOptions) error {
return nil
}
func syncRemoteRepo(client *api.Client, destRepo, srcRepo ghrepo.Interface, opts *SyncOptions) error {
func executeRemoteRepoSync(client *api.Client, destRepo, srcRepo ghrepo.Interface, opts *SyncOptions) error {
commit, err := latestCommit(client, srcRepo, opts.Branch)
if err != nil {
return err
@ -270,9 +296,7 @@ func syncRemoteRepo(client *api.Client, destRepo, srcRepo ghrepo.Interface, opts
err = syncFork(client, destRepo, opts.Branch, commit.Object.SHA, opts.Force)
var httpErr api.HTTPError
if err != nil && errors.As(err, &httpErr) && notFastForwardErrorMessage.MatchString(httpErr.Message) {
destStr := fmt.Sprintf("%s:%s", ghrepo.FullName(destRepo), opts.Branch)
return fmt.Errorf("can't sync because there are diverging commits, you can use `--force` to overwrite the commits on %s",
destStr)
return divergingError
}
return err

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

@ -12,7 +12,6 @@ import (
"github.com/cli/cli/pkg/cmdutil"
"github.com/cli/cli/pkg/httpmock"
"github.com/cli/cli/pkg/iostreams"
"github.com/cli/cli/pkg/prompt"
"github.com/google/shlex"
"github.com/stretchr/testify/assert"
)
@ -64,21 +63,6 @@ func TestNewCmdSync(t *testing.T) {
Force: true,
},
},
{
name: "confirm",
tty: true,
input: "--confirm",
output: SyncOptions{
SkipConfirm: true,
},
},
{
name: "notty without confirm",
tty: false,
input: "",
wantErr: true,
errMsg: "`--confirm` required when not running interactively",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -112,16 +96,11 @@ func TestNewCmdSync(t *testing.T) {
assert.Equal(t, tt.output.SrcArg, gotOpts.SrcArg)
assert.Equal(t, tt.output.Branch, gotOpts.Branch)
assert.Equal(t, tt.output.Force, gotOpts.Force)
assert.Equal(t, tt.output.SkipConfirm, gotOpts.SkipConfirm)
})
}
}
func Test_SyncRun(t *testing.T) {
stubConfirm := func(as *prompt.AskStubber) {
as.StubOne(true)
}
tests := []struct {
name string
tty bool
@ -129,9 +108,7 @@ func Test_SyncRun(t *testing.T) {
remotes []*context.Remote
httpStubs func(*httpmock.Registry)
gitStubs func(*mockGitClient)
askStubs func(*prompt.AskStubber)
wantStdout string
wantStderr string
wantErr bool
errMsg string
}{
@ -152,15 +129,12 @@ func Test_SyncRun(t *testing.T) {
mgc.On("CurrentBranch").Return("trunk", nil).Once()
mgc.On("Merge", []string{"--ff-only", "refs/remotes/origin/trunk"}).Return(nil).Once()
},
askStubs: stubConfirm,
wantStdout: "✓ Synced .:trunk from OWNER/REPO:trunk\n",
},
{
name: "sync local repo with parent - notty",
tty: false,
opts: &SyncOptions{
SkipConfirm: true,
},
opts: &SyncOptions{},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
httpmock.GraphQL(`query RepositoryInfo\b`),
@ -174,7 +148,6 @@ func Test_SyncRun(t *testing.T) {
mgc.On("CurrentBranch").Return("trunk", nil).Once()
mgc.On("Merge", []string{"--ff-only", "refs/remotes/origin/trunk"}).Return(nil).Once()
},
askStubs: stubConfirm,
wantStdout: "",
},
{
@ -196,7 +169,6 @@ func Test_SyncRun(t *testing.T) {
mgc.On("CurrentBranch").Return("trunk", nil).Once()
mgc.On("Merge", []string{"--ff-only", "refs/remotes/origin/trunk"}).Return(nil).Once()
},
askStubs: stubConfirm,
wantStdout: "✓ Synced .:trunk from OWNER2/REPO2:trunk\n",
},
{
@ -213,7 +185,6 @@ func Test_SyncRun(t *testing.T) {
mgc.On("CurrentBranch").Return("test", nil).Once()
mgc.On("Merge", []string{"--ff-only", "refs/remotes/origin/test"}).Return(nil).Once()
},
askStubs: stubConfirm,
wantStdout: "✓ Synced .:test from OWNER/REPO:test\n",
},
{
@ -235,8 +206,6 @@ func Test_SyncRun(t *testing.T) {
mgc.On("CurrentBranch").Return("trunk", nil).Once()
mgc.On("Reset", []string{"--hard", "refs/remotes/origin/trunk"}).Return(nil).Once()
},
askStubs: stubConfirm,
wantStderr: "! Using --force will cause diverging commits on .:trunk to be discarded\n",
wantStdout: "✓ Synced .:trunk from OWNER/REPO:trunk\n",
},
{
@ -253,9 +222,8 @@ func Test_SyncRun(t *testing.T) {
mgc.On("HasLocalBranch", []string{"trunk"}).Return(true).Once()
mgc.On("IsAncestor", []string{"trunk", "origin/trunk"}).Return(false, nil).Once()
},
askStubs: stubConfirm,
wantErr: true,
errMsg: "can't sync because there are diverging commits, you can use `--force` to overwrite the commits on .:trunk",
wantErr: true,
errMsg: "can't sync because there are diverging changes, you can use `--force` to overwrite the changes",
},
{
name: "sync local repo with parent and local changes",
@ -272,9 +240,8 @@ func Test_SyncRun(t *testing.T) {
mgc.On("IsAncestor", []string{"trunk", "origin/trunk"}).Return(true, nil).Once()
mgc.On("IsDirty").Return(true, nil).Once()
},
askStubs: stubConfirm,
wantErr: true,
errMsg: "can't sync because there are local changes, please commit or stash them then try again",
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",
@ -295,7 +262,6 @@ func Test_SyncRun(t *testing.T) {
mgc.On("Merge", []string{"--ff-only", "refs/remotes/origin/trunk"}).Return(nil).Once()
mgc.On("Checkout", []string{"test"}).Return(nil).Once()
},
askStubs: stubConfirm,
wantStdout: "✓ Synced .:trunk from OWNER/REPO:trunk\n",
},
{
@ -318,15 +284,13 @@ func Test_SyncRun(t *testing.T) {
httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/trunk"),
httpmock.StringResponse(`{}`))
},
askStubs: stubConfirm,
wantStdout: "✓ Synced OWNER/REPO-FORK:trunk from OWNER/REPO:trunk\n",
},
{
name: "sync remote fork with parent - notty",
tty: false,
opts: &SyncOptions{
DestArg: "OWNER/REPO-FORK",
SkipConfirm: true,
DestArg: "OWNER/REPO-FORK",
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
@ -376,7 +340,6 @@ func Test_SyncRun(t *testing.T) {
httpmock.REST("PATCH", "repos/OWNER/REPO/git/refs/heads/trunk"),
httpmock.StringResponse(`{}`))
},
askStubs: stubConfirm,
wantStdout: "✓ Synced OWNER/REPO:trunk from OWNER2/REPO2:trunk\n",
},
{
@ -397,7 +360,6 @@ func Test_SyncRun(t *testing.T) {
httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/test"),
httpmock.StringResponse(`{}`))
},
askStubs: stubConfirm,
wantStdout: "✓ Synced OWNER/REPO-FORK:test from OWNER/REPO:test\n",
},
{
@ -421,8 +383,6 @@ func Test_SyncRun(t *testing.T) {
httpmock.REST("PATCH", "repos/OWNER/REPO-FORK/git/refs/heads/trunk"),
httpmock.StringResponse(`{}`))
},
askStubs: stubConfirm,
wantStderr: "! Using --force will cause diverging commits on OWNER/REPO-FORK:trunk to be discarded\n",
wantStdout: "✓ Synced OWNER/REPO-FORK:trunk from OWNER/REPO:trunk\n",
},
{
@ -452,9 +412,8 @@ func Test_SyncRun(t *testing.T) {
}, nil
})
},
askStubs: stubConfirm,
wantErr: true,
errMsg: "can't sync because there are diverging commits, you can use `--force` to overwrite the commits on OWNER/REPO-FORK:trunk",
wantErr: true,
errMsg: "can't sync because there are diverging changes, you can use `--force` to overwrite the changes",
},
}
for _, tt := range tests {
@ -466,7 +425,7 @@ func Test_SyncRun(t *testing.T) {
return &http.Client{Transport: reg}, nil
}
io, _, stdout, stderr := iostreams.Test()
io, _, stdout, _ := iostreams.Test()
io.SetStdinTTY(tt.tty)
io.SetStdoutTTY(tt.tty)
tt.opts.IO = io
@ -485,12 +444,6 @@ func Test_SyncRun(t *testing.T) {
tt.opts.Git = newMockGitClient(t, tt.gitStubs)
as, teardown := prompt.InitAskStubber()
defer teardown()
if tt.askStubs != nil {
tt.askStubs(as)
}
t.Run(tt.name, func(t *testing.T) {
defer reg.Verify(t)
err := syncRun(tt.opts)
@ -500,7 +453,6 @@ func Test_SyncRun(t *testing.T) {
return
}
assert.NoError(t, err)
assert.Equal(t, tt.wantStderr, stderr.String())
assert.Equal(t, tt.wantStdout, stdout.String())
})
}