diff --git a/git/client.go b/git/client.go index f2c834754e1..078709c2bc1 100644 --- a/git/client.go +++ b/git/client.go @@ -326,7 +326,7 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, // ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` part of git config. func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg BranchConfig) { prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch)) - args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix)} + args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|pushremote)$", prefix)} cmd, err := c.Command(ctx, args...) if err != nil { return @@ -343,19 +343,20 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc keys := strings.Split(parts[0], ".") switch keys[len(keys)-1] { case "remote": - if strings.Contains(parts[1], ":") { - u, err := ParseURL(parts[1]) - if err != nil { - continue - } - cfg.RemoteURL = u - } else if !isFilesystemPath(parts[1]) { - cfg.RemoteName = parts[1] - } + parseRemoteURLOrName(parts[1], &cfg.RemoteURL, &cfg.RemoteName) + case "pushremote": + parseRemoteURLOrName(parts[1], &cfg.PushRemoteURL, &cfg.PushRemoteName) case "merge": cfg.MergeRef = parts[1] } } + if cfg.PushRemoteURL == nil && cfg.PushRemoteName == "" { + if conf, err := c.Config(ctx, "remote.pushDefault"); err == nil && conf != "" { + parseRemoteURLOrName(conf, &cfg.PushRemoteURL, &cfg.PushRemoteName) + } else { + cfg.PushRemoteName = cfg.RemoteName + } + } if out, err = c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}"); err == nil { cfg.Push = strings.TrimSuffix(string(out), "\n") } @@ -700,6 +701,16 @@ func parseRemotes(remotesStr []string) RemoteSet { return remotes } +func parseRemoteURLOrName(value string, remoteURL **url.URL, remoteName *string) { + if strings.Contains(value, ":") { + if u, err := ParseURL(value); err == nil { + *remoteURL = u + } + } else if !isFilesystemPath(value) { + *remoteName = value + } +} + func populateResolvedRemotes(remotes RemoteSet, resolved []string) { for _, l := range resolved { parts := strings.SplitN(l, " ", 2) diff --git a/git/client_test.go b/git/client_test.go index 5448b0be7d1..a58e3f5fc43 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -707,31 +707,160 @@ func TestClientCommitBody(t *testing.T) { } func TestClientReadBranchConfig(t *testing.T) { + type cmdTest struct { + exitStatus int + stdOut string + stdErr string + wantArgs string + } tests := []struct { name string - cmdExitStatus []int - cmdStdout []string - cmdStderr []string - wantCmdArgs []string + cmds []cmdTest wantBranchConfig BranchConfig }{ { - name: "read branch config", - cmdExitStatus: []int{0, 0}, - cmdStdout: []string{"branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk", "origin/trunk"}, - cmdStderr: []string{"", ""}, - wantCmdArgs: []string{`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge)$`, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`}, - wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", Push: "origin/trunk"}, + name: "read branch config, central", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, central, push.default = upstream", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk-remote", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk-remote", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk-remote", PushRemoteName: "origin", Push: "origin/trunk-remote"}, + }, + { + name: "read branch config, central, push.default = current", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/main", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/main", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, central, push.default = current, target branch not pushed, no existing remote branch", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote .\nbranch.trunk.merge refs/heads/trunk-middle", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + stdOut: "origin", + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + exitStatus: 1, + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{MergeRef: "refs/heads/trunk-middle", PushRemoteName: "origin"}, + }, + { + name: "read branch config, triangular, push.default = current, has existing remote branch, branch.trunk.pushremote effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, triangular, push.default = current, has existing remote branch, remote.pushDefault effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + stdOut: "origin", + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, triangular, push.default = current, no existing remote branch, branch.trunk.pushremote effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + exitStatus: 1, + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin"}, + }, + { + name: "read branch config, triangular, push.default = current, no existing remote branch, remote.pushDefault effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote)$`, + }, + { + stdOut: "origin", + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + exitStatus: 1, + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var cmds []*exec.Cmd var cmdCtxs []commandCtx - for i := 0; i < len(tt.cmdExitStatus); i++ { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus[i], tt.cmdStdout[i], tt.cmdStderr[i]) + for _, c := range tt.cmds { + cmd, cmdCtx := createCommandContext(t, c.exitStatus, c.stdOut, c.stdErr) cmds = append(cmds, cmd) cmdCtxs = append(cmdCtxs, cmdCtx) + } i := -1 client := Client{ @@ -743,8 +872,8 @@ func TestClientReadBranchConfig(t *testing.T) { }, } branchConfig := client.ReadBranchConfig(context.Background(), "trunk") - for i := 0; i < len(tt.cmdExitStatus); i++ { - assert.Equal(t, tt.wantCmdArgs[i], strings.Join(cmds[i].Args[3:], " ")) + for i := 0; i < len(tt.cmds); i++ { + assert.Equal(t, tt.cmds[i].wantArgs, strings.Join(cmds[i].Args[3:], " ")) } assert.Equal(t, tt.wantBranchConfig, branchConfig) }) diff --git a/git/objects.go b/git/objects.go index 138584fccf0..21f09900627 100644 --- a/git/objects.go +++ b/git/objects.go @@ -71,8 +71,10 @@ type Commit struct { } type BranchConfig struct { - RemoteName string - RemoteURL *url.URL - MergeRef string - Push string + RemoteName string + RemoteURL *url.URL + MergeRef string + PushRemoteURL *url.URL + PushRemoteName string + Push string } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 13a47300188..1a5de08e1ae 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -562,6 +562,7 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -627,6 +628,7 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -675,6 +677,7 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -726,6 +729,7 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") @@ -822,6 +826,7 @@ func Test_createRun(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/my-feat2 `)) // determineTrackingBranch + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/my-feat2") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD @@ -1007,6 +1012,7 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") @@ -1041,6 +1047,7 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") @@ -1481,6 +1488,7 @@ func Test_determineTrackingBranch(t *testing.T) { name: "empty", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, @@ -1492,6 +1500,7 @@ func Test_determineTrackingBranch(t *testing.T) { name: "no match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify -- HEAD refs/remotes/origin/feature refs/remotes/upstream/feature", 0, "abc HEAD\nbca refs/remotes/origin/feature") }, @@ -1513,6 +1522,7 @@ func Test_determineTrackingBranch(t *testing.T) { name: "match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/feature") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature refs/remotes/upstream/feature$`, 0, heredoc.Doc(` deadbeef HEAD @@ -1542,6 +1552,7 @@ func Test_determineTrackingBranch(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/great-feat `)) + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/great-feat") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/great-feat refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index e12cea7dc84..2adc34dc3db 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -251,22 +251,21 @@ func (f *finder) parseCurrentBranch() (string, int, error) { } var branchOwner string - if branchConfig.RemoteURL != nil { + if branchConfig.PushRemoteURL != nil { // the branch merges from a remote specified by URL if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { branchOwner = r.RepoOwner() } - } else if branchConfig.RemoteName != "" { - // the branch merges from a remote specified by name + } else if branchConfig.PushRemoteName != "" { rem, _ := f.remotesFn() - if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { + if r, err := rem.FindByName(branchConfig.PushRemoteName); err == nil { branchOwner = r.RepoOwner() } } if branchOwner != "" { if branchConfig.Push != "" { - prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") + prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") } else if pushDefault, _ := f.pushDefault(); (pushDefault == "upstream" || pushDefault == "tracking") && strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 731e6a530b3..4591a76358d 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -328,6 +328,7 @@ func TestFind(t *testing.T) { branchConfig: func(branch string) (c git.BranchConfig) { c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteName = "origin" + c.PushRemoteName = "origin" c.Push = "origin/blue-upstream-berries" return }, @@ -373,6 +374,7 @@ func TestFind(t *testing.T) { u, _ := url.Parse("https://github.com/UPSTREAMOWNER/REPO") c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteURL = u + c.PushRemoteURL = u return }, pushDefault: func() (string, error) { return "upstream", nil }, @@ -411,6 +413,7 @@ func TestFind(t *testing.T) { branchConfig: func(branch string) (c git.BranchConfig) { c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteName = "origin" + c.PushRemoteName = "origin" c.Push = "origin/blue-upstream-berries" return }, diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 1b6da842714..33fc096c8e8 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -201,16 +201,16 @@ func prSelectorForCurrentBranch(gitClient *git.Client, baseRepo ghrepo.Interface if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { branchOwner = r.RepoOwner() } - } else if branchConfig.RemoteName != "" { + } else if branchConfig.PushRemoteName != "" { // the branch merges from a remote specified by name - if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { + if r, err := rem.FindByName(branchConfig.PushRemoteName); err == nil { branchOwner = r.RepoOwner() } } if branchOwner != "" { if branchConfig.Push != "" { - selector = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") + selector = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") } else if pushDefault, _ := gitClient.Config(context.Background(), "push.default"); (pushDefault == "upstream" || pushDefault == "tracking") && strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { selector = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index d09abf5b965..e60764c4f2c 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -383,6 +383,7 @@ func Test_prSelectorForCurrentBranchPushDefaultUpstream(t *testing.T) { branch.Frederick888/main.remote git@github.com:Frederick888/playground.git branch.Frederick888/main.merge refs/heads/main `)) + rs.Register(`git config remote.pushDefault`, 1, "") rs.Register(`git rev-parse --verify --quiet --abbrev-ref Frederick888/main@\{push\}`, 1, "") rs.Register(`git config push\.default`, 0, "upstream") @@ -414,6 +415,7 @@ func Test_prSelectorForCurrentBranchPushDefaultTracking(t *testing.T) { branch.Frederick888/main.remote git@github.com:Frederick888/playground.git branch.Frederick888/main.merge refs/heads/main `)) + rs.Register(`git config remote.pushDefault`, 1, "") rs.Register(`git rev-parse --verify --quiet --abbrev-ref Frederick888/main@\{push\}`, 1, "") rs.Register(`git config push\.default`, 0, "tracking")