diff --git a/git/client.go b/git/client.go index 82ead58a4..956d13671 100644 --- a/git/client.go +++ b/git/client.go @@ -402,23 +402,25 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, nil } - pushDefaultOut, err := c.Config(ctx, "remote.pushDefault") + // Check to see if there is a pushDefault ref set for the repo + remotePushDefaultOut, err := c.Config(ctx, "remote.pushDefault") if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { return BranchConfig{}, err } - revParseOut, err := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}") - if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { - return BranchConfig{}, err - } + // Check to see if we can resolve the @{push} revision syntax. This is the easiest way to get + // the name of the push remote. + //We ignore errors resolving simple push.Default settings as these are handled downstream + revParseOut, _ := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}") - return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(pushDefaultOut, "\n"), firstLine(revParseOut)), nil + return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n"), firstLine(revParseOut)), nil } -func parseBranchConfig(configLines []string, pushDefault string, revParse string) BranchConfig { +func parseBranchConfig(branchConfigLines []string, remotePushDefault string, revParse string) BranchConfig { var cfg BranchConfig - for _, line := range configLines { + // Read the config lines for the specific branch + for _, line := range branchConfigLines { parts := strings.SplitN(line, " ", 2) if len(parts) < 2 { continue @@ -429,6 +431,7 @@ func parseBranchConfig(configLines []string, pushDefault string, revParse string remoteURL, remoteName := parseRemoteURLOrName(parts[1]) cfg.RemoteURL = remoteURL cfg.RemoteName = remoteName + // pushremote usually indicates a "triangular" workflow case "pushremote": pushRemoteURL, pushRemoteName := parseRemoteURLOrName(parts[1]) cfg.PushRemoteURL = pushRemoteURL @@ -440,18 +443,27 @@ func parseBranchConfig(configLines []string, pushDefault string, revParse string } } + // PushRemote{URL|Name} takes precedence over remotePushDefault, so we'll only + // use remotePushDefault if we don't have a push remote. if cfg.PushRemoteURL == nil && cfg.PushRemoteName == "" { - if pushDefault != "" { - pushRemoteURL, pushRemoteName := parseRemoteURLOrName(pushDefault) + // remotePushDefault usually indicates a "triangular" workflow + if remotePushDefault != "" { + pushRemoteURL, pushRemoteName := parseRemoteURLOrName(remotePushDefault) cfg.PushRemoteURL = pushRemoteURL cfg.PushRemoteName = pushRemoteName } else { + // Without a PushRemote{URL|Name} or a remotePushDefault, we assume that the + // push remote ref is the same as the remote ref. This is likely + // a "centralized" workflow. cfg.PushRemoteName = cfg.RemoteName } } + // The value returned by revParse is the easiest way to get the name of the push ref cfg.Push = revParse - cfg.PushDefaultName = pushDefault + + // Some `gh pr` workflows don't work if this is set to 'simple' or 'current' + cfg.RemotePushDefault = remotePushDefault return cfg } diff --git a/git/client_test.go b/git/client_test.go index 801d7c025..ba13c889a 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -787,7 +787,7 @@ func TestClientReadBranchConfig(t *testing.T) { Stdout: "branch.trunk.remote origin\n", }, `path/to/git config remote.pushDefault`: { - Stdout: "pushdefault", + Stdout: "remotePushDefault", }, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { Stdout: "origin/trunk", @@ -795,10 +795,10 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "pushdefault", - Push: "origin/trunk", - PushDefaultName: "pushdefault", + RemoteName: "origin", + PushRemoteName: "remotePushDefault", + Push: "origin/trunk", + RemotePushDefault: "remotePushDefault", }, wantError: nil, }, @@ -809,7 +809,7 @@ func TestClientReadBranchConfig(t *testing.T) { Stdout: "branch.trunk.remote origin\n", }, `path/to/git config remote.pushDefault`: { - Stdout: "pushdefault", + Stdout: "remotePushDefault", }, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { ExitStatus: 1, @@ -817,49 +817,28 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "pushdefault", - PushDefaultName: "pushdefault", + RemoteName: "origin", + PushRemoteName: "remotePushDefault", + RemotePushDefault: "remotePushDefault", }, wantError: nil, }, { - name: "when git reads the config, pushDefault is set, and rev-parse fails, it should return an empty BranchConfig and the error", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\n", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "pushdefault", - }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - ExitStatus: 2, - Stderr: "rev-parse error", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{}, - wantError: &GitError{ - ExitCode: 2, - Stderr: "rev-parse error", - }, - }, - { - name: "when git reads the config but pushdefault fails, it should return and empty BranchConfig and the error", + name: "when git reads the config but remotePushDefault fails, it should return and empty BranchConfig and the error", cmds: mockedCommands{ `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { Stdout: "branch.trunk.remote origin\n", }, `path/to/git config remote.pushDefault`: { ExitStatus: 2, - Stderr: "pushdefault error", + Stderr: "remotePushDefault error", }, }, branch: "trunk", wantBranchConfig: BranchConfig{}, wantError: &GitError{ ExitCode: 2, - Stderr: "pushdefault error", + Stderr: "remotePushDefault error", }, }, { @@ -963,11 +942,11 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - Push: "origin/trunk", - PushDefaultName: "origin", + RemoteName: "upstream", + MergeRef: "refs/heads/main", + PushRemoteName: "origin", + Push: "origin/trunk", + RemotePushDefault: "origin", }, }, { @@ -985,10 +964,10 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - PushDefaultName: "current", + RemoteName: "upstream", + MergeRef: "refs/heads/main", + PushRemoteName: "origin", + RemotePushDefault: "current", }, }, { @@ -1006,10 +985,10 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - PushDefaultName: "origin", + RemoteName: "upstream", + MergeRef: "refs/heads/main", + PushRemoteName: "origin", + RemotePushDefault: "origin", }, }, } @@ -1021,15 +1000,15 @@ func TestClientReadBranchConfig(t *testing.T) { commandContext: cmdCtx, } branchConfig, err := client.ReadBranchConfig(context.Background(), tt.branch) - assert.Equal(t, tt.wantBranchConfig, branchConfig) if tt.wantError != nil { var gitError *GitError require.ErrorAs(t, err, &gitError) assert.Equal(t, tt.wantError.ExitCode, gitError.ExitCode) assert.Equal(t, tt.wantError.Stderr, gitError.Stderr) } else { - assert.NoError(t, err) + require.NoError(t, err) } + assert.Equal(t, tt.wantBranchConfig, branchConfig) }) } } @@ -1082,10 +1061,10 @@ func Test_parseBranchConfig(t *testing.T) { { name: "push default specified", configLines: []string{}, - pushDefault: "pushdefault", + pushDefault: "remotePushDefault", wantBranchConfig: BranchConfig{ - PushRemoteName: "pushdefault", - PushDefaultName: "pushdefault", + PushRemoteName: "remotePushDefault", + RemotePushDefault: "remotePushDefault", }, }, { @@ -1128,15 +1107,15 @@ func Test_parseBranchConfig(t *testing.T) { "branch.trunk.gh-merge-base gh-merge-base", "branch.trunk.merge refs/heads/trunk", }, - pushDefault: "pushdefault", + pushDefault: "remotePushDefault", revParse: "origin/trunk", wantBranchConfig: BranchConfig{ - RemoteName: "remote", - PushRemoteName: "pushremote", - MergeBase: "gh-merge-base", - MergeRef: "refs/heads/trunk", - Push: "origin/trunk", - PushDefaultName: "pushdefault", + RemoteName: "remote", + PushRemoteName: "pushremote", + MergeBase: "gh-merge-base", + MergeRef: "refs/heads/trunk", + Push: "origin/trunk", + RemotePushDefault: "remotePushDefault", }, }, { @@ -1158,7 +1137,7 @@ func Test_parseBranchConfig(t *testing.T) { assert.Equalf(t, tt.wantBranchConfig.MergeBase, branchConfig.MergeBase, "unexpected MergeBase") assert.Equalf(t, tt.wantBranchConfig.PushRemoteName, branchConfig.PushRemoteName, "unexpected PushRemoteName") assert.Equalf(t, tt.wantBranchConfig.Push, branchConfig.Push, "unexpected Push") - assert.Equalf(t, tt.wantBranchConfig.PushDefaultName, branchConfig.PushDefaultName, "unexpected PushDefaultName") + assert.Equalf(t, tt.wantBranchConfig.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault") if tt.wantBranchConfig.RemoteURL != nil { assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL") } diff --git a/git/objects.go b/git/objects.go index 8a9d086c6..ff4068173 100644 --- a/git/objects.go +++ b/git/objects.go @@ -64,10 +64,10 @@ type BranchConfig struct { RemoteName string RemoteURL *url.URL // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. - MergeBase string - MergeRef string - PushDefaultName string - PushRemoteURL *url.URL - PushRemoteName string - Push string + MergeBase string + MergeRef string + RemotePushDefault string + PushRemoteURL *url.URL + PushRemoteName string + Push string } diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 593f05e63..fe674ad6e 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -226,8 +226,9 @@ func prSelectorForCurrentBranch(branchConfig git.BranchConfig, baseRepo ghrepo.I if branchOwner != "" { selector := prHeadRef if branchConfig.Push != "" { + // If we have a resolved push revision ref, we defer to that selector = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") - } else if (branchConfig.PushDefaultName == "upstream" || branchConfig.PushDefaultName == "tracking") && + } else if (branchConfig.RemotePushDefault == "upstream" || branchConfig.RemotePushDefault == "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 97524acba..586e22183 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -666,8 +666,8 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { Host: "github.com", Path: "Frederick888/playground.git", }, - MergeRef: "refs/heads/main", - PushDefaultName: "upstream", + MergeRef: "refs/heads/main", + RemotePushDefault: "upstream", }, baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"), prHeadRef: "Frederick888/main", @@ -690,8 +690,8 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { Host: "github.com", Path: "Frederick888/playground.git", }, - MergeRef: "refs/heads/main", - PushDefaultName: "tracking", + MergeRef: "refs/heads/main", + RemotePushDefault: "tracking", }, baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"), prHeadRef: "Frederick888/main",