From f43da8ea9a4c4c3caccbaa082232b36e26fad380 Mon Sep 17 00:00:00 2001
From: Tyler McGoffin <jtmcg@github.com>
Date: Tue, 7 Jan 2025 12:19:13 -0800
Subject: [PATCH] Refactor ReadBranchConfig for test coverage of newly returned
 erros

---
 git/client.go      | 22 +++++++++--
 git/client_test.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 4 deletions(-)

diff --git a/git/client.go b/git/client.go
index 501d1ba9d..b356e9ef5 100644
--- a/git/client.go
+++ b/git/client.go
@@ -383,19 +383,33 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte,
 func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchConfig, error) {
 	var cfg BranchConfig
 
+	branchConfigOutput, err := c.readGitBranchConfig(ctx, branch)
+	if err != nil {
+		return cfg, err
+	}
+
+	return c.createBranchConfig(outputLines(branchConfigOutput)), nil
+}
+
+func (c *Client) readGitBranchConfig(ctx context.Context, branch string) ([]byte, error) {
 	prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch))
 	args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|%s)$", prefix, MergeBaseConfig)}
 	cmd, err := c.Command(ctx, args...)
 	if err != nil {
-		return cfg, err
+		return []byte{}, err
 	}
 
 	out, err := cmd.Output()
 	if err != nil {
-		return cfg, err
+		return []byte{}, err
 	}
 
-	for _, line := range outputLines(out) {
+	return out, nil
+}
+
+func (c *Client) createBranchConfig(gitBranchConfigOutput []string) BranchConfig {
+	var cfg BranchConfig
+	for _, line := range gitBranchConfigOutput {
 		parts := strings.SplitN(line, " ", 2)
 		if len(parts) < 2 {
 			continue
@@ -418,7 +432,7 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon
 			cfg.MergeBase = parts[1]
 		}
 	}
-	return cfg, nil
+	return cfg
 }
 
 // SetBranchConfig sets the named value on the given branch.
diff --git a/git/client_test.go b/git/client_test.go
index 182ce67ea..448908591 100644
--- a/git/client_test.go
+++ b/git/client_test.go
@@ -754,6 +754,100 @@ func TestClientReadBranchConfig(t *testing.T) {
 	}
 }
 
+func Test_readGitBranchConfig(t *testing.T) {
+	tests := []struct {
+		name        string
+		branch      string
+		cmdExitCode int
+		cmdStdout   string
+		cmdStderr   string
+		wantCmdArgs string
+		wantErr     *GitError
+	}{
+		{
+			name:        "read branch config",
+			branch:      "trunk",
+			cmdStdout:   "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk",
+			wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`,
+		},
+		{
+			name:        "command failure",
+			branch:      "trunk",
+			wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`,
+			cmdExitCode: 1,
+			cmdStderr:   "error",
+			wantErr:     &GitError{ExitCode: 1, Stderr: "error"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cmd, cmdCtx := createCommandContext(t, tt.cmdExitCode, tt.cmdStdout, tt.cmdStderr)
+			client := Client{
+				GitPath:        "path/to/git",
+				commandContext: cmdCtx,
+			}
+			out, err := client.readGitBranchConfig(context.Background(), tt.branch)
+			if tt.wantErr != nil {
+				assert.Equal(t, tt.wantErr.ExitCode, err.(*GitError).ExitCode)
+				assert.Equal(t, tt.wantErr.Stderr, err.(*GitError).Stderr)
+			} else {
+				assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " "))
+				assert.Equal(t, tt.cmdStdout, string(out))
+			}
+		})
+	}
+}
+
+func Test_createBranchConfig(t *testing.T) {
+	tests := []struct {
+		name                  string
+		gitBranchConfigOutput []string
+		wantBranchConfig      BranchConfig
+	}{
+		{
+			name:                  "remote branch",
+			gitBranchConfigOutput: []string{"branch.trunk.remote origin"},
+			wantBranchConfig: BranchConfig{
+				RemoteName: "origin",
+			},
+		},
+		{
+			name:                  "merge ref",
+			gitBranchConfigOutput: []string{"branch.trunk.merge refs/heads/trunk"},
+			wantBranchConfig: BranchConfig{
+				MergeRef: "refs/heads/trunk",
+			},
+		},
+		{
+			name:                  "merge base",
+			gitBranchConfigOutput: []string{"branch.trunk.gh-merge-base gh-merge-base"},
+			wantBranchConfig: BranchConfig{
+				MergeBase: "gh-merge-base",
+			},
+		},
+		{
+			name: "remote, merge ref, and merge base all specified",
+			gitBranchConfigOutput: []string{
+				"branch.trunk.remote origin",
+				"branch.trunk.merge refs/heads/trunk",
+				"branch.trunk.gh-merge-base gh-merge-base",
+			},
+			wantBranchConfig: BranchConfig{
+				RemoteName: "origin",
+				MergeRef:   "refs/heads/trunk",
+				MergeBase:  "gh-merge-base",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client := Client{}
+			branchConfig := client.createBranchConfig(tt.gitBranchConfigOutput)
+			assert.Equal(t, tt.wantBranchConfig, branchConfig)
+		})
+	}
+}
+
 func TestClientDeleteLocalTag(t *testing.T) {
 	tests := []struct {
 		name          string