jtmcg · Jan 13, 2025 · Jan 7, 2025 · Jan 7, 2025 · Jan 7, 2025 · Jan 7, 2025
@@ -377,19 +377,36 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte,
 }
 
 // ReadBranchConfig parses the `branch.BRANCH.(remote|merge|gh-merge-base)` part of git config.
-func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg BranchConfig) {
+// If no branch config is found or there is an error in the command, it returns an empty BranchConfig.
+// Downstream consumers of ReadBranchConfig should consider the behavior they desire if this errors,
+// as an empty config is not necessarily breaking.
+func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchConfig, 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
+		return BranchConfig{}, err
 	}
+
 	out, err := cmd.Output()
 	if err != nil {
-		return
+		// 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
+		}
+		return BranchConfig{}, nil
 	}
 
-	for _, line := range outputLines(out) {
+	return parseBranchConfig(outputLines(out)), nil
+}
+
+func parseBranchConfig(configLines []string) BranchConfig {
+	var cfg BranchConfig
+
+	for _, line := range configLines {
 		parts := strings.SplitN(line, " ", 2)
 		if len(parts) < 2 {
 			continue
@@ -412,7 +429,7 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc
 			cfg.MergeBase = parts[1]
 		}
 	}
-	return
+	return cfg
 }
 
 // SetBranchConfig sets the named value on the given branch.

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -730,14 +731,35 @@ func TestClientReadBranchConfig(t *testing.T) {
 		cmdExitStatus    int
 		cmdStdout        string
 		cmdStderr        string
-		wantCmdArgs      string
+		branch           string
 		wantBranchConfig BranchConfig
+		wantError        *GitError
 	}{
 		{
 			name:             "read branch config",
+			cmdExitStatus:    0,
 			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)$`,
+			branch:           "trunk",
 			wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk"},
+			wantError:        nil,
+		},
+		{
+			name:             "git config runs successfully but returns no output (Exit Code 1)",
+			cmdExitStatus:    1,
+			cmdStdout:        "",
+			cmdStderr:        "",
+			branch:           "trunk",
+			wantBranchConfig: BranchConfig{},
+			wantError:        nil,
+		},
+		{
+			name:             "output error (Exit Code > 1)",
+			cmdExitStatus:    2,
+			cmdStdout:        "",
+			cmdStderr:        "git error message",
+			branch:           "trunk",
+			wantBranchConfig: BranchConfig{},
+			wantError:        &GitError{},
 		},
 	}
 	for _, tt := range tests {
@@ -747,9 +769,85 @@ func TestClientReadBranchConfig(t *testing.T) {
 				GitPath:        "path/to/git",
 				commandContext: cmdCtx,
 			}
-			branchConfig := client.ReadBranchConfig(context.Background(), "trunk")
-			assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " "))
+			branchConfig, err := client.ReadBranchConfig(context.Background(), tt.branch)
+			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 {
+				assert.ErrorAs(t, err, &tt.wantError)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func Test_parseBranchConfig(t *testing.T) {
+	tests := []struct {
+		name             string
+		configLines      []string
+		wantBranchConfig BranchConfig
+	}{
+		{
+			name:        "remote branch",
+			configLines: []string{"branch.trunk.remote origin"},
+			wantBranchConfig: BranchConfig{
+				RemoteName: "origin",
+			},
+		},
+		{
+			name:        "merge ref",
+			configLines: []string{"branch.trunk.merge refs/heads/trunk"},
+			wantBranchConfig: BranchConfig{
+				MergeRef: "refs/heads/trunk",
+			},
+		},
+		{
+			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",
+			configLines: []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",
+			},
+		},
+		{
+			name: "remote URL",
+			configLines: []string{
+				"branch.Frederick888/main.remote [email protected]:Frederick888/playground.git",
+				"branch.Frederick888/main.merge refs/heads/main",
+			},
+			wantBranchConfig: BranchConfig{
+				MergeRef: "refs/heads/main",
+				RemoteURL: &url.URL{
+					Scheme: "ssh",
+					User:   url.User("git"),
+					Host:   "github.com",
+					Path:   "/Frederick888/playground.git",
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			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)
+			if tt.wantBranchConfig.RemoteURL != nil {
+				assert.Equal(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String())
+			}
 		})
 	}
 }

@@ -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,25 +444,25 @@ 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
 			}
 		}
 	}
 
 	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)^`)
@@ -680,7 +680,10 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) {
 	var headRepo ghrepo.Interface
 	var headRemote *ghContext.Remote
 
-	headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch)
+	headBranchConfig, err := gitClient.ReadBranchConfig(context.Background(), headBranch)
+	if err != nil {
+		return nil, err
+	}
 	if isPushEnabled {
 		// determine whether the head branch is already pushed to a remote
 		if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found {