Refine error handling of ReadBranchConfig

cmd.Output() will return an error when the git command ran successfully
but had no output. To handle this, we can check Stderr, as we expect it to
be populated for any ExitErrors or otherwise when there is a command
failure.

This allows for propagation of this error handling up the call chain, so
we are now returning errors if the call to git fails instead of just
handing off an empty BranchConfig and suppressing the errors.

Additionally, I've removed some more naked returns that I found in
pkg/cmd/pr/create.go createRun

git/client.go

@@ -391,7 +391,13 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon
 
 	out, err := cmd.Output()
 	if err != nil {
-		return BranchConfig{}, err
+		// This will error if no matches are found but the git command still ran successfully. We only
+		// want to return an error if the command failed to run, usually and ExitError, which will be
+		// indicated by output on Stderr.
+		if err.(*GitError).Stderr != "" {
+			return BranchConfig{}, err
+		}
+		return BranchConfig{}, nil
 	}
 
 	return parseBranchConfig(outputLines(out)), nil

git/client_test.go

@@ -744,7 +744,7 @@ func TestClientReadBranchConfig(t *testing.T) {
 			wantError:        nil,
 		},
 		{
-			name:             "command error",
+			name:             "output error",
 			cmdExitStatus:    1,
 			cmdStdout:        "",
 			cmdStderr:        "git error message",
@@ -752,6 +752,15 @@ func TestClientReadBranchConfig(t *testing.T) {
 			wantBranchConfig: BranchConfig{},
 			wantError:        &GitError{ExitCode: 1, Stderr: "git error message"},
 		},
+		{
+			name:             "git config runs successfully but returns no output",
+			cmdExitStatus:    1,
+			cmdStdout:        "",
+			cmdStderr:        "",
+			branch:           "trunk",
+			wantBranchConfig: BranchConfig{},
+			wantError:        nil,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -764,7 +773,10 @@ func TestClientReadBranchConfig(t *testing.T) {
 			wantCmdArgs := fmt.Sprintf("path/to/git config --get-regexp ^branch\\.%s\\.(remote|merge|gh-merge-base)$", tt.branch)
 			assert.Equal(t, wantCmdArgs, strings.Join(cmd.Args[3:], " "))
 			assert.Equal(t, tt.wantBranchConfig, branchConfig)
-			if tt.wantError != nil {
+			if err != nil {
+				if tt.wantError == nil {
+					t.Fatalf("expected no error but got %v", err)
+				}
 				assert.Equal(t, tt.wantError.ExitCode, err.(*GitError).ExitCode)
 				assert.Equal(t, tt.wantError.Stderr, err.(*GitError).Stderr)
 			}
@@ -774,34 +786,34 @@ func TestClientReadBranchConfig(t *testing.T) {
 
 func Test_parseBranchConfig(t *testing.T) {
 	tests := []struct {
-		name                  string
-		gitBranchConfigOutput []string
-		wantBranchConfig      BranchConfig
+		name             string
+		configLines      []string
+		wantBranchConfig BranchConfig
 	}{
 		{
-			name:                  "remote branch",
-			gitBranchConfigOutput: []string{"branch.trunk.remote origin"},
+			name:        "remote branch",
+			configLines: []string{"branch.trunk.remote origin"},
 			wantBranchConfig: BranchConfig{
 				RemoteName: "origin",
 			},
 		},
 		{
-			name:                  "merge ref",
-			gitBranchConfigOutput: []string{"branch.trunk.merge refs/heads/trunk"},
+			name:        "merge ref",
+			configLines: []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"},
+			name:        "merge base",
+			configLines: []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{
+			configLines: []string{
 				"branch.trunk.remote origin",
 				"branch.trunk.merge refs/heads/trunk",
 				"branch.trunk.gh-merge-base gh-merge-base",
@@ -814,7 +826,7 @@ func Test_parseBranchConfig(t *testing.T) {
 		},
 		{
 			name: "remote URL",
-			gitBranchConfigOutput: []string{
+			configLines: []string{
 				"branch.Frederick888/main.remote git@github.com:Frederick888/playground.git",
 				"branch.Frederick888/main.merge refs/heads/main",
 			},
@@ -831,7 +843,7 @@ func Test_parseBranchConfig(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			branchConfig := parseBranchConfig(tt.gitBranchConfigOutput)
+			branchConfig := parseBranchConfig(tt.configLines)
 			assert.Equal(t, tt.wantBranchConfig.RemoteName, branchConfig.RemoteName)
 			assert.Equal(t, tt.wantBranchConfig.MergeRef, branchConfig.MergeRef)
 			assert.Equal(t, tt.wantBranchConfig.MergeBase, branchConfig.MergeBase)

pkg/cmd/pr/create/create.go

@@ -263,17 +263,17 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co
 	return cmd
 }
 
-func createRun(opts *CreateOptions) (err error) {
+func createRun(opts *CreateOptions) error {
 	ctx, err := NewCreateContext(opts)
 	if err != nil {
-		return
+		return err
 	}
 
 	client := ctx.Client
 
 	state, err := NewIssueState(*ctx, *opts)
 	if err != nil {
-		return
+		return err
 	}
 
 	var openURL string
@@ -288,15 +288,15 @@ func createRun(opts *CreateOptions) (err error) {
 		}
 		err = handlePush(*opts, *ctx)
 		if err != nil {
-			return
+			return err
 		}
 		openURL, err = generateCompareURL(*ctx, *state)
 		if err != nil {
-			return
+			return err
 		}
 		if !shared.ValidURL(openURL) {
 			err = fmt.Errorf("cannot open in browser: maximum URL length exceeded")
-			return
+			return err
 		}
 		return previewPR(*opts, openURL)
 	}
@@ -344,7 +344,7 @@ func createRun(opts *CreateOptions) (err error) {
 	if !opts.EditorMode && (opts.FillVerbose || opts.Autofill || opts.FillFirst || (opts.TitleProvided && opts.BodyProvided)) {
 		err = handlePush(*opts, *ctx)
 		if err != nil {
-			return
+			return err
 		}
 		return submitPR(*opts, *ctx, *state)
 	}
@@ -368,7 +368,7 @@ func createRun(opts *CreateOptions) (err error) {
 			var template shared.Template
 			template, err = tpl.Select(opts.Template)
 			if err != nil {
-				return
+				return err
 			}
 			if state.Title == "" {
 				state.Title = template.Title()
@@ -378,18 +378,18 @@ func createRun(opts *CreateOptions) (err error) {
 
 		state.Title, state.Body, err = opts.TitledEditSurvey(state.Title, state.Body)
 		if err != nil {
-			return
+			return err
 		}
 		if state.Title == "" {
 			err = fmt.Errorf("title can't be blank")
-			return
+			return err
 		}
 	} else {
 
 		if !opts.TitleProvided {
 			err = shared.TitleSurvey(opts.Prompter, opts.IO, state)
 			if err != nil {
-				return
+				return err
 			}
 		}
 
@@ -403,12 +403,12 @@ func createRun(opts *CreateOptions) (err error) {
 				if opts.Template != "" {
 					template, err = tpl.Select(opts.Template)
 					if err != nil {
-						return
+						return err
 					}
 				} else {
 					template, err = tpl.Choose()
 					if err != nil {
-						return
+						return err
 					}
 				}
 
@@ -419,13 +419,13 @@ func createRun(opts *CreateOptions) (err error) {
 
 			err = shared.BodySurvey(opts.Prompter, state, templateContent)
 			if err != nil {
-				return
+				return err
 			}
 		}
 
 		openURL, err = generateCompareURL(*ctx, *state)
 		if err != nil {
-			return
+			return err
 		}
 
 		allowPreview := !state.HasMetadata() && shared.ValidURL(openURL) && !opts.DryRun
@@ -444,12 +444,12 @@ func createRun(opts *CreateOptions) (err error) {
 			}
 			err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.BaseRepo, fetcher, state)
 			if err != nil {
-				return
+				return err
 			}
 
 			action, err = shared.ConfirmPRSubmission(opts.Prompter, !state.HasMetadata() && !opts.DryRun, false, state.Draft)
 			if err != nil {
-				return
+				return err
 			}
 		}
 	}
@@ -457,12 +457,12 @@ func createRun(opts *CreateOptions) (err error) {
 	if action == shared.CancelAction {
 		fmt.Fprintln(opts.IO.ErrOut, "Discarding.")
 		err = cmdutil.CancelError
-		return
+		return err
 	}
 
 	err = handlePush(*opts, *ctx)
 	if err != nil {
-		return
+		return err
 	}
 
 	if action == shared.PreviewAction {
@@ -479,7 +479,7 @@ func createRun(opts *CreateOptions) (err error) {
 	}
 
 	err = errors.New("expected to cancel, preview, or submit")
-	return
+	return err
 }
 
 var regexPattern = regexp.MustCompile(`(?m)^`)
@@ -682,8 +682,7 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) {
 
 	headBranchConfig, err := gitClient.ReadBranchConfig(context.Background(), headBranch)
 	if err != nil {
-		// We can still proceed without the branch config, so print to stderr but continue
-		fmt.Fprintf(opts.IO.ErrOut, "Unable to read git branch config: %v\n", err)
+		return nil, err
 	}
 	if isPushEnabled {
 		// determine whether the head branch is already pushed to a remote

pkg/cmd/pr/status/status.go

@@ -94,7 +94,10 @@ func statusRun(opts *StatusOptions) error {
 		}
 
 		remotes, _ := opts.Remotes()
-		branchConfig, _ := opts.GitClient.ReadBranchConfig(ctx, currentBranch)
+		branchConfig, err := opts.GitClient.ReadBranchConfig(ctx, currentBranch)
+		if err != nil {
+			return err
+		}
 		currentPRNumber, currentPRHeadRef, err = prSelectorForCurrentBranch(branchConfig, baseRepo, currentBranch, remotes)
 		if err != nil {
 			return fmt.Errorf("could not query for pull request for current branch: %w", err)