jj: label single-change diffs with the description#250
Conversation
dlyongemallo
left a comment
dlyongemallo
left a comment
There was a problem hiding this comment.
Please fix the failing tests and also change the git commit message so that its format is consistent with this repo's conventions.
| ---@param right Rev | ||
| ---@return string|nil | ||
| function JjAdapter:single_change_subject(left, right) | ||
| if not (left.commit and right.commit) then |
There was a problem hiding this comment.
You need to check for JjRev.NULL_TREE_SHA here, as it's a non-empty string (hence truthy). Otherwise jj show will run for a root commit diff.
| return nil | ||
| end | ||
|
|
||
| ---Convert revs to the string shown in the file panel. |
There was a problem hiding this comment.
Tighten the base docstring here so it's clear that the changed behaviour in the JjAdapter is intentional:
---Convert revs to the display-only label shown in the file panel.
---Unlike rev_to_pretty_string the result need not be parseable as a rev.
---The default prefers the user's rev_arg; adapters may substitute a
---friendlier label.There was a problem hiding this comment.
Pull request overview
This PR introduces a new adapter hook (rev_to_panel_name) to control the “Showing changes for” label in the diff file panel, enabling the jj adapter to display a single-change diff’s change description instead of the raw revision argument.
Changes:
- Add
VCSAdapter:rev_to_panel_name(rev_arg, left, right)(defaulting to the prior behavior). - Implement jj-specific panel naming by detecting single-parent ranges and using the change description’s first line.
- Switch DiffView construction/update paths to use
rev_to_panel_name, and add a functional jj test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lua/diffview/vcs/adapters/jj/init.lua | Adds jj logic to extract a single change’s description and overrides rev_to_panel_name. |
| lua/diffview/vcs/adapter.lua | Introduces the new rev_to_panel_name hook with a default implementation. |
| lua/diffview/tests/functional/jj_adapter_spec.lua | Adds integration coverage for jj’s rev_to_panel_name behavior on single-change ranges. |
| lua/diffview/scene/views/diff/diff_view.lua | Uses rev_to_panel_name for the file panel label across init / set_revs / refresh flows. |
| lua/diffview/api/views/diff/diff_view.lua | Updates API DiffView construction to use rev_to_panel_name for the file panel label. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not self.rev_arg then | ||
| self.panel.rev_pretty_name = self.adapter:rev_to_pretty_string(self.left, self.right) | ||
| self.panel.rev_pretty_name = self.adapter:rev_to_panel_name(nil, self.left, self.right) | ||
| end |
| panel = FilePanel( | ||
| self.adapter, | ||
| self.files, | ||
| self.path_args, | ||
| self.rev_arg or self.adapter:rev_to_pretty_string(self.left, self.right) | ||
| self.adapter:rev_to_panel_name(self.rev_arg, self.left, self.right) | ||
| ), |
| self.left = new_left | ||
| self.right = new_right | ||
| self.panel.rev_pretty_name = new_rev_arg | ||
| self.panel.rev_pretty_name = self.adapter:rev_to_panel_name(new_rev_arg, self.left, self.right) |
| panel = FilePanel( | ||
| adapter, | ||
| self.files, | ||
| self.path_args, | ||
| self.rev_arg or adapter:rev_to_pretty_string(opt.left, opt.right) | ||
| adapter:rev_to_panel_name(self.rev_arg, opt.left, opt.right) | ||
| ), |
There was a problem hiding this comment.
self.files, sefl.path_args, and self.rev_arg are all nil here, as they're not assigned until Diffview:init() runs. Actually, why is panel even being constructed here? Is something relying on opt.panel being set before Diffview:init() is called?
|
@wuzzeb Are you still working on this? I've made a number of changes to the |
|
Yeah sorry I've been travelling, I will work on it in the next few days |
|
Since a bunch changed in main I just rebased and force-pushed. I like the overall jj changes you made in main and this now compliments them with adding the description. |
| "-r", | ||
| right_rev, | ||
| "-T", | ||
| [[parents.first().commit_id() ++ "\x1f" ++ description.first_line() ++ "\n"]] |
There was a problem hiding this comment.
This seems fragile. It's depending the literal string \x1f being interpreted by jj in its template strings. Why not just use \n? Why leak a control character into the label which then has to be stripped out (line 610)? I think the way you've done this enables an invisible glyph to accidentally be copied by a user, which can lead to mysterious bugs.
| } | ||
| ) | ||
|
|
||
| if code ~= 0 or not out[1] then |
There was a problem hiding this comment.
Can't out itself be nil, or out[1] be ""?
| return | ||
| end | ||
|
|
||
| if left.commit == JjRev.NULL_TREE_SHA then |
There was a problem hiding this comment.
This drops the initial commit description. Is this intentional?
| panel = FilePanel( | ||
| adapter, | ||
| self.files, | ||
| self.path_args, | ||
| self.rev_arg or adapter:rev_to_pretty_string(opt.left, opt.right) | ||
| adapter:rev_to_panel_name(self.rev_arg, opt.left, opt.right) | ||
| ), |
There was a problem hiding this comment.
self.files, sefl.path_args, and self.rev_arg are all nil here, as they're not assigned until Diffview:init() runs. Actually, why is panel even being constructed here? Is something relying on opt.panel being set before Diffview:init() is called?
| end | ||
|
|
||
| ---Convert revs to the display-only label shown in the file panel. | ||
| ---Unlike rev_to_pretty_string the result need not be parseable as a rev. |
There was a problem hiding this comment.
Should have backticks around rev_to_pretty_string and rev_arg.
| return | ||
| end | ||
|
|
||
| subject = vim.trim(subject or "") |
There was a problem hiding this comment.
subject can't be nil here (because return would've happened on line 612).
dlyongemallo
dismissed
their stale review
I'll make the minor follow-up changes myself.
Diffview currently uses the raw rev argument as the file-panel "Showing changes for" label whenever :DiffviewOpen is called with an explicit revision argument. Since with jj you are running
jj descas the work progresses, the label can instead show the description of the change. This needs a new hook rev_to_panel_name because the existing rev_to_pretty_string is used as parsable rev text in other code paths (such as opening history), so rev_to_pretty_string must keep the string as a valid revision.The new rev_to_panel_name hook is added and is identical in behavior to the existing code for all other adapters besides jj. For jj, we detect if the current diff is a single change and if so show the description. Although, now that I think about it, maybe we could expand and list all the changes in the diff? Well, this is a good first step.