Remove @{push} from branch config

This commit is contained in:
Tyler McGoffin 2025-01-24 10:20:04 -08:00
parent 5a8dd35ba7
commit e4d8ed0e60
5 changed files with 151 additions and 130 deletions

View file

@ -408,15 +408,10 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon
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(remotePushDefaultOut, "\n"), firstLine(revParseOut)), nil
return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n")), nil
}
func parseBranchConfig(branchConfigLines []string, remotePushDefault string, revParse string) BranchConfig {
func parseBranchConfig(branchConfigLines []string, remotePushDefault string) BranchConfig {
var cfg BranchConfig
// Read the config lines for the specific branch
@ -460,9 +455,6 @@ func parseBranchConfig(branchConfigLines []string, remotePushDefault string, rev
}
}
// The value returned by revParse is the easiest way to get the name of the push ref
cfg.Push = revParse
// Some `gh pr` workflows don't work if this is set to 'simple' or 'current'
cfg.RemotePushDefault = remotePushDefault
@ -497,6 +489,15 @@ func (c *Client) PushDefault(ctx context.Context) (string, error) {
return "", err
}
// ParsePushRevision gets the value of the @{push} revision syntax
// An error here doesn't necessarily mean something are broke, but may mean that the @{push}
// revision syntax couldn't be resolved, such as in non-centralized workflows with
// push.default = simple. Downstream consumers should consider how to handle this error.
func (c *Client) ParsePushRevision(ctx context.Context, branch string) (string, error) {
revParseOut, err := c.revParse(ctx, "--abbrev-ref", branch+"@{push}")
return firstLine(revParseOut), err
}
func (c *Client) DeleteLocalTag(ctx context.Context, tag string) error {
args := []string{"tag", "-d", tag}
cmd, err := c.Command(ctx, args...)

View file

@ -760,7 +760,7 @@ func TestClientReadBranchConfig(t *testing.T) {
},
},
{
name: "when the git reads the config, pushDefault isn't set, and rev-parse succeeds, it should return the correct BranchConfig",
name: "when the git reads the config, pushDefault isn't set, it should return the correct BranchConfig",
cmds: mockedCommands{
`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: {
Stdout: "branch.trunk.remote origin\n",
@ -768,20 +768,16 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
ExitStatus: 1,
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
RemoteName: "origin",
PushRemoteName: "origin",
Push: "origin/trunk",
},
wantError: nil,
},
{
name: "when the git reads the config, pushDefault is set, and rev-parse succeeds, it should return the correct BranchConfig",
name: "when the git reads the config, pushDefault is set, it should return the correct BranchConfig",
cmds: mockedCommands{
`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: {
Stdout: "branch.trunk.remote origin\n",
@ -789,15 +785,11 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
Stdout: "remotePushDefault",
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
RemoteName: "origin",
PushRemoteName: "remotePushDefault",
Push: "origin/trunk",
RemotePushDefault: "remotePushDefault",
},
wantError: nil,
@ -811,9 +803,6 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
Stdout: "remotePushDefault",
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
ExitStatus: 1,
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
@ -850,9 +839,6 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
ExitStatus: 1,
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
@ -860,7 +846,6 @@ func TestClientReadBranchConfig(t *testing.T) {
MergeRef: "refs/heads/trunk",
MergeBase: "merge-base",
PushRemoteName: "origin",
Push: "origin/trunk",
},
wantError: nil,
},
@ -873,16 +858,12 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
ExitStatus: 1,
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk-remote",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
RemoteName: "origin",
MergeRef: "refs/heads/trunk-remote",
PushRemoteName: "origin",
Push: "origin/trunk-remote",
},
},
{
@ -894,16 +875,12 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
ExitStatus: 1,
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
RemoteName: "origin",
MergeRef: "refs/heads/main",
PushRemoteName: "origin",
Push: "origin/trunk",
},
},
{
@ -915,16 +892,12 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
ExitStatus: 1,
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
RemoteName: "upstream",
MergeRef: "refs/heads/main",
PushRemoteName: "origin",
Push: "origin/trunk",
},
},
{
@ -936,16 +909,12 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
Stdout: "origin",
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
Stdout: "origin/trunk",
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
RemoteName: "upstream",
MergeRef: "refs/heads/main",
PushRemoteName: "origin",
Push: "origin/trunk",
RemotePushDefault: "origin",
},
},
@ -958,9 +927,6 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
Stdout: "current",
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
ExitStatus: 1,
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
@ -979,9 +945,6 @@ func TestClientReadBranchConfig(t *testing.T) {
`path/to/git config remote.pushDefault`: {
Stdout: "origin",
},
`path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: {
ExitStatus: 1,
},
},
branch: "trunk",
wantBranchConfig: BranchConfig{
@ -1018,7 +981,6 @@ func Test_parseBranchConfig(t *testing.T) {
name string
configLines []string
pushDefault string
revParse string
wantBranchConfig BranchConfig
}{
{
@ -1050,14 +1012,6 @@ func Test_parseBranchConfig(t *testing.T) {
PushRemoteName: "pushremote",
},
},
{
name: "rev parse specified",
configLines: []string{},
revParse: "origin/trunk",
wantBranchConfig: BranchConfig{
Push: "origin/trunk",
},
},
{
name: "push default specified",
configLines: []string{},
@ -1108,13 +1062,11 @@ func Test_parseBranchConfig(t *testing.T) {
"branch.trunk.merge refs/heads/trunk",
},
pushDefault: "remotePushDefault",
revParse: "origin/trunk",
wantBranchConfig: BranchConfig{
RemoteName: "remote",
PushRemoteName: "pushremote",
MergeBase: "gh-merge-base",
MergeRef: "refs/heads/trunk",
Push: "origin/trunk",
RemotePushDefault: "remotePushDefault",
},
},
@ -1131,12 +1083,11 @@ func Test_parseBranchConfig(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
branchConfig := parseBranchConfig(tt.configLines, tt.pushDefault, tt.revParse)
branchConfig := parseBranchConfig(tt.configLines, tt.pushDefault)
assert.Equalf(t, tt.wantBranchConfig.RemoteName, branchConfig.RemoteName, "unexpected RemoteName")
assert.Equalf(t, tt.wantBranchConfig.MergeRef, branchConfig.MergeRef, "unexpected MergeRef")
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.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault")
if tt.wantBranchConfig.RemoteURL != nil {
assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL")
@ -1253,6 +1204,60 @@ func TestClientPushDefault(t *testing.T) {
}
}
func TestClientParsePushRevision(t *testing.T) {
tests := []struct {
name string
branch string
commandResult commandResult
wantParsedPushRevision string
wantError *GitError
}{
{
name: "@{push} resolves to origin/branchName",
branch: "branchName",
commandResult: commandResult{
ExitStatus: 0,
Stdout: "origin/branchName",
},
wantParsedPushRevision: "origin/branchName",
},
{
name: "@{push} doesn't resolve",
commandResult: commandResult{
ExitStatus: 128,
Stderr: "fatal: git error",
},
wantParsedPushRevision: "",
wantError: &GitError{
ExitCode: 128,
Stderr: "fatal: git error",
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cmd := fmt.Sprintf("path/to/git rev-parse --abbrev-ref %s@{push}", tt.branch)
cmdCtx := createMockedCommandContext(t, mockedCommands{
args(cmd): tt.commandResult,
})
client := Client{
GitPath: "path/to/git",
commandContext: cmdCtx,
}
pushDefault, err := client.ParsePushRevision(context.Background(), tt.branch)
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 {
require.NoError(t, err)
}
assert.Equal(t, tt.wantParsedPushRevision, pushDefault)
})
}
}
func TestClientDeleteLocalTag(t *testing.T) {
tests := []struct {
name string

View file

@ -68,8 +68,7 @@ type BranchConfig struct {
MergeRef string
// These are used to handle triangular workflows. They can be defined by either
// a remote.pushDefault or a branch.<name>.pushremote value set on the git config.
RemotePushDefault string
PushRemoteURL *url.URL
PushRemoteName string
Push string
RemotePushDefault string
}

View file

@ -33,13 +33,14 @@ type progressIndicator interface {
}
type finder struct {
baseRepoFn func() (ghrepo.Interface, error)
branchFn func() (string, error)
remotesFn func() (remotes.Remotes, error)
httpClient func() (*http.Client, error)
pushDefault func() (string, error)
branchConfig func(string) (git.BranchConfig, error)
progress progressIndicator
baseRepoFn func() (ghrepo.Interface, error)
branchFn func() (string, error)
remotesFn func() (remotes.Remotes, error)
httpClient func() (*http.Client, error)
pushDefault func() (string, error)
parsePushRevision func(string) (string, error)
branchConfig func(string) (git.BranchConfig, error)
progress progressIndicator
baseRefRepo ghrepo.Interface
prNumber int
@ -61,6 +62,9 @@ func NewFinder(factory *cmdutil.Factory) PRFinder {
pushDefault: func() (string, error) {
return factory.GitClient.PushDefault(context.Background())
},
parsePushRevision: func(branch string) (string, error) {
return factory.GitClient.ParsePushRevision(context.Background(), branch)
},
progress: factory.IOStreams,
branchConfig: func(s string) (git.BranchConfig, error) {
return factory.GitClient.ReadBranchConfig(context.Background(), s)
@ -213,7 +217,10 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
return nil, nil, err
}
prRefs, err := parsePRRefs(f.branchName, branchConfig, pushDefault, f.baseRefRepo, rems)
// Suppressing the error as we have other means of computing the PRRefs if this fails.
parsedPushRevision, _ := f.parsePushRevision(f.branchName)
prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, f.baseRefRepo, rems)
if err != nil {
return nil, nil, err
}
@ -280,18 +287,18 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) {
return repo, prNumber, nil
}
func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) {
func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) {
prRefs := PRRefs{
BaseRepo: baseRefRepo,
}
// If @{push} resolves, then we have all the information we need to determine the head repo
// and branch name. It is of the form <remote>/<branch>.
if branchConfig.Push != "" {
if parsedPushRevision != "" {
for _, r := range rems {
// Find the remote who's name matches the push <remote> prefix
if strings.HasPrefix(branchConfig.Push, r.Name+"/") {
prRefs.BranchName = strings.TrimPrefix(branchConfig.Push, r.Name+"/")
if strings.HasPrefix(parsedPushRevision, r.Name+"/") {
prRefs.BranchName = strings.TrimPrefix(parsedPushRevision, r.Name+"/")
prRefs.HeadRepo = r.Repo
return prRefs, nil
}
@ -301,7 +308,7 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, pushDe
for i, r := range rems {
remoteNames[i] = r.Name
}
return PRRefs{}, fmt.Errorf("no remote for %q found in %q", branchConfig.Push, strings.Join(remoteNames, ", "))
return PRRefs{}, fmt.Errorf("no remote for %q found in %q", parsedPushRevision, strings.Join(remoteNames, ", "))
}
// To get the HeadRepo, we look to the git config. The PushRemote{Name | URL} comes from

View file

@ -16,13 +16,14 @@ import (
)
type args struct {
baseRepoFn func() (ghrepo.Interface, error)
branchFn func() (string, error)
branchConfig func(string) (git.BranchConfig, error)
pushDefault func() (string, error)
selector string
fields []string
baseBranch string
baseRepoFn func() (ghrepo.Interface, error)
branchFn func() (string, error)
branchConfig func(string) (git.BranchConfig, error)
pushDefault func() (string, error)
parsePushRevision func(string) (string, error)
selector string
fields []string
baseBranch string
}
func TestFind(t *testing.T) {
@ -100,7 +101,8 @@ func TestFind(t *testing.T) {
branchConfig: stubBranchConfig(git.BranchConfig{
PushRemoteName: remoteOrigin.Remote.Name,
}, nil),
pushDefault: stubPushDefault("simple", nil),
pushDefault: stubPushDefault("simple", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -212,8 +214,9 @@ func TestFind(t *testing.T) {
branchFn: func() (string, error) {
return "blueberries", nil
},
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -254,8 +257,9 @@ func TestFind(t *testing.T) {
branchFn: func() (string, error) {
return "blueberries", nil
},
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -295,8 +299,9 @@ func TestFind(t *testing.T) {
branchFn: func() (string, error) {
return "blueberries", nil
},
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -328,8 +333,9 @@ func TestFind(t *testing.T) {
branchFn: func() (string, error) {
return "blueberries", nil
},
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -379,7 +385,8 @@ func TestFind(t *testing.T) {
MergeRef: "refs/heads/blue-upstream-berries",
PushRemoteName: "upstream",
}, nil),
pushDefault: stubPushDefault("upstream", nil),
pushDefault: stubPushDefault("upstream", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -413,7 +420,8 @@ func TestFind(t *testing.T) {
MergeRef: "refs/heads/blue-upstream-berries",
PushRemoteURL: remoteUpstream.Remote.FetchURL,
}, nil),
pushDefault: stubPushDefault("upstream", nil),
pushDefault: stubPushDefault("upstream", nil),
parsePushRevision: stubParsedPushRevision("", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -443,10 +451,9 @@ func TestFind(t *testing.T) {
branchFn: func() (string, error) {
return "blueberries", nil
},
branchConfig: stubBranchConfig(git.BranchConfig{
Push: "other/blueberries",
}, nil),
pushDefault: stubPushDefault("simple", nil),
branchConfig: stubBranchConfig(git.BranchConfig{}, nil),
pushDefault: stubPushDefault("simple", nil),
parsePushRevision: stubParsedPushRevision("other/blueberries", nil),
},
httpStub: func(r *httpmock.Registry) {
r.Register(
@ -567,10 +574,11 @@ func TestFind(t *testing.T) {
httpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
baseRepoFn: tt.args.baseRepoFn,
branchFn: tt.args.branchFn,
branchConfig: tt.args.branchConfig,
pushDefault: tt.args.pushDefault,
baseRepoFn: tt.args.baseRepoFn,
branchFn: tt.args.branchFn,
branchConfig: tt.args.branchConfig,
pushDefault: tt.args.pushDefault,
parsePushRevision: tt.args.parsePushRevision,
remotesFn: stubRemotes(context.Remotes{
&remoteOrigin,
&remoteOther,
@ -641,14 +649,15 @@ func Test_parsePRRefs(t *testing.T) {
}
tests := []struct {
name string
branchConfig git.BranchConfig
pushDefault string
currentBranchName string
baseRefRepo ghrepo.Interface
rems context.Remotes
wantPRRefs PRRefs
wantErr error
name string
branchConfig git.BranchConfig
pushDefault string
parsedPushRevision string
currentBranchName string
baseRefRepo ghrepo.Interface
rems context.Remotes
wantPRRefs PRRefs
wantErr error
}{
{
name: "When the branch is called 'blueberries' with an empty branch config, it returns the correct PRRefs",
@ -675,12 +684,10 @@ func Test_parsePRRefs(t *testing.T) {
wantErr: nil,
},
{
name: "When the branch name doesn't match the branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name",
branchConfig: git.BranchConfig{
Push: "origin/pushBranch",
},
currentBranchName: "blueberries",
baseRefRepo: remoteOrigin.Repo,
name: "When the branch name doesn't match the branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name",
parsedPushRevision: "origin/pushBranch",
currentBranchName: "blueberries",
baseRefRepo: remoteOrigin.Repo,
rems: context.Remotes{
&remoteOrigin,
},
@ -692,12 +699,10 @@ func Test_parsePRRefs(t *testing.T) {
wantErr: nil,
},
{
name: "When the push revision doesn't match a remote, it returns an error",
branchConfig: git.BranchConfig{
Push: "origin/differentPushBranch",
},
currentBranchName: "blueberries",
baseRefRepo: remoteOrigin.Repo,
name: "When the push revision doesn't match a remote, it returns an error",
parsedPushRevision: "origin/differentPushBranch",
currentBranchName: "blueberries",
baseRefRepo: remoteOrigin.Repo,
rems: context.Remotes{
&remoteUpstream,
&remoteOther,
@ -706,12 +711,10 @@ func Test_parsePRRefs(t *testing.T) {
wantErr: fmt.Errorf("no remote for %q found in %q", "origin/differentPushBranch", "upstream, other"),
},
{
name: "When the branch name doesn't match a different branch name in BranchConfig.Push and the remote isn't 'origin', it returns the BranchConfig.Push branch name",
branchConfig: git.BranchConfig{
Push: "other/pushBranch",
},
currentBranchName: "blueberries",
baseRefRepo: remoteOrigin.Repo,
name: "When the branch name doesn't match a different branch name in BranchConfig.Push and the remote isn't 'origin', it returns the BranchConfig.Push branch name",
parsedPushRevision: "other/pushBranch",
currentBranchName: "blueberries",
baseRefRepo: remoteOrigin.Repo,
rems: context.Remotes{
&remoteOther,
},
@ -819,7 +822,7 @@ func Test_parsePRRefs(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.pushDefault, tt.baseRefRepo, tt.rems)
prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.baseRefRepo, tt.rems)
if tt.wantErr != nil {
require.Equal(t, tt.wantErr, err)
} else {
@ -887,3 +890,9 @@ func stubPushDefault(pushDefault string, err error) func() (string, error) {
return pushDefault, err
}
}
func stubParsedPushRevision(parsedPushRevision string, err error) func(string) (string, error) {
return func(_ string) (string, error) {
return parsedPushRevision, err
}
}