From 9d586b6936c11f963a4c1d2012e10fe6815d37bd Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 28 Jun 2021 21:59:41 +0100 Subject: [PATCH 1/4] Fix modified files list in webhooks when there is a space There is an unfortunate bug with GetCommitFileStatus where files with spaces are misparsed and split at the space. There is a second bug because modern gits detect renames meaning that this function no longer works correctly. There is a third bug in that merge commits don't have their modified files detected correctly. Fix #15865 Signed-off-by: Andrew Thornton --- modules/git/commit.go | 50 ++++++++++++++++++++++++++++++++----------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index f4d6075fe2d2..19bd1f18ba33 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -15,6 +15,8 @@ import ( "os/exec" "strconv" "strings" + + "code.gitea.io/gitea/modules/log" ) // Commit represents a git commit. @@ -438,27 +440,51 @@ func GetCommitFileStatus(repoPath, commitID string) (*CommitFileStatus, error) { done := make(chan struct{}) fileStatus := NewCommitFileStatus() go func() { - scanner := bufio.NewScanner(stdout) - for scanner.Scan() { - fields := strings.Fields(scanner.Text()) - if len(fields) < 2 { - continue + rd := bufio.NewReader(stdout) + peek, err := rd.Peek(1) + if err != nil { + if err != io.EOF { + log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) } - - switch fields[0][0] { + close(done) + return + } + if peek[0] == '\n' || peek[0] == '\x00' { + _, _ = rd.Discard(1) + } + for { + modifier, err := rd.ReadSlice('\x00') + if err != nil { + if err != io.EOF { + log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) + } + close(done) + return + } + file, err := rd.ReadString('\x00') + if err != nil { + if err != io.EOF { + log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) + } + close(done) + return + } + file = file[:len(file)-1] + switch modifier[0] { case 'A': - fileStatus.Added = append(fileStatus.Added, fields[1]) + fileStatus.Added = append(fileStatus.Added, file) case 'D': - fileStatus.Removed = append(fileStatus.Removed, fields[1]) + fileStatus.Removed = append(fileStatus.Removed, file) case 'M': - fileStatus.Modified = append(fileStatus.Modified, fields[1]) + fileStatus.Modified = append(fileStatus.Modified, file) } } - done <- struct{}{} }() stderr := new(bytes.Buffer) - err := NewCommand("show", "--name-status", "--pretty=format:''", commitID).RunInDirPipeline(repoPath, w, stderr) + args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-t", "-z", "-1", commitID} + + err := NewCommand(args...).RunInDirPipeline(repoPath, w, stderr) w.Close() // Close writer to exit parsing goroutine if err != nil { return nil, ConcatenateError(err, stderr.String()) From 2e41a83c4bd60c973e3df965e90b48b47ece11b2 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 29 Jun 2021 19:50:19 +0100 Subject: [PATCH 2/4] make unit-testable Signed-off-by: Andrew Thornton --- modules/git/commit.go | 76 ++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 19bd1f18ba33..893226c8a2cc 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -434,51 +434,53 @@ func NewCommitFileStatus() *CommitFileStatus { } } -// GetCommitFileStatus returns file status of commit in given repository. -func GetCommitFileStatus(repoPath, commitID string) (*CommitFileStatus, error) { - stdout, w := io.Pipe() - done := make(chan struct{}) - fileStatus := NewCommitFileStatus() - go func() { - rd := bufio.NewReader(stdout) - peek, err := rd.Peek(1) +func parseCommitFileStatus(fileStatus *CommitFileStatus, stdout io.Reader) { + rd := bufio.NewReader(stdout) + peek, err := rd.Peek(1) + if err != nil { + if err != io.EOF { + log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) + } + return + } + if peek[0] == '\n' || peek[0] == '\x00' { + _, _ = rd.Discard(1) + } + for { + modifier, err := rd.ReadSlice('\x00') if err != nil { if err != io.EOF { log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) } - close(done) return } - if peek[0] == '\n' || peek[0] == '\x00' { - _, _ = rd.Discard(1) - } - for { - modifier, err := rd.ReadSlice('\x00') - if err != nil { - if err != io.EOF { - log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) - } - close(done) - return - } - file, err := rd.ReadString('\x00') - if err != nil { - if err != io.EOF { - log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) - } - close(done) - return - } - file = file[:len(file)-1] - switch modifier[0] { - case 'A': - fileStatus.Added = append(fileStatus.Added, file) - case 'D': - fileStatus.Removed = append(fileStatus.Removed, file) - case 'M': - fileStatus.Modified = append(fileStatus.Modified, file) + file, err := rd.ReadString('\x00') + if err != nil { + if err != io.EOF { + log.Error("Unexpected error whilst reading from git log --name-status. Error: %v", err) } + return } + file = file[:len(file)-1] + switch modifier[0] { + case 'A': + fileStatus.Added = append(fileStatus.Added, file) + case 'D': + fileStatus.Removed = append(fileStatus.Removed, file) + case 'M': + fileStatus.Modified = append(fileStatus.Modified, file) + } + } +} + +// GetCommitFileStatus returns file status of commit in given repository. +func GetCommitFileStatus(repoPath, commitID string) (*CommitFileStatus, error) { + stdout, w := io.Pipe() + done := make(chan struct{}) + fileStatus := NewCommitFileStatus() + go func() { + parseCommitFileStatus(fileStatus, stdout) + close(done) }() stderr := new(bytes.Buffer) From f0eb6b2fabece8ce0a325e1a883907aed10cc474 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 29 Jun 2021 19:50:58 +0100 Subject: [PATCH 3/4] we don't need trees Signed-off-by: Andrew Thornton --- modules/git/commit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 893226c8a2cc..3ce2b03886d1 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -484,7 +484,7 @@ func GetCommitFileStatus(repoPath, commitID string) (*CommitFileStatus, error) { }() stderr := new(bytes.Buffer) - args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-t", "-z", "-1", commitID} + args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1", commitID} err := NewCommand(args...).RunInDirPipeline(repoPath, w, stderr) w.Close() // Close writer to exit parsing goroutine From 24c65c565f9811553e2671b93131ea5b74728292 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 29 Jun 2021 20:07:57 +0100 Subject: [PATCH 4/4] add parsing unit test Signed-off-by: Andrew Thornton --- modules/git/commit_test.go | 106 +++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 0925a6ce6ac1..57132c00dc69 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -130,3 +130,109 @@ func TestHasPreviousCommit(t *testing.T) { assert.NoError(t, err) assert.False(t, selfNot) } + +func TestParseCommitFileStatus(t *testing.T) { + type testcase struct { + output string + added []string + removed []string + modified []string + } + + kases := []testcase{ + { + // Merge commit + output: "MM\x00options/locale/locale_en-US.ini\x00", + modified: []string{ + "options/locale/locale_en-US.ini", + }, + added: []string{}, + removed: []string{}, + }, + { + // Spaces commit + output: "D\x00b\x00D\x00b b/b\x00A\x00b b/b b/b b/b\x00A\x00b b/b b/b b/b b/b\x00", + removed: []string{ + "b", + "b b/b", + }, + modified: []string{}, + added: []string{ + "b b/b b/b b/b", + "b b/b b/b b/b b/b", + }, + }, + { + // larger commit + output: "M\x00go.mod\x00M\x00go.sum\x00M\x00modules/ssh/ssh.go\x00M\x00vendor/github.com/gliderlabs/ssh/circle.yml\x00M\x00vendor/github.com/gliderlabs/ssh/context.go\x00A\x00vendor/github.com/gliderlabs/ssh/go.mod\x00A\x00vendor/github.com/gliderlabs/ssh/go.sum\x00M\x00vendor/github.com/gliderlabs/ssh/server.go\x00M\x00vendor/github.com/gliderlabs/ssh/session.go\x00M\x00vendor/github.com/gliderlabs/ssh/ssh.go\x00M\x00vendor/golang.org/x/sys/unix/mkerrors.sh\x00M\x00vendor/golang.org/x/sys/unix/syscall_darwin.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_darwin_amd64.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_darwin_arm64.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_freebsd_386.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_freebsd_amd64.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_freebsd_arm.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_freebsd_arm64.go\x00M\x00vendor/golang.org/x/sys/unix/zerrors_linux.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_darwin_amd64.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_darwin_arm64.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_dragonfly_amd64.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_freebsd_386.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_freebsd_amd64.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_freebsd_arm.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_freebsd_arm64.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_netbsd_386.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_netbsd_amd64.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_netbsd_arm.go\x00M\x00vendor/golang.org/x/sys/unix/ztypes_netbsd_arm64.go\x00M\x00vendor/modules.txt\x00", + modified: []string{ + "go.mod", + "go.sum", + "modules/ssh/ssh.go", + "vendor/github.com/gliderlabs/ssh/circle.yml", + "vendor/github.com/gliderlabs/ssh/context.go", + "vendor/github.com/gliderlabs/ssh/server.go", + "vendor/github.com/gliderlabs/ssh/session.go", + "vendor/github.com/gliderlabs/ssh/ssh.go", + "vendor/golang.org/x/sys/unix/mkerrors.sh", + "vendor/golang.org/x/sys/unix/syscall_darwin.go", + "vendor/golang.org/x/sys/unix/zerrors_darwin_amd64.go", + "vendor/golang.org/x/sys/unix/zerrors_darwin_arm64.go", + "vendor/golang.org/x/sys/unix/zerrors_freebsd_386.go", + "vendor/golang.org/x/sys/unix/zerrors_freebsd_amd64.go", + "vendor/golang.org/x/sys/unix/zerrors_freebsd_arm.go", + "vendor/golang.org/x/sys/unix/zerrors_freebsd_arm64.go", + "vendor/golang.org/x/sys/unix/zerrors_linux.go", + "vendor/golang.org/x/sys/unix/ztypes_darwin_amd64.go", + "vendor/golang.org/x/sys/unix/ztypes_darwin_arm64.go", + "vendor/golang.org/x/sys/unix/ztypes_dragonfly_amd64.go", + "vendor/golang.org/x/sys/unix/ztypes_freebsd_386.go", + "vendor/golang.org/x/sys/unix/ztypes_freebsd_amd64.go", + "vendor/golang.org/x/sys/unix/ztypes_freebsd_arm.go", + "vendor/golang.org/x/sys/unix/ztypes_freebsd_arm64.go", + "vendor/golang.org/x/sys/unix/ztypes_netbsd_386.go", + "vendor/golang.org/x/sys/unix/ztypes_netbsd_amd64.go", + "vendor/golang.org/x/sys/unix/ztypes_netbsd_arm.go", + "vendor/golang.org/x/sys/unix/ztypes_netbsd_arm64.go", + "vendor/modules.txt", + }, + added: []string{ + "vendor/github.com/gliderlabs/ssh/go.mod", + "vendor/github.com/gliderlabs/ssh/go.sum", + }, + removed: []string{}, + }, + { + // git 1.7.2 adds an unnecessary \x00 on merge commit + output: "\x00MM\x00options/locale/locale_en-US.ini\x00", + modified: []string{ + "options/locale/locale_en-US.ini", + }, + added: []string{}, + removed: []string{}, + }, + { + // git 1.7.2 adds an unnecessary \n on normal commit + output: "\nD\x00b\x00D\x00b b/b\x00A\x00b b/b b/b b/b\x00A\x00b b/b b/b b/b b/b\x00", + removed: []string{ + "b", + "b b/b", + }, + modified: []string{}, + added: []string{ + "b b/b b/b b/b", + "b b/b b/b b/b b/b", + }, + }, + } + + for _, kase := range kases { + fileStatus := NewCommitFileStatus() + parseCommitFileStatus(fileStatus, strings.NewReader(kase.output)) + + assert.Equal(t, kase.added, fileStatus.Added) + assert.Equal(t, kase.removed, fileStatus.Removed) + assert.Equal(t, kase.modified, fileStatus.Modified) + } + +}