Skip to content

Commit 4a9fd95

Browse files
jtmcgjtmcg
committed
Add comments and a bit of code cleanup
1 parent 0184380 commit 4a9fd95

File tree

5 files changed

+73
-81
lines changed

5 files changed

+73
-81
lines changed

‎git/client.go‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -402,23 +402,25 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon
402402
return BranchConfig{}, nil
403403
}
404404

405-
pushDefaultOut, err := c.Config(ctx, "remote.pushDefault")
405+
// Check to see if there is a pushDefault ref set for the repo
406+
remotePushDefaultOut, err := c.Config(ctx, "remote.pushDefault")
406407
if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 {
407408
return BranchConfig{}, err
408409
}
409410

410-
revParseOut, err := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}")
411-
if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 {
412-
return BranchConfig{}, err
413-
}
411+
// Check to see if we can resolve the @{push} revision syntax. This is the easiest way to get
412+
// the name of the push remote.
413+
//We ignore errors resolving simple push.Default settings as these are handled downstream
414+
revParseOut, _ := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}")
414415

415-
return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(pushDefaultOut, "\n"), firstLine(revParseOut)), nil
416+
return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n"), firstLine(revParseOut)), nil
416417
}
417418

418-
func parseBranchConfig(configLines []string, pushDefault string, revParse string) BranchConfig {
419+
func parseBranchConfig(branchConfigLines []string, remotePushDefault string, revParse string) BranchConfig {
419420
var cfg BranchConfig
420421

421-
for _, line := range configLines {
422+
// Read the config lines for the specific branch
423+
for _, line := range branchConfigLines {
422424
parts := strings.SplitN(line, " ", 2)
423425
if len(parts) < 2 {
424426
continue
@@ -429,6 +431,7 @@ func parseBranchConfig(configLines []string, pushDefault string, revParse string
429431
remoteURL, remoteName := parseRemoteURLOrName(parts[1])
430432
cfg.RemoteURL = remoteURL
431433
cfg.RemoteName = remoteName
434+
// pushremote usually indicates a "triangular" workflow
432435
case "pushremote":
433436
pushRemoteURL, pushRemoteName := parseRemoteURLOrName(parts[1])
434437
cfg.PushRemoteURL = pushRemoteURL
@@ -440,18 +443,27 @@ func parseBranchConfig(configLines []string, pushDefault string, revParse string
440443
}
441444
}
442445

446+
// PushRemote{URL|Name} takes precedence over remotePushDefault, so we'll only
447+
// use remotePushDefault if we don't have a push remote.
443448
if cfg.PushRemoteURL == nil && cfg.PushRemoteName == "" {
444-
if pushDefault != "" {
445-
pushRemoteURL, pushRemoteName := parseRemoteURLOrName(pushDefault)
449+
// remotePushDefault usually indicates a "triangular" workflow
450+
if remotePushDefault != "" {
451+
pushRemoteURL, pushRemoteName := parseRemoteURLOrName(remotePushDefault)
446452
cfg.PushRemoteURL = pushRemoteURL
447453
cfg.PushRemoteName = pushRemoteName
448454
} else {
455+
// Without a PushRemote{URL|Name} or a remotePushDefault, we assume that the
456+
// push remote ref is the same as the remote ref. This is likely
457+
// a "centralized" workflow.
449458
cfg.PushRemoteName = cfg.RemoteName
450459
}
451460
}
452461

462+
// The value returned by revParse is the easiest way to get the name of the push ref
453463
cfg.Push = revParse
454-
cfg.PushDefaultName = pushDefault
464+
465+
// Some `gh pr` workflows don't work if this is set to 'simple' or 'current'
466+
cfg.RemotePushDefault = remotePushDefault
455467

456468
return cfg
457469
}

‎git/client_test.go‎

Lines changed: 38 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -787,18 +787,18 @@ func TestClientReadBranchConfig(t *testing.T) {
787787
Stdout: "branch.trunk.remote origin\n",
788788
},
789789
`path/to/git config remote.pushDefault`: {
790-
Stdout: "pushdefault",
790+
Stdout: "remotePushDefault",
791791
},
792792
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
793793
Stdout: "origin/trunk",
794794
},
795795
},
796796
branch: "trunk",
797797
wantBranchConfig: BranchConfig{
798-
RemoteName: "origin",
799-
PushRemoteName: "pushdefault",
800-
Push: "origin/trunk",
801-
PushDefaultName: "pushdefault",
798+
RemoteName: "origin",
799+
PushRemoteName: "remotePushDefault",
800+
Push: "origin/trunk",
801+
RemotePushDefault: "remotePushDefault",
802802
},
803803
wantError: nil,
804804
},
@@ -809,57 +809,36 @@ func TestClientReadBranchConfig(t *testing.T) {
809809
Stdout: "branch.trunk.remote origin\n",
810810
},
811811
`path/to/git config remote.pushDefault`: {
812-
Stdout: "pushdefault",
812+
Stdout: "remotePushDefault",
813813
},
814814
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
815815
ExitStatus: 1,
816816
},
817817
},
818818
branch: "trunk",
819819
wantBranchConfig: BranchConfig{
820-
RemoteName: "origin",
821-
PushRemoteName: "pushdefault",
822-
PushDefaultName: "pushdefault",
820+
RemoteName: "origin",
821+
PushRemoteName: "remotePushDefault",
822+
RemotePushDefault: "remotePushDefault",
823823
},
824824
wantError: nil,
825825
},
826826
{
827-
name: "when git reads the config, pushDefault is set, and rev-parse fails, it should return an empty BranchConfig and the error",
827+
name: "when git reads the config but remotePushDefault fails, it should return and empty BranchConfig and the error",
828828
cmds: mockedCommands{
829829
`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: {
830830
Stdout: "branch.trunk.remote origin\n",
831831
},
832832
`path/to/git config remote.pushDefault`: {
833-
Stdout: "pushdefault",
834-
},
835-
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
836833
ExitStatus: 2,
837-
Stderr: "rev-parse error",
834+
Stderr: "remotePushDefault error",
838835
},
839836
},
840837
branch: "trunk",
841838
wantBranchConfig: BranchConfig{},
842839
wantError: &GitError{
843840
ExitCode: 2,
844-
Stderr: "rev-parse error",
845-
},
846-
},
847-
{
848-
name: "when git reads the config but pushdefault fails, it should return and empty BranchConfig and the error",
849-
cmds: mockedCommands{
850-
`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: {
851-
Stdout: "branch.trunk.remote origin\n",
852-
},
853-
`path/to/git config remote.pushDefault`: {
854-
ExitStatus: 2,
855-
Stderr: "pushdefault error",
856-
},
857-
},
858-
branch: "trunk",
859-
wantBranchConfig: BranchConfig{},
860-
wantError: &GitError{
861-
ExitCode: 2,
862-
Stderr: "pushdefault error",
841+
Stderr: "remotePushDefault error",
863842
},
864843
},
865844
{
@@ -963,11 +942,11 @@ func TestClientReadBranchConfig(t *testing.T) {
963942
},
964943
branch: "trunk",
965944
wantBranchConfig: BranchConfig{
966-
RemoteName: "upstream",
967-
MergeRef: "refs/heads/main",
968-
PushRemoteName: "origin",
969-
Push: "origin/trunk",
970-
PushDefaultName: "origin",
945+
RemoteName: "upstream",
946+
MergeRef: "refs/heads/main",
947+
PushRemoteName: "origin",
948+
Push: "origin/trunk",
949+
RemotePushDefault: "origin",
971950
},
972951
},
973952
{
@@ -985,10 +964,10 @@ func TestClientReadBranchConfig(t *testing.T) {
985964
},
986965
branch: "trunk",
987966
wantBranchConfig: BranchConfig{
988-
RemoteName: "upstream",
989-
MergeRef: "refs/heads/main",
990-
PushRemoteName: "origin",
991-
PushDefaultName: "current",
967+
RemoteName: "upstream",
968+
MergeRef: "refs/heads/main",
969+
PushRemoteName: "origin",
970+
RemotePushDefault: "current",
992971
},
993972
},
994973
{
@@ -1006,10 +985,10 @@ func TestClientReadBranchConfig(t *testing.T) {
1006985
},
1007986
branch: "trunk",
1008987
wantBranchConfig: BranchConfig{
1009-
RemoteName: "upstream",
1010-
MergeRef: "refs/heads/main",
1011-
PushRemoteName: "origin",
1012-
PushDefaultName: "origin",
988+
RemoteName: "upstream",
989+
MergeRef: "refs/heads/main",
990+
PushRemoteName: "origin",
991+
RemotePushDefault: "origin",
1013992
},
1014993
},
1015994
}
@@ -1021,15 +1000,15 @@ func TestClientReadBranchConfig(t *testing.T) {
10211000
commandContext: cmdCtx,
10221001
}
10231002
branchConfig, err := client.ReadBranchConfig(context.Background(), tt.branch)
1024-
assert.Equal(t, tt.wantBranchConfig, branchConfig)
10251003
if tt.wantError != nil {
10261004
var gitError *GitError
10271005
require.ErrorAs(t, err, &gitError)
10281006
assert.Equal(t, tt.wantError.ExitCode, gitError.ExitCode)
10291007
assert.Equal(t, tt.wantError.Stderr, gitError.Stderr)
10301008
} else {
1031-
assert.NoError(t, err)
1009+
require.NoError(t, err)
10321010
}
1011+
assert.Equal(t, tt.wantBranchConfig, branchConfig)
10331012
})
10341013
}
10351014
}
@@ -1082,10 +1061,10 @@ func Test_parseBranchConfig(t *testing.T) {
10821061
{
10831062
name: "push default specified",
10841063
configLines: []string{},
1085-
pushDefault: "pushdefault",
1064+
pushDefault: "remotePushDefault",
10861065
wantBranchConfig: BranchConfig{
1087-
PushRemoteName: "pushdefault",
1088-
PushDefaultName: "pushdefault",
1066+
PushRemoteName: "remotePushDefault",
1067+
RemotePushDefault: "remotePushDefault",
10891068
},
10901069
},
10911070
{
@@ -1128,15 +1107,15 @@ func Test_parseBranchConfig(t *testing.T) {
11281107
"branch.trunk.gh-merge-base gh-merge-base",
11291108
"branch.trunk.merge refs/heads/trunk",
11301109
},
1131-
pushDefault: "pushdefault",
1110+
pushDefault: "remotePushDefault",
11321111
revParse: "origin/trunk",
11331112
wantBranchConfig: BranchConfig{
1134-
RemoteName: "remote",
1135-
PushRemoteName: "pushremote",
1136-
MergeBase: "gh-merge-base",
1137-
MergeRef: "refs/heads/trunk",
1138-
Push: "origin/trunk",
1139-
PushDefaultName: "pushdefault",
1113+
RemoteName: "remote",
1114+
PushRemoteName: "pushremote",
1115+
MergeBase: "gh-merge-base",
1116+
MergeRef: "refs/heads/trunk",
1117+
Push: "origin/trunk",
1118+
RemotePushDefault: "remotePushDefault",
11401119
},
11411120
},
11421121
{
@@ -1158,7 +1137,7 @@ func Test_parseBranchConfig(t *testing.T) {
11581137
assert.Equalf(t, tt.wantBranchConfig.MergeBase, branchConfig.MergeBase, "unexpected MergeBase")
11591138
assert.Equalf(t, tt.wantBranchConfig.PushRemoteName, branchConfig.PushRemoteName, "unexpected PushRemoteName")
11601139
assert.Equalf(t, tt.wantBranchConfig.Push, branchConfig.Push, "unexpected Push")
1161-
assert.Equalf(t, tt.wantBranchConfig.PushDefaultName, branchConfig.PushDefaultName, "unexpected PushDefaultName")
1140+
assert.Equalf(t, tt.wantBranchConfig.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault")
11621141
if tt.wantBranchConfig.RemoteURL != nil {
11631142
assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL")
11641143
}

‎git/objects.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,10 @@ type BranchConfig struct {
6464
RemoteName string
6565
RemoteURL *url.URL
6666
// MergeBase is the optional base branch to target in a new PR if `--base` is not specified.
67-
MergeBase string
68-
MergeRef string
69-
PushDefaultName string
70-
PushRemoteURL *url.URL
71-
PushRemoteName string
72-
Push string
67+
MergeBase string
68+
MergeRef string
69+
RemotePushDefault string
70+
PushRemoteURL *url.URL
71+
PushRemoteName string
72+
Push string
7373
}

‎pkg/cmd/pr/status/status.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,9 @@ func prSelectorForCurrentBranch(branchConfig git.BranchConfig, baseRepo ghrepo.I
226226
if branchOwner != "" {
227227
selector := prHeadRef
228228
if branchConfig.Push != "" {
229+
// If we have a resolved push revision ref, we defer to that
229230
selector = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/")
230-
} else if (branchConfig.PushDefaultName == "upstream" || branchConfig.PushDefaultName == "tracking") &&
231+
} else if (branchConfig.RemotePushDefault == "upstream" || branchConfig.RemotePushDefault == "tracking") &&
231232
strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") {
232233
selector = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/")
233234
}

‎pkg/cmd/pr/status/status_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -666,8 +666,8 @@ func Test_prSelectorForCurrentBranch(t *testing.T) {
666666
Host: "github.com",
667667
Path: "Frederick888/playground.git",
668668
},
669-
MergeRef: "refs/heads/main",
670-
PushDefaultName: "upstream",
669+
MergeRef: "refs/heads/main",
670+
RemotePushDefault: "upstream",
671671
},
672672
baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"),
673673
prHeadRef: "Frederick888/main",
@@ -690,8 +690,8 @@ func Test_prSelectorForCurrentBranch(t *testing.T) {
690690
Host: "github.com",
691691
Path: "Frederick888/playground.git",
692692
},
693-
MergeRef: "refs/heads/main",
694-
PushDefaultName: "tracking",
693+
MergeRef: "refs/heads/main",
694+
RemotePushDefault: "tracking",
695695
},
696696
baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"),
697697
prHeadRef: "Frederick888/main",

0 commit comments

Comments
 (0)