From 0f85304e3e144b06ee142da461e43d0f0443ed4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 24 Feb 2021 14:37:29 +0100 Subject: [PATCH 1/3] 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. --- git/git.go | 41 +++------ pkg/cmd/pr/merge/merge.go | 5 +- pkg/cmd/pr/merge/merge_test.go | 157 +++++++++++++++++++++++++++------ 3 files changed, 148 insertions(+), 55 deletions(-) diff --git a/git/git.go b/git/git.go index 684b00c10..abff866d6 100644 --- a/git/git.go +++ b/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 diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 67498b726..c979a7f91 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/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) diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index dd4ff80cc..538f75056 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/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() From d97e8fe172d272d1718109234c7a4f24def7f871 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 24 Feb 2021 15:05:56 +0100 Subject: [PATCH 2/3] Add live tests for some methods in the `git` package We relied too much on stubs for these methods. These new tests actually invoke `git` commands in the context of a test repository. --- git/fixtures/.gitignore | 1 + git/fixtures/simple.git/HEAD | 1 + git/fixtures/simple.git/config | 9 +++++ git/fixtures/simple.git/index | Bin 0 -> 65 bytes git/fixtures/simple.git/logs/HEAD | 2 + git/fixtures/simple.git/logs/refs/heads/main | 2 + .../4b/825dc642cb6eb9a060e54bf8d69288fbee4904 | Bin 0 -> 15 bytes .../6f/1a2405cace1633d89a79c74c65f22fe78f9659 | Bin 0 -> 191 bytes .../d1/e0abfb7d158ed544a202a6958c62d4fc22e12f | 3 ++ git/fixtures/simple.git/refs/heads/main | 1 + git/git_test.go | 35 ++++++++++++++++++ 11 files changed, 54 insertions(+) create mode 100644 git/fixtures/.gitignore create mode 100644 git/fixtures/simple.git/HEAD create mode 100644 git/fixtures/simple.git/config create mode 100644 git/fixtures/simple.git/index create mode 100644 git/fixtures/simple.git/logs/HEAD create mode 100644 git/fixtures/simple.git/logs/refs/heads/main create mode 100644 git/fixtures/simple.git/objects/4b/825dc642cb6eb9a060e54bf8d69288fbee4904 create mode 100644 git/fixtures/simple.git/objects/6f/1a2405cace1633d89a79c74c65f22fe78f9659 create mode 100644 git/fixtures/simple.git/objects/d1/e0abfb7d158ed544a202a6958c62d4fc22e12f create mode 100644 git/fixtures/simple.git/refs/heads/main diff --git a/git/fixtures/.gitignore b/git/fixtures/.gitignore new file mode 100644 index 000000000..abae30d02 --- /dev/null +++ b/git/fixtures/.gitignore @@ -0,0 +1 @@ +*.git/COMMIT_EDITMSG diff --git a/git/fixtures/simple.git/HEAD b/git/fixtures/simple.git/HEAD new file mode 100644 index 000000000..b870d8262 --- /dev/null +++ b/git/fixtures/simple.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/main diff --git a/git/fixtures/simple.git/config b/git/fixtures/simple.git/config new file mode 100644 index 000000000..f0858dd73 --- /dev/null +++ b/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 diff --git a/git/fixtures/simple.git/index b/git/fixtures/simple.git/index new file mode 100644 index 0000000000000000000000000000000000000000..65d675154f23ffb2d0196e017d44a5e7017550f5 GIT binary patch literal 65 zcmZ?q402{*U|<4bhL9jvS0E+HV4z^Y<=qr}%;|LA&IJiiy? 1614174263 +0100 commit (initial): Initial commit +d1e0abfb7d158ed544a202a6958c62d4fc22e12f 6f1a2405cace1633d89a79c74c65f22fe78f9659 Mona the Cat 1614174275 +0100 commit: Second commit diff --git a/git/fixtures/simple.git/logs/refs/heads/main b/git/fixtures/simple.git/logs/refs/heads/main new file mode 100644 index 000000000..216887f9e --- /dev/null +++ b/git/fixtures/simple.git/logs/refs/heads/main @@ -0,0 +1,2 @@ +0000000000000000000000000000000000000000 d1e0abfb7d158ed544a202a6958c62d4fc22e12f Mona the Cat 1614174263 +0100 commit (initial): Initial commit +d1e0abfb7d158ed544a202a6958c62d4fc22e12f 6f1a2405cace1633d89a79c74c65f22fe78f9659 Mona the Cat 1614174275 +0100 commit: Second commit diff --git a/git/fixtures/simple.git/objects/4b/825dc642cb6eb9a060e54bf8d69288fbee4904 b/git/fixtures/simple.git/objects/4b/825dc642cb6eb9a060e54bf8d69288fbee4904 new file mode 100644 index 0000000000000000000000000000000000000000..adf64119a33d7621aeeaa505d30adb58afaa5559 GIT binary patch literal 15 Wcmb zVTQ@QwM_vh`=W;kP>SeF4um-cNi*AE#Z#)Wgc)P3NrYxg=7$g26^awfsivtoAEkIA zMvEL~A9KJ$H6x0{YWSgRKj7MT23-ZdSmC`5x^E|cE}O28^p<=302ds&iE#38vCdjE t-0@N6e{FM<-1h>1E5>}kHaL|J-S!2v!y@`TwDRCyhaSOcegWSFR-)K;Tv-4B literal 0 HcmV?d00001 diff --git a/git/fixtures/simple.git/objects/d1/e0abfb7d158ed544a202a6958c62d4fc22e12f b/git/fixtures/simple.git/objects/d1/e0abfb7d158ed544a202a6958c62d4fc22e12f new file mode 100644 index 000000000..ec3ada617 --- /dev/null +++ b/git/fixtures/simple.git/objects/d1/e0abfb7d158ed544a202a6958c62d4fc22e12f @@ -0,0 +1,3 @@ +xNK +0tS ILc +"+@M뢷7"^@ bZxFhb轴;lKr3<33Kc#-"k8Z.2d=^*)ES&iqɏiϋP jAy 3*H/ \ No newline at end of file diff --git a/git/fixtures/simple.git/refs/heads/main b/git/fixtures/simple.git/refs/heads/main new file mode 100644 index 000000000..8316cdaf5 --- /dev/null +++ b/git/fixtures/simple.git/refs/heads/main @@ -0,0 +1 @@ +6f1a2405cace1633d89a79c74c65f22fe78f9659 diff --git a/git/git_test.go b/git/git_test.go index 899ca3b49..1cd816fa3 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -1,12 +1,47 @@ 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) + } +} + func Test_UncommittedChangeCount(t *testing.T) { type c struct { Label string From 492f45422ecd796416acaaaaac2e5a37ae43dbc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 26 Feb 2021 13:07:38 +0100 Subject: [PATCH 3/3] Add a note about the style of git tests --- git/git_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/git/git_test.go b/git/git_test.go index 1cd816fa3..685b4d1c0 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -42,6 +42,12 @@ func TestCommitBody(t *testing.T) { } } +/* + 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