From 62c278e4ab66090f48b7937717b28a657cab8688 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 2 Jul 2021 20:23:37 +0100 Subject: [PATCH] Fix modified files list in webhooks when there is a space (#16288) * 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 | 64 +++++++++++++++------- modules/git/commit_test.go | 106 +++++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 18 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index f4d6075fe..3ce2b0388 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. @@ -432,33 +434,59 @@ func NewCommitFileStatus() *CommitFileStatus { } } +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) + } + 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) + } + 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() { - scanner := bufio.NewScanner(stdout) - for scanner.Scan() { - fields := strings.Fields(scanner.Text()) - if len(fields) < 2 { - continue - } - - switch fields[0][0] { - case 'A': - fileStatus.Added = append(fileStatus.Added, fields[1]) - case 'D': - fileStatus.Removed = append(fileStatus.Removed, fields[1]) - case 'M': - fileStatus.Modified = append(fileStatus.Modified, fields[1]) - } - } - done <- struct{}{} + parseCommitFileStatus(fileStatus, stdout) + close(done) }() 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", "-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()) diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 0925a6ce6..57132c00d 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) + } + +}