From 0541d286c1421ddb950b73c223111711491d68ce Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 27 Feb 2024 14:41:45 +0100 Subject: [PATCH 1/6] Extract new test helper for git client Commits --- git/client_test.go | 122 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 22 deletions(-) diff --git a/git/client_test.go b/git/client_test.go index a1183b08d..27584ad08 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,19 +460,39 @@ 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", + name: "get commits", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{ + { + Sha: "6a6872b918c601a0e730710ad8473938a7516d30", + Title: "testing testability test", + Body: "", + }, + }, + }, wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, wantCommits: []*Commit{{ Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -478,8 +500,16 @@ func TestClientCommits(t *testing.T) { }}, }, { - name: "get commits with body", - cmdStdout: "6a6872b918c601a0e730710ad8473938a7516d30,testing testability test,This is the body", + name: "get commits 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,%s,%b --cherry SHA1...SHA2`, wantCommits: []*Commit{{ Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -488,37 +518,85 @@ func TestClientCommits(t *testing.T) { }}, }, { - name: "no commits between SHAs", + name: "no commits between SHAs", + testData: stubbedCommitsCommandData{ + Commits: []stubbedCommit{}, + }, wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --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,%s,%b --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(",") + sb.WriteString(commit.Title) + sb.WriteString(",") + sb.WriteString(commit.Body) + } + 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", From 2af0c899d870853f20e6627ccc622ea6d27f86ac Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 27 Feb 2024 17:26:51 +0100 Subject: [PATCH 2/6] Remove unnecessary split conditional from client Commits Since this is guarded by the line starting with a git sha, it must be a line of the form 'sha,title,body', so there can never be only two entries since the Split fn will produce an empty string in the last spot in the case of a missing body. --- git/client.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/git/client.go b/git/client.go index 21adc8bb1..6db7a92f9 100644 --- a/git/client.go +++ b/git/client.go @@ -256,11 +256,6 @@ func (c *Client) Commits(ctx context.Context, baseRef, headRef string) ([]*Commi Title: split[title], } - if len(split) == 2 { - commits = append(commits, c) - continue - } - c.Body = split[body] // This consumes all lines until the next commit and adds them to the body. for { From fb353caf194b0b3340fa83eecd72b606e4859ea4 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 27 Feb 2024 17:33:17 +0100 Subject: [PATCH 3/6] Add new tests to git client Commits This adds coverage for mutliple commits and commits with newlines in bodies. --- git/client_test.go | 93 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 2 deletions(-) diff --git a/git/client_test.go b/git/client_test.go index 27584ad08..df44fac9d 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -483,7 +483,7 @@ func TestClientCommits(t *testing.T) { wantErrorMsg string }{ { - name: "get commits", + name: "single commit no body", testData: stubbedCommitsCommandData{ Commits: []stubbedCommit{ { @@ -500,7 +500,7 @@ func TestClientCommits(t *testing.T) { }}, }, { - name: "get commits with body", + name: "single commit with body", testData: stubbedCommitsCommandData{ Commits: []stubbedCommit{ { @@ -517,6 +517,94 @@ func TestClientCommits(t *testing.T) { Body: "This is the body", }}, }, + { + 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,%s,%b --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,%s,%b --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,%s,%b --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{ @@ -572,6 +660,7 @@ func TestCommitsHelperProcess(t *testing.T) { sb.WriteString(commit.Title) sb.WriteString(",") sb.WriteString(commit.Body) + sb.WriteString("\n") } fmt.Fprint(os.Stdout, sb.String()) } From 2b0484f5aa5bd393e769f78bde15051d3a430b27 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 27 Feb 2024 17:38:57 +0100 Subject: [PATCH 4/6] Use null byte separators when fetching comments from git --- git/client.go | 11 ++++++++--- git/client_test.go | 18 +++++++++--------- pkg/cmd/pr/create/create_test.go | 24 ++++++++++++------------ 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/git/client.go b/git/client.go index 6db7a92f9..308491e1f 100644 --- a/git/client.go +++ b/git/client.go @@ -234,7 +234,12 @@ func isGitSha(s string) bool { } 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", "--cherry", fmt.Sprintf("%s...%s", baseRef, headRef)} cmd, err := c.Command(ctx, args...) if err != nil { return nil, err @@ -249,7 +254,7 @@ func (c *Client) Commits(ctx context.Context, baseRef, headRef string) ([]*Commi body := 2 lines := outputLines(out) for i := 0; i < len(lines); i++ { - split := strings.SplitN(lines[i], ",", 3) + split := strings.SplitN(lines[i], "\u0000", 3) if isGitSha(split[sha]) { c := &Commit{ Sha: split[sha], @@ -264,7 +269,7 @@ func (c *Client) Commits(ctx context.Context, baseRef, headRef string) ([]*Commi break } - possibleSplit := strings.SplitN(lines[i], ",", 3) + possibleSplit := strings.SplitN(lines[i], "\u0000", 3) if len(possibleSplit) > 2 && isGitSha(possibleSplit[sha]) { i-- break diff --git a/git/client_test.go b/git/client_test.go index df44fac9d..a8ac173fe 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -493,7 +493,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantCommits: []*Commit{{ Sha: "6a6872b918c601a0e730710ad8473938a7516d30", Title: "testing testability test", @@ -510,7 +510,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantCommits: []*Commit{{ Sha: "6a6872b918c601a0e730710ad8473938a7516d30", Title: "testing testability test", @@ -533,7 +533,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantCommits: []*Commit{ { Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -562,7 +562,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantCommits: []*Commit{ { Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -591,7 +591,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantCommits: []*Commit{ { Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -610,7 +610,7 @@ func TestClientCommits(t *testing.T) { testData: stubbedCommitsCommandData{ Commits: []stubbedCommit{}, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantErrorMsg: "could not find any commits between SHA1 and SHA2", }, { @@ -619,7 +619,7 @@ func TestClientCommits(t *testing.T) { ErrMsg: "git error message", ExitStatus: 1, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H,%s,%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, wantErrorMsg: "failed to run git: git error message", }, } @@ -656,9 +656,9 @@ func TestCommitsHelperProcess(t *testing.T) { var sb strings.Builder for _, commit := range td.Commits { sb.WriteString(commit.Sha) - sb.WriteString(",") + sb.WriteString("\u0000") sb.WriteString(commit.Title) - sb.WriteString(",") + sb.WriteString("\u0000") sb.WriteString(commit.Body) sb.WriteString("\n") } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index a8baa4883..c9906b7ec 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\n7a6ea13\u0000commit 1\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 --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 --cherry origin/master...feature", 0, - "3a9b48085046d156c5acce8f3b3a0532cd706a4a,first commit of pr,first commit description\n", + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\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 --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\n"+ + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit description\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 --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\n"+ + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first commit with super long description, with super long description, with super long description, with super long description.\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{}) { From 2ee6737053591c49ce6a16deb6f30ed0d704e676 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 27 Feb 2024 20:44:04 +0100 Subject: [PATCH 5/6] Use regex to split apart the git client Commit logs Theoretically this should be clearer and more robust than the previous version which had some custom loop logic while trying to parse newlines and determine whether it had reached a new commit entry by trying to parse a git sha. This would not have worked correctly if a commit body contained a sha on a new line. --- git/client.go | 76 +++++++++++++++++--------------- git/client_test.go | 15 ++++--- pkg/cmd/pr/create/create_test.go | 20 ++++----- 3 files changed, 59 insertions(+), 52 deletions(-) diff --git a/git/client.go b/git/client.go index 308491e1f..491981fae 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: +// \u00006a6872b918c601a0e730710ad8473938a7516d30\u0000title 1\u0000Body 1\u0000\n +// \u00007a6872b918c601a0e730710ad8473938a7516d31\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,19 +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) { // 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", "--cherry", fmt.Sprintf("%s...%s", baseRef, headRef)} + 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 @@ -248,41 +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], "\u0000", 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 - 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 - } + // Or with an optional body: + // 6a6872b918c601a0e730710ad8473938a7516d30\u0000title 1\u0000\u0000\n - possibleSplit := strings.SplitN(lines[i], "\u0000", 3) - if len(possibleSplit) > 2 && isGitSha(possibleSplit[sha]) { - i-- - break - } - c.Body += "\n" - c.Body += lines[i] - } - commits = append(commits, c) - } + // Therefore after splitting we will have: + // ["6a6872b918c601a0e730710ad8473938a7516d30", "title 1", "Body 1", ""] + + // 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 a8ac173fe..420683659 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -493,7 +493,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + 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", @@ -510,7 +510,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + 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", @@ -533,7 +533,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, wantCommits: []*Commit{ { Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -562,7 +562,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, wantCommits: []*Commit{ { Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -591,7 +591,7 @@ func TestClientCommits(t *testing.T) { }, }, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry SHA1...SHA2`, wantCommits: []*Commit{ { Sha: "6a6872b918c601a0e730710ad8473938a7516d30", @@ -610,7 +610,7 @@ func TestClientCommits(t *testing.T) { testData: stubbedCommitsCommandData{ Commits: []stubbedCommit{}, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + 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", }, { @@ -619,7 +619,7 @@ func TestClientCommits(t *testing.T) { ErrMsg: "git error message", ExitStatus: 1, }, - wantCmdArgs: `path/to/git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b --cherry SHA1...SHA2`, + 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", }, } @@ -660,6 +660,7 @@ func TestCommitsHelperProcess(t *testing.T) { sb.WriteString(commit.Title) sb.WriteString("\u0000") sb.WriteString(commit.Body) + sb.WriteString("\u0000") sb.WriteString("\n") } fmt.Fprint(os.Stdout, sb.String()) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index c9906b7ec..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\u0000commit 0\u0000\n7a6ea13\u0000commit 1\u0000") + 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%x00%s%x00%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%x00%s%x00%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\u0000first commit of pr\u0000first 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%x00%s%x00%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\u0000second commit of pr\u0000\n"+ - "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first 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,10 +1115,10 @@ func Test_createRun(t *testing.T) { }, cmdStubs: func(cs *run.CommandStubber) { cs.Register( - "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%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\u0000second commit of pr\u0000second commit description\n"+ - "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000first commit of pr\u0000first 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) { From 5fe6eebfeafda8c411eed69223b9805c73ca08f5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 28 Feb 2024 13:38:39 +0100 Subject: [PATCH 6/6] Update incorrect regex comment for client Commits --- git/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git/client.go b/git/client.go index 491981fae..b0194affc 100644 --- a/git/client.go +++ b/git/client.go @@ -21,8 +21,8 @@ import ( var remoteRE = regexp.MustCompile(`(.+)\s+(.+)\s+\((push|fetch)\)`) // This regexp exists to match lines of the following form: -// \u00006a6872b918c601a0e730710ad8473938a7516d30\u0000title 1\u0000Body 1\u0000\n -// \u00007a6872b918c601a0e730710ad8473938a7516d31\u0000title 2\u0000Body 2\u0000 +// 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