Bug: jj backend passes paths as fileset expressions, corrupting or breaking diffs for punctuated filenames

[anicke]

Summary

(*Jj).FileDiff appends the file path after -- on the jj diff command line. jj parses those arguments as fileset expressions — a query language with globs, set operators and quoting — not as literal paths. A filename only works if it happens to be a valid bare fileset string.

Any filename containing punctuation is therefore at risk, in one of two ways. Of 31 filenames tested through (*Jj).FileDiff, 23 produced a wrong result:

• 19 fail loudly. Characters such as $, (, :, # don't parse. The diff pane shows error loading diff and the file can't be reviewed.
• 5 fail silently. * and ? are globs; &, |, ~ are set operators. jj exits 0, and revdiff renders a diff for the wrong set of files — either empty, or other files' diffs concatenated into this one. Nothing indicates a problem.

The silent class is the more serious one: it shows wrong content under a correct-looking filename, and annotations made there are exported against line numbers that don't exist.

Repro — loud failure

jj git init
printf 'one\ntwo\n' > '$test.txt'
jj describe -m base && jj new
printf 'one\ntwoX\n' > '$test.txt'
revdiff

The file lists in the tree normally (jj diff --summary takes no path arg). Selecting it shows:

error loading diff: get file diff for $test.txt: jj diff --git --context=1000000 -- $test.txt:
Error: Failed to parse fileset: Syntax error
  --> 1:1
  |
1 | $test.txt
  | ^---
  = expected <strict_identifier>, <bare_string>, or <expression>

Works when quoted: jj diff --git -- 'root-file:"$test.txt"'

Repro — silent failure

Two files, where one name globs onto the other:

jj git init
printf 'one\n' > 'a*b.txt'; printf 'one\n' > 'axb.txt'
jj describe -m base && jj new
printf 'two\n' > 'a*b.txt'; printf 'two\n' > 'axb.txt'

FileDiff{Path: "a*b.txt"} returns no error and 8 rows for what is a one-line change:

[0] type="-" old=1 new=0 "one"
[1] type="+" old=0 new=1 "two"
[2] type=" " old=2 new=2 "diff --git a/axb.txt b/axb.txt"     <- other file's header as context
[3] type=" " old=3 new=3 "index 5626abf0f7..f719efd430 100644"
[4] type="-" old=4 new=0 "-- a/axb.txt"                        <- marker mangled into a removal
[5] type="+" old=0 new=4 "++ b/axb.txt"
[6] type="-" old=1 new=0 "one"                                 <- numbering restarts
[7] type="+" old=0 new=1 "two"

axb.txt's content is rendered inside a*b.txt's diff. Because parseUnifiedDiff expects a single-file diff, the embedded diff --git/index headers are absorbed as context lines and the ---/+++ markers become fake add/remove rows with the first character eaten. Line numbers restart per embedded file.

This scales with the number of matches — in a 31-file test repo the same call returned 184 rows.

Annotations key on path + line number, and both are wrong here, so an annotation placed on one of these rows is written to -o output against a line that doesn't exist. That makes this a correctness bug in exported review output, not only a display bug.

Root cause

Two call sites, both app/diff/jj.go:

Site	Call	Behavior
jj.go:149	(*Jj).FileDiff — append(args, "--", req.Path)	the failures above
jj.go:193	(*Jj).totalOldLines — jj file show ... -- <file>	silent; error discarded, returns 0, so compact mode drops the trailing ⋯ N lines ⋯ divider

git and hg take literal pathspecs; jj replaced that with the fileset language. fileset appears nowhere in the codebase — the semantics were never accounted for.

Full character results

31 filenames of the form a<char>b.txt, each driven through (*Jj).FileDiff:

Outcome	n	Characters
Silent glob	2	* ?
Silent empty	3	& | ~
Parse error	19	! " # $ % ' ( ) , : ; < = > [ ^ ` { }
Correct	8	+ - . @ ] _ ␣ (incl. plain baseline; ] is safe, [ is not)

Representative ASCII punctuation, not an exhaustive byte sweep.

Not affected

• Blame — jj file annotate (jjblame.go:24) takes a genuine path. Raw name works; cwd-file:"…" fails there (No such path). Must not be changed.
• --all-files — jj file list (directory.go:51) takes no path args.
• git — verified with $ok.txt and :top.txt.
• hg — hg.go:155 passes literal paths (hg needs explicit glob:/re:). Reasoned from code only — hg not installed, not executed.

Suggested fix

Quote the path as a file pattern at the two affected sites only:

// jj parses post-`--` args as fileset expressions, so $ ! ( ) : , break the
// parse and * ? & | ~ silently resolve to the wrong set of files.
func jjFilesetPath(path string) string {
	esc := strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(path)
	return `root-file:"` + esc + `"`
}

Escaping verified for a"b.txt → root-file:"a\"b.txt" and a\b.txt → root-file:"a\\b.txt".

• Prefer root-file: over cwd-file: — NewJj gets vcsRoot (renderer_setup.go:53) so they're equivalent today, but root-file: survives a workDir change.
• Do not apply to jjblame.go.
• Worth adding separately: reject input with more than one diff --git header in parseUnifiedDiff. That's the layer that turned a bad path argument into plausible-looking rows.

Tests

No jj test uses a path with punctuation. A regression test should assert the argv passed to jj — asserting "a diff came back" would pass for a*b.txt, which currently returns wrong rows with no error.

Environment

revdiff master e3eb732 · jj 0.44.0-af45d57 · go 1.27.0 linux/amd64

[anicke]

I can create a PR with a fix.

[umputun]

Fixed by #346. Your diagnosis identified both affected call sites and correctly separated the loud parse failures from the silent empty or wrong-file results. You were also right that jj file annotate must keep taking a literal path.

cwd-file: rather than root-file:. root-file: is workspace-relative and rejects an absolute path at parse time, so it would have broken --only=/abs/path in a jj repo: FallbackRenderer passes the user's original absolute --only string through to FileDiff, and the parse error returns before the disk-read fallback that renders the file today. The Zed task in the docs uses $ZED_FILE, which is always absolute. cwd-file: also handles ../ siblings, and it matches how jj emits paths, since jj runs with its working directory set to the repo root.

Quoting is unconditional because revdiff requires Jujutsu 0.27 or newer. Versions 0.23 through 0.26 have no jj file annotate -T, so the blame gutter does not work. Earlier versions also reject the commit-log template behind the commit-info popup. Version 0.27 also removed the ui.allow-filesets opt-out, so every supported jj parses filesets. README now states the requirement.

Your third suggestion, rejecting input with more than one diff --git header in parseUnifiedDiff, is not in this change. It is a different layer, shared with the git and hg renderers, and worth doing on its own.