diff --git a/core/git/BUILD.bazel b/core/git/BUILD.bazel index 107b1e05..27d3c7df 100644 --- a/core/git/BUILD.bazel +++ b/core/git/BUILD.bazel @@ -18,6 +18,7 @@ go_test( deps = [ "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@net_pgregory_rapid//:rapid", "@org_uber_go_zap//:zap", ], ) diff --git a/core/git/git.go b/core/git/git.go index 043f8a37..0fdbd0bd 100644 --- a/core/git/git.go +++ b/core/git/git.go @@ -15,7 +15,6 @@ package git import ( - "bufio" "bytes" "context" "encoding/hex" @@ -162,24 +161,29 @@ func (c *impl) SubmoduleUpdate(ctx context.Context) error { // DiffWithStatus returns the list of changed files with their status between two refs, // parsed from `git diff --name-status`. For renames, Path is the destination path. func (c *impl) DiffWithStatus(ctx context.Context, baseRef, targetRef string) ([]DiffEntry, error) { - out, err := c.Diff(ctx, baseRef, targetRef, "--name-status") + out, err := c.Diff(ctx, baseRef, targetRef, "--name-status", "-z") if err != nil { return nil, err } var entries []DiffEntry - for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { - if line == "" { + fields := bytes.Split(out, []byte{0}) + for i := 0; i < len(fields); { + if len(fields[i]) == 0 { + i++ continue } - fields := strings.Fields(line) - if len(fields) < 2 { - continue + status := string(fields[i]) + i++ + pathCount := 1 + if status[0] == 'R' || status[0] == 'C' { + pathCount = 2 + } + if i+pathCount > len(fields) { + break } - // Status may have a score suffix (e.g. "R100") — use only the first character. - status := string(fields[0][0]) - // For renames/copies there are two paths; use the destination (last field). - path := fields[len(fields)-1] - entries = append(entries, DiffEntry{Status: status, Path: path}) + path := string(fields[i+pathCount-1]) + i += pathCount + entries = append(entries, DiffEntry{Status: status[:1], Path: path}) } return entries, nil } @@ -195,33 +199,34 @@ func (c *impl) GetCommitTimeSecond(ctx context.Context, ref string) (int64, erro // FileHashes gets a mapping of files to their hashes based on `git ls-tree --full-tree -r `. func (c *impl) FileHashes(ctx context.Context, ref string) (map[string][]byte, error) { - out, err := c.runner.output(ctx, c.directory, "git", "ls-tree", "--full-tree", "-r", ref) + out, err := c.runner.output(ctx, c.directory, "git", "ls-tree", "--full-tree", "-r", "-z", ref) if err != nil { return nil, err } fileHashes := make(map[string][]byte) - scanner := bufio.NewScanner(bytes.NewReader(out)) - // git ls-tree --full-tree output format: - // 100644 blob d236089b460070849370c1c813874ae9bea598a8 file1 - // 100644 blob 9bccabefa0c7a4d68b219e2b62d6d3cc5271cf44 file2 - // 100644 blob a14cddefd9112ee21e58f6c78ad224dc44a64297 file3 - for scanner.Scan() { - line := scanner.Text() - x := strings.Split(line, "\t") - parts := strings.Fields(x[0]) // strings.Split is guaranteed to return an array of length 1 - if len(parts) < 3 || len(x) < 2 { - c.logger.Warnw("skipping ls-tree line due to unexpected format", "line", line) + for _, record := range bytes.Split(out, []byte{0}) { + if len(record) == 0 { + continue + } + fields := bytes.SplitN(record, []byte{'\t'}, 2) + if len(fields) != 2 { + c.logger.Warnw("skipping ls-tree record due to unexpected format", "record", string(record)) + continue + } + metadata := strings.Fields(string(fields[0])) + if len(metadata) < 3 { + c.logger.Warnw("skipping ls-tree record due to unexpected metadata", "record", string(record)) continue } - hash, err := hex.DecodeString(parts[2]) + hash, err := hex.DecodeString(metadata[2]) if err != nil { - c.logger.Warnw("skipping ls-tree line due to parsing error", "line", line, zap.Error(err)) + c.logger.Warnw("skipping ls-tree record due to parsing error", "record", string(record), zap.Error(err)) continue } - fileHashes[x[1]] = hash + fileHashes[string(fields[1])] = hash } - return fileHashes, scanner.Err() + return fileHashes, nil } // commandRunner abstracts command execution for testability. diff --git a/core/git/git_test.go b/core/git/git_test.go index 5514593d..4ecc5a93 100644 --- a/core/git/git_test.go +++ b/core/git/git_test.go @@ -15,14 +15,19 @@ package git import ( + "bytes" "context" "errors" + "os" + "os/exec" + "path/filepath" "reflect" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" + "pgregory.net/rapid" ) type runnerCall struct { @@ -157,7 +162,7 @@ func TestDiffWithStatus_parsesNameStatusOutput(t *testing.T) { }{ { name: "modified and added files", - output: []byte("M\tsrc/foo.go\nA\tsrc/bar.go\n"), + output: []byte("M\x00src/foo.go\x00A\x00src/bar.go\x00"), want: []DiffEntry{ {Status: "M", Path: "src/foo.go"}, {Status: "A", Path: "src/bar.go"}, @@ -165,7 +170,7 @@ func TestDiffWithStatus_parsesNameStatusOutput(t *testing.T) { }, { name: "rename uses destination path", - output: []byte("R100\told/path.go\tnew/path.go\n"), + output: []byte("R100\x00old/path.go\x00new/path.go\x00"), want: []DiffEntry{{Status: "R", Path: "new/path.go"}}, }, { @@ -224,8 +229,7 @@ func TestDefaultGit_FileHashes(t *testing.T) { { name: "happy case", giveOutput: []byte( - `100644 blob d236 file1 -100644 blob 9bcc file2`), + "100644 blob d236\tfile1\x00100644 blob 9bcc\tfile2\x00"), wantHashes: map[string][]byte{ "file1": []byte{0xd2, 0x36}, "file2": []byte{0x9b, 0xcc}, @@ -233,12 +237,12 @@ func TestDefaultGit_FileHashes(t *testing.T) { }, { name: "ignore bad format", - giveOutput: []byte("100644 blob d236 file1"), + giveOutput: []byte("100644 blob d236 file1\x00"), wantHashes: map[string][]byte{}, }, { name: "ignore bad hex", - giveOutput: []byte("100644 blob not_a_hex file1"), + giveOutput: []byte("100644 blob not_a_hex\tfile1\x00"), wantHashes: map[string][]byte{}, }, { @@ -264,3 +268,108 @@ func TestDefaultGit_FileHashes(t *testing.T) { }) } } + +func TestGitPathnames_roundTripAcrossRealGitBoundary(t *testing.T) { + paths := []string{ + "file .txt", + "file\tname.txt", + "file\"name.txt", + "file\\name.txt", + "nested/é雪.txt", + } + repo := t.TempDir() + + runGit(t, repo, "init", "--quiet") + runGit(t, repo, "config", "user.email", "tango-test@example.com") + runGit(t, repo, "config", "user.name", "Tango Test") + runGit(t, repo, "commit", "--quiet", "--allow-empty", "-m", "base") + for _, path := range paths { + absolutePath := filepath.Join(repo, filepath.FromSlash(path)) + require.NoError(t, os.MkdirAll(filepath.Dir(absolutePath), 0o755)) + require.NoError(t, os.WriteFile(absolutePath, []byte(path), 0o644)) + } + runGit(t, repo, "add", "--all") + runGit(t, repo, "commit", "--quiet", "-m", "paths") + + client := New(repo, zap.NewNop().Sugar()) + diff, err := client.DiffWithStatus(t.Context(), "HEAD^", "HEAD") + require.NoError(t, err) + diffPaths := make(map[string]string, len(diff)) + for _, entry := range diff { + diffPaths[entry.Path] = entry.Status + } + + hashes, err := client.FileHashes(t.Context(), "HEAD") + require.NoError(t, err) + hashPaths := make(map[string]struct{}, len(hashes)) + for path := range hashes { + hashPaths[path] = struct{}{} + } + + wantDiffPaths := make(map[string]string, len(paths)) + wantHashPaths := make(map[string]struct{}, len(paths)) + for _, path := range paths { + wantDiffPaths[path] = "A" + wantHashPaths[path] = struct{}{} + } + require.Equal(t, wantDiffPaths, diffPaths, "DiffWithStatus must preserve exact paths") + require.Equal(t, wantHashPaths, hashPaths, "FileHashes must preserve exact paths") +} + +func TestDiffWithStatus_preservesGeneratedPathnames(t *testing.T) { + rapid.Check(t, func(t *rapid.T) { + paths := rapid.SliceOfNDistinct( + legalGitPath(), + 1, + 4, + func(path string) string { return path }, + ).Draw(t, "paths") + + var output bytes.Buffer + for _, path := range paths { + output.WriteString("A") + output.WriteByte(0) + output.WriteString(path) + output.WriteByte(0) + } + client := &impl{directory: "/repo", runner: &mockRunner{out: output.Bytes()}} + diff, err := client.DiffWithStatus(t.Context(), "base", "head") + require.NoError(t, err) + diffPaths := make(map[string]string, len(diff)) + for _, entry := range diff { + diffPaths[entry.Path] = entry.Status + } + + wantDiffPaths := make(map[string]string, len(paths)) + for _, path := range paths { + wantDiffPaths[path] = "A" + } + require.Equal(t, wantDiffPaths, diffPaths, "DiffWithStatus must preserve exact paths") + }) +} + +func legalGitPath() *rapid.Generator[string] { + return rapid.Custom(func(t *rapid.T) string { + depth := rapid.SampledFrom([]int{0, 1, 1, 2, 2}).Draw(t, "depth") + components := make([]string, 0, depth+1) + for i := range depth { + components = append(components, "dir"+legalGitPathToken(t, "directory", i)) + } + components = append(components, "file"+legalGitPathToken(t, "file", depth)+".txt") + return filepath.ToSlash(filepath.Join(components...)) + }) +} + +func legalGitPathToken(t *rapid.T, kind string, index int) string { + return rapid.SampledFrom([]string{ + " ", " ", "\t", "\"", "'", "\\", "é", "雪", + }).Draw(t, kind+string(rune('0'+index))) +} + +func runGit(t *testing.T, directory string, args ...string) { + t.Helper() + cmd := exec.CommandContext(t.Context(), "git", args...) + cmd.Dir = directory + output, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, output) +}