From e31bfd0092e5482247c6a6a90ca1d8c4c6c7a966 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 27 Jan 2025 16:40:47 -0800 Subject: [PATCH] Cleaned up some naming and comments --- git/client.go | 20 +++++++------------- git/client_test.go | 20 +++++++++++++------- git/objects.go | 13 +++++++------ pkg/cmd/pr/shared/finder.go | 6 +++--- pkg/cmd/pr/shared/finder_test.go | 10 +++++++--- 5 files changed, 37 insertions(+), 32 deletions(-) diff --git a/git/client.go b/git/client.go index be747e962..30cb4306d 100644 --- a/git/client.go +++ b/git/client.go @@ -389,13 +389,11 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, err } - // This is the error we expect if a git command does not run successfully. - // If the ExitCode is 1, then we just didn't find any config for the branch. - // We will use this error to check against the commands that are allowed to - // return an empty result. - var gitError *GitError branchCfgOut, err := cmd.Output() if err != nil { + // This is the error we expect if the git command does not run successfully. + // If the ExitCode is 1, then we just didn't find any config for the branch. + var gitError *GitError if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { return BranchConfig{}, err } @@ -417,14 +415,9 @@ func parseBranchConfig(branchConfigLines []string) BranchConfig { keys := strings.Split(parts[0], ".") switch keys[len(keys)-1] { case "remote": - remoteURL, remoteName := parseRemoteURLOrName(parts[1]) - cfg.RemoteURL = remoteURL - cfg.RemoteName = remoteName - // pushremote usually indicates a "triangular" workflow + cfg.RemoteURL, cfg.RemoteName = parseRemoteURLOrName(parts[1]) case "pushremote": - pushRemoteURL, pushRemoteName := parseRemoteURLOrName(parts[1]) - cfg.PushRemoteURL = pushRemoteURL - cfg.PushRemoteName = pushRemoteName + cfg.PushRemoteURL, cfg.PushRemoteName = parseRemoteURLOrName(parts[1]) case "merge": cfg.MergeRef = parts[1] case MergeBaseConfig: @@ -449,7 +442,8 @@ func (c *Client) SetBranchConfig(ctx context.Context, branch, name, value string } // PushDefault returns the value of push.default in the config. If the value -// is not set, it returns "simple" (the default value). +// is not set, it returns "simple" (the default git value). See +// https://git-scm.com/docs/git-config#Documentation/git-config.txt-pushdefault func (c *Client) PushDefault(ctx context.Context) (string, error) { pushDefault, err := c.Config(ctx, "push.default") if err == nil { diff --git a/git/client_test.go b/git/client_test.go index 99f783881..872c8595b 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -14,6 +14,7 @@ import ( "strings" "testing" + "github.com/MakeNowJust/heredoc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -763,7 +764,12 @@ func TestClientReadBranchConfig(t *testing.T) { 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 upstream\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.pushremote origin\nbranch.trunk.gh-merge-base gh-merge-base\n", + Stdout: heredoc.Doc(` + branch.trunk.remote upstream + branch.trunk.merge refs/heads/trunk + branch.trunk.pushremote origin + branch.trunk.gh-merge-base gh-merge-base + `), }, }, branch: "trunk", @@ -1869,16 +1875,16 @@ func TestCommandMocking(t *testing.T) { jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS") if !ok { fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS") - // Exit 1 is reserved for empty command responses by git. This can be desirable in some cases, - // so returning an arbitrary exit code to avoid suppressing this if an exit code 1 is allowed. + // Exit 1 is used for empty key values in the git config. This is non-breaking in those use cases, + // so this is returning a non-zero exit code to avoid suppressing this error for those use cases. os.Exit(16) } var commands mockedCommands if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil { fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS") - // Exit 1 is reserved for empty command responses by git. This can be desirable in some cases, - // so returning an arbitrary exit code to avoid suppressing this if an exit code 1 is allowed. + // Exit 1 is used for empty key values in the git config. This is non-breaking in those use cases, + // so this is returning a non-zero exit code to avoid suppressing this error for those use cases. os.Exit(16) } @@ -1888,8 +1894,8 @@ func TestCommandMocking(t *testing.T) { commandResult, ok := commands[args(strings.Join(realArgs, " "))] if !ok { fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " ")) - // Exit 1 is reserved for empty command responses by git. This can be desirable in some cases, - // so returning an arbitrary exit code to avoid suppressing this if an exit code 1 is allowed. + // Exit 1 is used for empty key values in the git config. This is non-breaking in those use cases, + // so this is returning a non-zero exit code to avoid suppressing this error for those use cases. os.Exit(16) } diff --git a/git/objects.go b/git/objects.go index b57d79bec..9db528b8c 100644 --- a/git/objects.go +++ b/git/objects.go @@ -60,13 +60,14 @@ type Commit struct { Body string } +// These are the keys we read from the git branch. config. type BranchConfig struct { - RemoteName string - RemoteURL *url.URL + RemoteName string // .remote if string + RemoteURL *url.URL // .remote if url + MergeRef string // .merge + PushRemoteName string // .pushremote if string + PushRemoteURL *url.URL // .pushremote if url + // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. MergeBase string - MergeRef string - // These are usually used to handle triangular workflows. - PushRemoteURL *url.URL - PushRemoteName string } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 205939931..05ec24c9a 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -109,10 +109,10 @@ type PRRefs struct { BaseRepo ghrepo.Interface } -// GetPRLabel returns the string that the GitHub API uses to identify the PR. This is +// GetPRHeadLabel returns the string that the GitHub API uses to identify the PR. This is // either just the branch name or, if the PR is originating from a fork, the fork owner // and the branch name, like :. -func (s *PRRefs) GetPRLabel() string { +func (s *PRRefs) GetPRHeadLabel() string { if ghrepo.IsSame(s.HeadRepo, s.BaseRepo) { return s.BranchName } @@ -240,7 +240,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - pr, err = findForBranch(httpClient, f.baseRefRepo, opts.BaseBranch, prRefs.GetPRLabel(), opts.States, fields.ToSlice()) + pr, err = findForBranch(httpClient, f.baseRefRepo, opts.BaseBranch, prRefs.GetPRHeadLabel(), opts.States, fields.ToSlice()) if err != nil { return pr, f.baseRefRepo, err } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 1d5c1e3ac..a54412db5 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -419,7 +419,11 @@ func TestFind(t *testing.T) { wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { - name: "the current branch is configured to push to and pull from a URL (upstream, in this example) that is different from what the repo is configured to push to and pull from (origin, in this example) and push.default = upstream, it finds the PR associated with the upstream repo and returns origin as the base repo", + // The current BRANCH is configured to push to and pull from a URL (upstream, in this example) + // which is different from what the REPO is configured to push to and pull from (origin, in this example) + // and push.default = upstream. It should find the PR associated with the upstream repo and return + // origin as the base repo + name: "when push.default = upstream and the current branch is configured to push/pull from a different remote than the repo", args: args{ selector: "", fields: []string{"id", "number"}, @@ -902,7 +906,7 @@ func TestParsePRRefs(t *testing.T) { } } -func TestPRRefs_GetPRLabel(t *testing.T) { +func TestPRRefs_GetPRHeadLabel(t *testing.T) { originRepo := ghrepo.New("ORIGINOWNER", "REPO") upstreamRepo := ghrepo.New("UPSTREAMOWNER", "REPO") tests := []struct { @@ -931,7 +935,7 @@ func TestPRRefs_GetPRLabel(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, tt.prRefs.GetPRLabel()) + assert.Equal(t, tt.want, tt.prRefs.GetPRHeadLabel()) }) } }