-
Notifications
You must be signed in to change notification settings - Fork 5
feat(annotate): add majutsu-annotate.el #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: majutsu-file
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds annotation (blame) support to Majutsu, integrating it with the existing blob buffer functionality and providing convenient keybindings.
Changes:
- Introduces majutsu-annotate.el with a complete annotation system using overlays and chunk-based navigation
- Enhances blob buffers with the ability to visit workspace files and annotate from blob buffers
- Adds commit info display when navigating between file revisions in blob mode
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| majutsu-annotate.el | New file implementing jj file annotate with overlay-based UI, chunk navigation, and integration with blob/workspace files |
| majutsu.el | Adds require statement for the new majutsu-annotate module |
| majutsu-new.el | Adds blob buffer variable declarations and auto-visit workspace file after creating new changeset from blob |
| majutsu-file.el | Refactors blob-quit, adds blob-visit-file, commit info display, and annotate keybinding |
| majutsu-evil.el | Adds evil keybinding for majutsu-annotate-addition |
| majutsu-edit.el | Adds auto-visit workspace file when editing changeset from blob buffer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (let ((in-blob (and (bound-and-true-p majutsu-blob-mode) | ||
| majutsu-buffer-blob-root | ||
| majutsu-buffer-blob-path))) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing forward declarations for majutsu-buffer-blob-root and majutsu-buffer-blob-path. These variables are defined in majutsu-file.el but not declared here. Add (defvar majutsu-buffer-blob-root) and (defvar majutsu-buffer-blob-path) at the top of the file after the require statements to silence byte-compiler warnings.
| (defgroup majutsu-annotate nil | ||
| "Annotate (blame) support for Majutsu." | ||
| :group 'majutsu) | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing forward declarations for blob buffer variables and functions. Add the following declarations after line 28:
(defvar majutsu-buffer-blob-root)(defvar majutsu-buffer-blob-path)(defvar majutsu-buffer-blob-revision)(declare-function majutsu-read-revset "majutsu-log" (prompt &optional default))
These are used in majutsu-annotate-addition but not declared, which will cause byte-compiler warnings.
| (defvar majutsu-buffer-blob-root) | |
| (defvar majutsu-buffer-blob-path) | |
| (defvar majutsu-buffer-blob-revision) | |
| (declare-function majutsu-read-revset "majutsu-log" (prompt &optional default)) |
| (if-let* ((next (next-single-char-property-change | ||
| (point) 'majutsu-annotate-chunk))) | ||
| (goto-char next) | ||
| (user-error "No more chunks"))) | ||
|
|
||
| (defun majutsu-annotate-previous-chunk () | ||
| "Move to the previous annotation chunk." | ||
| (interactive) | ||
| (if-let* ((prev (previous-single-char-property-change | ||
| (point) 'majutsu-annotate-chunk))) | ||
| (goto-char prev) | ||
| (user-error "No more chunks"))) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chunk navigation functions use next-single-char-property-change and previous-single-char-property-change which search for text properties, but majutsu-annotate-chunk is stored as an overlay property (see line 207), not a text property. This means these navigation functions will not work correctly.
To fix this, you have two options:
- Store the chunk as a text property in addition to the overlay property
- Implement custom navigation that searches through overlays
Option 1 is simpler: add (put-text-property beg end 'majutsu-annotate-chunk chunk) after line 207.
| (if-let* ((next (next-single-char-property-change | |
| (point) 'majutsu-annotate-chunk))) | |
| (goto-char next) | |
| (user-error "No more chunks"))) | |
| (defun majutsu-annotate-previous-chunk () | |
| "Move to the previous annotation chunk." | |
| (interactive) | |
| (if-let* ((prev (previous-single-char-property-change | |
| (point) 'majutsu-annotate-chunk))) | |
| (goto-char prev) | |
| (user-error "No more chunks"))) | |
| (let ((pos (point)) | |
| (limit (point-max)) | |
| (found nil)) | |
| (while (and (< pos limit) (not found)) | |
| (setq pos (next-overlay-change pos)) | |
| (when (< pos limit) | |
| (when (cl-find-if (lambda (ov) | |
| (overlay-get ov 'majutsu-annotate-chunk)) | |
| (overlays-at pos)) | |
| (setq found pos)))) | |
| (if found | |
| (goto-char found) | |
| (user-error "No more chunks"))) | |
| (defun majutsu-annotate-previous-chunk () | |
| "Move to the previous annotation chunk." | |
| (interactive) | |
| (let ((pos (point)) | |
| (limit (point-min)) | |
| (found nil)) | |
| (while (and (> pos limit) (not found)) | |
| (setq pos (previous-overlay-change pos)) | |
| (when (> pos limit) | |
| (when (cl-find-if (lambda (ov) | |
| (overlay-get ov 'majutsu-annotate-chunk)) | |
| (overlays-at pos)) | |
| (setq found pos)))) | |
| (if found | |
| (goto-char found) | |
| (user-error "No more chunks"))) |
| (defun majutsu-annotate-toggle-read-only () | ||
| "Toggle read-only mode for annotation." | ||
| (majutsu-annotate-read-only-mode (if buffer-read-only 1 -1))) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majutsu-annotate-toggle-read-only function is defined but never used. It's not bound to any key in the keymaps and not called anywhere. Consider either:
- Adding a keybinding for it (e.g., "t" in
majutsu-annotate-read-only-mode-map) - Removing it if it's not intended to be used
| (defcustom majutsu-annotate-time-format "%F %H:%M" | ||
| "Format for time strings in annotation headings." | ||
| :type 'string | ||
| :group 'majutsu-annotate) | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majutsu-annotate-time-format custom variable is defined but never used in the code. The annotation heading uses the raw timestamp from jj output without formatting it with this variable. Either:
- Apply this format to the timestamp in
majutsu-annotate--format-heading(around line 221) - Remove this unused custom variable if timestamp formatting is not intended
| (defcustom majutsu-annotate-time-format "%F %H:%M" | |
| "Format for time strings in annotation headings." | |
| :type 'string | |
| :group 'majutsu-annotate) |
| ;;; majutsu-annotate.el --- Blame support for Majutsu -*- lexical-binding: t; -*- | ||
|
|
||
| ;; Copyright (C) 2026 0WD0 | ||
|
|
||
| ;; Author: 0WD0 <[email protected]> | ||
| ;; Maintainer: 0WD0 <[email protected]> | ||
| ;; Keywords: tools, vc | ||
| ;; URL: https://github.com/0WD0/majutsu | ||
|
|
||
| ;;; Commentary: | ||
|
|
||
| ;; Annotates each line in file-visiting buffer with information from | ||
| ;; the revision which last modified the line, using `jj file annotate'. | ||
|
|
||
| ;;; Code: | ||
|
|
||
| (require 'cl-lib) | ||
| (require 'subr-x) | ||
| (require 'majutsu-base) | ||
| (require 'majutsu-jj) | ||
| (require 'majutsu-process) | ||
|
|
||
| (declare-function majutsu-diff-revset "majutsu-diff" (revset &optional args range filesets)) | ||
|
|
||
| ;;; Options | ||
|
|
||
| (defgroup majutsu-annotate nil | ||
| "Annotate (blame) support for Majutsu." | ||
| :group 'majutsu) | ||
|
|
||
| (defcustom majutsu-annotate-heading-format "%-8a %C %s\n" | ||
| "Format for annotation headings. | ||
| The following %-specs are supported: | ||
| %h change-id (short) | ||
| %a author | ||
| %C committer time | ||
| %s summary (first line of description)" | ||
| :type 'string | ||
| :group 'majutsu-annotate) | ||
|
|
||
| (defcustom majutsu-annotate-time-format "%F %H:%M" | ||
| "Format for time strings in annotation headings." | ||
| :type 'string | ||
| :group 'majutsu-annotate) | ||
|
|
||
| (defcustom majutsu-annotate-read-only t | ||
| "Whether to initially make the annotated buffer read-only." | ||
| :type 'boolean | ||
| :group 'majutsu-annotate) | ||
|
|
||
| (defcustom majutsu-annotate-mode-lighter " Annotate" | ||
| "Mode-line lighter for `majutsu-annotate-mode'." | ||
| :type '(choice (const :tag "No lighter" "") string) | ||
| :group 'majutsu-annotate) | ||
|
|
||
| ;;; Faces | ||
|
|
||
| (defface majutsu-annotate-highlight | ||
| '((((class color) (background light)) | ||
| :extend t :background "grey80" :foreground "black") | ||
| (((class color) (background dark)) | ||
| :extend t :background "grey25" :foreground "white")) | ||
| "Face used for highlighting when annotating." | ||
| :group 'majutsu-faces) | ||
|
|
||
| (defface majutsu-annotate-heading | ||
| '((t :extend t :inherit majutsu-annotate-highlight | ||
| :weight normal :slant normal)) | ||
| "Face used for annotation headings." | ||
| :group 'majutsu-faces) | ||
|
|
||
| (defface majutsu-annotate-hash '((t nil)) | ||
| "Face used for change-ids when annotating." | ||
| :group 'majutsu-faces) | ||
|
|
||
| (defface majutsu-annotate-name '((t nil)) | ||
| "Face used for author names when annotating." | ||
| :group 'majutsu-faces) | ||
|
|
||
| (defface majutsu-annotate-date '((t nil)) | ||
| "Face used for dates when annotating." | ||
| :group 'majutsu-faces) | ||
|
|
||
| (defface majutsu-annotate-summary '((t nil)) | ||
| "Face used for commit summaries when annotating." | ||
| :group 'majutsu-faces) | ||
|
|
||
| ;;; Variables | ||
|
|
||
| (defvar-local majutsu-annotate-buffer-read-only nil) | ||
| (defvar-local majutsu-annotate--chunks nil | ||
| "List of annotation chunks in current buffer.") | ||
| (defvar-local majutsu-annotate--previous-chunk nil) | ||
|
|
||
| ;;; Chunk structure | ||
|
|
||
| (cl-defstruct majutsu-annotate-chunk | ||
| change-id author timestamp line-beg line-end description) | ||
|
|
||
| ;;; Keymaps | ||
|
|
||
| (defvar-keymap majutsu-annotate-mode-map | ||
| :doc "Keymap for `majutsu-annotate-mode'." | ||
| "C-c C-q" #'majutsu-annotate-quit) | ||
|
|
||
| (defvar-keymap majutsu-annotate-read-only-mode-map | ||
| :doc "Keymap for `majutsu-annotate-read-only-mode'." | ||
| "RET" #'majutsu-annotate-show-commit | ||
| "p" #'majutsu-annotate-previous-chunk | ||
| "n" #'majutsu-annotate-next-chunk | ||
| "b" #'majutsu-annotate-addition | ||
| "q" #'majutsu-annotate-quit | ||
| "M-w" #'majutsu-annotate-copy-hash) | ||
|
|
||
| ;;; Modes | ||
|
|
||
| (define-minor-mode majutsu-annotate-mode | ||
| "Display annotation information inline." | ||
| :lighter majutsu-annotate-mode-lighter | ||
| :interactive nil | ||
| (cond (majutsu-annotate-mode | ||
| (add-hook 'post-command-hook #'majutsu-annotate-goto-chunk-hook nil t) | ||
| (setq majutsu-annotate-buffer-read-only buffer-read-only) | ||
| (when majutsu-annotate-read-only | ||
| (read-only-mode 1))) | ||
| (t | ||
| (remove-hook 'post-command-hook #'majutsu-annotate-goto-chunk-hook t) | ||
| (unless majutsu-annotate-buffer-read-only | ||
| (read-only-mode -1)) | ||
| (majutsu-annotate-read-only-mode -1) | ||
| (majutsu-annotate--remove-overlays) | ||
| (setq majutsu-annotate--chunks nil)))) | ||
|
|
||
| (define-minor-mode majutsu-annotate-read-only-mode | ||
| "Provide keybindings for Majutsu-Annotate mode. | ||
| \\{majutsu-annotate-read-only-mode-map}") | ||
|
|
||
| (defun majutsu-annotate-toggle-read-only () | ||
| "Toggle read-only mode for annotation." | ||
| (majutsu-annotate-read-only-mode (if buffer-read-only 1 -1))) | ||
|
|
||
| (defun majutsu-annotate-goto-chunk-hook () | ||
| "Hook to run when point moves to a different chunk." | ||
| (when-let* ((chunk (majutsu-annotate-chunk-at (point)))) | ||
| (unless (eq chunk majutsu-annotate--previous-chunk) | ||
| (setq majutsu-annotate--previous-chunk chunk)))) | ||
|
|
||
| ;;; Parsing | ||
|
|
||
| (defun majutsu-annotate--parse-output (output) | ||
| "Parse OUTPUT from `jj file annotate' into chunks." | ||
| (let ((lines (split-string output "\n" t)) | ||
| chunks current-chunk current-change-id) | ||
| (dolist (line lines) | ||
| ;; Format: "CHANGE_ID AUTHOR DATE LINE_NUM: CONTENT" | ||
| ;; Example: "qpvuntsm foo 2001-02-03 08:05:08 1: line1" | ||
| (when (string-match | ||
| "^\\([a-z]+\\)\\s-+\\([^ ]+\\)\\s-+\\([0-9-]+\\s-+[0-9:]+\\)\\s-+\\([0-9]+\\):" | ||
| line) | ||
| (let ((change-id (match-string 1 line)) | ||
| (author (match-string 2 line)) | ||
| (timestamp (match-string 3 line)) | ||
| (line-num (string-to-number (match-string 4 line)))) | ||
| (if (and current-chunk (equal change-id current-change-id)) | ||
| ;; Extend current chunk | ||
| (setf (majutsu-annotate-chunk-line-end current-chunk) line-num) | ||
| ;; Start new chunk | ||
| (when current-chunk | ||
| (push current-chunk chunks)) | ||
| (setq current-change-id change-id) | ||
| (setq current-chunk | ||
| (make-majutsu-annotate-chunk | ||
| :change-id change-id | ||
| :author author | ||
| :timestamp timestamp | ||
| :line-beg line-num | ||
| :line-end line-num)))))) | ||
| (when current-chunk | ||
| (push current-chunk chunks)) | ||
| (nreverse chunks))) | ||
|
|
||
| (defun majutsu-annotate--get-description (change-id) | ||
| "Get the description for CHANGE-ID." | ||
| (string-trim | ||
| (majutsu-jj-string "log" "-r" change-id "--no-graph" "-T" "description"))) | ||
|
|
||
| ;;; Overlays | ||
|
|
||
| (defun majutsu-annotate--line-beginning-position (line) | ||
| "Return position of beginning of LINE." | ||
| (save-excursion | ||
| (goto-char (point-min)) | ||
| (forward-line (1- line)) | ||
| (point))) | ||
|
|
||
| (defun majutsu-annotate--make-overlays (chunks) | ||
| "Create overlays for CHUNKS." | ||
| (save-excursion | ||
| (save-restriction | ||
| (widen) | ||
| (dolist (chunk chunks) | ||
| (let* ((beg (majutsu-annotate--line-beginning-position | ||
| (majutsu-annotate-chunk-line-beg chunk))) | ||
| (end (majutsu-annotate--line-beginning-position | ||
| (1+ (majutsu-annotate-chunk-line-end chunk)))) | ||
| (ov (make-overlay beg end))) | ||
| (overlay-put ov 'majutsu-annotate-chunk chunk) | ||
| (overlay-put ov 'before-string | ||
| (majutsu-annotate--format-heading chunk)) | ||
| ;; Highlight first line | ||
| (let ((hl-ov (make-overlay beg (save-excursion | ||
| (goto-char beg) | ||
| (line-end-position))))) | ||
| (overlay-put hl-ov 'majutsu-annotate-highlight t) | ||
| (overlay-put hl-ov 'font-lock-face 'majutsu-annotate-highlight))))))) | ||
|
|
||
| (defun majutsu-annotate--format-heading (chunk) | ||
| "Format heading string for CHUNK." | ||
| (let* ((change-id (majutsu-annotate-chunk-change-id chunk)) | ||
| (author (majutsu-annotate-chunk-author chunk)) | ||
| (timestamp (majutsu-annotate-chunk-timestamp chunk)) | ||
| (desc (or (majutsu-annotate-chunk-description chunk) | ||
| (let ((d (majutsu-annotate--get-description change-id))) | ||
| (setf (majutsu-annotate-chunk-description chunk) d) | ||
| d))) | ||
| (summary (car (split-string desc "\n")))) | ||
| (propertize | ||
| (format-spec majutsu-annotate-heading-format | ||
| `((?h . ,(propertize change-id 'font-lock-face 'majutsu-annotate-hash)) | ||
| (?a . ,(propertize author 'font-lock-face 'majutsu-annotate-name)) | ||
| (?C . ,(propertize timestamp 'font-lock-face 'majutsu-annotate-date)) | ||
| (?s . ,(propertize summary 'font-lock-face 'majutsu-annotate-summary)))) | ||
| 'font-lock-face 'majutsu-annotate-heading))) | ||
|
|
||
| (defun majutsu-annotate--remove-overlays () | ||
| "Remove all annotation overlays." | ||
| (save-restriction | ||
| (widen) | ||
| (dolist (ov (overlays-in (point-min) (point-max))) | ||
| (when (or (overlay-get ov 'majutsu-annotate-chunk) | ||
| (overlay-get ov 'majutsu-annotate-highlight)) | ||
| (delete-overlay ov))))) | ||
|
|
||
| ;;; Chunk navigation | ||
|
|
||
| (defun majutsu-annotate-chunk-at (pos) | ||
| "Return the annotation chunk at POS." | ||
| (seq-some (lambda (ov) (overlay-get ov 'majutsu-annotate-chunk)) | ||
| (overlays-at pos))) | ||
|
|
||
| (defun majutsu-annotate-current-chunk () | ||
| "Return the annotation chunk at point." | ||
| (majutsu-annotate-chunk-at (point))) | ||
|
|
||
| ;;; Commands | ||
|
|
||
| ;;;###autoload | ||
| (defun majutsu-annotate-addition (&optional revision) | ||
| "Annotate the current file showing when each line was added. | ||
| With prefix argument, prompt for REVISION." | ||
| (interactive | ||
| (list (and current-prefix-arg | ||
| (majutsu-read-revset "Annotate from revision")))) | ||
| (unless (or buffer-file-name | ||
| (bound-and-true-p majutsu-buffer-blob-path)) | ||
| (user-error "Buffer is not visiting a file")) | ||
| (let* ((root (majutsu-toplevel)) | ||
| (file (or (and (bound-and-true-p majutsu-buffer-blob-path) | ||
| majutsu-buffer-blob-path) | ||
| (file-relative-name buffer-file-name root))) | ||
| (rev (or revision | ||
| (and (bound-and-true-p majutsu-buffer-blob-revision) | ||
| majutsu-buffer-blob-revision) | ||
| "@")) | ||
| (default-directory root) | ||
| (output (majutsu-jj-string "file" "annotate" "-r" rev file))) | ||
| (when (string-empty-p output) | ||
| (user-error "No annotation output")) | ||
| (let ((chunks (majutsu-annotate--parse-output output))) | ||
| (unless majutsu-annotate-mode | ||
| (majutsu-annotate-mode 1)) | ||
| (majutsu-annotate--remove-overlays) | ||
| (setq majutsu-annotate--chunks chunks) | ||
| (majutsu-annotate--make-overlays chunks) | ||
| (when majutsu-annotate-read-only | ||
| (majutsu-annotate-read-only-mode 1)) | ||
| (message "Annotating...done")))) | ||
|
|
||
| (defun majutsu-annotate-quit () | ||
| "Turn off Majutsu-Annotate mode." | ||
| (interactive) | ||
| (majutsu-annotate-mode -1)) | ||
|
|
||
| (defun majutsu-annotate-next-chunk () | ||
| "Move to the next annotation chunk." | ||
| (interactive) | ||
| (if-let* ((next (next-single-char-property-change | ||
| (point) 'majutsu-annotate-chunk))) | ||
| (goto-char next) | ||
| (user-error "No more chunks"))) | ||
|
|
||
| (defun majutsu-annotate-previous-chunk () | ||
| "Move to the previous annotation chunk." | ||
| (interactive) | ||
| (if-let* ((prev (previous-single-char-property-change | ||
| (point) 'majutsu-annotate-chunk))) | ||
| (goto-char prev) | ||
| (user-error "No more chunks"))) | ||
|
|
||
| (defun majutsu-annotate-show-commit () | ||
| "Show the commit for the current chunk." | ||
| (interactive) | ||
| (if-let* ((chunk (majutsu-annotate-current-chunk))) | ||
| (majutsu-diff-revset (majutsu-annotate-chunk-change-id chunk)) | ||
| (user-error "No chunk at point"))) | ||
|
|
||
| (defun majutsu-annotate-copy-hash () | ||
| "Copy the change-id of the current chunk to the kill ring." | ||
| (interactive) | ||
| (if (use-region-p) | ||
| (call-interactively #'copy-region-as-kill) | ||
| (if-let* ((chunk (majutsu-annotate-current-chunk))) | ||
| (kill-new (message "%s" (majutsu-annotate-chunk-change-id chunk))) | ||
| (user-error "No chunk at point")))) | ||
|
|
||
| ;;; _ | ||
| (provide 'majutsu-annotate) | ||
| ;;; majutsu-annotate.el ends here |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test coverage for the new majutsu-annotate.el module. Given that other modules in the codebase have test coverage (e.g., majutsu-bookmark-test.el, majutsu-diff-test.el), consider adding tests for:
majutsu-annotate--parse-outputwith various jj annotate output formats- Chunk navigation functions
- Edge cases like empty files or files with no annotations
| (blob-file (when (bound-and-true-p majutsu-blob-mode) | ||
| (expand-file-name majutsu-buffer-blob-path | ||
| majutsu-buffer-blob-root)))) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blob-file variable is computed even when majutsu-blob-mode is false (line 156), but it's only used when in-blob is true (line 166). Consider moving the blob-file computation inside the when in-blob block (line 165) to avoid unnecessary computation, similar to how it's done in majutsu-edit.el lines 52-53.
85f042d to
db956a1
Compare
db956a1 to
dca9db8
Compare
dca9db8 to
389975a
Compare
5f0afa0 to
f1a722f
Compare
22205bd to
07bcfaa
Compare
07bcfaa to
d3c5664
Compare
8e03c23 to
ffe7d5f
Compare
7aef861 to
e6e96e8
Compare
84cd2d1 to
2c4a75c
Compare
e6e96e8 to
e290101
Compare
0d6a949 to
6566835
Compare
6566835 to
117b817
Compare
No description provided.