feat(file): add blob buffer support #24
Conversation
WalkthroughAdds a majutsu-file module for blob viewing and navigation, introduces majutsu-revision-at-point and majutsu-buffer-blob-revision for point-based change resolution, changes diff visit behavior to support forced workspace visits and workspace-key bindings, updates callers to use majutsu-revision-at-point, and adjusts Evil key bindings and docs for RET/C-j behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiffBuf as "Diff Buffer"
participant VisitCmd as "majutsu-diff-visit-file / majutsu-diff-visit-workspace-file"
participant RevAtPoint as "majutsu-revision-at-point"
participant Workspace as "Workspace File"
participant Blob as "Blob Buffer"
User->>DiffBuf: move point in hunk / press key
alt Force workspace (C-<return> or workspace command)
DiffBuf->>VisitCmd: majutsu-diff-visit-workspace-file
else Context-aware (RET / default)
DiffBuf->>VisitCmd: majutsu-diff-visit-file
end
VisitCmd->>RevAtPoint: resolve revision/blob at point
RevAtPoint-->>VisitCmd: revision/blob info
alt Working-copy diff or FORCE-WORKSPACE
VisitCmd->>Workspace: open workspace file at mapped line/col
Workspace-->>User: show editable file
else Committed change
VisitCmd->>Blob: open blob buffer for revision at mapped line/col
Blob-->>User: show blob
end
sequenceDiagram
participant User
participant FileCmd as "majutsu-find-file"
participant MajutsuFile as "majutsu-file--show"
participant JJ as "jj (VCS)"
participant BlobBuf as "Blob Buffer"
participant BlobMode as "majutsu-blob-mode"
User->>FileCmd: invoke majutsu-find-file(revset, path)
FileCmd->>MajutsuFile: request contents for revset/path
MajutsuFile->>JJ: run `jj show` for rev:path
JJ-->>MajutsuFile: return file contents + metadata
MajutsuFile-->>FileCmd: contents + metadata
FileCmd->>BlobBuf: create/ensure blob buffer with metadata
FileCmd->>BlobMode: enable majutsu-blob-mode
BlobBuf-->>User: display blob with navigation keys
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs/majutsu.org`:
- Around line 135-138: Update the docs text to clarify that the claim "C-j /
C-<return> always visit the workspace file" is not true when Evil mode remaps
C-j: mention Evil specifically and state that C-j may be overridden by Evil's
section navigation, recommend using C-<return> (which is unaffected) or provide
the alternative keybinding (e.g., bind a different key or instruct users to
unmap/remap Evil's C-j) and include the terms "C-j", "C-<return>", and "Evil" so
readers can locate the related sentence.
In `@majutsu-diff.el`:
- Around line 865-869: majutsu-find-file is calling majutsu-find-file--display
and expecting a buffer, but majutsu-find-file--display currently just calls
(funcall display-fn buffer) and doesn't return the buffer; update
majutsu-find-file--display to explicitly return the buffer (e.g., call the
display function but ensure the function returns the buffer value) so callers
like majutsu-diff--goto-line-col receive a non-nil buf; touch
majutsu-find-file--display and ensure its return value is the buffer after
invoking display-fn.
In `@majutsu-file.el`:
- Around line 286-303: The log calls in majutsu-file-prev-change and
majutsu-file-next-change use the -G graph flag which can inject graph characters
into the output and confuse parsing of the change_id; update the
majutsu-jj-string invocations inside majutsu-file-prev-change and
majutsu-file-next-change to include the --no-graph option (or remove -G) so the
output is plain change_id like in majutsu-file--resolve-single-rev, ensuring the
parsed result is the raw change id.
- Line 27: The declare-function for majutsu-bury-or-kill-buffer is referencing a
symbol that isn't defined, causing majutsu-blob-quit to call an undefined
function; fix by either adding a proper definition for
majutsu-bury-or-kill-buffer in this file (implement the bury-or-kill behavior
expected by majutsu-blob-quit) or update the declare-function to point to the
correct file that actually provides the function name (or change calls in
majutsu-blob-quit to the real function name); locate the usage in
majutsu-blob-quit and the declaration line to ensure the symbol and its provider
file match.
- Around line 229-237: The revset builder uses single-quoted revset strings and
only escapes single quotes, which fails for filenames with apostrophes; update
majutsu-file--revset-for-files to produce double-quoted revset strings and
properly escape backslashes and double quotes in the path: change the escaped
computation to escape backslashes and double quotes (e.g. replace "\\" with
"\\\\" then "\"" with "\\\"" or equivalent), change file-set from "files('%s')"
to "files(\"%s\")", and keep the existing uses of file-set in the ('prev and
'next) format calls so revset generation uses the safe double-quoted path.
🧹 Nitpick comments (5)
majutsu-file.el (5)
67-79: Cache grows unboundedly - consider adding eviction or size limit.
majutsu-file--list-cacheaccumulates entries indefinitely. For long Emacs sessions with many revisions, this could consume significant memory.💡 Optional: Add cache size limit
+(defcustom majutsu-file-list-cache-size 100 + "Maximum number of entries in the file list cache." + :group 'majutsu + :type 'integer) + (defun majutsu-file--list (revset root) "Return list of file paths for REVSET in ROOT. Results are cached in `majutsu-file--list-cache`." (let* ((normalized (majutsu-file--normalize-revset revset)) (cache-key (cons root normalized)) (cached (assoc cache-key majutsu-file--list-cache))) (if cached (cdr cached) (let* ((default-directory root) (output (majutsu-jj-string "file" "list" "-r" normalized)) (paths (seq-remove #'string-empty-p (split-string output "\n")))) + ;; Evict oldest entries if cache is full + (when (>= (length majutsu-file--list-cache) majutsu-file-list-cache-size) + (setq majutsu-file--list-cache (butlast majutsu-file--list-cache))) (push (cons cache-key paths) majutsu-file--list-cache) paths))))
98-103: Docstring missing parameter documentation.The docstring should document the
ROOTparameter to clarify its purpose.📝 Suggested docstring improvement
(defun majutsu-file--path-at-point (root) - "Return path from context or nil." + "Return file path from context relative to ROOT, or nil if none found." (or (magit-section-value-if 'jj-file) (majutsu-diff--file-at-point) (when-let* ((file buffer-file-name)) (majutsu-file--relative-path root file))))
113-131: Duplicate implementation withmajutsu-diff--range-value.
majutsu-file--diff-range-valueduplicates the logic ofmajutsu-diff--range-valuedefined in majutsu-diff.el (lines 751-755). Similarly,majutsu-file--diff-revsetoverlaps withmajutsu-diff--default-revset.Since majutsu-file.el already requires majutsu-diff, consider reusing the existing functions from majutsu-diff.el to avoid code duplication. Based on learnings: "Diff-related functionality should be in
majutsu-diff.el".♻️ Suggested: Reuse functions from majutsu-diff.el
-(defun majutsu-file--diff-range-value (range prefix) - "Return the value in RANGE for argument starting with PREFIX." - (when range - (when-let* ((arg (seq-find (lambda (item) (string-prefix-p prefix item)) range))) - (substring arg (length prefix))))) - (defun majutsu-file--diff-revset () "Return the revset implied by the current diff buffer, if any." (when (derived-mode-p 'majutsu-diff-mode) - (let* ((range majutsu-buffer-diff-range) - (removed (eq (char-after (line-beginning-position)) ?-)) - (from (majutsu-file--diff-range-value range "--from=")) - (to (majutsu-file--diff-range-value range "--to=")) - (revisions (majutsu-file--diff-range-value range "--revisions="))) - (cond - ((and range (equal (car range) "-r") (cadr range)) (cadr range)) - (revisions revisions) - (from (if (and removed from) from (or to from))) - (t "@"))))) + (majutsu-diff--default-revset)))
180-186: Hardcoded diff-hl function names may be fragile.The code filters out specific
diff-hlhook functions by name. Ifdiff-hlrenames these functions in a future version, this filtering will silently stop working.💡 Consider a more robust approach
- (let ((buffer-file-name (expand-file-name path root)) - (after-change-major-mode-hook - (seq-difference after-change-major-mode-hook - '(global-diff-hl-mode-enable-in-buffer - global-diff-hl-mode-enable-in-buffers) - #'eq))) - (normal-mode (not enable-local-variables))) + (let ((buffer-file-name (expand-file-name path root)) + (after-change-major-mode-hook + (seq-remove (lambda (fn) + (and (symbolp fn) + (string-prefix-p "global-diff-hl-" (symbol-name fn)))) + after-change-major-mode-hook))) + (normal-mode (not enable-local-variables)))
214-227: Missing autoload cookie for interactive command.
majutsu-blob-quitis an interactive command that users may want to call directly, but it lacks the;;;###autoloadcookie. Other similar commands in this file (majutsu-find-file,majutsu-find-file-at-point) have autoload cookies.📝 Add autoload cookie
+;;;###autoload (defun majutsu-blob-quit () "Return to the workspace version of the current file."
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive blob buffer support to Majutsu, enabling users to view and navigate files at specific revisions. The implementation includes smart diff visiting behavior, line mapping between revisions, and integration with both standard and Evil keybindings.
Changes:
- Introduces
majutsu-file.elwith blob buffer implementation and file navigation at revisions - Adds
majutsu-change-at-pointutility to detect revisions from multiple contexts (sections, thing-at-point, blob buffers, diff buffers) - Updates diff visiting logic to intelligently choose between workspace files and blob buffers based on context
- Integrates Evil keybindings for blob mode navigation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| majutsu.el | Adds require for new majutsu-file module |
| majutsu-file.el | New file implementing blob buffers, file navigation at revisions, line mapping via diff hunks, and blob mode |
| majutsu-new.el | Updates to use majutsu-change-at-point for more flexible revision detection |
| majutsu-edit.el | Updates to use majutsu-change-at-point for revision detection |
| majutsu-diff.el | Adds smart diff visit behavior distinguishing workspace vs blob contexts, workspace file visiting command, and keybindings |
| majutsu-base.el | Implements majutsu-change-at-point and registers jj-revision as thing-at-point type |
| majutsu-evil.el | Adds Evil keybindings for blob mode and adjusts section bindings to avoid conflicts |
| docs/majutsu.org | Updates documentation to describe new diff visit behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@majutsu-base.el`:
- Around line 287-296: The regexp used in the jj-revision thing-at-point helpers
only matches letters ("[a-z]\{1,12\}") so hex commit IDs with digits aren't
recognized; update the looking-at pattern inside the bounds-of-thing-at-point
lambda (the put 'jj-revision 'bounds-of-thing-at-point entry) to include digits,
e.g. change "[a-z]\\{1,12\\}" to "[0-9a-z]\\{1,12\\}" so both
bounds-of-thing-at-point and the dependent thing-at-point lambda will correctly
detect commit IDs.
In `@majutsu-file.el`:
- Around line 80-83: Both jj invocations must terminate options to avoid
treating paths beginning with '-' as flags: update majutsu-file--show to call
majutsu-jj-string with "file" "show" "-r" revset "--" path, and update
majutsu-file--list to include a "--" after "list" (e.g. "file" "list" "--") so
future/positional arguments won't be parsed as options; keep
majutsu-file--map-line unchanged since it already uses "--".
0WD0
force-pushed
the
majutsu-file
branch
2 times, most recently
from
fbd18ba to
241b37c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@majutsu-diff.el`:
- Around line 811-825: The function majutsu-diff--visit-workspace-p fails to
treat a diff with only --from as visiting the workspace; update it so when
majutsu-buffer-diff-range yields a non-nil revisions value but the computed to
value is nil, it returns t (i.e., treat --from=X with no explicit --to as
--to=@). Locate majutsu-diff--visit-workspace-p and adjust the cond (using
majutsu-diff--range-value, majutsu-buffer-diff-range, and the local vars to and
revisions) to include a clause like (and revisions (null to)) that returns t
before the final fallback.
- Around line 761-772: majutsu-diff--default-revset should return the parent
revset "@-" when the current diff buffer indicates we're on a removed line;
update the logic in majutsu-diff--default-revset to detect removed lines via
majutsu-diff--on-removed-line-p and, when removed and no explicit
--revisions/--from/--to provide a usable revset, return "@-" instead of falling
through to the default "@". Ensure the existing checks for (revisions revisions)
and (from ...) remain, but change the final fallback branch that currently
returns "@" to return "@-" when (majutsu-diff--on-removed-line-p) is true,
otherwise keep "@".
In `@majutsu-file.el`:
- Around line 92-97: The call to majutsu-diff--file-at-point inside
majutsu-file--path-at-point can raise a void-function error when the diff module
isn't loaded; guard that call with a feature check (use (fboundp
'majutsu-diff--file-at-point)) and only invoke majutsu-diff--file-at-point when
that check passes, and optionally add a declare-function for
majutsu-diff--file-at-point at top of file to satisfy byte-compiler warnings.
Ensure the or expression uses the guarded conditional in place of the direct
call so majutsu-file--path-at-point never calls an undefined function.
- Around line 224-233: The function majutsu-blob-visit-file currently calls
find-file on the expanded path which will create a new file if the blob's file
was deleted; change majutsu-blob-visit-file to compute the target path (using
expand-file-name on majutsu-buffer-blob-path and majutsu-buffer-blob-root),
check it with file-exists-p, and call find-file only if the file exists
otherwise raise a user-error with a clear message indicating the workspace file
was not found.
🧹 Nitpick comments (2)
majutsu-file.el (2)
24-25: Define the hook viadefcustomso users can customize it.
majutsu-find-file-hookis user-facing; please switch it todefcustomwith a:type 'hook(and a group) to align with configuration conventions.✅ Suggested update
-(defvar majutsu-find-file-hook nil - "Hook run after creating a blob buffer.") +(defcustom majutsu-find-file-hook nil + "Hook run after creating a blob buffer." + :group 'majutsu + :type 'hook)As per coding guidelines, User options should be defined via
defcustomwith appropriate:type.
107-125: Consider moving diff-context helpers intomajutsu-diff.el(or reusing them).These helpers are diff-specific and overlap with the new diff utilities; keeping them in
majutsu-diff.el(or reusingmajutsu-diff--range-value/majutsu-diff--default-revset) reduces duplication and keeps ownership clear.Based on learnings, Diff-related functionality should be in
majutsu-diff.el.
| (defun majutsu-diff--default-revset () | ||
| "Return the revset implied by the current diff buffer." | ||
| (let* ((range majutsu-buffer-diff-range) | ||
| (removed (majutsu-diff--on-removed-line-p)) | ||
| (from (majutsu-diff--range-value range "--from=")) | ||
| (to (majutsu-diff--range-value range "--to=")) | ||
| (revisions (majutsu-diff--range-value range "--revisions="))) | ||
| (cond | ||
| ((and range (equal (car range) "-r") (cadr range)) (cadr range)) | ||
| (revisions revisions) | ||
| (from (if (and removed from) from (or to from))) | ||
| (t "@")))) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jj diff default behavior working copy parent revision
💡 Result:
In Jujutsu (jj):
jj diff(no args) defaults tojj diff -r @, i.e. it shows the diff for the working-copy revision@compared to its parent revision@-. If@is a merge, jj effectively merges the parents and diffs that merge-result against@. [1][2]jj diff -r <REV>means “diff<REV>vs its parent<REV>-”. [1][2]jj diff --from A(with no--to) means “diff A →@”, because an omitted side defaults to the working-copy commit. Likewise,jj diff --to Bmeans “diff@→ B”. [1][2]- By default jj snapshots the working copy at the start of each command, so
jj diffreflects your current working directory state unless you pass--ignore-working-copy. [2]
Sources: Jujutsu CLI reference / jj diff docs [1], jj-diff(1) man page [2].
🌐 Web query:
jj removed lines diff revision mapping @-``
💡 Result:
In Jujutsu (jj) revsets, @ means “the working-copy change”, and @- means the parent of the working copy (the change you’re based on). (jj-vcs.github.io)
How to interpret “removed lines” in a jj diff
jj diff(by default) is effectively comparing@- → @(parent → working copy). (jj-vcs.github.io)- In a Git-style diff (
--git), lines starting with-are lines that exist in thefromrevision (e.g.,@-) but not in thetorevision (e.g.,@). The hunk header@@ -old_start,old_len +new_start,new_len @@is the line-range mapping for old vs new.
Useful commands:
jj diff --git # shows changes from `@-` to @ in familiar unified-diff form
jj diff --from `@-` --to @ --gitIf you want the revision associated with removed lines
Once a line is deleted, it won’t appear in annotate of @. Instead, annotate the file in @- (before the deletion):
jj file annotate -r `@-` path/to/fileThis shows which change introduced each line in that file version. (docs.jj-vcs.dev)
Citations:
- 1: https://jj-vcs.github.io/jj/latest/tutorial/?utm_source=openai
- 2: https://jj-vcs.github.io/jj/v0.13.0/git-comparison/?utm_source=openai
- 3: https://jj-vcs.github.io/jj/v0.13.0/git-comparison/?utm_source=openai
- 4: https://docs.jj-vcs.dev/latest/cli-reference/?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check the actual code context around lines 761-772 to verify
# coding style compliance and no other issues
cd /tmp && find . -name "majutsu-diff.el" 2>/dev/null | head -1Repository: 0WD0/majutsu
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for majutsu-diff.el in the repo
fd "majutsu-diff.el" -type fRepository: 0WD0/majutsu
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Find majutsu-diff.el in the repository
fd "majutsu-diff.el"Repository: 0WD0/majutsu
Length of output: 71
🏁 Script executed:
#!/bin/bash
# Read the actual code at lines 761-772
sed -n '761,772p' majutsu-diff.elRepository: 0WD0/majutsu
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Get more context around those lines to see the function and verify the logic
sed -n '755,780p' majutsu-diff.elRepository: 0WD0/majutsu
Length of output: 1101
Add fallback to @- for removed lines in default diffs.
When jj diff runs with no explicit range, it compares @- (parent) to @ (working copy). Removed lines exist in @- but not in @. The current code falls through to (t "@"), which tries to open the removed line in the working copy where it doesn't exist.
Suggested fix
(defun majutsu-diff--default-revset ()
"Return the revset implied by the current diff buffer."
(let* ((range majutsu-buffer-diff-range)
(removed (majutsu-diff--on-removed-line-p))
(from (majutsu-diff--range-value range "--from="))
(to (majutsu-diff--range-value range "--to="))
(revisions (majutsu-diff--range-value range "--revisions=")))
(cond
((and range (equal (car range) "-r") (cadr range)) (cadr range))
(revisions revisions)
(from (if (and removed from) from (or to from)))
+ ((and removed (null range)) "@-")
(t "@"))))🤖 Prompt for AI Agents
In `@majutsu-diff.el` around lines 761 - 772, majutsu-diff--default-revset should
return the parent revset "@-" when the current diff buffer indicates we're on a
removed line; update the logic in majutsu-diff--default-revset to detect removed
lines via majutsu-diff--on-removed-line-p and, when removed and no explicit
--revisions/--from/--to provide a usable revset, return "@-" instead of falling
through to the default "@". Ensure the existing checks for (revisions revisions)
and (from ...) remain, but change the final fallback branch that currently
returns "@" to return "@-" when (majutsu-diff--on-removed-line-p) is true,
otherwise keep "@".
| (defun majutsu-blob-visit-file () | ||
| "Visit the workspace version of the current blob's file." | ||
| (interactive) | ||
| (unless (and (bound-and-true-p majutsu-blob-mode) | ||
| majutsu-buffer-blob-root | ||
| majutsu-buffer-blob-path) | ||
| (user-error "Not in a blob buffer")) | ||
| (let ((file (expand-file-name majutsu-buffer-blob-path | ||
| majutsu-buffer-blob-root))) | ||
| (find-file file))) |
There was a problem hiding this comment.
Avoid creating a new workspace file when the blob’s file was deleted.
find-file will create a new file if it doesn’t exist. Consider guarding with file-exists-p and a clear error.
💡 Suggested fix
(let ((file (expand-file-name majutsu-buffer-blob-path
majutsu-buffer-blob-root)))
- (find-file file)))
+ (if (file-exists-p file)
+ (find-file file)
+ (user-error "File does not exist in workspace: %s" majutsu-buffer-blob-path))))🤖 Prompt for AI Agents
In `@majutsu-file.el` around lines 224 - 233, The function majutsu-blob-visit-file
currently calls find-file on the expanded path which will create a new file if
the blob's file was deleted; change majutsu-blob-visit-file to compute the
target path (using expand-file-name on majutsu-buffer-blob-path and
majutsu-buffer-blob-root), check it with file-exists-p, and call find-file only
if the file exists otherwise raise a user-error with a clear message indicating
the workspace file was not found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@majutsu-file.el`:
- Around line 185-190: majutsu-find-file--display currently calls (funcall
display-fn buffer) but does not return the buffer, causing callers like
majutsu-diff-visit-file to receive nil; modify majutsu-find-file--display (which
computes root, path, buffer and calls display-fn) so it returns the buffer
(e.g., call display-fn for side-effects then ensure the function returns buffer)
so callers receive the buffer they expect.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@majutsu-base.el`:
- Around line 266-286: majutsu-change-at-point fails to extract a revision from
majutsu-buffer-diff-range because that variable is a list of strings and (cdr
(assoc "--revisions=" range)) always returns nil; update the derived-mode branch
in majutsu-change-at-point to search the list for an element whose string starts
with "--revisions=", and if found return the substring after the '=' (or split
on '=' and return the second part). Reference: majutsu-change-at-point,
majutsu-buffer-diff-range, and the literal "--revisions=" string.
8e03c23 to
ffe7d5f
Compare
0WD0
force-pushed
the
majutsu-file
branch
2 times, most recently
from
84cd2d1 to
2c4a75c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@majutsu-evil.el`:
- Around line 85-99: The current unbinding in
majutsu-evil--adjust-section-bindings runs only if the section keymap symbols
already exist, so C-j may be rebound when those majutsu files load later; change
the setup to defer unbinding by adding with-eval-after-load handlers (or a
majutsu-mode hook) that run the define-key nil for each map when its defining
feature/file loads. Concretely, keep majutsu-evil--adjust-section-bindings as
the unbind logic but call it inside with-eval-after-load blocks for the features
that create majutsu-diff-section-map, majutsu-file-section-map and
majutsu-hunk-section-map (or register it on majutsu-mode-hook) so the C-j keys
are removed even if the maps are defined after Evil setup.
In `@majutsu-file.el`:
- Around line 113-125: The fallback revset in majutsu-file--diff-revset returns
"@" for lines with no explicit range; change the final fallback to return "@-"
for removed lines. Update the final cond clause in majutsu-file--diff-revset
(currently (t "@")) to check the local removed variable and return (if removed
"@-" "@") so removed lines resolve to the parent revision.
| (defun majutsu-file--diff-revset () | ||
| "Return the revset implied by the current diff buffer, if any." | ||
| (when (derived-mode-p 'majutsu-diff-mode) | ||
| (let* ((range majutsu-buffer-diff-range) | ||
| (removed (eq (char-after (line-beginning-position)) ?-)) | ||
| (from (majutsu-file--diff-range-value range "--from=")) | ||
| (to (majutsu-file--diff-range-value range "--to=")) | ||
| (revisions (majutsu-file--diff-range-value range "--revisions="))) | ||
| (cond | ||
| ((and range (equal (car range) "-r") (cadr range)) (cadr range)) | ||
| (revisions revisions) | ||
| (from (if (and removed from) from (or to from))) | ||
| (t "@"))))) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jj version control system diff removed lines default revset behavior
💡 Result:
In jj (Jujutsu), jj diff’s “removed lines” are always computed relative to whatever revisions you’re diffing:
-
Default revset/behavior: if you run
jj diffwith no options, it defaults to-r @. (docs.jj-vcs.dev)
That means: show the changes in the working-copy commit@compared to its parent(s) (for merges: compared to the merged parents). (docs.jj-vcs.dev)
So “removed lines” are lines present in@-(or the merged parents) but not in@. -
Meaning of
-rforjj diff:jj diff -r Ris a special case where-rmeans “compare revisionRto its parentR-”. (docs.jj-vcs.dev) -
If you want removals vs a different base, specify it explicitly, e.g.:
jj diff --from trunk() --to @(removed lines = removed sincetrunk()), orjj diff -r@-`` (removed lines introduced by the parent commit, relative to@--). (docs.jj-vcs.dev)
Citations:
- 1: https://docs.jj-vcs.dev/latest/cli-reference/?utm_source=openai
- 2: https://docs.jj-vcs.dev/latest/cli-reference/?utm_source=openai
- 3: https://docs.jj-vcs.dev/latest/guides/cli-revision-options/?utm_source=openai
- 4: https://docs.jj-vcs.dev/latest/cli-reference/?utm_source=openai
🌐 Web query:
jj vcs "@-" revision meaning parent diff old side
💡 Result:
In jj (Jujutsu) revset syntax:
@= the current working-copy commit in your workspace. (jj-vcs.github.io)x-= the parent(s) of revisionx, so@-means “the parent of the current working-copy commit.” (steveklabnik.github.io)
For diffs:
jj diff -r @(the default) shows the change from the parent tree to@’s tree. So in a typical diff viewer, the “old/left” side is@-and the “new/right” side is@. (jj-vcs.github.io)- More explicitly, you can do
jj diff --from@---to @. (--fromis the “old” side;--tois the “new” side.) (jj-vcs.github.io) - If
@is a merge commit, the “old” side is the automatic merge of all parents’ contents, compared to@. (jj-vcs.github.io)
Citations:
- 1: https://jj-vcs.github.io/jj/latest/revsets/?utm_source=openai
- 2: https://steveklabnik.github.io/jujutsu-tutorial/branching-merging-and-conflicts/revsets.html?utm_source=openai
- 3: https://jj-vcs.github.io/jj/latest/cli-reference/?utm_source=openai
- 4: https://jj-vcs.github.io/jj/latest/cli-reference/?utm_source=openai
- 5: https://jj-vcs.github.io/jj/latest/cli-reference/?utm_source=openai
Default diff revset for removed lines should be @-, not @.
Line 124 incorrectly falls back to @ for removed lines when no explicit range is given. Since jj diff defaults to comparing @- (old/parent side) against @ (new side), removed lines originate from @- and should resolve to that revision. Opening @ instead shows the wrong (new) side where the lines don't exist.
Proposed fix
(cond
((and range (equal (car range) "-r") (cadr range)) (cadr range))
(revisions revisions)
(from (if (and removed from) from (or to from)))
+ ((and removed (null range)) "@-")
(t "@")))))🤖 Prompt for AI Agents
In `@majutsu-file.el` around lines 113 - 125, The fallback revset in
majutsu-file--diff-revset returns "@" for lines with no explicit range; change
the final fallback to return "@-" for removed lines. Update the final cond
clause in majutsu-file--diff-revset (currently (t "@")) to check the local
removed variable and return (if removed "@-" "@") so removed lines resolve to
the parent revision.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majutsu-evil.el (1)
97-128:⚠️ Potential issue | 🟡 MinorUpdate README keymap docs for new/adjusted bindings.
The new navigation bindings should be reflected in
README.orgto keep docs accurate.Based on learnings: Ensure
README.orgreflects real keymaps and transients after every binding change.
- Add majutsu-file.el for viewing files at revisions - Support blob buffer navigation (prev/next change) - Smart diff visit behavior (worktree vs blob) - Line mapping between revisions via diff hunks - Add change-at-point detection for edit/new commands - Add evil keybindings for blob mode
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.