From 7541ee6aecee2335f2f5b11e63ae43c2ef0b8ae9 Mon Sep 17 00:00:00 2001 From: Josh Soref <2119212+jsoref@users.noreply.github.com> Date: Mon, 20 Feb 2023 23:10:50 -0500 Subject: [PATCH] Retry `git clone` on git clone failure in `gh repo fork --clone` (#6962) --- go.mod | 2 +- go.sum | 4 +- pkg/cmd/repo/fork/fork.go | 28 +++++- pkg/cmd/repo/fork/fork_test.go | 154 ++++++++++++++++++++------------- 4 files changed, 123 insertions(+), 65 deletions(-) diff --git a/go.mod b/go.mod index fcba58a19..7732a2f98 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/AlecAivazis/survey/v2 v2.3.6 github.com/MakeNowJust/heredoc v1.0.0 github.com/briandowns/spinner v1.18.1 - github.com/cenkalti/backoff/v4 v4.1.3 + github.com/cenkalti/backoff/v4 v4.2.0 github.com/charmbracelet/glamour v0.5.1-0.20220727184942-e70ff2d969da github.com/charmbracelet/lipgloss v0.5.0 github.com/cli/go-gh v1.0.0 diff --git a/go.sum b/go.sum index 03be7b86e..1a0e3123e 100644 --- a/go.sum +++ b/go.sum @@ -45,8 +45,8 @@ github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuP github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= github.com/briandowns/spinner v1.18.1 h1:yhQmQtM1zsqFsouh09Bk/jCjd50pC3EOGsh28gLVvwY= github.com/briandowns/spinner v1.18.1/go.mod h1:mQak9GHqbspjC/5iUx3qMlIho8xBS/ppAL/hX5SmPJU= -github.com/cenkalti/backoff/v4 v4.1.3 h1:cFAlzYUlVYDysBEH2T5hyJZMh3+5+WCBvSnK6Q8UtC4= -github.com/cenkalti/backoff/v4 v4.1.3/go.mod h1:scbssz8iZGpm3xbr14ovlUdkxfGXNInqkPWOWmG2CLw= +github.com/cenkalti/backoff/v4 v4.2.0 h1:HN5dHm3WBOgndBH6E8V0q2jIYIR3s9yglV8k/+MN3u4= +github.com/cenkalti/backoff/v4 v4.2.0/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE= github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU= github.com/charmbracelet/glamour v0.5.1-0.20220727184942-e70ff2d969da h1:FGz53GWQRiKQ/5xUsoCCkewSQIC7u81Scaxx2nUy3nM= github.com/charmbracelet/glamour v0.5.1-0.20220727184942-e70ff2d969da/go.mod h1:HXz79SMFnF9arKxqeoHWxmo1BhplAH7wehlRhKQIL94= diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 0721485a8..6b4c5ca9a 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -2,6 +2,7 @@ package fork import ( "context" + "errors" "fmt" "net/http" "net/url" @@ -9,6 +10,7 @@ import ( "time" "github.com/MakeNowJust/heredoc" + "github.com/cenkalti/backoff/v4" "github.com/cli/cli/v2/api" ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" @@ -32,6 +34,7 @@ type ForkOptions struct { BaseRepo func() (ghrepo.Interface, error) Remotes func() (ghContext.Remotes, error) Since func(time.Time) time.Duration + BackOff backoff.BackOff GitArgs []string Repository string @@ -46,6 +49,10 @@ type ForkOptions struct { DefaultBranchOnly bool } +type errWithExitCode interface { + ExitCode() int +} + // TODO warn about useless flags (--remote, --remote-name) when running from outside a repository // TODO output over STDOUT not STDERR // TODO remote-name has no effect on its own; error that or change behavior @@ -317,8 +324,25 @@ func forkRun(opts *ForkOptions) error { } } if cloneDesired { - forkedRepoURL := ghrepo.FormatRemoteURL(forkedRepo, protocol) - cloneDir, err := gitClient.Clone(ctx, forkedRepoURL, opts.GitArgs) + // Allow injecting alternative BackOff in tests. + if opts.BackOff == nil { + bo := backoff.NewConstantBackOff(3 * time.Second) + opts.BackOff = bo + } + + cloneDir, err := backoff.RetryWithData(func() (string, error) { + forkedRepoURL := ghrepo.FormatRemoteURL(forkedRepo, protocol) + dir, err := gitClient.Clone(ctx, forkedRepoURL, opts.GitArgs) + if err == nil { + return dir, err + } + var execError errWithExitCode + if errors.As(err, &execError) && execError.ExitCode() == 128 { + return "", err + } + return "", backoff.Permanent(err) + }, backoff.WithContext(backoff.WithMaxRetries(opts.BackOff, 3), ctx)) + if err != nil { return fmt.Errorf("failed to clone fork: %w", err) } diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 8022cd2dc..5ec1c5f28 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/cenkalti/backoff/v4" "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" @@ -663,78 +664,111 @@ func TestRepoFork(t *testing.T) { }, wantErrOut: "āœ“ Created fork OWNER/REPO\nāœ“ Renamed fork to OWNER/NEW_REPO\n", }, + { + name: "retries clone up to four times if necessary", + opts: &ForkOptions{ + Repository: "OWNER/REPO", + Clone: true, + BackOff: &backoff.ZeroBackOff{}, + }, + httpStubs: forkPost, + execStubs: func(cs *run.CommandStubber) { + cs.Register(`git clone https://github.com/someone/REPO\.git`, 128, "") + cs.Register(`git clone https://github.com/someone/REPO\.git`, 128, "") + cs.Register(`git clone https://github.com/someone/REPO\.git`, 128, "") + cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO fetch upstream`, 0, "") + }, + }, + { + name: "does not retry clone if error occurs and exit code is not 128", + opts: &ForkOptions{ + Repository: "OWNER/REPO", + Clone: true, + BackOff: &backoff.ZeroBackOff{}, + }, + httpStubs: forkPost, + execStubs: func(cs *run.CommandStubber) { + cs.Register(`git clone https://github.com/someone/REPO\.git`, 128, "") + cs.Register(`git clone https://github.com/someone/REPO\.git`, 65, "") + }, + wantErr: true, + errMsg: `failed to clone fork: failed to run git: git -c credential.helper= -c credential.helper=!"[^"]+" auth git-credential clone https://github.com/someone/REPO\.git exited with status 65`, + }, } for _, tt := range tests { - ios, _, stdout, stderr := iostreams.Test() - ios.SetStdinTTY(tt.tty) - ios.SetStdoutTTY(tt.tty) - ios.SetStderrTTY(tt.tty) - tt.opts.IO = ios - - tt.opts.BaseRepo = func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - } - - reg := &httpmock.Registry{} - if tt.httpStubs != nil { - tt.httpStubs(reg) - } - tt.opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - - cfg := config.NewBlankConfig() - if tt.cfgStubs != nil { - tt.cfgStubs(cfg) - } - tt.opts.Config = func() (config.Config, error) { - return cfg, nil - } - - tt.opts.Remotes = func() (context.Remotes, error) { - if tt.remotes == nil { - return []*context.Remote{ - { - Remote: &git.Remote{ - Name: "origin", - FetchURL: &url.URL{}, - }, - Repo: ghrepo.New("OWNER", "REPO"), - }, - }, nil - } - return tt.remotes, nil - } - - tt.opts.GitClient = &git.Client{ - GhPath: "some/path/gh", - GitPath: "some/path/git", - } - - //nolint:staticcheck // SA1019: prompt.InitAskStubber is deprecated: use NewAskStubber - as, teardown := prompt.InitAskStubber() - defer teardown() - if tt.askStubs != nil { - tt.askStubs(as) - } - cs, restoreRun := run.Stub() - defer restoreRun(t) - if tt.execStubs != nil { - tt.execStubs(cs) - } - t.Run(tt.name, func(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdinTTY(tt.tty) + ios.SetStdoutTTY(tt.tty) + ios.SetStderrTTY(tt.tty) + tt.opts.IO = ios + + tt.opts.BaseRepo = func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + } + + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + cfg := config.NewBlankConfig() + if tt.cfgStubs != nil { + tt.cfgStubs(cfg) + } + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + + tt.opts.Remotes = func() (context.Remotes, error) { + if tt.remotes == nil { + return []*context.Remote{ + { + Remote: &git.Remote{ + Name: "origin", + FetchURL: &url.URL{}, + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + return tt.remotes, nil + } + + tt.opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } + + //nolint:staticcheck // SA1019: prompt.InitAskStubber is deprecated: use NewAskStubber + as, teardown := prompt.InitAskStubber() + defer teardown() + if tt.askStubs != nil { + tt.askStubs(as) + } + + cs, restoreRun := run.Stub() + defer restoreRun(t) + if tt.execStubs != nil { + tt.execStubs(cs) + } + if tt.opts.Since == nil { tt.opts.Since = func(t time.Time) time.Duration { return 2 * time.Second } } + defer reg.Verify(t) err := forkRun(tt.opts) if tt.wantErr { - assert.Error(t, err) - assert.Equal(t, tt.errMsg, err.Error()) + assert.Error(t, err, tt.errMsg) return }