Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .claude/rules/gotchas.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<score> old new` / `C<score>` pairs. `(*Git).pathArgs(req)` emits `-M -- <old> <new>` when `OldPath != "" && OldPath != Path` so git pairs the rename into a minimal diff (otherwise `-- <path>`); 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 -- <untracked>` 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:"<escaped>"` (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.
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
18 changes: 16 additions & 2 deletions app/diff/jj.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 + `"`
}
Comment on lines +209 to +212

// 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
Expand Down
108 changes: 108 additions & 0 deletions app/diff/jj_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
1 change: 1 addition & 0 deletions site/docs.html
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ <h1>Documentation</h1>

<h2 id="requirements">Requirements</h2>
<p><code>git</code>, <code>hg</code> (Mercurial), or <code>jj</code> (Jujutsu) is used to generate diffs. VCS is optional when using <code>--only</code> for standalone file review or <code>--stdin</code> for scratch-buffer review. In colocated <code>git</code>+<code>jj</code> repositories, revdiff uses the <code>jj</code> working-copy model.</p>
<p>Jujutsu must be 0.27 or newer. Versions 0.23 through 0.26 have no <code>jj file annotate -T</code>, so the blame gutter does not work. Versions before 0.23 also reject the commit-log template behind the commit-info popup.</p>

<h2 id="installation">Installation</h2>
<h3>Homebrew (macOS/Linux)</h3>
Expand Down