From e91b97b4c50341f0342917de058290f17afafc99 Mon Sep 17 00:00:00 2001 From: Nate Smith Date: Wed, 17 Feb 2021 12:33:22 -0600 Subject: [PATCH] fully restore fork remote renaming behavior (#2982) * fully restore fork remote renaming behavior * catch blank remote name and error + arg tests * hard wrap fork usage * do not rename if remote-name supplied * tweak error text --- pkg/cmd/repo/fork/fork.go | 25 +++-- pkg/cmd/repo/fork/fork_test.go | 177 ++++++++++++++++++++++++++++++--- 2 files changed, 180 insertions(+), 22 deletions(-) diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index e801f0c13..2e4f49eb6 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -1,6 +1,7 @@ package fork import ( + "errors" "fmt" "net/http" "net/url" @@ -35,6 +36,7 @@ type ForkOptions struct { PromptClone bool PromptRemote bool RemoteName string + Rename bool } var Since = func(t time.Time) time.Duration { @@ -61,7 +63,12 @@ func NewCmdFork(f *cmdutil.Factory, runF func(*ForkOptions) error) *cobra.Comman Short: "Create a fork of a repository", Long: `Create a fork of a repository. -With no argument, creates a fork of the current repository. Otherwise, forks the specified repository. +With no argument, creates a fork of the current repository. Otherwise, forks +the specified repository. + +By default, the new fork is set to be your 'origin' remote and any existing +origin remote is renamed to 'upstream'. To alter this behavior, you can set +a name for the new fork's remote with --remote-name. Additional 'git clone' flags can be passed in by listing them after '--'.`, RunE: func(cmd *cobra.Command, args []string) error { @@ -71,6 +78,10 @@ Additional 'git clone' flags can be passed in by listing them after '--'.`, opts.GitArgs = args[1:] } + if opts.RemoteName == "" { + return &cmdutil.FlagError{Err: errors.New("--remote-name cannot be blank")} + } + if promptOk && !cmd.Flags().Changed("clone") { opts.PromptClone = true } @@ -79,6 +90,10 @@ Additional 'git clone' flags can be passed in by listing them after '--'.`, opts.PromptRemote = true } + if !cmd.Flags().Changed("remote-name") { + opts.Rename = true + } + if runF != nil { return runF(opts) } @@ -237,11 +252,7 @@ func forkRun(opts *ForkOptions) error { } if _, err := remotes.FindByName(remoteName); err == nil { - if connectedToTerminal { - return fmt.Errorf("a remote called '%s' already exists. You can rerun this command with --remote-name to specify a different remote name.", remoteName) - } else { - // TODO next major version we should break this behavior and force users to opt into - // remote renaming in a scripting context via --remote-name + if opts.Rename { renameTarget := "upstream" renameCmd, err := git.GitCommand("remote", "rename", remoteName, renameTarget) if err != nil { @@ -251,6 +262,8 @@ func forkRun(opts *ForkOptions) error { if err != nil { return err } + } else { + return fmt.Errorf("a git remote named '%s' already exists", remoteName) } } diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 767c5244e..f8e412b82 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -1,6 +1,7 @@ package fork import ( + "bytes" "net/http" "net/url" "regexp" @@ -21,6 +22,129 @@ import ( "github.com/stretchr/testify/assert" ) +func TestNewCmdFork(t *testing.T) { + tests := []struct { + name string + cli string + tty bool + wants ForkOptions + wantErr bool + }{ + { + name: "repo with git args", + cli: "foo/bar -- --foo=bar", + wants: ForkOptions{ + Repository: "foo/bar", + GitArgs: []string{"TODO"}, + RemoteName: "origin", + Rename: true, + }, + }, + { + name: "git args without repo", + cli: "-- --foo bar", + wantErr: true, + }, + { + name: "repo", + cli: "foo/bar", + wants: ForkOptions{ + Repository: "foo/bar", + RemoteName: "origin", + Rename: true, + }, + }, + { + name: "blank remote name", + cli: "--remote --remote-name=''", + wantErr: true, + }, + { + name: "remote name", + cli: "--remote --remote-name=foo", + wants: ForkOptions{ + RemoteName: "foo", + Rename: false, + Remote: true, + }, + }, + { + name: "blank nontty", + cli: "", + wants: ForkOptions{ + RemoteName: "origin", + Rename: true, + }, + }, + { + name: "blank tty", + cli: "", + tty: true, + wants: ForkOptions{ + RemoteName: "origin", + PromptClone: true, + PromptRemote: true, + Rename: true, + }, + }, + { + name: "clone", + cli: "--clone", + wants: ForkOptions{ + RemoteName: "origin", + Rename: true, + }, + }, + { + name: "remote", + cli: "--remote", + wants: ForkOptions{ + RemoteName: "origin", + Remote: true, + Rename: true, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, _ := iostreams.Test() + + f := &cmdutil.Factory{ + IOStreams: io, + } + + io.SetStdoutTTY(tt.tty) + io.SetStdinTTY(tt.tty) + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + + var gotOpts *ForkOptions + cmd := NewCmdFork(f, func(opts *ForkOptions) error { + gotOpts = opts + return nil + }) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + assert.Equal(t, tt.wants.RemoteName, gotOpts.RemoteName) + assert.Equal(t, tt.wants.Remote, gotOpts.Remote) + assert.Equal(t, tt.wants.PromptRemote, gotOpts.PromptRemote) + assert.Equal(t, tt.wants.PromptClone, gotOpts.PromptClone) + }) + } +} + func runCommand(httpClient *http.Client, remotes []*context.Remote, isTTY bool, cli string) (*test.CmdOut, error) { io, stdin, stdout, stderr := iostreams.Test() io.SetStdoutTTY(isTTY) @@ -98,22 +222,6 @@ func TestRepoFork_nontty(t *testing.T) { } -func TestRepoFork_existing_remote_error(t *testing.T) { - defer stubSince(2 * time.Second)() - reg := &httpmock.Registry{} - defer reg.StubWithFixturePath(200, "./forkResult.json")() - httpClient := &http.Client{Transport: reg} - - _, err := runCommand(httpClient, nil, true, "--remote") - if err == nil { - t.Fatal("expected error running command `repo fork`") - } - - assert.Equal(t, "a remote called 'origin' already exists. You can rerun this command with --remote-name to specify a different remote name.", err.Error()) - - reg.Verify(t) -} - func TestRepoFork_no_conflicting_remote(t *testing.T) { remotes := []*context.Remote{ { @@ -144,6 +252,43 @@ func TestRepoFork_no_conflicting_remote(t *testing.T) { assert.Equal(t, "", output.Stderr()) } +func TestRepoFork_existing_remote_error(t *testing.T) { + defer stubSince(2 * time.Second)() + reg := &httpmock.Registry{} + defer reg.StubWithFixturePath(200, "./forkResult.json")() + httpClient := &http.Client{Transport: reg} + + _, err := runCommand(httpClient, nil, true, "--remote --remote-name='origin'") + if err == nil { + t.Fatal("expected error running command `repo fork`") + } + + assert.Equal(t, "a git remote named 'origin' already exists", err.Error()) + + reg.Verify(t) +} + +func TestRepoFork_in_parent_tty(t *testing.T) { + defer stubSince(2 * time.Second)() + reg := &httpmock.Registry{} + defer reg.StubWithFixturePath(200, "./forkResult.json")() + httpClient := &http.Client{Transport: reg} + + cs, restore := run.Stub() + defer restore(t) + + cs.Register("git remote rename origin upstream", 0, "") + cs.Register(`git remote add -f origin https://github\.com/someone/REPO\.git`, 0, "") + + output, err := runCommand(httpClient, nil, true, "--remote") + if err != nil { + t.Fatalf("error running command `repo fork`: %v", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "āœ“ Created fork someone/REPO\nāœ“ Added remote origin\n", output.Stderr()) + reg.Verify(t) +} func TestRepoFork_in_parent_nontty(t *testing.T) { defer stubSince(2 * time.Second)() reg := &httpmock.Registry{}