diff --git a/internal/pipeline/diff.go b/internal/pipeline/diff.go index ea6c05c..ff0b568 100644 --- a/internal/pipeline/diff.go +++ b/internal/pipeline/diff.go @@ -88,9 +88,9 @@ func parseUnifiedDiff(raw string) (ParsedDiff, error) { case strings.HasPrefix(trimmed, "new file mode"): current.Added = true case strings.HasPrefix(trimmed, "rename from "): - current.OldPath = strings.TrimPrefix(trimmed, "rename from ") + current.OldPath = unquoteGitPath(strings.TrimPrefix(trimmed, "rename from ")) case strings.HasPrefix(trimmed, "rename to "): - current.Path = strings.TrimPrefix(trimmed, "rename to ") + current.Path = unquoteGitPath(strings.TrimPrefix(trimmed, "rename to ")) case strings.HasPrefix(trimmed, "Binary files ") || strings.HasPrefix(trimmed, "GIT binary patch"): current.Binary = true case strings.HasPrefix(trimmed, "@@ "): @@ -118,6 +118,9 @@ func splitLines(raw string) []string { func parseDiffGitPaths(line string) (string, string) { rest := strings.TrimSpace(strings.TrimPrefix(line, "diff --git ")) + if path, ok := sameUnquotedDiffPath(rest); ok { + return path, path + } oldPath, rest, ok := consumeDiffPath(rest) if !ok { return "", "" @@ -129,17 +132,44 @@ func parseDiffGitPaths(line string) (string, string) { return trimDiffPrefix(oldPath), trimDiffPrefix(newPath) } +// sameUnquotedDiffPath reads an unquoted "a/ b/" pair that names +// one file on both sides. Git quotes a path for special characters but not for +// spaces, so splitting at the first space would cut such a path short. Equal +// halves around the middle space are unambiguous; a rename falls through to +// token parsing and takes its paths from the "rename from"/"rename to" lines. +func sameUnquotedDiffPath(rest string) (string, bool) { + if strings.HasPrefix(rest, `"`) || len(rest)%2 == 0 { + return "", false + } + mid := len(rest) / 2 + if rest[mid] != ' ' { + return "", false + } + oldPath, newPath := trimDiffPrefix(rest[:mid]), trimDiffPrefix(rest[mid+1:]) + if oldPath == "" || oldPath != newPath { + return "", false + } + return oldPath, true +} + func parseHeaderPath(raw string) string { value := strings.TrimSpace(raw) if value == "/dev/null" { return "" } + return trimDiffPrefix(unquoteGitPath(value)) +} + +// unquoteGitPath decodes a path git wrote as a C-style quoted string, which it +// does for names holding control characters, quotes, backslashes, or (with the +// default core.quotePath) non-ASCII bytes. +func unquoteGitPath(value string) string { if strings.HasPrefix(value, `"`) { if unquoted, err := strconv.Unquote(value); err == nil { - value = unquoted + return unquoted } } - return trimDiffPrefix(value) + return value } func trimDiffPrefix(path string) string { diff --git a/internal/pipeline/diff_test.go b/internal/pipeline/diff_test.go index 1c48512..7dc836f 100644 --- a/internal/pipeline/diff_test.go +++ b/internal/pipeline/diff_test.go @@ -150,6 +150,117 @@ func TestParseUnifiedDiffQuotedPathsWithSpaces(t *testing.T) { } } +// Git leaves paths with spaces unquoted in "diff --git" headers and appends a +// tab to "---"/"+++" paths that contain a space. These shapes mirror its output. +func TestParseUnifiedDiffUnquotedPathsWithSpaces(t *testing.T) { + raw := strings.Join([]string{ + "diff --git a/data/2024-06-08 - a.json b/data/2024-06-08 - a.json", + "deleted file mode 100644", + "index 1111111..0000000", + "--- a/data/2024-06-08 - a.json\t", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-{}", + "diff --git a/data/2024-06-08 - b.json b/data/2024-06-08 - b.json", + "deleted file mode 100644", + "index 2222222..0000000", + "--- a/data/2024-06-08 - b.json\t", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-{}", + "diff --git a/data/2024-06-09 - c.json b/data/2024-06-09 - c.json", + "new file mode 100644", + "index 0000000..3333333", + "--- /dev/null", + "+++ b/data/2024-06-09 - c.json\t", + "@@ -0,0 +1 @@", + "+{}", + "diff --git a/data/empty old.txt b/data/empty old.txt", + "deleted file mode 100644", + "index e69de29..0000000", + "diff --git a/data/empty new.txt b/data/empty new.txt", + "new file mode 100644", + "index 0000000..e69de29", + "diff --git a/data/blob old.bin b/data/blob old.bin", + "deleted file mode 100644", + "index 4444444..0000000", + "Binary files a/data/blob old.bin and /dev/null differ", + "diff --git a/data/blob new.bin b/data/blob new.bin", + "new file mode 100644", + "index 0000000..5555555", + "Binary files /dev/null and b/data/blob new.bin differ", + "diff --git a/scripts/run me.sh b/scripts/run me.sh", + "old mode 100644", + "new mode 100755", + "diff --git a/docs/old name.md b/docs/new name.md", + "similarity index 100%", + "rename from docs/old name.md", + "rename to docs/new name.md", + "diff --git a/docs/moved file.md b/docs/moved file 2.md", + "similarity index 80%", + "rename from docs/moved file.md", + "rename to docs/moved file 2.md", + "index 6666666..7777777 100644", + "--- a/docs/moved file.md\t", + "+++ b/docs/moved file 2.md\t", + "@@ -1 +1 @@", + "-old", + "+new", + "", + }, "\n") + + got, err := parseUnifiedDiff(raw) + if err != nil { + t.Fatalf("parseUnifiedDiff: %v", err) + } + want := []struct { + oldPath, path string + deleted, added, binary bool + }{ + {oldPath: "data/2024-06-08 - a.json", path: "data/2024-06-08 - a.json", deleted: true}, + {oldPath: "data/2024-06-08 - b.json", path: "data/2024-06-08 - b.json", deleted: true}, + {oldPath: "data/2024-06-09 - c.json", path: "data/2024-06-09 - c.json", added: true}, + {oldPath: "data/empty old.txt", path: "data/empty old.txt", deleted: true}, + {oldPath: "data/empty new.txt", path: "data/empty new.txt", added: true}, + {oldPath: "data/blob old.bin", path: "data/blob old.bin", deleted: true, binary: true}, + {oldPath: "data/blob new.bin", path: "data/blob new.bin", added: true, binary: true}, + {oldPath: "scripts/run me.sh", path: "scripts/run me.sh"}, + {oldPath: "docs/old name.md", path: "docs/new name.md"}, + {oldPath: "docs/moved file.md", path: "docs/moved file 2.md"}, + } + if len(got.Patches) != len(want) || len(got.PlanDiff.Files) != len(want) { + t.Fatalf("patches = %d, plan files = %d, want %d", len(got.Patches), len(got.PlanDiff.Files), len(want)) + } + for i, w := range want { + patch := got.Patches[i] + if patch.OldPath != w.oldPath || patch.Path != w.path || patch.Deleted != w.deleted || patch.Added != w.added || patch.Binary != w.binary { + t.Errorf("patch %d = old %q path %q deleted %v added %v binary %v, want %+v", + i, patch.OldPath, patch.Path, patch.Deleted, patch.Added, patch.Binary, w) + } + if plan := got.PlanDiff.Files[i]; plan.OldPath != w.oldPath || plan.Path != w.path { + t.Errorf("plan file %d = old %q path %q, want old %q path %q", i, plan.OldPath, plan.Path, w.oldPath, w.path) + } + } +} + +func TestParseUnifiedDiffQuotedRenamePaths(t *testing.T) { + raw := strings.Join([]string{ + `diff --git "a/docs/caf\303\251 notes.md" "b/docs/caf\303\251 notes 2.md"`, + "similarity index 100%", + `rename from "docs/caf\303\251 notes.md"`, + `rename to "docs/caf\303\251 notes 2.md"`, + "", + }, "\n") + + got, err := parseUnifiedDiff(raw) + if err != nil { + t.Fatalf("parseUnifiedDiff: %v", err) + } + if len(got.Patches) != 1 || got.Patches[0].OldPath != "docs/café notes.md" || got.Patches[0].Path != "docs/café notes 2.md" { + t.Fatalf("patches = %#v, want decoded rename paths", got.Patches) + } +} + func TestParseUnifiedDiffRejectsBadHunkHeader(t *testing.T) { _, err := parseUnifiedDiff("diff --git a/main.go b/main.go\n@@ bad @@\n") if err == nil { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 8c27765..a99f2fc 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -6492,6 +6492,47 @@ func TestSelectionOnlyAcceptsRenameSourcePath(t *testing.T) { } } +func TestSelectionOnlyAcceptsDeletedPathsWithSpaces(t *testing.T) { + ctx := context.Background() + provider, req := dryRunHarness(t) + removeRepoAgentFixture(provider) + + deleted := []string{"data/2024-06-08 - a.json", "data/2024-06-08 - b.json"} + repoDir := provider.fixtureRepoDir + gitCommandMustSucceed(t, repoDir, "checkout", "main") + for _, path := range deleted { + writeFile(t, filepath.Join(repoDir, path), "{}\n") + } + gitCommandMustSucceed(t, repoDir, "add", "data") + gitCommandMustSucceed(t, repoDir, "commit", "-m", "add dated files") + provider.pr.Base.SHA = gitCommandMustSucceed(t, repoDir, "rev-parse", "HEAD") + gitCommandMustSucceed(t, repoDir, "checkout", "-B", "feature") + gitCommandMustSucceed(t, repoDir, append([]string{"rm", "--"}, deleted...)...) + gitCommandMustSucceed(t, repoDir, "commit", "-m", "delete dated files") + provider.pr.Head.SHA = gitCommandMustSucceed(t, repoDir, "rev-parse", "HEAD") + provider.diff.Raw = gitCommandOutput(t, repoDir, "diff", provider.pr.Base.SHA, provider.pr.Head.SHA) + + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("selection-session", `{ + "schema_version": 1, + "selected_agents": [], + "thread_actions": [], + "reasoning": "the diff only deletes files" + }`, 10, 2)) + + result, err := selectionOnlyForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Now: fixedNow, + }, selectionRequestFromReview(req, t.TempDir())) + if err != nil { + t.Fatalf("SelectionOnly: %v", err) + } + if !reflect.DeepEqual(result.ChangedFiles, deleted) { + t.Fatalf("changed files = %#v, want %#v", result.ChangedFiles, deleted) + } +} + func TestDryRunReviewerFindingOnDeletedPathIsDecoded(t *testing.T) { ctx := context.Background() store := openPipelineStore(t)