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 <art27@cantab.net>

diff --git a/modules/git/commit.go b/modules/git/commit.go
index f4d6075fe2d22138576cff788f9c32ba2af11480..3ce2b03886d1f4c0b00710ce98e3c2b4c3cb0d46 100644 (file)
--- 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 0925a6ce6ac1c8b2864bf389805f11d806bad6eb..57132c00dc69ca5c13de4c7817579f0f059ad2da 100644 (file)
--- 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)
+       }
+
+}