Skip to content

Respect --title and --body in pr create web mode (second attempt) #11306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
babakks wants to merge 2 commits into from
Closed

Respect --title and --body in pr create web mode (second attempt) #11306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c604996
fix(pr create): respect `--title` and `--body` in web mode
babakks Jul 15, 2025
ca68f44
test(pr create): improve web mode tests
babakks Jul 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions pkg/cmd/pr/create/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,34 +596,33 @@ func createRun(opts *CreateOptions) error {

var regexPattern = regexp.MustCompile(`(?m)^`)

func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, useFirstCommit bool, addBody bool) error {
func computeDefaultTitleBody(ctx CreateContext, useFirstCommit bool, addBody bool) (string, string, error) {
commits, err := ctx.GitClient.Commits(context.Background(), ctx.BaseTrackingBranch, ctx.PRRefs.UnqualifiedHeadRef())
if err != nil {
return err
return "", "", err
}

if len(commits) == 1 || useFirstCommit {
state.Title = commits[len(commits)-1].Title
state.Body = commits[len(commits)-1].Body
} else {
state.Title = humanize(ctx.PRRefs.UnqualifiedHeadRef())
var body strings.Builder
for i := len(commits) - 1; i >= 0; i-- {
fmt.Fprintf(&body, "- **%s**\n", commits[i].Title)
if addBody {
x := regexPattern.ReplaceAllString(commits[i].Body, " ")
fmt.Fprintf(&body, "%s", x)

if i > 0 {
fmt.Fprintln(&body)
fmt.Fprintln(&body)
}
return commits[len(commits)-1].Title, commits[len(commits)-1].Body, nil
}

title := humanize(ctx.PRRefs.UnqualifiedHeadRef())
var sb strings.Builder
for i := len(commits) - 1; i >= 0; i-- {
fmt.Fprintf(&sb, "- **%s**\n", commits[i].Title)
if addBody {
x := regexPattern.ReplaceAllString(commits[i].Body, " ")
fmt.Fprintf(&sb, "%s", x)

if i > 0 {
fmt.Fprintln(&sb)
fmt.Fprintln(&sb)
}
}
state.Body = body.String()
}
body := sb.String()

return nil
return title, body, nil
}
Comment on lines +599 to 626
Copy link
Copy Markdown
Contributor

@andyfeller andyfeller Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: I like how this is now focused on returning a title and body based on the state of commits, leaving it to the caller what to do with them.


func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) {
Expand All @@ -646,12 +645,21 @@ func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadata
ProjectTitles: opts.Projects,
Milestones: milestoneTitles,
Draft: opts.IsDraft,
Title: opts.Title,
Body: opts.Body,
}

if opts.FillVerbose || opts.Autofill || opts.FillFirst || !opts.TitleProvided || !opts.BodyProvided {
err := initDefaultTitleBody(ctx, state, opts.FillFirst, opts.FillVerbose)
title, body, err := computeDefaultTitleBody(ctx, opts.FillFirst, opts.FillVerbose)
if err != nil && (opts.FillVerbose || opts.Autofill || opts.FillFirst) {
return nil, fmt.Errorf("could not compute title or body defaults: %w", err)
} else {
if !opts.TitleProvided {
state.Title = title
}
if !opts.BodyProvided {
state.Body = body
}
}
Comment on lines +656 to 663
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else block after a return statement is unnecessary. Consider removing the else and dedenting the code block for better readability.

Suggested change
} else {
if !opts.TitleProvided {
state.Title = title
}
if !opts.BodyProvided {
state.Body = body
}
}
}
if !opts.TitleProvided {
state.Title = title
}
if !opts.BodyProvided {
state.Body = body
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@andyfeller andyfeller Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest this but held off while trying to understand these changes in context of the regression.

Comment on lines 652 to 663
Copy link
Copy Markdown
Contributor

@andyfeller andyfeller Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I think --web logic that follows NewIssueState(...) should be consolidated and rewritten into this function in order to make it easier to understand what this command will do along with plain language explanation of what should happen in these different circumstances:

Suggested change
if opts.FillVerbose || opts.Autofill || opts.FillFirst || !opts.TitleProvided || !opts.BodyProvided {
err := initDefaultTitleBody(ctx, state, opts.FillFirst, opts.FillVerbose)
title, body, err := computeDefaultTitleBody(ctx, opts.FillFirst, opts.FillVerbose)
if err != nil && (opts.FillVerbose || opts.Autofill || opts.FillFirst) {
return nil, fmt.Errorf("could not compute title or body defaults: %w", err)
} else {
if !opts.TitleProvided {
state.Title = title
}
if !opts.BodyProvided {
state.Body = body
}
}
if opts.FillVerbose || opts.Autofill || opts.FillFirst || !opts.WebMode {
title, body, err := computeDefaultTitleBody(ctx, opts.FillFirst, opts.FillVerbose)
if err != nil && (opts.FillVerbose || opts.Autofill || opts.FillFirst) {
return nil, fmt.Errorf("could not compute title or body defaults: %w", err)
}
if !opts.TitleProvided {
state.Title = title
}
if !opts.BodyProvided {
state.Body = body
}

Here's how I think about this logic:

  1. gh pr create should use --title and/or --body over any other options when provided

  2. gh pr create should provide the default title and/or body where --title and/or --body are not specified under the given circumstances:

    1. --fill is provided to use commit info
    2. --fill-first is provided to use first commit
    3. --fill-verbose is provided to use commit message and body
    4. --web is absent (the GitHub.com UI takes over the default title and body responsibility)

My concern is that the state being overridden in the other places makes this hard to follow when not consolidated:

cli/pkg/cmd/pr/create/create.go

Lines 314 to 316 in dbff7c5

if !opts.IO.CanPrompt() && !opts.WebMode && !(opts.FillVerbose || opts.Autofill || opts.FillFirst) && (!opts.TitleProvided || !opts.BodyProvided) {
return cmdutil.FlagErrorf("must provide `--title` and `--body` (or `--fill` or `fill-first` or `--fillverbose`) when not running interactively")
}

cli/pkg/cmd/pr/create/create.go

Lines 393 to 400 in ca68f44

if opts.WebMode {
if !(opts.Autofill || opts.FillFirst) {
state.Title = opts.Title
state.Body = opts.Body
}
if opts.Template != "" {
state.Template = opts.Template
}

}

Expand Down
184 changes: 183 additions & 1 deletion pkg/cmd/pr/create/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,13 @@ func Test_createRun(t *testing.T) {
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "")
// Here we simulate a non-empty git history, and we want to see that the title/body fields are not populated from it, since no fill option is provided.
cs.Register(
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
0,
"56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
)
cs.Register("git rev-parse --symbolic-full-name feature@{push}", 0, "refs/remotes/origin/feature")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
Expand All @@ -1147,6 +1153,182 @@ func Test_createRun(t *testing.T) {
expectedErrOut: "Opening https://github.com/OWNER/REPO/compare/master...feature in your browser.\n",
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1",
},
{
name: "web with --fill",
setup: func(opts *CreateOptions, t *testing.T) func() {
opts.WebMode = true
opts.HeadBranch = "feature"
opts.Autofill = true
return func() {}
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
0,
"56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
)
},
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=-+%2A%2Afirst+commit+of+pr%2A%2A%0A-+%2A%2Asecond+commit+of+pr%2A%2A%0A&expand=1&title=feature",
},
{
name: "web with --fill-first",
setup: func(opts *CreateOptions, t *testing.T) func() {
opts.WebMode = true
opts.HeadBranch = "feature"
opts.FillFirst = true
return func() {}
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
0,
"56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
)
},
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=first+commit+description&expand=1&title=first+commit+of+pr",
},
{
name: "web with --title",
tty: true,
setup: func(opts *CreateOptions, t *testing.T) func() {
opts.WebMode = true
opts.TitleProvided = true
opts.Title = "foo"
return func() {}
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
reg.StubRepoResponse("OWNER", "REPO")
reg.Register(
httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
},
cmdStubs: func(cs *run.CommandStubber) {
// Here we simulate a non-empty git history, and we want to see that the body field is not populated from it, since no fill option is provided.
cs.Register(
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
0,
"56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
)
cs.Register("git rev-parse --symbolic-full-name feature@{push}", 0, "refs/remotes/origin/feature")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
pm.SelectFunc = func(p, _ string, opts []string) (int, error) {
if p == "Where should we push the 'feature' branch?" {
return 0, nil
} else {
return -1, prompter.NoSuchPromptErr(p)
}
}
},
expectedErrOut: "Opening https://github.com/OWNER/REPO/compare/master...feature in your browser.\n",
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1&title=foo",
},
{
name: "web with --body",
tty: true,
setup: func(opts *CreateOptions, t *testing.T) func() {
opts.WebMode = true
opts.BodyProvided = true
opts.Body = "foo"
return func() {}
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
reg.StubRepoResponse("OWNER", "REPO")
reg.Register(
httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
},
cmdStubs: func(cs *run.CommandStubber) {
// Here we simulate a non-empty git history, and we want to see that the title field is not populated from it, since no fill option is provided.
cs.Register(
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
0,
"56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
)
cs.Register("git rev-parse --symbolic-full-name feature@{push}", 0, "refs/remotes/origin/feature")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
pm.SelectFunc = func(p, _ string, opts []string) (int, error) {
if p == "Where should we push the 'feature' branch?" {
return 0, nil
} else {
return -1, prompter.NoSuchPromptErr(p)
}
}
},
expectedErrOut: "Opening https://github.com/OWNER/REPO/compare/master...feature in your browser.\n",
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=foo&expand=1",
},
{
name: "web with --title and --body",
tty: true,
setup: func(opts *CreateOptions, t *testing.T) func() {
opts.WebMode = true
opts.TitleProvided = true
opts.Title = "foo"
opts.BodyProvided = true
opts.Body = "bar"
return func() {}
},
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
reg.StubRepoResponse("OWNER", "REPO")
reg.Register(
httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
},
cmdStubs: func(cs *run.CommandStubber) {
// Since both --title and --body options are provided, and also --fill is set, then no git log history
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment contains a grammatical error. 'and also --fill is set' should be 'but no --fill is set' or similar, as this test case doesn't set any fill options.

Suggested change
// Since both --title and --body options are provided, and also --fill is set, then no git log history
// Since both --title and --body options are provided, but no --fill is set, then no git log history

Copilot uses AI. Check for mistakes.
// should be used, thus no stub for it.
cs.Register("git rev-parse --symbolic-full-name feature@{push}", 0, "refs/remotes/origin/feature")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
pm.SelectFunc = func(p, _ string, opts []string) (int, error) {
if p == "Where should we push the 'feature' branch?" {
return 0, nil
} else {
return -1, prompter.NoSuchPromptErr(p)
}
}
},
expectedErrOut: "Opening https://github.com/OWNER/REPO/compare/master...feature in your browser.\n",
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=bar&expand=1&title=foo",
},
{
name: "web with --title, --body and --fill (#10547)",
setup: func(opts *CreateOptions, t *testing.T) func() {
opts.WebMode = true
opts.HeadBranch = "feature"
opts.TitleProvided = true
opts.Title = "foo"
opts.BodyProvided = true
opts.Body = "bar"
opts.Autofill = true
return func() {}
},
cmdStubs: func(cs *run.CommandStubber) {
// Here we simulate a non-empty git history, and we want to see that the title/body fields are not populated from it, since no fill option is provided.
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is misleading. This test case has opts.Autofill = true set, so a fill option IS provided. The comment should reflect that the explicitly provided title/body values should take precedence over the autofilled values.

Suggested change
// Here we simulate a non-empty git history, and we want to see that the title/body fields are not populated from it, since no fill option is provided.
// Here we simulate a non-empty git history. Although the fill option is enabled (opts.Autofill = true), the explicitly provided title and body values (opts.Title and opts.Body) take precedence and are used instead of autofilled values.

Copilot uses AI. Check for mistakes.
cs.Register(
"git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature",
0,
"56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+
"3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n",
)
},
expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=bar&expand=1&title=foo",
},
{
name: "web project",
tty: true,
Expand Down
Loading