fix(git): preserve pathnames in command output

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",
     ],
 )

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 <ref>`.
 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.

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,15 +162,15 @@ 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"},
 			},
 		},
 		{
 			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,21 +229,20 @@ 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},
 			},
 		},
 		{
 			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)
+}