From cdead50d572c4b9f830873f18724ac07afe4d3e4 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 11:05:15 -0800 Subject: [PATCH] Moved remote.pushDefault out of ReadBranchConfig and into finder --- git/client.go | 46 ++--- git/client_test.go | 308 +++++++++---------------------- git/objects.go | 5 +- pkg/cmd/pr/shared/finder.go | 46 +++-- pkg/cmd/pr/shared/finder_test.go | 95 +++++++++- 5 files changed, 223 insertions(+), 277 deletions(-) diff --git a/git/client.go b/git/client.go index c188b27bb..be747e962 100644 --- a/git/client.go +++ b/git/client.go @@ -402,16 +402,10 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, nil } - // 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 - } - - return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n")), nil + return parseBranchConfig(outputLines(branchCfgOut)), nil } -func parseBranchConfig(branchConfigLines []string, remotePushDefault string) BranchConfig { +func parseBranchConfig(branchConfigLines []string) BranchConfig { var cfg BranchConfig // Read the config lines for the specific branch @@ -438,26 +432,6 @@ func parseBranchConfig(branchConfigLines []string, remotePushDefault string) Bra } } - // 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 == "" { - // 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 - cfg.PushRemoteURL = cfg.RemoteURL - } - } - - // Some `gh pr` workflows don't work if this is set to 'simple' or 'current' - cfg.RemotePushDefault = remotePushDefault - return cfg } @@ -489,6 +463,22 @@ func (c *Client) PushDefault(ctx context.Context) (string, error) { return "", err } +// RemotePushDefault returns the value of remote.pushDefault in the config. If +// the value is not set, it returns an empty string. +func (c *Client) RemotePushDefault(ctx context.Context) (string, error) { + remotePushDefault, err := c.Config(ctx, "remote.pushDefault") + if err == nil { + return remotePushDefault, nil + } + + var gitError *GitError + if ok := errors.As(err, &gitError); ok && gitError.ExitCode == 1 { + return "", nil + } + + 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 diff --git a/git/client_test.go b/git/client_test.go index 5d8ee9d65..99f783881 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -760,199 +760,20 @@ func TestClientReadBranchConfig(t *testing.T) { }, }, { - name: "when the git reads the config, pushDefault isn't set, it should return the correct BranchConfig", + name: "when the config is read, 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", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "origin", - }, - wantError: nil, - }, - { - 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", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "remotePushDefault", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "remotePushDefault", - RemotePushDefault: "remotePushDefault", - }, - wantError: nil, - }, - { - name: "when git reads the config, pushDefault is set, an the branch hasn't been pushed, 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", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "remotePushDefault", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "remotePushDefault", - RemotePushDefault: "remotePushDefault", - }, - wantError: nil, - }, - { - 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: "remotePushDefault error", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{}, - wantError: &GitError{ - ExitCode: 2, - Stderr: "remotePushDefault error", - }, - }, - { - name: "read branch config, central", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base merge-base", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/trunk", - MergeBase: "merge-base", - PushRemoteName: "origin", - }, - wantError: nil, - }, - { - name: "read branch config, central, push.default = upstream", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk-remote", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/trunk-remote", - PushRemoteName: "origin", - }, - }, - { - name: "read branch config, central, push.default = upstream, no existing remote branch", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/main", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - }, - }, - { - name: "read branch config, triangular, push.default = current, has existing remote branch, branch.trunk.pushremote effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, + Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.pushremote origin\nbranch.trunk.gh-merge-base gh-merge-base\n", }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "upstream", - MergeRef: "refs/heads/main", PushRemoteName: "origin", + MergeRef: "refs/heads/trunk", + MergeBase: "gh-merge-base", }, - }, - { - name: "read branch config, triangular, push.default = current, has existing remote branch, remote.pushDefault effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "origin", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - RemotePushDefault: "origin", - }, - }, - { - name: "read branch config, triangular, push.default = current, no existing remote branch, branch.trunk.pushremote effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "current", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - RemotePushDefault: "current", - }, - }, - { - name: "read branch config, triangular, push.default = current, no existing remote branch, remote.pushDefault effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "origin", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - RemotePushDefault: "origin", - }, + wantError: nil, }, } for _, tt := range tests { @@ -980,15 +801,13 @@ func Test_parseBranchConfig(t *testing.T) { tests := []struct { name string configLines []string - pushDefault string wantBranchConfig BranchConfig }{ { name: "remote branch", configLines: []string{"branch.trunk.remote origin"}, wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "origin", + RemoteName: "origin", }, }, { @@ -1006,92 +825,73 @@ func Test_parseBranchConfig(t *testing.T) { }, }, { - name: "push remote", + name: "pushremote", configLines: []string{"branch.trunk.pushremote pushremote"}, wantBranchConfig: BranchConfig{ PushRemoteName: "pushremote", }, }, - { - name: "push default specified", - configLines: []string{}, - pushDefault: "remotePushDefault", - wantBranchConfig: BranchConfig{ - PushRemoteName: "remotePushDefault", - RemotePushDefault: "remotePushDefault", - }, - }, { name: "remote and pushremote are specified by name", configLines: []string{ - "branch.trunk.remote origin", - "branch.trunk.pushremote pushremote", + "branch.trunk.remote upstream", + "branch.trunk.pushremote origin", }, wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "pushremote", + RemoteName: "upstream", + PushRemoteName: "origin", }, }, { name: "remote and pushremote are specified by url", configLines: []string{ - "branch.Frederick888/main.remote git@github.com:Frederick888/remote.git", - "branch.Frederick888/main.pushremote git@github.com:Frederick888/pushremote.git", + "branch.trunk.remote git@github.com:UPSTREAMOWNER/REPO.git", + "branch.trunk.pushremote git@github.com:ORIGINOWNER/REPO.git", }, wantBranchConfig: BranchConfig{ RemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", - Path: "/Frederick888/remote.git", + Path: "/UPSTREAMOWNER/REPO.git", }, PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", - Path: "/Frederick888/pushremote.git", + Path: "/ORIGINOWNER/REPO.git", }, }, }, { - name: "remote, pushremote, gh-merge-base, merge ref, push default, and rev parse all specified", + name: "remote, pushremote, gh-merge-base, and merge ref all specified", configLines: []string{ "branch.trunk.remote remote", "branch.trunk.pushremote pushremote", "branch.trunk.gh-merge-base gh-merge-base", "branch.trunk.merge refs/heads/trunk", }, - pushDefault: "remotePushDefault", - wantBranchConfig: BranchConfig{ - RemoteName: "remote", - PushRemoteName: "pushremote", - MergeBase: "gh-merge-base", - MergeRef: "refs/heads/trunk", - RemotePushDefault: "remotePushDefault", - }, - }, - { - name: "pushremote and pushDefault are not specified, but a remoteName is provided", - configLines: []string{ - "branch.trunk.remote remote", - }, wantBranchConfig: BranchConfig{ RemoteName: "remote", - PushRemoteName: "remote", + PushRemoteName: "pushremote", + MergeBase: "gh-merge-base", + MergeRef: "refs/heads/trunk", }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - branchConfig := parseBranchConfig(tt.configLines, tt.pushDefault) + branchConfig := parseBranchConfig(tt.configLines) 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.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault") if tt.wantBranchConfig.RemoteURL != nil { assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL") } + if tt.wantBranchConfig.PushRemoteURL != nil { + assert.Equalf(t, tt.wantBranchConfig.PushRemoteURL.String(), branchConfig.PushRemoteURL.String(), "unexpected PushRemoteURL") + } }) } } @@ -1204,6 +1004,68 @@ func TestClientPushDefault(t *testing.T) { } } +func TestClientRemotePushDefault(t *testing.T) { + tests := []struct { + name string + commandResult commandResult + wantRemotePushDefault string + wantError *GitError + }{ + { + name: "remote.pushDefault is not set", + commandResult: commandResult{ + ExitStatus: 1, + Stderr: "error: key does not contain a section: remote.pushDefault", + }, + wantRemotePushDefault: "", + wantError: nil, + }, + { + name: "remote.pushDefault is set to origin", + commandResult: commandResult{ + ExitStatus: 0, + Stdout: "origin", + }, + wantRemotePushDefault: "origin", + wantError: nil, + }, + { + name: "remote.pushDefault errors", + commandResult: commandResult{ + ExitStatus: 128, + Stderr: "fatal: git error", + }, + wantRemotePushDefault: "", + wantError: &GitError{ + ExitCode: 128, + Stderr: "fatal: git error", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmdCtx := createMockedCommandContext(t, mockedCommands{ + `path/to/git config remote.pushDefault`: tt.commandResult, + }, + ) + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + pushDefault, err := client.RemotePushDefault(context.Background()) + 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.wantRemotePushDefault, pushDefault) + }) + } +} + func TestClientParsePushRevision(t *testing.T) { tests := []struct { name string diff --git a/git/objects.go b/git/objects.go index eab7583ae..d66d5b9bb 100644 --- a/git/objects.go +++ b/git/objects.go @@ -68,7 +68,6 @@ type BranchConfig struct { MergeRef string // These are used to handle triangular workflows. They can be defined by either // a remote.pushDefault or a branch..pushremote value set on the git config. - PushRemoteURL *url.URL - PushRemoteName string - RemotePushDefault string + PushRemoteURL *url.URL + PushRemoteName string } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index d76a62140..8589840a9 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -38,6 +38,7 @@ type finder struct { remotesFn func() (remotes.Remotes, error) httpClient func() (*http.Client, error) pushDefault func() (string, error) + remotePushDefault func() (string, error) parsePushRevision func(string) (string, error) branchConfig func(string) (git.BranchConfig, error) progress progressIndicator @@ -62,6 +63,9 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { pushDefault: func() (string, error) { return factory.GitClient.PushDefault(context.Background()) }, + remotePushDefault: func() (string, error) { + return factory.GitClient.RemotePushDefault(context.Background()) + }, parsePushRevision: func(branch string) (string, error) { return factory.GitClient.ParsePushRevision(context.Background(), branch) }, @@ -217,10 +221,15 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - // Suppressing the error as we have other means of computing the PRRefs if this fails. + // Suppressing these errors as we have other means of computing the PRRefs when these fail. parsedPushRevision, _ := f.parsePushRevision(f.branchName) - prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, f.baseRefRepo, rems) + remotePushDefault, err := f.remotePushDefault() + if err != nil { + return nil, nil, err + } + + prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, f.baseRefRepo, rems) if err != nil { return nil, nil, err } @@ -287,7 +296,7 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { return repo, prNumber, nil } -func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { +func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, remotePushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { prRefs := PRRefs{ BaseRepo: baseRefRepo, } @@ -311,8 +320,15 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsed 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 - // one of the following, in order of precedence: + // We assume the PR's branch name is the same as whatever f.BranchFn() returned earlier + // unless the user has specified push.default = upstream or tracking, then we use the + // branch name from the merge ref. + prRefs.BranchName = currentBranchName + if pushDefault == "upstream" || pushDefault == "tracking" { + prRefs.BranchName = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } + + // To get the HeadRepo, we look to the git config. The HeadRepo comes from one of the following, in order of precedence: // 1. branch..pushRemote // 2. remote.pushDefault // 3. branch..remote @@ -324,14 +340,18 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsed if r, err := ghrepo.FromURL(branchConfig.PushRemoteURL); err == nil { prRefs.HeadRepo = r } - } - - // We assume the PR's branch name is the same as whatever f.BranchFn() returned earlier. - // unless the user has specified push.default = upstream or tracking, then we use the - // branch name from the merge ref. - prRefs.BranchName = currentBranchName - if pushDefault == "upstream" || pushDefault == "tracking" { - prRefs.BranchName = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } else if remotePushDefault != "" { + if r, err := rems.FindByName(remotePushDefault); err == nil { + prRefs.HeadRepo = r.Repo + } + } else if branchConfig.RemoteName != "" { + if r, err := rems.FindByName(branchConfig.RemoteName); err == nil { + prRefs.HeadRepo = r.Repo + } + } else if branchConfig.RemoteURL != nil { + if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { + prRefs.HeadRepo = r + } } // The PR merges from a branch in the same repo as the base branch (usually the default branch) diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index db56ce775..89835b028 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -20,6 +20,7 @@ type args struct { branchFn func() (string, error) branchConfig func(string) (git.BranchConfig, error) pushDefault func() (string, error) + remotePushDefault func() (string, error) parsePushRevision func(string) (string, error) selector string fields []string @@ -102,6 +103,7 @@ func TestFind(t *testing.T) { PushRemoteName: remoteOrigin.Remote.Name, }, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -132,8 +134,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), + remotePushDefault: stubRemotePushDefault("", nil), }, wantErr: true, }, @@ -154,8 +157,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), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: nil, wantPR: 13, @@ -170,8 +174,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), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -192,8 +197,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), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -217,6 +223,7 @@ func TestFind(t *testing.T) { branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), parsePushRevision: stubParsedPushRevision("", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -259,6 +266,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -301,6 +309,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -335,6 +344,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -386,6 +396,7 @@ func TestFind(t *testing.T) { PushRemoteName: "upstream", }, nil), pushDefault: stubPushDefault("upstream", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -421,6 +432,7 @@ func TestFind(t *testing.T) { PushRemoteURL: remoteUpstream.Remote.FetchURL, }, nil), pushDefault: stubPushDefault("upstream", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -453,6 +465,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("other/blueberries", nil), }, httpStub: func(r *httpmock.Registry) { @@ -513,7 +526,8 @@ func TestFind(t *testing.T) { branchConfig: stubBranchConfig(git.BranchConfig{ MergeRef: "refs/pull/13/head", }, nil), - pushDefault: stubPushDefault("simple", nil), + pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -578,6 +592,7 @@ func TestFind(t *testing.T) { branchFn: tt.args.branchFn, branchConfig: tt.args.branchConfig, pushDefault: tt.args.pushDefault, + remotePushDefault: tt.args.remotePushDefault, parsePushRevision: tt.args.parsePushRevision, remotesFn: stubRemotes(context.Remotes{ &remoteOrigin, @@ -653,6 +668,7 @@ func Test_parsePRRefs(t *testing.T) { branchConfig git.BranchConfig pushDefault string parsedPushRevision string + remotePushDefault string currentBranchName string baseRefRepo ghrepo.Interface rems context.Remotes @@ -819,10 +835,63 @@ func Test_parsePRRefs(t *testing.T) { }, wantErr: nil, }, + { + name: "When remote.pushDefault is set, it returns the correct PRRefs", + branchConfig: git.BranchConfig{}, + remotePushDefault: remoteUpstream.Remote.Name, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the remote name is set on the branch, it returns the correct PRRefs", + branchConfig: git.BranchConfig{ + RemoteName: remoteUpstream.Remote.Name, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the remote URL is set on the branch, it returns the correct PRRefs", + branchConfig: git.BranchConfig{ + RemoteURL: remoteUpstream.Remote.FetchURL, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.baseRefRepo, tt.rems) + prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.remotePushDefault, tt.baseRefRepo, tt.rems) if tt.wantErr != nil { require.Equal(t, tt.wantErr, err) } else { @@ -891,6 +960,12 @@ func stubPushDefault(pushDefault string, err error) func() (string, error) { } } +func stubRemotePushDefault(remotePushDefault string, err error) func() (string, error) { + return func() (string, error) { + return remotePushDefault, err + } +} + func stubParsedPushRevision(parsedPushRevision string, err error) func(string) (string, error) { return func(_ string) (string, error) { return parsedPushRevision, err