Skip to content

Commit e91b97b

Browse files
author
Nate Smith
authored
fully restore fork remote renaming behavior (cli#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
1 parent 4a897f7 commit e91b97b

File tree

2 files changed

+180
-22
lines changed

2 files changed

+180
-22
lines changed

‎pkg/cmd/repo/fork/fork.go‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package fork
22

33
import (
4+
"errors"
45
"fmt"
56
"net/http"
67
"net/url"
@@ -35,6 +36,7 @@ type ForkOptions struct {
3536
PromptClone bool
3637
PromptRemote bool
3738
RemoteName string
39+
Rename bool
3840
}
3941

4042
var Since = func(t time.Time) time.Duration {
@@ -61,7 +63,12 @@ func NewCmdFork(f *cmdutil.Factory, runF func(*ForkOptions) error) *cobra.Comman
6163
Short: "Create a fork of a repository",
6264
Long: `Create a fork of a repository.
6365
64-
With no argument, creates a fork of the current repository. Otherwise, forks the specified repository.
66+
With no argument, creates a fork of the current repository. Otherwise, forks
67+
the specified repository.
68+
69+
By default, the new fork is set to be your 'origin' remote and any existing
70+
origin remote is renamed to 'upstream'. To alter this behavior, you can set
71+
a name for the new fork's remote with --remote-name.
6572
6673
Additional 'git clone' flags can be passed in by listing them after '--'.`,
6774
RunE: func(cmd *cobra.Command, args []string) error {
@@ -71,6 +78,10 @@ Additional 'git clone' flags can be passed in by listing them after '--'.`,
7178
opts.GitArgs = args[1:]
7279
}
7380

81+
if opts.RemoteName == "" {
82+
return &cmdutil.FlagError{Err: errors.New("--remote-name cannot be blank")}
83+
}
84+
7485
if promptOk && !cmd.Flags().Changed("clone") {
7586
opts.PromptClone = true
7687
}
@@ -79,6 +90,10 @@ Additional 'git clone' flags can be passed in by listing them after '--'.`,
7990
opts.PromptRemote = true
8091
}
8192

93+
if !cmd.Flags().Changed("remote-name") {
94+
opts.Rename = true
95+
}
96+
8297
if runF != nil {
8398
return runF(opts)
8499
}
@@ -237,11 +252,7 @@ func forkRun(opts *ForkOptions) error {
237252
}
238253

239254
if _, err := remotes.FindByName(remoteName); err == nil {
240-
if connectedToTerminal {
241-
return fmt.Errorf("a remote called '%s' already exists. You can rerun this command with --remote-name to specify a different remote name.", remoteName)
242-
} else {
243-
// TODO next major version we should break this behavior and force users to opt into
244-
// remote renaming in a scripting context via --remote-name
255+
if opts.Rename {
245256
renameTarget := "upstream"
246257
renameCmd, err := git.GitCommand("remote", "rename", remoteName, renameTarget)
247258
if err != nil {
@@ -251,6 +262,8 @@ func forkRun(opts *ForkOptions) error {
251262
if err != nil {
252263
return err
253264
}
265+
} else {
266+
return fmt.Errorf("a git remote named '%s' already exists", remoteName)
254267
}
255268
}
256269

‎pkg/cmd/repo/fork/fork_test.go‎

Lines changed: 161 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package fork
22

33
import (
4+
"bytes"
45
"net/http"
56
"net/url"
67
"regexp"
@@ -21,6 +22,129 @@ import (
2122
"github.com/stretchr/testify/assert"
2223
)
2324

25+
func TestNewCmdFork(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
cli string
29+
tty bool
30+
wants ForkOptions
31+
wantErr bool
32+
}{
33+
{
34+
name: "repo with git args",
35+
cli: "foo/bar -- --foo=bar",
36+
wants: ForkOptions{
37+
Repository: "foo/bar",
38+
GitArgs: []string{"TODO"},
39+
RemoteName: "origin",
40+
Rename: true,
41+
},
42+
},
43+
{
44+
name: "git args without repo",
45+
cli: "-- --foo bar",
46+
wantErr: true,
47+
},
48+
{
49+
name: "repo",
50+
cli: "foo/bar",
51+
wants: ForkOptions{
52+
Repository: "foo/bar",
53+
RemoteName: "origin",
54+
Rename: true,
55+
},
56+
},
57+
{
58+
name: "blank remote name",
59+
cli: "--remote --remote-name=''",
60+
wantErr: true,
61+
},
62+
{
63+
name: "remote name",
64+
cli: "--remote --remote-name=foo",
65+
wants: ForkOptions{
66+
RemoteName: "foo",
67+
Rename: false,
68+
Remote: true,
69+
},
70+
},
71+
{
72+
name: "blank nontty",
73+
cli: "",
74+
wants: ForkOptions{
75+
RemoteName: "origin",
76+
Rename: true,
77+
},
78+
},
79+
{
80+
name: "blank tty",
81+
cli: "",
82+
tty: true,
83+
wants: ForkOptions{
84+
RemoteName: "origin",
85+
PromptClone: true,
86+
PromptRemote: true,
87+
Rename: true,
88+
},
89+
},
90+
{
91+
name: "clone",
92+
cli: "--clone",
93+
wants: ForkOptions{
94+
RemoteName: "origin",
95+
Rename: true,
96+
},
97+
},
98+
{
99+
name: "remote",
100+
cli: "--remote",
101+
wants: ForkOptions{
102+
RemoteName: "origin",
103+
Remote: true,
104+
Rename: true,
105+
},
106+
},
107+
}
108+
109+
for _, tt := range tests {
110+
t.Run(tt.name, func(t *testing.T) {
111+
io, _, _, _ := iostreams.Test()
112+
113+
f := &cmdutil.Factory{
114+
IOStreams: io,
115+
}
116+
117+
io.SetStdoutTTY(tt.tty)
118+
io.SetStdinTTY(tt.tty)
119+
120+
argv, err := shlex.Split(tt.cli)
121+
assert.NoError(t, err)
122+
123+
var gotOpts *ForkOptions
124+
cmd := NewCmdFork(f, func(opts *ForkOptions) error {
125+
gotOpts = opts
126+
return nil
127+
})
128+
cmd.SetArgs(argv)
129+
cmd.SetIn(&bytes.Buffer{})
130+
cmd.SetOut(&bytes.Buffer{})
131+
cmd.SetErr(&bytes.Buffer{})
132+
133+
_, err = cmd.ExecuteC()
134+
if tt.wantErr {
135+
assert.Error(t, err)
136+
return
137+
}
138+
assert.NoError(t, err)
139+
140+
assert.Equal(t, tt.wants.RemoteName, gotOpts.RemoteName)
141+
assert.Equal(t, tt.wants.Remote, gotOpts.Remote)
142+
assert.Equal(t, tt.wants.PromptRemote, gotOpts.PromptRemote)
143+
assert.Equal(t, tt.wants.PromptClone, gotOpts.PromptClone)
144+
})
145+
}
146+
}
147+
24148
func runCommand(httpClient *http.Client, remotes []*context.Remote, isTTY bool, cli string) (*test.CmdOut, error) {
25149
io, stdin, stdout, stderr := iostreams.Test()
26150
io.SetStdoutTTY(isTTY)
@@ -98,22 +222,6 @@ func TestRepoFork_nontty(t *testing.T) {
98222

99223
}
100224

101-
func TestRepoFork_existing_remote_error(t *testing.T) {
102-
defer stubSince(2 * time.Second)()
103-
reg := &httpmock.Registry{}
104-
defer reg.StubWithFixturePath(200, "./forkResult.json")()
105-
httpClient := &http.Client{Transport: reg}
106-
107-
_, err := runCommand(httpClient, nil, true, "--remote")
108-
if err == nil {
109-
t.Fatal("expected error running command `repo fork`")
110-
}
111-
112-
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())
113-
114-
reg.Verify(t)
115-
}
116-
117225
func TestRepoFork_no_conflicting_remote(t *testing.T) {
118226
remotes := []*context.Remote{
119227
{
@@ -144,6 +252,43 @@ func TestRepoFork_no_conflicting_remote(t *testing.T) {
144252
assert.Equal(t, "", output.Stderr())
145253
}
146254

255+
func TestRepoFork_existing_remote_error(t *testing.T) {
256+
defer stubSince(2 * time.Second)()
257+
reg := &httpmock.Registry{}
258+
defer reg.StubWithFixturePath(200, "./forkResult.json")()
259+
httpClient := &http.Client{Transport: reg}
260+
261+
_, err := runCommand(httpClient, nil, true, "--remote --remote-name='origin'")
262+
if err == nil {
263+
t.Fatal("expected error running command `repo fork`")
264+
}
265+
266+
assert.Equal(t, "a git remote named 'origin' already exists", err.Error())
267+
268+
reg.Verify(t)
269+
}
270+
271+
func TestRepoFork_in_parent_tty(t *testing.T) {
272+
defer stubSince(2 * time.Second)()
273+
reg := &httpmock.Registry{}
274+
defer reg.StubWithFixturePath(200, "./forkResult.json")()
275+
httpClient := &http.Client{Transport: reg}
276+
277+
cs, restore := run.Stub()
278+
defer restore(t)
279+
280+
cs.Register("git remote rename origin upstream", 0, "")
281+
cs.Register(`git remote add -f origin https://github\.com/someone/REPO\.git`, 0, "")
282+
283+
output, err := runCommand(httpClient, nil, true, "--remote")
284+
if err != nil {
285+
t.Fatalf("error running command `repo fork`: %v", err)
286+
}
287+
288+
assert.Equal(t, "", output.String())
289+
assert.Equal(t, "✓ Created fork someone/REPO\n✓ Added remote origin\n", output.Stderr())
290+
reg.Verify(t)
291+
}
147292
func TestRepoFork_in_parent_nontty(t *testing.T) {
148293
defer stubSince(2 * time.Second)()
149294
reg := &httpmock.Registry{}

0 commit comments

Comments
 (0)