diff --git a/git/client.go b/git/client.go index 21adc8bb1..b0194affc 100644 --- a/git/client.go +++ b/git/client.go @@ -20,6 +20,26 @@ import ( var remoteRE = regexp.MustCompile(`(.+)\s+(.+)\s+\((push|fetch)\)`) +// This regexp exists to match lines of the following form: +// 6a6872b918c601a0e730710ad8473938a7516d30\u0000title 1\u0000Body 1\u0000\n +// 7a6872b918c601a0e730710ad8473938a7516d31\u0000title 2\u0000Body 2\u0000 +// +// This is the format we use when collecting commit information, +// with null bytes as separators. Using null bytes this way allows for us +// to easily maintain newlines that might be in the body. +// +// The ?m modifier is the multi-line modifier, meaning that ^ and $ +// match the beginning and end of lines, respectively. +// +// The [\S\s] matches any whitespace or non-whitespace character, +// which is different from .* because it allows for newlines as well. +// +// The ? following .* and [\S\s] is a lazy modifier, meaning that it will +// match as few characters as possible while still satisfying the rest of the regexp. +// This is important because it allows us to match the first null byte after the title and body, +// rather than the last null byte in the entire string. +var commitLogRE = regexp.MustCompile(`(?m)^[0-9a-fA-F]{7,40}\x00.*?\x00[\S\s]*?\x00$`) + type errWithExitCode interface { ExitCode() int } @@ -227,14 +247,13 @@ func (c *Client) UncommittedChangeCount(ctx context.Context) (int, error) { return count, nil } -func isGitSha(s string) bool { - shaRegex := regexp.MustCompile(`^[0-9a-fA-F]{7,40}$`) - ret := shaRegex.MatchString(s) - return ret -} - func (c *Client) Commits(ctx context.Context, baseRef, headRef string) ([]*Commit, error) { - args := []string{"-c", "log.ShowSignature=false", "log", "--pretty=format:%H,%s,%b", "--cherry", fmt.Sprintf("%s...%s", baseRef, headRef)} + // The formatting directive %x00 indicates that git should include the null byte as a separator. + // We use this because it is not a valid character to include in a commit message. Previously, + // commas were used here but when we Split on them, we would get incorrect results if commit titles + // happened to contain them. + // https://git-scm.com/docs/pretty-formats#Documentation/pretty-formats.txt-emx00em + args := []string{"-c", "log.ShowSignature=false", "log", "--pretty=format:%H%x00%s%x00%b%x00", "--cherry", fmt.Sprintf("%s...%s", baseRef, headRef)} cmd, err := c.Command(ctx, args...) if err != nil { return nil, err @@ -243,46 +262,33 @@ func (c *Client) Commits(ctx context.Context, baseRef, headRef string) ([]*Commi if err != nil { return nil, err } + commits := []*Commit{} - sha := 0 - title := 1 - body := 2 - lines := outputLines(out) - for i := 0; i < len(lines); i++ { - split := strings.SplitN(lines[i], ",", 3) - if isGitSha(split[sha]) { - c := &Commit{ - Sha: split[sha], - Title: split[title], - } + commitLogs := commitLogRE.FindAllString(string(out), -1) + for _, commitLog := range commitLogs { + // Each line looks like this: + // 6a6872b918c601a0e730710ad8473938a7516d30\u0000title 1\u0000Body 1\u0000\n - if len(split) == 2 { - commits = append(commits, c) - continue - } + // Or with an optional body: + // 6a6872b918c601a0e730710ad8473938a7516d30\u0000title 1\u0000\u0000\n - c.Body = split[body] - // This consumes all lines until the next commit and adds them to the body. - for { - i++ - if i >= len(lines) { - break - } + // Therefore after splitting we will have: + // ["6a6872b918c601a0e730710ad8473938a7516d30", "title 1", "Body 1", ""] - possibleSplit := strings.SplitN(lines[i], ",", 3) - if len(possibleSplit) > 2 && isGitSha(possibleSplit[sha]) { - i-- - break - } - c.Body += "\n" - c.Body += lines[i] - } - commits = append(commits, c) - } + // Or with an optional body: + // ["6a6872b918c601a0e730710ad8473938a7516d30", "title 1", "", ""] + commitLogParts := strings.Split(commitLog, "\u0000") + commits = append(commits, &Commit{ + Sha: commitLogParts[0], + Title: commitLogParts[1], + Body: commitLogParts[2], + }) } + if len(commits) == 0 { return nil, fmt.Errorf("could not find any commits between %s and %s", baseRef, headRef) } + return commits, nil } diff --git a/git/client_test.go b/git/client_test.go index a1183b08d..420683659 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -3,6 +3,7 @@ package git import ( "bytes" "context" + "encoding/json" "errors" "fmt" "os" @@ -13,6 +14,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestClientCommand(t *testing.T) { @@ -458,29 +460,57 @@ func TestClientUncommittedChangeCount(t *testing.T) { } } +type stubbedCommit struct { + Sha string + Title string + Body string +} + +type stubbedCommitsCommandData struct { + ExitStatus int + + ErrMsg string + + Commits []stubbedCommit +} + func TestClientCommits(t *testing.T) { tests := []struct { - name string - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantCommits []*Commit - wantErrorMsg string + name string + testData stubbedCommitsCommandData + wantCmdArgs string + wantCommits []*Commit + wantErrorMsg string }{ { - name: "get commits", - cmdStdout: "6a6872b918c601a0e730710ad8473938a7516d30,testing testability test", - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + name: "single commit no body", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "", + }, + }, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, wantCommits: []*Commit{{ Sha: "6a6872b918c601a0e730710ad8473938a7516d30", Title: "testing testability test", }}, }, { - name: "get commits with body", - cmdStdout: "6a6872b918c601a0e730710ad8473938a7516d30,testing testability test,This is the body", - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + name: "single commit with body", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "This is the body", + }, + }, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, wantCommits: []*Commit{{ Sha: "6a6872b918c601a0e730710ad8473938a7516d30", Title: "testing testability test", @@ -488,37 +518,175 @@ func TestClientCommits(t *testing.T) { }}, }, { - name: "no commits between SHAs", - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + name: "multiple commits with bodies", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "This is the body", + }, + { + Sha: "7a6872b918c601a0e730710ad8473938a7516d31", + Title: "testing testability test 2", + Body: "This is the body 2", + }, + }, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, + wantCommits: []*Commit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "This is the body", + }, + { + Sha: "7a6872b918c601a0e730710ad8473938a7516d31", + Title: "testing testability test 2", + Body: "This is the body 2", + }, + }, + }, + { + name: "multiple commits mixed bodies", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + }, + { + Sha: "7a6872b918c601a0e730710ad8473938a7516d31", + Title: "testing testability test 2", + Body: "This is the body 2", + }, + }, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, + wantCommits: []*Commit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + }, + { + Sha: "7a6872b918c601a0e730710ad8473938a7516d31", + Title: "testing testability test 2", + Body: "This is the body 2", + }, + }, + }, + { + name: "multiple commits newlines in bodies", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "This is the body\nwith a newline", + }, + { + Sha: "7a6872b918c601a0e730710ad8473938a7516d31", + Title: "testing testability test 2", + Body: "This is the body 2", + }, + }, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, + wantCommits: []*Commit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "This is the body\nwith a newline", + }, + { + Sha: "7a6872b918c601a0e730710ad8473938a7516d31", + Title: "testing testability test 2", + Body: "This is the body 2", + }, + }, + }, + { + name: "no commits between SHAs", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{}, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, wantErrorMsg: "could not find any commits between SHA1 and SHA2", }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, - wantErrorMsg: "failed to run git: git error message", + name: "git error", + testData: stubbedCommitsCommandData{ + ErrMsg: "git error message", + ExitStatus: 1, + }, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, + wantErrorMsg: "failed to run git: git error message", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmd, cmdCtx := createCommitsCommandContext(t, tt.testData) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } commits, err := client.Commits(context.Background(), "SHA1", "SHA2") - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) + require.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg != "" { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } else { - assert.NoError(t, err) + require.NoError(t, err) } - assert.Equal(t, tt.wantCommits, commits) + require.Equal(t, tt.wantCommits, commits) }) } } +func TestCommitsHelperProcess(t *testing.T) { + if os.Getenv("GH_WANT_HELPER_PROCESS") != "1" { + return + } + + var td stubbedCommitsCommandData + _ = json.Unmarshal([]byte(os.Getenv("GH_COMMITS_TEST_DATA")), &td) + + if td.ErrMsg != "" { + fmt.Fprint(os.Stderr, td.ErrMsg) + } else { + var sb strings.Builder + for _, commit := range td.Commits { + sb.WriteString(commit.Sha) + sb.WriteString("\u0000") + sb.WriteString(commit.Title) + sb.WriteString("\u0000") + sb.WriteString(commit.Body) + sb.WriteString("\u0000") + sb.WriteString("\n") + } + fmt.Fprint(os.Stdout, sb.String()) + } + + os.Exit(td.ExitStatus) +} + +func createCommitsCommandContext(t *testing.T, testData stubbedCommitsCommandData) (*exec.Cmd, commandCtx) { + t.Helper() + + b, err := json.Marshal(testData) + require.NoError(t, err) + + cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestCommitsHelperProcess", "--") + cmd.Env = []string{ + "GH_WANT_HELPER_PROCESS=1", + "GH_COMMITS_TEST_DATA=" + string(b), + } + return cmd, func(ctx context.Context, exe string, args ...string) *exec.Cmd { + cmd.Args = append(cmd.Args, exe) + cmd.Args = append(cmd.Args, args...) + return cmd + } +} + func TestClientLastCommit(t *testing.T) { client := Client{ RepoDir: "./fixtures/simple.git", diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index a8baa4883..7d981e686 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -627,7 +627,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "d3476a1,commit 0\n7a6ea13,commit 1") + cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "d3476a1\u0000commit 0\u0000\u0000\n7a6ea13\u0000commit 1\u0000\u0000") }, promptStubs: func(pm *prompter.PrompterMock) { pm.MarkdownEditorFunc = func(p, d string, ba bool) (string, error) { @@ -851,7 +851,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry origin/master...feature`, 0, "") + cs.Register(`git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature`, 0, "") cs.Register(`git rev-parse --show-toplevel`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1004,9 +1004,9 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register( - "git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry origin/master...feature", + "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, - "3a9b48085046d156c5acce8f3b3a0532cd706a4a,first commit of pr,first commit description\n", + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n", ) cs.Register(`git rev-parse --show-toplevel`, 0, "") }, @@ -1079,10 +1079,10 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register( - "git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry origin/master...feature", + "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, - "56b6f8bb7c9e3a30093cd17e48934ce354148e80,second commit of pr\n"+ - "3a9b48085046d156c5acce8f3b3a0532cd706a4a,first commit of pr,first commit description\n", + "56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000\u0000\n"+ + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\u0000\n", ) }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { @@ -1115,20 +1115,20 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register( - "git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry origin/master...feature", + "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, - "56b6f8bb7c9e3a30093cd17e48934ce354148e80,second commit of pr,second commit description\n"+ - "3a9b48085046d156c5acce8f3b3a0532cd706a4a,first commit of pr,first commit with super long description, with super long description, with super long description, with super long description.\n", + "56b6f8bb7c9e3a30093cd17e48934ce354148e80\u0000second commit of pr\u0000second commit description\u0000\n"+ + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit with super long description, with super long description, with super long description, with super long description.\u0000\n", ) }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` - { + { "data": { "createPullRequest": { "pullRequest": { "URL": "https://github.com/OWNER/REPO/pull/12" - } } } + } } } } `, func(input map[string]interface{}) {