From d675476148e63802274e2948bd31bac445453528 Mon Sep 17 00:00:00 2001 From: David Yonge-Mallo Date: Sat, 4 Jul 2026 10:13:28 +0200 Subject: [PATCH] refactor(jj): tidy single-change diff labelling Promote `rev_to_panel_name` to a `VCSAdapter` method with a default fallback. Reimplement `single_change_subject` on `jj show --no-patch`. --- lua/diffview/api/views/diff/diff_view.lua | 19 +---- lua/diffview/scene/views/diff/diff_view.lua | 21 +++--- .../tests/functional/jj_adapter_spec.lua | 69 ++++++++++++++++--- .../tests/functional/set_revs_spec.lua | 32 +++------ lua/diffview/vcs/adapter.lua | 4 +- lua/diffview/vcs/adapters/jj/init.lua | 52 ++++++++------ 6 files changed, 112 insertions(+), 85 deletions(-) diff --git a/lua/diffview/api/views/diff/diff_view.lua b/lua/diffview/api/views/diff/diff_view.lua index ca466cb8..af655ed5 100644 --- a/lua/diffview/api/views/diff/diff_view.lua +++ b/lua/diffview/api/views/diff/diff_view.lua @@ -7,7 +7,6 @@ require("diffview.bootstrap") local DiffView = lazy.access("diffview.scene.views.diff.diff_view", "DiffView") ---@type DiffView|LazyModule local File = lazy.access("diffview.vcs.file", "File") ---@type vcs.File|LazyModule local FileEntry = lazy.access("diffview.scene.file_entry", "FileEntry") ---@type FileEntry|LazyModule -local FilePanel = lazy.access("diffview.scene.views.diff.file_panel", "FilePanel") ---@type FilePanel|LazyModule local Rev = lazy.access("diffview.vcs.adapters.git.rev", "GitRev") ---@type GitRev|LazyModule local RevType = lazy.access("diffview.vcs.rev", "RevType") ---@type RevType|LazyModule local vcs_utils = lazy.require("diffview.vcs") ---@module "diffview.vcs" @@ -18,14 +17,6 @@ local logger = DiffviewGlobal.logger local M = {} -local function rev_to_panel_name(adapter, rev_arg, left, right) - if adapter.rev_to_panel_name then - return adapter:rev_to_panel_name(rev_arg, left, right) - end - - return rev_arg or adapter:rev_to_pretty_string(left, right) -end - ---@class FileData ---@field path string Path relative to git root. ---@field oldpath string|nil If the file has been renamed, this should be the old path, oterhwise nil. @@ -69,15 +60,7 @@ function CDiffView:init(opt) self.fetch_files = opt.update_files self.get_file_data = opt.get_file_data - self:super(vim.tbl_extend("force", opt, { - adapter = adapter, - panel = FilePanel( - adapter, - self.files, - self.path_args, - rev_to_panel_name(adapter, self.rev_arg, opt.left, opt.right) - ), - })) + self:super(vim.tbl_extend("force", opt, { adapter = adapter })) if type(opt.files) == "table" and not vim.tbl_isempty(opt.files) then local files = self:create_file_entries(opt.files) diff --git a/lua/diffview/scene/views/diff/diff_view.lua b/lua/diffview/scene/views/diff/diff_view.lua index 24d72b34..5eac5b83 100644 --- a/lua/diffview/scene/views/diff/diff_view.lua +++ b/lua/diffview/scene/views/diff/diff_view.lua @@ -31,14 +31,6 @@ local M = {} local same_rev = lazy.access(rev_lib, "same_rev") --[[@as fun(a: Rev?, b: Rev?): boolean ]] -local function rev_to_panel_name(adapter, rev_arg, left, right) - if adapter.rev_to_panel_name then - return adapter:rev_to_panel_name(rev_arg, left, right) - end - - return rev_arg or adapter:rev_to_pretty_string(left, right) -end - ---@class DiffViewOptions ---@field show_untracked? boolean ---@field selected_file? string Path to the preferred initially selected file. @@ -84,7 +76,7 @@ function DiffView:init(opt) self.adapter, self.files, self.path_args, - rev_to_panel_name(self.adapter, self.rev_arg, self.left, self.right) + self.adapter:rev_to_panel_name(self.rev_arg, self.left, self.right) ), }) @@ -407,7 +399,7 @@ function DiffView:set_revs(new_rev_arg, opts) self.rev_arg = new_rev_arg self.left = new_left self.right = new_right - self.panel.rev_pretty_name = rev_to_panel_name(self.adapter, new_rev_arg, self.left, self.right) + self.panel.rev_pretty_name = self.adapter:rev_to_panel_name(new_rev_arg, self.left, self.right) -- Migrate selection persistence to the new scope key. if self._selection_scope_key then @@ -732,8 +724,13 @@ local update_files_impl = debounce.debounce_trailing( perf:lap("updated head rev") end - self.panel.rev_pretty_name = - rev_to_panel_name(self.adapter, self.rev_arg, self.left, self.right) + -- Recompute the panel label unconditionally: adapter-resolved metadata + -- (e.g., a jj change description) can shift even when `left`/`right` + -- are structurally unchanged, e.g., `jj describe` on `@` rewrites the + -- description but leaves the LOCAL rev's `object_name()` fixed at + -- "UNKNOWN", so `refresh_revs` returns nil for that case. Placed after + -- the head-rev update so a HEAD move is reflected in the label too. + self.panel.rev_pretty_name = self.adapter:rev_to_panel_name(self.rev_arg, self.left, self.right) perf:lap("updated rev label") local index_stat = pl:stat(pl:join(self.adapter.ctx.dir, "index")) diff --git a/lua/diffview/tests/functional/jj_adapter_spec.lua b/lua/diffview/tests/functional/jj_adapter_spec.lua index 91c39260..05c7c67e 100644 --- a/lua/diffview/tests/functional/jj_adapter_spec.lua +++ b/lua/diffview/tests/functional/jj_adapter_spec.lua @@ -627,46 +627,93 @@ describe("diffview.vcs.adapters.jj", function() "update file", adapter:rev_to_panel_name(left:abbrev() .. ".." .. right:abbrev(), left, right) ) + end) - local non_parent = adapter.Rev(RevType.COMMIT, string.rep("1", 40)) - eq("main..@", adapter:rev_to_panel_name("main..@", non_parent, right)) + it("substitutes the description for a LOCAL right (`:DiffviewOpen`)", function() + if not jj_available() then + pending("jj not installed") + return + end + + repo.write("file.txt", "first\n") + repo.jj({ "describe", "-m", "initial" }) + repo.jj({ "new" }) + repo.write("file.txt", "second\n") + repo.jj({ "describe", "-m", "working copy change" }) + + local adapter = repo.adapter() + local parent_hash = + run({ "jj", "show", "-T", "parents.first().commit_id()", "@", "--no-patch" }, repo.dir) + local left = adapter.Rev(RevType.COMMIT, parent_hash) + local right = adapter.Rev(RevType.LOCAL) + + eq("working copy change", adapter:rev_to_panel_name(nil, left, right)) end) - it("uses the current change description for a default no-arg diff", function() + it("includes the description for an initial-commit diff (root..@)", function() if not jj_available() then pending("jj not installed") return end + repo.write("file.txt", "first\n") + repo.jj({ "describe", "-m", "the very first change" }) + + local adapter = repo.adapter() + local commit_hash = run({ "jj", "show", "-T", "commit_id", "@", "--no-patch" }, repo.dir) + -- Initial commit's parent is the synthetic root; its commit_id IS + -- `NULL_TREE_SHA`. The description should still surface. + local left = adapter.Rev(RevType.COMMIT, adapter.Rev.NULL_TREE_SHA) + local right = adapter.Rev(RevType.COMMIT, commit_hash) + + eq("the very first change", adapter:rev_to_panel_name(nil, left, right)) + end) + + it("routes through `parse_revs` for the working-copy `:DiffviewOpen` case", function() + if not jj_available() then + pending("jj not installed") + return + end + + -- End-to-end coverage of the primary use case: `parse_revs(nil, {})` + -- returns a LOCAL right whose `object_name()` is `"UNKNOWN"`, so + -- `single_change_subject` must reach jj via the `@` fallback. repo.write("file.txt", "first\n") repo.jj({ "describe", "-m", "initial" }) repo.jj({ "new" }) repo.write("file.txt", "second\n") - repo.jj({ "describe", "-m", "current change" }) + repo.jj({ "describe", "-m", "working copy change" }) local adapter = repo.adapter() local left, right = adapter:parse_revs(nil, {}) - eq(RevType.COMMIT, left.type) - eq(RevType.LOCAL, right.type) - eq("current change", adapter:rev_to_panel_name(nil, left, right)) + eq("working copy change", adapter:rev_to_panel_name(nil, left, right)) end) - it("falls back for null-tree ranges", function() + it("falls back to `rev_arg` when left is not a parent of right", function() if not jj_available() then pending("jj not installed") return end + -- Three-commit chain so grandparent is a real (non-null-tree) commit. repo.write("file.txt", "first\n") - repo.jj({ "describe", "-m", "initial" }) + repo.jj({ "describe", "-m", "first" }) + repo.jj({ "new" }) + repo.write("file.txt", "second\n") + repo.jj({ "describe", "-m", "second" }) + repo.jj({ "new" }) + repo.write("file.txt", "third\n") + repo.jj({ "describe", "-m", "third" }) local adapter = repo.adapter() + local grandparent_hash = + run({ "jj", "show", "-T", "commit_id", "@--", "--no-patch" }, repo.dir) local commit_hash = run({ "jj", "show", "-T", "commit_id", "@", "--no-patch" }, repo.dir) - local left = adapter.Rev.new_null_tree() + local non_parent = adapter.Rev(RevType.COMMIT, grandparent_hash) local right = adapter.Rev(RevType.COMMIT, commit_hash) - eq("root range", adapter:rev_to_panel_name("root range", left, right)) + eq("main..@", adapter:rev_to_panel_name("main..@", non_parent, right)) end) end) diff --git a/lua/diffview/tests/functional/set_revs_spec.lua b/lua/diffview/tests/functional/set_revs_spec.lua index f9197446..8af64597 100644 --- a/lua/diffview/tests/functional/set_revs_spec.lua +++ b/lua/diffview/tests/functional/set_revs_spec.lua @@ -62,6 +62,14 @@ describe("DiffView:set_revs", function() rev_to_pretty_string = function(_, left, right) return (left.commit or "?") .. ".." .. (right.commit or "?") end, + -- Distinctive sentinel that encodes all three inputs so tests catch + -- both a regression that copies raw `rev_arg` into the panel without + -- consulting the adapter, and one that forwards the wrong `left`/ + -- `right` (e.g., pre-update values). + rev_to_panel_name = function(self, rev_arg, left, right) + return rev_arg + and ("PANEL[" .. rev_arg .. "|" .. self:rev_to_pretty_string(left, right) .. "]") + end, instanceof = function() return false end, @@ -131,27 +139,9 @@ describe("DiffView:set_revs", function() view:set_revs("ccc333..ddd444") - eq("ccc333..ddd444", view.panel.rev_pretty_name) - end) - - it("uses adapter panel labels when available", function() - local old_left = make_rev("aaa111") - local old_right = make_rev("bbb222") - local new_left = make_rev("ccc333") - local new_right = make_rev("ddd444") - - local adapter = make_adapter({ - ["ccc333..ddd444"] = { new_left, new_right }, - }) - adapter.rev_to_panel_name = function(_, rev_arg, left, right) - return table.concat({ "label", rev_arg, left.commit, right.commit }, ":") - end - - local view = make_view(adapter, old_left, old_right, "aaa111..bbb222") - - view:set_revs("ccc333..ddd444") - - eq("label:ccc333..ddd444:ccc333:ddd444", view.panel.rev_pretty_name) + -- Prove the adapter method is consulted with the NEW `left`/`right` + -- (post-update); the mock's sentinel encodes all three inputs. + eq("PANEL[ccc333..ddd444|ccc333..ddd444]", view.panel.rev_pretty_name) end) it("does nothing when parse_revs fails", function() diff --git a/lua/diffview/vcs/adapter.lua b/lua/diffview/vcs/adapter.lua index e9e8972a..12d02093 100644 --- a/lua/diffview/vcs/adapter.lua +++ b/lua/diffview/vcs/adapter.lua @@ -654,8 +654,8 @@ function VCSAdapter:rev_to_pretty_string(left, right) 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. ----The default prefers the user's rev_arg; adapters may substitute a +---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. ---@param rev_arg string? ---@param left Rev diff --git a/lua/diffview/vcs/adapters/jj/init.lua b/lua/diffview/vcs/adapters/jj/init.lua index 9c814369..1dba31e1 100644 --- a/lua/diffview/vcs/adapters/jj/init.lua +++ b/lua/diffview/vcs/adapters/jj/init.lua @@ -576,33 +576,32 @@ end ---@param right Rev ---@return string|nil function JjAdapter:single_change_subject(left, right) - if not (left and left.commit and right) then + -- Preconditions: `left` is a resolved commit, `right` is either a resolved + -- commit or LOCAL (which spells `@` on the CLI); reject the null-tree + -- sentinel because `jj show` on it emits a template error. + if not left.commit or right.commit == JjRev.NULL_TREE_SHA then return end - - if left.commit == JjRev.NULL_TREE_SHA then - return - end - - local right_rev - if right.type == RevType.LOCAL then - right_rev = "@" - elseif right.commit and right.commit ~= JjRev.NULL_TREE_SHA then - right_rev = right.commit - else + local right_arg = right.commit or (right.type == RevType.LOCAL and "@") + if not right_arg then return end + -- Emit `parent_hash\ndescription`. `\n` is officially supported in jj + -- template strings, and `description.first_line()` guarantees no embedded + -- newlines, so the output splits cleanly across two lines. `self:args()` + -- carries any user-configured global flags (e.g., `--repository`) ahead + -- of the subcommand, matching how `get_show_args`/`get_log_args` build + -- their invocations. local out, code = self:exec_sync( utils.vec_join( self:args(), "--ignore-working-copy", - "log", - "--no-graph", - "-r", - right_rev, + "show", + "--no-patch", "-T", - [[parents.first().commit_id() ++ "\x1f" ++ description.first_line() ++ "\n"]] + [[parents.first().commit_id() ++ "\n" ++ description.first_line()]], + right_arg ), { cwd = self.ctx.toplevel, @@ -612,19 +611,30 @@ function JjAdapter:single_change_subject(left, right) } ) - if code ~= 0 or not out[1] then + -- Explicit `not out` catches the bootstrap-fail path where `exec_sync` + -- returns bare nils; we don't want to rely on `code ~= 0`'s short-circuit + -- to shield the following `out[1]` index. + if code ~= 0 or not out or not out[1] then return end - local parent, subject = vim.trim(out[1]):match("^([^\31]*)\31(.*)$") + local parent = vim.trim(out[1]) if parent ~= left.commit then return end - subject = vim.trim(subject or "") - return subject ~= "" and subject or nil + local subject = vim.trim(out[2] or "") + if subject == "" then + return + end + + return subject 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. +---For a single-change range, use the change description; otherwise fall +---back to the caller's `rev_arg` or the default pretty string. ---@param rev_arg string? ---@param left Rev ---@param right Rev