diff --git a/.claude/rules/gotchas.md b/.claude/rules/gotchas.md index 674517e7..e786c306 100644 --- a/.claude/rules/gotchas.md +++ b/.claude/rules/gotchas.md @@ -9,6 +9,7 @@ - `--all-files` mode uses `DirectoryReader` (git ls-files) to list all tracked files; `--include` wraps any renderer with `IncludeFilter` for prefix-based inclusion, `--exclude` wraps with `ExcludeFilter` for prefix-based exclusion (include narrows first, then exclude removes). Those wrappers only filter `ChangedFiles`; untracked files come straight from the VCS `UntrackedFiles` call and bypass the renderer chain, so `filterUntracked` (in `renderer_setup.go`) re-applies the same prefixes via `diff.FilterPaths` at the composition root — a future change to include/exclude filtering must touch both paths or untracked scoping silently re-breaks. `--include` is mutually exclusive with `--only`. `--all-files` is mutually exclusive with refs, `--staged`, and `--only`. `--stdin` is mutually exclusive with refs, `--staged`, `--only`, `--all-files`, `--include`, and `--exclude`. `--compare-old/--compare-new` is mutually exclusive with refs, `--staged`, `--only`, `--all-files`, `--stdin`, `--include`, `--exclude`, and `--annotations`. - Rename-aware diffs (git-only): `FileDiff` takes a single `diff.FileDiffRequest` value (`Ref`, `Path`, `OldPath`, `Staged`, `ContextLines`) — not positional args — across all three interfaces (`diff.Renderer`, `ui.Renderer`, `review.FileDiffer`) and every implementation/wrapper. `FileEntry.OldPath` is the rename origin (empty for non-renames), populated only by `(*Git).ChangedFiles` from git's `R old new` / `C` pairs. `(*Git).pathArgs(req)` emits `-M -- ` when `OldPath != "" && OldPath != Path` so git pairs the rename into a minimal diff (otherwise `-- `); the old-side probes `totalOldLines`/`binarySizeDesc` read `OldPath` when set. hg and jj never set `OldPath` (jj decomposes renames into delete+add; hg `status` reports `R` as *removed*), so non-git renderers ignore it — no behavior change there. UI: `sidepane.FileTree` exposes `OldPath(path)` (parallel to `FileStatus(path)`, populated in `NewFileTree` + `Rebuild`); `fetchEffectiveFileDiff` threads it into the request, and `view.go` renders `old → new` in the diff-pane header via `loadedFileState.oldName` / `fileLoadedMsg.oldName` (still passed through `truncateHeaderTitle`). Review stats (`ComputeStats`) and the annotations preloader (`lookupLineSet`) also set `OldPath` so their per-file diffs match the displayed rename-aware diff. - Untracked renames (git-only): a plain `mv old new` (no `git mv`/staging) leaves `old` as an unstaged deletion and `new` untracked, so `git diff -M` never pairs them — `(*Git).ChangedFiles` reports only `D old`. `(*Git).UntrackedRenames(untracked)` recovers the pairing off a **throwaway index**: `(*Git).tempIndexWithIntentToAdd` copies `.git/index` to a temp file (resolved via `git rev-parse --git-path index`), runs `git add -N -- ` against the copy with `GIT_INDEX_FILE` set (via `runGitEnv`/`runVCSEnv`), then `git diff --name-status -M` reports the pairs as renames; the real index and working tree are never touched. It returns `FileEntry{Status: FileRenamed, Path: new, OldPath: old}` only for renames whose new side is in the passed (already include/exclude-filtered) untracked set. Wiring: `vcsSetup.untrackedRenamesFn` (git case only → `g.UntrackedRenames`, nil for hg/jj) → `ModelConfig.LoadUntrackedRenames` → `Model.loadUntrackedRenames`. `loadFiles` merges via `detectUntrackedRenames` (gated to unstaged working-tree mode: `m.cfg.ref == "" && !m.cfg.staged`) + `mergeUntrackedEntries`, which drops the standalone `D old` and skips the new path from the plain-untracked append. `FileDiff` renders these via `untrackedRenameDiff` (same throwaway-index trick), selected by `isUntrackedRename(req)` = `Ref == "" && !Staged && OldPath != "" && OldPath != Path` — the only producer of `OldPath` in unstaged mode is `UntrackedRenames`, so that condition uniquely identifies the case. `(*Git).parseNameStatusEntries` is the shared NUL-field name-status parser for both `ChangedFiles` and `UntrackedRenames`. All git calls in this path use `(*Git).renameIndexEnv` which sets `GIT_INDEX_FILE` **and** `GIT_LITERAL_PATHSPECS=1` so a working-tree filename that looks like pathspec magic (e.g. `:(top)x`) is treated literally rather than as a pathspec. A fresh repo with no commits has no `.git/index`; `UntrackedRenames` treats that `fs.ErrNotExist` as "no renames possible" and returns `nil, nil`. The `--annotations` preloader (`app/annotations_load.go`) mirrors this: `preloadAnnotations` takes the same `untrackedRenamesFn`, and `(*preloader).foldUntrackedRenames` upgrades untracked entries to `FileRenamed` + records `OldPath` + drops the origin deletion under the same gate, so `lookupLineSet` resolves the rename-aware diff and `(-)`/context annotations on untracked renames round-trip (without it the preloader read `new` as all-added and dropped them). +- jj paths are fileset expressions, not literal paths (issue #341): `jj diff` and `jj file show` parse post-`--` arguments as jj's fileset query language, so `$ ( ) : #` fail to parse and `* ? & | ~` silently resolve to a *different* set of files — the latter concatenates other files' diffs into one file's view, which `parseUnifiedDiff` absorbs as bogus context/add/remove rows under a correct-looking filename. Every path handed to those two commands goes through `(*Jj).pathArg`, which wraps it as `cwd-file:""` (backslash and double-quote escaped). This is the jj analogue of git's `GIT_LITERAL_PATHSPECS=1`. Unconditional, because **revdiff's jj floor is 0.27** (README). Measured against official binaries: blame is broken throughout 0.20-0.26 (`jj file annotate` absent before 0.23, present but without `-T` through 0.26), and the commit-info popup additionally fails before 0.23 because the `\x00` escapes in `jjCommitLogTemplate` do not parse. So at least one advertised feature is broken on every jj below 0.27, though the core diff pane itself works further back. 0.27 is also where the `ui.allow-filesets` opt-out was removed, so every *supported* jj parses filesets and the quoting needs no capability probe or version branch. Do not add one unless the floor is deliberately lowered below 0.27. **`cwd-file:` not `root-file:`** — jj runs with `cmd.Dir = workDir` and emits paths relative to it, so `cwd-file:` resolves whatever `ChangedFiles` reported. `root-file:` rejects an absolute path outright, which regressed `--only=/abs/path` (the documented Zed task passes `$ZED_FILE`, always absolute) from a working context-only fallback to `error loading diff`; it also rejects a `../` sibling and silently matches the wrong file for a path below a `workDir` that is not the repo root. **Do not apply it to `jj file annotate`** (`jjblame.go`) — that takes a genuine path and rejects a fileset with `No such path`; likewise `jj file list` (`directory.go`) takes no path args. hg passes literal paths (it needs an explicit `glob:`/`re:` prefix) and needs no equivalent. - `diff.readReaderAsContext()` is the shared parser for file-backed and stdin-backed context-only views. Preserve its behavior if you change binary detection, line-length handling, or line numbering. - Overlay popups managed by `overlay.Manager`. `Compose()` uses ANSI-aware compositing via `charmbracelet/x/ansi.Cut`. `HandleKey()` returns `Outcome` — Model switches on `OutcomeKind` for side effects (file jumps, theme apply/persist). Overlay kinds: help, annot-list, theme-select, info, file-picker. One overlay at a time — opening any overlay auto-closes whichever was previously open - File picker key binding: `jump_file` defaults to `P`, deliberately NOT `ctrl+p` — host terminals claim that chord (agterm binds it to `session_palette` by default, so it never reaches the TUI) and a swallowed key reads to users as a missing feature. Consequence of choosing a printable default: `filePickerOverlay.handleKey` calls `appendPrintableRunes` FIRST, and that consumes every unmodified printable rune (plus `KeySpace`) regardless of which action the key resolves to — so inside an open picker `P` extends the filter and the `action == keymap.ActionJumpFile` close branch below it is unreachable for the default binding. That ordering is required: reversing it would break filtering for whichever *bare printable* keys happen to be bound to `jump_file`, `up` or `down` — `P`/`j`/`k` under the defaults, but equally any bare printable key a user maps to those three actions. Conversely the three action branches are reachable by anything `appendPrintableRunes` does not consume — an alt-modified rune (`map alt+f jump_file`), a function key (`map f1 jump_file`), a ctrl chord — so toggle-close survives through any non-printable binding, not only a modified chord. Any future overlay action bound to a bare printable key inherits the same one-way behavior. diff --git a/README.md b/README.md index fe0b5aaf..99c5585e 100644 --- a/README.md +++ b/README.md @@ -43,6 +43,7 @@ Built for a specific use case: reviewing code changes, plans, and documents with ## Requirements - `git`, `hg`, or `jj` (used to generate diffs; optional when using `--only` or `--stdin`) +- Jujutsu must be 0.27 or newer. Versions 0.23 through 0.26 have no `jj file annotate -T`, so the blame gutter does not work. Versions before 0.23 also reject the commit-log template behind the commit-info popup. ## Installation diff --git a/app/diff/jj.go b/app/diff/jj.go index 406f7fa7..88f667fc 100644 --- a/app/diff/jj.go +++ b/app/diff/jj.go @@ -146,7 +146,7 @@ func (j *Jj) FileDiff(req FileDiffRequest) ([]DiffLine, error) { args := make([]string, 0, 5+len(rangeArgs)) args = append(args, "diff", "--git", jjContextArg(req.ContextLines)) args = append(args, rangeArgs...) - args = append(args, "--", req.Path) + args = append(args, "--", j.pathArg(req.Path)) out, err := j.runJj(args...) if err != nil { @@ -190,13 +190,27 @@ func (j *Jj) totalOldLines(ref, file string) int { if oldRef == "" { oldRef = "@-" } - out, err := j.runJj("file", "show", "-r", oldRef, "--", file) + out, err := j.runJj("file", "show", "-r", oldRef, "--", j.pathArg(file)) if err != nil { return 0 } return countLines(out) } +// pathArg quotes a path as a jj fileset pattern. jj parses post-`--` arguments as fileset +// expressions rather than names, so a bare path is a query: `$ ( ) :` fail to parse and +// `* ? & | ~` resolve to a different set of files. Unconditional because revdiff needs jj +// 0.27+ anyway (see README), and 0.27 is where the ui.allow-filesets opt-out was removed — +// every jj that can run revdiff parses filesets. +// +// cwd-file: rather than root-file: because jj runs with cmd.Dir = workDir and emits paths +// relative to it, so absolute and "../" paths resolve too (see .claude/rules/gotchas.md). +// Not for `jj file annotate` (jjblame.go), which takes a literal path and rejects a pattern. +func (j *Jj) pathArg(path string) string { + esc := strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(path) + return `cwd-file:"` + esc + `"` +} + // jjContextArg returns the --context argument for jj diff given the caller's // requested context size. A non-positive contextLines or one at or above // fullContextSentinel returns the full-file arg; any other value returns diff --git a/app/diff/jj_test.go b/app/diff/jj_test.go index f312d2ab..4cc97c00 100644 --- a/app/diff/jj_test.go +++ b/app/diff/jj_test.go @@ -603,3 +603,111 @@ func TestJj_FileDiff_SmallContext(t *testing.T) { } assert.Equal(t, 19, fullCtx, "expected 19 context lines with full-file context") } + +func TestJj_PathArg(t *testing.T) { + j := &Jj{} + tests := []struct { + name, path, want string + }{ + {"plain", "hello.txt", `cwd-file:"hello.txt"`}, + {"nested", "app/diff/jj.go", `cwd-file:"app/diff/jj.go"`}, + {"dollar", "$test.txt", `cwd-file:"$test.txt"`}, + {"glob star", "a*b.txt", `cwd-file:"a*b.txt"`}, + {"set operator", "a|b.txt", `cwd-file:"a|b.txt"`}, + {"double quote", `a"b.txt`, `cwd-file:"a\"b.txt"`}, + {"backslash", `a\b.txt`, `cwd-file:"a\\b.txt"`}, + {"backslash then quote", `a\"b.txt`, `cwd-file:"a\\\"b.txt"`}, + {"absolute", "/tmp/repo/a.txt", `cwd-file:"/tmp/repo/a.txt"`}, + {"parent relative", "../top.txt", `cwd-file:"../top.txt"`}, + {"empty", "", `cwd-file:""`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, j.pathArg(tt.path)) + }) + } +} + +func TestJj_FileDiff_PunctuatedPaths(t *testing.T) { + // issue #341: unquoted paths parsed as filesets, so punctuation broke or globbed + dir := setupJjRepo(t) + j := NewJj(dir) + + names := []string{"a*b.txt", "axb.txt", "$test.txt", "a(b).txt", "a|b.txt", "a?b.txt"} + for _, n := range names { + writeFile(t, dir, n, n+" old\n") + } + jjCmd(t, dir, "describe", "-m", "init", "--quiet") + jjCmd(t, dir, "new", "-m", "modify", "--quiet") + for _, n := range names { + writeFile(t, dir, n, n+" new\n") + } + + for _, n := range names { + t.Run(n, func(t *testing.T) { + lines, err := j.FileDiff(FileDiffRequest{Path: n}) + require.NoError(t, err) + require.Len(t, lines, 2, "expected exactly the one-line change for %q, got %+v", n, lines) + assert.Equal(t, ChangeRemove, lines[0].ChangeType) + assert.Equal(t, n+" old", lines[0].Content) + assert.Equal(t, ChangeAdd, lines[1].ChangeType) + assert.Equal(t, n+" new", lines[1].Content) + }) + } +} + +func TestJj_FileDiff_AbsolutePath(t *testing.T) { + // issue #341 follow-up: root-file: rejected absolute paths, breaking --only=/abs/path + dir := setupJjRepo(t) + j := NewJj(dir) + + writeFile(t, dir, "changed.txt", "old\n") + writeFile(t, dir, "same.txt", "steady\n") + jjCmd(t, dir, "describe", "-m", "init", "--quiet") + jjCmd(t, dir, "new", "-m", "modify", "--quiet") + writeFile(t, dir, "changed.txt", "new\n") + + resolved, err := filepath.EvalSymlinks(dir) + require.NoError(t, err) + + lines, err := j.FileDiff(FileDiffRequest{Path: filepath.Join(resolved, "changed.txt")}) + require.NoError(t, err) + require.Len(t, lines, 2) + assert.Equal(t, "old", lines[0].Content) + assert.Equal(t, "new", lines[1].Content) + + lines, err = j.FileDiff(FileDiffRequest{Path: filepath.Join(resolved, "same.txt")}) + require.NoError(t, err, "an unchanged file must return empty, not an error") + assert.Empty(t, lines) +} + +func TestJj_TotalOldLines_PunctuatedPath(t *testing.T) { + // issue #341: jj file show parses its path as a fileset, so the count came back 0 + dir := setupJjRepo(t) + j := NewJj(dir) + + var sb strings.Builder + for i := 1; i <= 20; i++ { + fmt.Fprintf(&sb, "line %d\n", i) + } + writeFile(t, dir, "$big.txt", sb.String()) + jjCmd(t, dir, "describe", "-m", "init", "--quiet") + jjCmd(t, dir, "new", "-m", "modify", "--quiet") + + sb.Reset() + for i := 1; i <= 20; i++ { + if i == 5 { + fmt.Fprintf(&sb, "line %d CHANGED\n", i) + continue + } + fmt.Fprintf(&sb, "line %d\n", i) + } + writeFile(t, dir, "$big.txt", sb.String()) + + assert.Equal(t, 20, j.totalOldLines("", "$big.txt")) + + lines, err := j.FileDiff(FileDiffRequest{Path: "$big.txt", ContextLines: 2}) + require.NoError(t, err) + require.NotEmpty(t, lines) + assert.Equal(t, ChangeDivider, lines[len(lines)-1].ChangeType, "expected a trailing divider") +} diff --git a/site/docs.html b/site/docs.html index 37f3c017..f0431f22 100644 --- a/site/docs.html +++ b/site/docs.html @@ -112,6 +112,7 @@

Documentation

Requirements

git, hg (Mercurial), or jj (Jujutsu) is used to generate diffs. VCS is optional when using --only for standalone file review or --stdin for scratch-buffer review. In colocated git+jj repositories, revdiff uses the jj working-copy model.

+

Jujutsu must be 0.27 or newer. Versions 0.23 through 0.26 have no jj file annotate -T, so the blame gutter does not work. Versions before 0.23 also reject the commit-log template behind the commit-info popup.

Installation

Homebrew (macOS/Linux)