STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #8768 from cli/wm-af/fix-pr-fill

Browse source 
Fix regression around commas in commit titles during `pr create`
This commit is contained in:
William Martin 2024-02-28 13:53:29 +01:00 • committed by GitHub
commit d72752830c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 250 additions and 76 deletions
Download patch file Download diff file Expand all files Collapse all files

84
git/client.go
View file

@ -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
}

218
git/client_test.go
View file

@ -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",

24
pkg/cmd/pr/create/create_test.go
View file

@ -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{}) {