fix(jj): quote file paths so punctuated filenames diff correctly - #346
Merged
Merged
Conversation
jj parses post-`--` arguments as fileset expressions, not literal paths. Names containing `$ ( ) : #` failed to parse and showed "error loading diff"; names containing `* ? & | ~` parsed as globs or set operators, so jj returned a diff for a different set of files. The second class is silent: jj exits 0 and parseUnifiedDiff absorbs the extra `diff --git` headers as context and turns the `---`/`+++` markers into fake add/remove rows, so the pane shows other files' content with restarted line numbers under a correct-looking filename. Annotations placed there export against line numbers that do not exist. Both affected call sites now route through (*Jj).filesetPath, which wraps the path as root-file:"<escaped>". jj file annotate takes a genuine path and rejects a fileset, so blame is deliberately left alone. Related to #341
…solve root-file: is workspace-relative only, so it rejects an absolute path at parse time. That regressed --only=/abs/path in a jj repo: FallbackRenderer hands the user's original absolute string to Jj.FileDiff, and the error now returns before the context-only disk read that used to render the file. The documented Zed task passes $ZED_FILE, which is always absolute. cwd-file: fits every path that reaches here. jj runs with cmd.Dir = workDir and reports paths relative to it, so cwd-file: resolves exactly what ChangedFiles returned, plus absolute paths and ../ siblings. The prior godoc had the rationale backwards: root-file: is what breaks when workDir is not the repo root, not what survives it.
Measured against official binaries: `jj file annotate` is absent before 0.23 and gains -T only at 0.27, and the \x00 escapes in jjCommitLogTemplate are rejected before 0.23. So the blame gutter and the commit-info popup have never worked on jj below 0.27, whatever the diff pane did. That settles how to quote paths. 0.27 also removed the ui.allow-filesets opt-out, so every jj that can run revdiff parses filesets, and the quoting needs no capability probe or version branch. README now states the floor. Renames filesetPath to pathArg; no behavior change.
The first wording implied blame and the commit-info popup both fail on every jj below 0.27. Only blame does. The commit-info popup works from 0.23, where the commit-log template starts parsing; before that both are broken. The core diff pane itself works further back still. site/docs.html carried no jj version at all and has to track README.
There was a problem hiding this comment.
🟢 Approval recommended
The fix addresses a concrete correctness issue with strong regression coverage, and the remaining feedback is minor/clarity-level.
Pull request overview
Fixes Jujutsu backend path handling by ensuring file paths are passed to jj as literal paths (via quoted fileset patterns), preventing parse errors and—more importantly—silent mis-diffs for punctuated filenames that could corrupt exported annotation output.
Changes:
- Route
jj diffandjj file showpath arguments through a new(*Jj).pathArghelper that quotes/escapes paths ascwd-file:"...". - Add JJ regression tests covering punctuated filenames, absolute paths, and compact-mode
totalOldLines. - Document the JJ minimum supported version (0.27+) in README and site docs, and capture the gotcha in
.claude/rules/gotchas.md.
File summaries
| File | Description |
|---|---|
app/diff/jj.go |
Quote/escape JJ file arguments to prevent fileset parsing/globbing and wrong diffs. |
app/diff/jj_test.go |
Add regression tests for punctuated/absolute paths and totalOldLines behavior. |
README.md |
Document JJ minimum version requirement for working blame/commit info. |
site/docs.html |
Mirror JJ minimum version requirement in website docs. |
.claude/rules/gotchas.md |
Record JJ fileset-path gotcha and rationale for cwd-file: quoting. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+209
to
+212
| func (j *Jj) pathArg(path string) string { | ||
| esc := strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(path) | ||
| return `cwd-file:"` + esc + `"` | ||
| } |
"Earlier versions" made the reader infer the antecedent from the sentence before. Copilot flagged it on #346.
Deploying revdiff with
|
| Latest commit: |
69c0406
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://93a9409b.revdiff.pages.dev |
| Branch Preview URL: | https://jj-fileset-paths.revdiff.pages.dev |
raine
pushed a commit
to raine/revdiff
that referenced
this pull request
Sep 12, 2026
…utun#346) jj parses arguments after `--` as fileset expressions, not literal paths, so `(*Jj).FileDiff` and `(*Jj).totalOldLines` were handing it queries instead of filenames. Names containing `$ ( ) : #` failed to parse and showed `error loading diff`. Two other classes fail silently, with jj exiting 0. `*` and `?` are globs, so jj returns a diff for a *different* set of files, and `parseUnifiedDiff` absorbs the extra headers as bogus context and add/remove rows under a correct-looking filename; annotations placed on those rows export against line numbers that do not exist, which corrupts review output rather than only the display. `&`, `|` and `~` are set operators that resolve to nothing, so the file renders as an empty diff with nothing on screen saying why. Both call sites now route through `(*Jj).pathArg`, which quotes the path as `cwd-file:"<escaped>"`. `jj file annotate` is deliberately left alone, since it takes a literal path and rejects a pattern with `No such path`. `cwd-file:` rather than `root-file:`. `root-file:` is workspace-relative and rejects an absolute path at parse time, which would have broken `--only=/abs/path` in a jj repo: `FallbackRenderer` hands the user's original absolute `--only` string to `FileDiff`, and the parse error returns before the context-only disk read that renders the file today. The documented Zed task passes `$ZED_FILE`, which is always absolute. Jujutsu runs with `cmd.Dir = workDir` and emits paths relative to it, so `cwd-file:` resolves what `ChangedFiles` reported, plus absolute paths and `../` siblings. Quoting is unconditional because revdiff needs jj 0.27 or newer anyway, and 0.27 is where the `ui.allow-filesets` opt-out was removed. Official binaries establish that floor: `jj file annotate` is absent before 0.23 and gains `-T` only at 0.27, and the `\x00` escapes in `jjCommitLogTemplate` do not parse before 0.23. So the blame gutter is broken throughout 0.20 to 0.26 and the commit-info popup additionally before 0.23. README, `site/docs.html` and the gotchas note now state it. Tests cover the escaping table and exercise punctuated names and the compact-mode `totalOldLines` path against a real jj repo; those cases fail on master. The absolute-path case guards `cwd-file:` against `root-file:`. Related to umputun#341
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
jj parses arguments after
--as fileset expressions, not literal paths, so(*Jj).FileDiffand(*Jj).totalOldLineswere handing it queries instead of filenames. Names containing$ ( ) : #failed to parse and showederror loading diff. Two other classes fail silently, with jj exiting 0.*and?are globs, so jj returns a diff for a different set of files, andparseUnifiedDiffabsorbs the extra headers as bogus context and add/remove rows under a correct-looking filename; annotations placed on those rows export against line numbers that do not exist, which corrupts review output rather than only the display.&,|and~are set operators that resolve to nothing, so the file renders as an empty diff with nothing on screen saying why.Both call sites now route through
(*Jj).pathArg, which quotes the path ascwd-file:"<escaped>".jj file annotateis deliberately left alone, since it takes a literal path and rejects a pattern withNo such path.cwd-file:rather thanroot-file:.root-file:is workspace-relative and rejects an absolute path at parse time, which would have broken--only=/abs/pathin a jj repo:FallbackRendererhands the user's original absolute--onlystring toFileDiff, and the parse error returns before the context-only disk read that renders the file today. The documented Zed task passes$ZED_FILE, which is always absolute. Jujutsu runs withcmd.Dir = workDirand emits paths relative to it, socwd-file:resolves whatChangedFilesreported, plus absolute paths and../siblings.Quoting is unconditional because revdiff needs jj 0.27 or newer anyway, and 0.27 is where the
ui.allow-filesetsopt-out was removed. Official binaries establish that floor:jj file annotateis absent before 0.23 and gains-Tonly at 0.27, and the\x00escapes injjCommitLogTemplatedo not parse before 0.23. So the blame gutter is broken throughout 0.20 to 0.26 and the commit-info popup additionally before 0.23. README,site/docs.htmland the gotchas note now state it.Tests cover the escaping table and exercise punctuated names and the compact-mode
totalOldLinespath against a real jj repo; those cases fail on master. The absolute-path case guardscwd-file:againstroot-file:.Related to #341