diff --git a/git/client.go b/git/client.go index 3cc53ca96..16e8f32b5 100644 --- a/git/client.go +++ b/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 diff --git a/git/client_test.go b/git/client_test.go index 5eb7f4585..4b721656e 100644 --- a/git/client_test.go +++ b/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) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index b2229b976..124bd4b07 100644 --- a/pkg/cmd/pr/create/create.go +++ b/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 diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index c15c0f7fd..d22ae4e2f 100644 --- a/pkg/cmd/pr/status/status.go +++ b/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)