Skip to content

Commit

Permalink
Find push remote using branch.<name>.pushRemote and remote.pushDefault
Browse files Browse the repository at this point in the history 
When using a push.default = current triangular workflow, apart from
using @{push} to determine the remote branch name, we should also follow
the

1. branch.<name>.pushRemote
2. remote.pushDefault
3. branch.<name>.remote

...list to determine which remote Git pushes to.
  • Loading branch information
@Frederick888
Frederick888 committed Jul 25, 2024
1 parent dc44d57 commit ad62216
Show file tree
Hide file tree
Showing 8 changed files with 193 additions and 36 deletions.
31 changes: 21 additions & 10 deletions git/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
}
Expand Down Expand Up @@ -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)
Expand Down
157 changes: 143 additions & 14 deletions git/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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)
})
Expand Down
10 changes: 6 additions & 4 deletions git/objects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
11 changes: 11 additions & 0 deletions pkg/cmd/pr/create/create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, "")
Expand Down Expand Up @@ -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, "")
Expand Down Expand Up @@ -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, "")
Expand Down Expand Up @@ -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, "")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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, "")
Expand Down Expand Up @@ -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, "")
Expand Down Expand Up @@ -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")
},
Expand All @@ -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")
},
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 4 additions & 5 deletions pkg/cmd/pr/shared/finder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/")
Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/pr/shared/finder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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
},
Expand Down
Loading

0 comments on commit ad62216

Please sign in to comment.