X Tutup
Skip to content

Commit 0f85304

Browse files
mislavmislav
committed
Avoid crash in pr merge when verifying whether a PR had diverged
A PR is not guaranteed to have commits, it seems, so add a guard against assuming that there is always a head commit.
1 parent 98df059 commit 0f85304

File tree

3 files changed

+142
-49
lines changed

3 files changed

+142
-49
lines changed

‎git/git.go‎

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -180,43 +180,30 @@ func Commits(baseRef, headRef string) ([]*Commit, error) {
180180
return commits, nil
181181
}
182182

183-
func LastCommit() (*Commit, error) {
184-
logCmd, err := GitCommand("-c", "log.ShowSignature=false", "log", "--pretty=format:%H,%s", "-1")
183+
func lookupCommit(sha, format string) ([]byte, error) {
184+
logCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:"+format, sha)
185185
if err != nil {
186186
return nil, err
187187
}
188+
return run.PrepareCmd(logCmd).Output()
189+
}
188190

189-
output, err := run.PrepareCmd(logCmd).Output()
191+
func LastCommit() (*Commit, error) {
192+
output, err := lookupCommit("HEAD", "%H,%s")
190193
if err != nil {
191194
return nil, err
192195
}
193196

194-
lines := outputLines(output)
195-
if len(lines) != 1 {
196-
return nil, ErrNotOnAnyBranch
197-
}
198-
199-
split := strings.SplitN(lines[0], ",", 2)
200-
if len(split) != 2 {
201-
return nil, ErrNotOnAnyBranch
202-
}
203-
197+
idx := bytes.IndexByte(output, ',')
204198
return &Commit{
205-
Sha: split[0],
206-
Title: split[1],
199+
Sha: string(output[0:idx]),
200+
Title: strings.TrimSpace(string(output[idx+1:])),
207201
}, nil
208202
}
209203

210204
func CommitBody(sha string) (string, error) {
211-
showCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
212-
if err != nil {
213-
return "", err
214-
}
215-
output, err := run.PrepareCmd(showCmd).Output()
216-
if err != nil {
217-
return "", err
218-
}
219-
return string(output), nil
205+
output, err := lookupCommit(sha, "%b")
206+
return string(output), err
220207
}
221208

222209
// Push publishes a git ref to a remote and sets up upstream configuration

‎pkg/cmd/pr/merge/merge.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,9 +157,8 @@ func mergeRun(opts *MergeOptions) error {
157157
return nil
158158
}
159159

160-
if opts.SelectorArg == "" {
161-
localBranchLastCommit, err := git.LastCommit()
162-
if err == nil {
160+
if opts.SelectorArg == "" && len(pr.Commits.Nodes) > 0 {
161+
if localBranchLastCommit, err := git.LastCommit(); err == nil {
163162
if localBranchLastCommit.Sha != pr.Commits.Nodes[0].Commit.Oid {
164163
fmt.Fprintf(opts.IO.ErrOut,
165164
"%s Pull request #%d (%s) has diverged from local branch\n", cs.Yellow("!"), pr.Number, pr.Title)

‎pkg/cmd/pr/merge/merge_test.go‎

Lines changed: 129 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"strings"
1010
"testing"
1111

12+
"github.com/MakeNowJust/heredoc"
1213
"github.com/cli/cli/api"
1314
"github.com/cli/cli/context"
1415
"github.com/cli/cli/git"
@@ -336,7 +337,7 @@ func TestPrMerge_deleteBranch(t *testing.T) {
336337
cs, cmdTeardown := run.Stub()
337338
defer cmdTeardown(t)
338339

339-
cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
340+
cs.Register(`git .+ show .+ HEAD`, 1, "")
340341
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
341342
cs.Register(`git checkout master`, 0, "")
342343
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
@@ -347,8 +348,11 @@ func TestPrMerge_deleteBranch(t *testing.T) {
347348
t.Fatalf("Got unexpected error running `pr merge` %s", err)
348349
}
349350

350-
//nolint:staticcheck // prefer exact matchers over ExpectLines
351-
test.ExpectLines(t, output.Stderr(), `Merged pull request #10 \(Blueberries are a good fruit\)`, `Deleted branch.*blueberries`)
351+
assert.Equal(t, "", output.String())
352+
assert.Equal(t, heredoc.Doc(`
353+
✓ Merged pull request #10 (Blueberries are a good fruit)
354+
✓ Deleted branch blueberries and switched to branch master
355+
`), output.Stderr())
352356
}
353357

354358
func TestPrMerge_deleteNonCurrentBranch(t *testing.T) {
@@ -380,8 +384,11 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) {
380384
t.Fatalf("Got unexpected error running `pr merge` %s", err)
381385
}
382386

383-
//nolint:staticcheck // prefer exact matchers over ExpectLines
384-
test.ExpectLines(t, output.Stderr(), `Merged pull request #10 \(Blueberries are a good fruit\)`, `Deleted branch.*blueberries`)
387+
assert.Equal(t, "", output.String())
388+
assert.Equal(t, heredoc.Doc(`
389+
✓ Merged pull request #10 (Blueberries are a good fruit)
390+
✓ Deleted branch blueberries
391+
`), output.Stderr())
385392
}
386393

387394
func TestPrMerge_noPrNumberGiven(t *testing.T) {
@@ -402,28 +409,86 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) {
402409
cs, cmdTeardown := run.Stub()
403410
defer cmdTeardown(t)
404411

405-
cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
412+
cs.Register(`git .+ show .+ HEAD`, 1, "")
406413
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
407414

408415
output, err := runCommand(http, "blueberries", true, "pr merge --merge")
409416
if err != nil {
410417
t.Fatalf("error running command `pr merge`: %v", err)
411418
}
412419

413-
r := regexp.MustCompile(`Merged pull request #10 \(Blueberries are a good fruit\)`)
420+
assert.Equal(t, "", output.String())
421+
assert.Equal(t, heredoc.Doc(`
422+
✓ Merged pull request #10 (Blueberries are a good fruit)
423+
`), output.Stderr())
424+
}
414425

415-
if !r.MatchString(output.Stderr()) {
416-
t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr())
426+
func Test_nonDivergingPullRequest(t *testing.T) {
427+
http := initFakeHTTP()
428+
defer http.Verify(t)
429+
http.Register(
430+
httpmock.GraphQL(`query PullRequestForBranch\b`),
431+
httpmock.StringResponse(`
432+
{ "data": { "repository": { "pullRequests": { "nodes": [{
433+
"headRefName": "blueberries",
434+
"headRepositoryOwner": {"login": "OWNER"},
435+
"id": "PR_10",
436+
"title": "Blueberries are a good fruit",
437+
"number": 10,
438+
"commits": {
439+
"nodes": [{
440+
"commit": {
441+
"oid": "COMMITSHA1"
442+
}
443+
}],
444+
"totalCount": 1
445+
}
446+
}] } } } }`))
447+
http.Register(
448+
httpmock.GraphQL(`mutation PullRequestMerge\b`),
449+
httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
450+
assert.Equal(t, "PR_10", input["pullRequestId"].(string))
451+
assert.Equal(t, "MERGE", input["mergeMethod"].(string))
452+
assert.NotContains(t, input, "commitHeadline")
453+
}))
454+
455+
cs, cmdTeardown := run.Stub()
456+
defer cmdTeardown(t)
457+
458+
cs.Register(`git .+ show .+ HEAD`, 0, "COMMITSHA1,title")
459+
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
460+
461+
output, err := runCommand(http, "blueberries", true, "pr merge --merge")
462+
if err != nil {
463+
t.Fatalf("error running command `pr merge`: %v", err)
417464
}
465+
466+
assert.Equal(t, heredoc.Doc(`
467+
✓ Merged pull request #10 (Blueberries are a good fruit)
468+
`), output.Stderr())
418469
}
419470

420471
func Test_divergingPullRequestWarning(t *testing.T) {
421472
http := initFakeHTTP()
422473
defer http.Verify(t)
423474
http.Register(
424475
httpmock.GraphQL(`query PullRequestForBranch\b`),
425-
// FIXME: references fixture from another package
426-
httpmock.FileResponse("../view/fixtures/prViewPreviewWithMetadataByBranch.json"))
476+
httpmock.StringResponse(`
477+
{ "data": { "repository": { "pullRequests": { "nodes": [{
478+
"headRefName": "blueberries",
479+
"headRepositoryOwner": {"login": "OWNER"},
480+
"id": "PR_10",
481+
"title": "Blueberries are a good fruit",
482+
"number": 10,
483+
"commits": {
484+
"nodes": [{
485+
"commit": {
486+
"oid": "COMMITSHA1"
487+
}
488+
}],
489+
"totalCount": 1
490+
}
491+
}] } } } }`))
427492
http.Register(
428493
httpmock.GraphQL(`mutation PullRequestMerge\b`),
429494
httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
@@ -435,19 +500,58 @@ func Test_divergingPullRequestWarning(t *testing.T) {
435500
cs, cmdTeardown := run.Stub()
436501
defer cmdTeardown(t)
437502

438-
cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "deadbeef,title")
503+
cs.Register(`git .+ show .+ HEAD`, 0, "COMMITSHA2,title")
439504
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
440505

441506
output, err := runCommand(http, "blueberries", true, "pr merge --merge")
442507
if err != nil {
443508
t.Fatalf("error running command `pr merge`: %v", err)
444509
}
445510

446-
r := regexp.MustCompile(`. Pull request #10 \(Blueberries are a good fruit\) has diverged from local branch\n. Merged pull request #10 \(Blueberries are a good fruit\)\n`)
511+
assert.Equal(t, heredoc.Doc(`
512+
! Pull request #10 (Blueberries are a good fruit) has diverged from local branch
513+
✓ Merged pull request #10 (Blueberries are a good fruit)
514+
`), output.Stderr())
515+
}
447516

448-
if !r.MatchString(output.Stderr()) {
449-
t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr())
517+
func Test_pullRequestWithoutCommits(t *testing.T) {
518+
http := initFakeHTTP()
519+
defer http.Verify(t)
520+
http.Register(
521+
httpmock.GraphQL(`query PullRequestForBranch\b`),
522+
httpmock.StringResponse(`
523+
{ "data": { "repository": { "pullRequests": { "nodes": [{
524+
"headRefName": "blueberries",
525+
"headRepositoryOwner": {"login": "OWNER"},
526+
"id": "PR_10",
527+
"title": "Blueberries are a good fruit",
528+
"number": 10,
529+
"commits": {
530+
"nodes": [],
531+
"totalCount": 0
532+
}
533+
}] } } } }`))
534+
http.Register(
535+
httpmock.GraphQL(`mutation PullRequestMerge\b`),
536+
httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
537+
assert.Equal(t, "PR_10", input["pullRequestId"].(string))
538+
assert.Equal(t, "MERGE", input["mergeMethod"].(string))
539+
assert.NotContains(t, input, "commitHeadline")
540+
}))
541+
542+
cs, cmdTeardown := run.Stub()
543+
defer cmdTeardown(t)
544+
545+
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
546+
547+
output, err := runCommand(http, "blueberries", true, "pr merge --merge")
548+
if err != nil {
549+
t.Fatalf("error running command `pr merge`: %v", err)
450550
}
551+
552+
assert.Equal(t, heredoc.Doc(`
553+
✓ Merged pull request #10 (Blueberries are a good fruit)
554+
`), output.Stderr())
451555
}
452556

453557
func TestPrMerge_rebase(t *testing.T) {
@@ -517,8 +621,10 @@ func TestPrMerge_squash(t *testing.T) {
517621
t.Fatalf("error running command `pr merge`: %v", err)
518622
}
519623

520-
//nolint:staticcheck // prefer exact matchers over ExpectLines
521-
test.ExpectLines(t, output.Stderr(), "Squashed and merged pull request #3")
624+
assert.Equal(t, "", output.String())
625+
assert.Equal(t, heredoc.Doc(`
626+
✓ Squashed and merged pull request #3 (The title of the PR)
627+
`), output.Stderr())
522628
}
523629

524630
func TestPrMerge_alreadyMerged(t *testing.T) {
@@ -614,7 +720,6 @@ func TestPRMerge_interactive(t *testing.T) {
614720
cs, cmdTeardown := run.Stub()
615721
defer cmdTeardown(t)
616722

617-
cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
618723
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
619724

620725
as, surveyTeardown := prompt.InitAskStubber()
@@ -643,6 +748,7 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
643748
"headRefName": "blueberries",
644749
"headRepositoryOwner": {"login": "OWNER"},
645750
"id": "THE-ID",
751+
"title": "It was the best of times",
646752
"number": 3
647753
}] } } } }`))
648754
http.Register(
@@ -667,7 +773,6 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
667773
cs, cmdTeardown := run.Stub()
668774
defer cmdTeardown(t)
669775

670-
cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
671776
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
672777
cs.Register(`git checkout master`, 0, "")
673778
cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
@@ -684,8 +789,11 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
684789
t.Fatalf("Got unexpected error running `pr merge` %s", err)
685790
}
686791

687-
//nolint:staticcheck // prefer exact matchers over ExpectLines
688-
test.ExpectLines(t, output.Stderr(), "Merged pull request #3", "Deleted branch blueberries and switched to branch master")
792+
assert.Equal(t, "", output.String())
793+
assert.Equal(t, heredoc.Doc(`
794+
✓ Merged pull request #3 (It was the best of times)
795+
✓ Deleted branch blueberries and switched to branch master
796+
`), output.Stderr())
689797
}
690798

691799
func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) {
@@ -777,7 +885,6 @@ func TestPRMerge_interactiveCancelled(t *testing.T) {
777885
cs, cmdTeardown := run.Stub()
778886
defer cmdTeardown(t)
779887

780-
cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
781888
cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
782889

783890
as, surveyTeardown := prompt.InitAskStubber()

0 commit comments

Comments
 (0)
X Tutup