Merge pull request #3036 from cli/pr-merge-no-commits

Avoid crash in `pr merge` when the pull request has no commits

git/fixtures/.gitignore

@@ -0,0 +1 @@
+*.git/COMMIT_EDITMSG

git/fixtures/simple.git/HEAD

@@ -0,0 +1 @@
+ref: refs/heads/main

git/fixtures/simple.git/config

@@ -0,0 +1,9 @@
+[core]
+	repositoryformatversion = 0
+	filemode = true
+	;bare = true
+	ignorecase = true
+	precomposeunicode = true
+[user]
+	name = Mona the Cat
+	email = monalisa@github.com

git/fixtures/simple.git/logs/HEAD

@@ -0,0 +1,2 @@
+0000000000000000000000000000000000000000 d1e0abfb7d158ed544a202a6958c62d4fc22e12f Mona the Cat <monalisa@github.com> 1614174263 +0100	commit (initial): Initial commit
+d1e0abfb7d158ed544a202a6958c62d4fc22e12f 6f1a2405cace1633d89a79c74c65f22fe78f9659 Mona the Cat <monalisa@github.com> 1614174275 +0100	commit: Second commit

git/fixtures/simple.git/logs/refs/heads/main

@@ -0,0 +1,2 @@
+0000000000000000000000000000000000000000 d1e0abfb7d158ed544a202a6958c62d4fc22e12f Mona the Cat <monalisa@github.com> 1614174263 +0100	commit (initial): Initial commit
+d1e0abfb7d158ed544a202a6958c62d4fc22e12f 6f1a2405cace1633d89a79c74c65f22fe78f9659 Mona the Cat <monalisa@github.com> 1614174275 +0100	commit: Second commit

git/fixtures/simple.git/objects/d1/e0abfb7d158ed544a202a6958c62d4fc22e12f

@@ -0,0 +1,3 @@
+x
²NK
+б0t²S╪╫ ILc
+"┌+Н╪@▓╬≤MМК╒╥7"^@≤мС▀╣bпZНxF°Н├h█▌аbХ╫╢;▓l╞²K
яТр©r╝3<ЙД│3бм3°Kc#-ЧЗ"нk8дZ.═╛2Йd╢=б^*)ESш&Цiq÷┬и▐П╜Б≥i│├о▀P┤
j┌╡A╒yА÷И
3*H/

git/fixtures/simple.git/refs/heads/main

@@ -0,0 +1 @@
+6f1a2405cace1633d89a79c74c65f22fe78f9659

git/git.go

@@ -180,43 +180,30 @@ func Commits(baseRef, headRef string) ([]*Commit, error) {
 	return commits, nil
 }
 
+func lookupCommit(sha, format string) ([]byte, error) {
+	logCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:"+format, sha)
+	if err != nil {
+		return nil, err
+	}
+	return run.PrepareCmd(logCmd).Output()
+}
+
 func LastCommit() (*Commit, error) {
-	logCmd, err := GitCommand("-c", "log.ShowSignature=false", "log", "--pretty=format:%H,%s", "-1")
+	output, err := lookupCommit("HEAD", "%H,%s")
 	if err != nil {
 		return nil, err
 	}
 
-	output, err := run.PrepareCmd(logCmd).Output()
-	if err != nil {
-		return nil, err
-	}
-
-	lines := outputLines(output)
-	if len(lines) != 1 {
-		return nil, ErrNotOnAnyBranch
-	}
-
-	split := strings.SplitN(lines[0], ",", 2)
-	if len(split) != 2 {
-		return nil, ErrNotOnAnyBranch
-	}
-
+	idx := bytes.IndexByte(output, ',')
 	return &Commit{
-		Sha:   split[0],
-		Title: split[1],
+		Sha:   string(output[0:idx]),
+		Title: strings.TrimSpace(string(output[idx+1:])),
 	}, nil
 }
 
 func CommitBody(sha string) (string, error) {
-	showCmd, err := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha)
-	if err != nil {
-		return "", err
-	}
-	output, err := run.PrepareCmd(showCmd).Output()
-	if err != nil {
-		return "", err
-	}
-	return string(output), nil
+	output, err := lookupCommit(sha, "%b")
+	return string(output), err
 }
 
 // Push publishes a git ref to a remote and sets up upstream configuration

git/git_test.go

@@ -1,12 +1,53 @@
 package git
 
 import (
+	"os"
 	"reflect"
 	"testing"
 
 	"github.com/cli/cli/internal/run"
 )
 
+func setGitDir(t *testing.T, dir string) {
+	// TODO: also set XDG_CONFIG_HOME, GIT_CONFIG_NOSYSTEM
+	old_GIT_DIR := os.Getenv("GIT_DIR")
+	os.Setenv("GIT_DIR", dir)
+	t.Cleanup(func() {
+		os.Setenv("GIT_DIR", old_GIT_DIR)
+	})
+}
+
+func TestLastCommit(t *testing.T) {
+	setGitDir(t, "./fixtures/simple.git")
+	c, err := LastCommit()
+	if err != nil {
+		t.Fatalf("LastCommit error: %v", err)
+	}
+	if c.Sha != "6f1a2405cace1633d89a79c74c65f22fe78f9659" {
+		t.Errorf("expected sha %q, got %q", "6f1a2405cace1633d89a79c74c65f22fe78f9659", c.Sha)
+	}
+	if c.Title != "Second commit" {
+		t.Errorf("expected title %q, got %q", "Second commit", c.Title)
+	}
+}
+
+func TestCommitBody(t *testing.T) {
+	setGitDir(t, "./fixtures/simple.git")
+	body, err := CommitBody("6f1a2405cace1633d89a79c74c65f22fe78f9659")
+	if err != nil {
+		t.Fatalf("CommitBody error: %v", err)
+	}
+	if body != "I'm starting to get the hang of things\n" {
+		t.Errorf("expected %q, got %q", "I'm starting to get the hang of things\n", body)
+	}
+}
+
+/*
+	NOTE: below this are stubbed git tests, i.e. those that do not actually invoke `git`. If possible, utilize
+	`setGitDir()` to allow new tests to interact with `git`. For write operations, you can use `t.TempDir()` to
+	host a temporary git repository that is safe to be changed.
+*/
+
 func Test_UncommittedChangeCount(t *testing.T) {
 	type c struct {
 		Label    string

pkg/cmd/pr/merge/merge.go

@@ -157,9 +157,8 @@ func mergeRun(opts *MergeOptions) error {
 		return nil
 	}
 
-	if opts.SelectorArg == "" {
-		localBranchLastCommit, err := git.LastCommit()
-		if err == nil {
+	if opts.SelectorArg == "" && len(pr.Commits.Nodes) > 0 {
+		if localBranchLastCommit, err := git.LastCommit(); err == nil {
 			if localBranchLastCommit.Sha != pr.Commits.Nodes[0].Commit.Oid {
 				fmt.Fprintf(opts.IO.ErrOut,
 					"%s Pull request #%d (%s) has diverged from local branch\n", cs.Yellow("!"), pr.Number, pr.Title)

pkg/cmd/pr/merge/merge_test.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/api"
 	"github.com/cli/cli/context"
 	"github.com/cli/cli/git"
@@ -336,7 +337,7 @@ func TestPrMerge_deleteBranch(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
+	cs.Register(`git .+ show .+ HEAD`, 1, "")
 	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
 	cs.Register(`git checkout master`, 0, "")
 	cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
@@ -347,8 +348,11 @@ func TestPrMerge_deleteBranch(t *testing.T) {
 		t.Fatalf("Got unexpected error running `pr merge` %s", err)
 	}
 
-	//nolint:staticcheck // prefer exact matchers over ExpectLines
-	test.ExpectLines(t, output.Stderr(), `Merged pull request #10 \(Blueberries are a good fruit\)`, `Deleted branch.*blueberries`)
+	assert.Equal(t, "", output.String())
+	assert.Equal(t, heredoc.Doc(`
+		✓ Merged pull request #10 (Blueberries are a good fruit)
+		✓ Deleted branch blueberries and switched to branch master
+	`), output.Stderr())
 }
 
 func TestPrMerge_deleteNonCurrentBranch(t *testing.T) {
@@ -380,8 +384,11 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) {
 		t.Fatalf("Got unexpected error running `pr merge` %s", err)
 	}
 
-	//nolint:staticcheck // prefer exact matchers over ExpectLines
-	test.ExpectLines(t, output.Stderr(), `Merged pull request #10 \(Blueberries are a good fruit\)`, `Deleted branch.*blueberries`)
+	assert.Equal(t, "", output.String())
+	assert.Equal(t, heredoc.Doc(`
+		✓ Merged pull request #10 (Blueberries are a good fruit)
+		✓ Deleted branch blueberries
+	`), output.Stderr())
 }
 
 func TestPrMerge_noPrNumberGiven(t *testing.T) {
@@ -402,7 +409,7 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
+	cs.Register(`git .+ show .+ HEAD`, 1, "")
 	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
 
 	output, err := runCommand(http, "blueberries", true, "pr merge --merge")
@@ -410,20 +417,33 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) {
 		t.Fatalf("error running command `pr merge`: %v", err)
 	}
 
-	r := regexp.MustCompile(`Merged pull request #10 \(Blueberries are a good fruit\)`)
-
-	if !r.MatchString(output.Stderr()) {
-		t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr())
-	}
+	assert.Equal(t, "", output.String())
+	assert.Equal(t, heredoc.Doc(`
+		✓ Merged pull request #10 (Blueberries are a good fruit)
+	`), output.Stderr())
 }
 
-func Test_divergingPullRequestWarning(t *testing.T) {
+func Test_nonDivergingPullRequest(t *testing.T) {
 	http := initFakeHTTP()
 	defer http.Verify(t)
 	http.Register(
 		httpmock.GraphQL(`query PullRequestForBranch\b`),
-		// FIXME: references fixture from another package
-		httpmock.FileResponse("../view/fixtures/prViewPreviewWithMetadataByBranch.json"))
+		httpmock.StringResponse(`
+		{ "data": { "repository": { "pullRequests": { "nodes": [{
+			"headRefName": "blueberries",
+			"headRepositoryOwner": {"login": "OWNER"},
+			"id": "PR_10",
+			"title": "Blueberries are a good fruit",
+			"number": 10,
+			"commits": {
+				"nodes": [{
+					"commit": {
+						"oid": "COMMITSHA1"
+					}
+				}],
+				"totalCount": 1
+			}
+		}] } } } }`))
 	http.Register(
 		httpmock.GraphQL(`mutation PullRequestMerge\b`),
 		httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
@@ -435,7 +455,7 @@ func Test_divergingPullRequestWarning(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "deadbeef,title")
+	cs.Register(`git .+ show .+ HEAD`, 0, "COMMITSHA1,title")
 	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
 
 	output, err := runCommand(http, "blueberries", true, "pr merge --merge")
@@ -443,11 +463,95 @@ func Test_divergingPullRequestWarning(t *testing.T) {
 		t.Fatalf("error running command `pr merge`: %v", err)
 	}
 
-	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`)
+	assert.Equal(t, heredoc.Doc(`
+		✓ Merged pull request #10 (Blueberries are a good fruit)
+	`), output.Stderr())
+}
 
-	if !r.MatchString(output.Stderr()) {
-		t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr())
+func Test_divergingPullRequestWarning(t *testing.T) {
+	http := initFakeHTTP()
+	defer http.Verify(t)
+	http.Register(
+		httpmock.GraphQL(`query PullRequestForBranch\b`),
+		httpmock.StringResponse(`
+		{ "data": { "repository": { "pullRequests": { "nodes": [{
+			"headRefName": "blueberries",
+			"headRepositoryOwner": {"login": "OWNER"},
+			"id": "PR_10",
+			"title": "Blueberries are a good fruit",
+			"number": 10,
+			"commits": {
+				"nodes": [{
+					"commit": {
+						"oid": "COMMITSHA1"
+					}
+				}],
+				"totalCount": 1
+			}
+		}] } } } }`))
+	http.Register(
+		httpmock.GraphQL(`mutation PullRequestMerge\b`),
+		httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
+			assert.Equal(t, "PR_10", input["pullRequestId"].(string))
+			assert.Equal(t, "MERGE", input["mergeMethod"].(string))
+			assert.NotContains(t, input, "commitHeadline")
+		}))
+
+	cs, cmdTeardown := run.Stub()
+	defer cmdTeardown(t)
+
+	cs.Register(`git .+ show .+ HEAD`, 0, "COMMITSHA2,title")
+	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
+
+	output, err := runCommand(http, "blueberries", true, "pr merge --merge")
+	if err != nil {
+		t.Fatalf("error running command `pr merge`: %v", err)
 	}
+
+	assert.Equal(t, heredoc.Doc(`
+		! Pull request #10 (Blueberries are a good fruit) has diverged from local branch
+		✓ Merged pull request #10 (Blueberries are a good fruit)
+	`), output.Stderr())
+}
+
+func Test_pullRequestWithoutCommits(t *testing.T) {
+	http := initFakeHTTP()
+	defer http.Verify(t)
+	http.Register(
+		httpmock.GraphQL(`query PullRequestForBranch\b`),
+		httpmock.StringResponse(`
+		{ "data": { "repository": { "pullRequests": { "nodes": [{
+			"headRefName": "blueberries",
+			"headRepositoryOwner": {"login": "OWNER"},
+			"id": "PR_10",
+			"title": "Blueberries are a good fruit",
+			"number": 10,
+			"commits": {
+				"nodes": [],
+				"totalCount": 0
+			}
+		}] } } } }`))
+	http.Register(
+		httpmock.GraphQL(`mutation PullRequestMerge\b`),
+		httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
+			assert.Equal(t, "PR_10", input["pullRequestId"].(string))
+			assert.Equal(t, "MERGE", input["mergeMethod"].(string))
+			assert.NotContains(t, input, "commitHeadline")
+		}))
+
+	cs, cmdTeardown := run.Stub()
+	defer cmdTeardown(t)
+
+	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
+
+	output, err := runCommand(http, "blueberries", true, "pr merge --merge")
+	if err != nil {
+		t.Fatalf("error running command `pr merge`: %v", err)
+	}
+
+	assert.Equal(t, heredoc.Doc(`
+		✓ Merged pull request #10 (Blueberries are a good fruit)
+	`), output.Stderr())
 }
 
 func TestPrMerge_rebase(t *testing.T) {
@@ -517,8 +621,10 @@ func TestPrMerge_squash(t *testing.T) {
 		t.Fatalf("error running command `pr merge`: %v", err)
 	}
 
-	//nolint:staticcheck // prefer exact matchers over ExpectLines
-	test.ExpectLines(t, output.Stderr(), "Squashed and merged pull request #3")
+	assert.Equal(t, "", output.String())
+	assert.Equal(t, heredoc.Doc(`
+		✓ Squashed and merged pull request #3 (The title of the PR)
+	`), output.Stderr())
 }
 
 func TestPrMerge_alreadyMerged(t *testing.T) {
@@ -614,7 +720,6 @@ func TestPRMerge_interactive(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
 	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
 
 	as, surveyTeardown := prompt.InitAskStubber()
@@ -643,6 +748,7 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
 			"headRefName": "blueberries",
 			"headRepositoryOwner": {"login": "OWNER"},
 			"id": "THE-ID",
+			"title": "It was the best of times",
 			"number": 3
 		}] } } } }`))
 	http.Register(
@@ -667,7 +773,6 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
 	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
 	cs.Register(`git checkout master`, 0, "")
 	cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "")
@@ -684,8 +789,11 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) {
 		t.Fatalf("Got unexpected error running `pr merge` %s", err)
 	}
 
-	//nolint:staticcheck // prefer exact matchers over ExpectLines
-	test.ExpectLines(t, output.Stderr(), "Merged pull request #3", "Deleted branch blueberries and switched to branch master")
+	assert.Equal(t, "", output.String())
+	assert.Equal(t, heredoc.Doc(`
+		✓ Merged pull request #3 (It was the best of times)
+		✓ Deleted branch blueberries and switched to branch master
+	`), output.Stderr())
 }
 
 func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) {
@@ -777,7 +885,6 @@ func TestPRMerge_interactiveCancelled(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "")
 	cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "")
 
 	as, surveyTeardown := prompt.InitAskStubber()