From 953e3b7410ff259b5c20e5e68b5b132f99aa90d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20F=2E=20Bortl=C3=ADk?= Date: Wed, 19 Aug 2026 01:00:54 +0200 Subject: [PATCH] fix(jj): detect renamed files correctly --- .../tests/functional/jj_adapter_spec.lua | 183 ++++++++++++++++++ lua/diffview/vcs/adapters/jj/init.lua | 84 +++++--- 2 files changed, 245 insertions(+), 22 deletions(-) diff --git a/lua/diffview/tests/functional/jj_adapter_spec.lua b/lua/diffview/tests/functional/jj_adapter_spec.lua index 05c7c67e..914e956f 100644 --- a/lua/diffview/tests/functional/jj_adapter_spec.lua +++ b/lua/diffview/tests/functional/jj_adapter_spec.lua @@ -774,6 +774,148 @@ describe("diffview.vcs.adapters.jj", function() end) ) + it( + "reports the full old and new paths for a renamed file", + helpers.async_test(function() + if not jj_available() then + pending("jj not installed") + return + end + + -- Same directory, common suffix (`.lua`) -- this is the shape `jj diff + -- --summary` would abbreviate to `src/{main => renamed}.lua`. Content is left + -- untouched so jj's similarity heuristic reliably reports this as a rename + -- rather than add+delete. + repo.write("src/main.lua", 'print("v1")\n') + repo.jj({ "describe", "-m", "initial" }) + repo.jj({ "new" }) + + os.rename(repo.dir .. "/src/main.lua", repo.dir .. "/src/renamed.lua") + + local adapter = repo.adapter() + local left = adapter.Rev( + RevType.COMMIT, + run({ "jj", "show", "-T", "commit_id", "@-", "--no-patch" }, repo.dir) + ) + local right = adapter.Rev(RevType.LOCAL) + local args = adapter:rev_to_args(left, right) + + local err, files = await( + adapter:tracked_files( + left, + right, + args, + "working", + { default_layout = Diff2, merge_layout = Diff2 } + ) + ) + + assert.is_nil(err) + assert.equals(1, #files) + assert.equals("R", files[1].status) + assert.equals("src/renamed.lua", files[1].path) + assert.equals("src/main.lua", files[1].oldpath) + end) + ) + + it( + "reports the full old and new paths when they contain literal `{`/`}`", + helpers.async_test(function() + if not jj_available() then + pending("jj not installed") + return + end + + -- Regression coverage for a corner case that trips up a naive + -- `--summary` parser: renaming `a{x}/f.txt` to `a{y}/f.txt` renders + -- as `R {a{x} => a{y}}/f.txt`, where jj's own wrapping braces sit + -- right next to literal braces from the directory names. A parser + -- that just matches the first `{`/`}` pair gets this wrong (e.g. + -- recovering `a{x}}/f.txt` instead of `a{x}/f.txt`). `-T + -- TRACKED_FILES_TEMPLATE` sidesteps the problem entirely by reading + -- `source().path()`/`target().path()` directly, so this should + -- round-trip correctly regardless of what characters the path + -- contains. + repo.write("a{x}/f.txt", "hi\n") + repo.jj({ "describe", "-m", "add" }) + repo.jj({ "new" }) + + vim.fn.mkdir(repo.dir .. "/a{y}", "p") + os.rename(repo.dir .. "/a{x}/f.txt", repo.dir .. "/a{y}/f.txt") + + local adapter = repo.adapter() + local left = adapter.Rev( + RevType.COMMIT, + run({ "jj", "show", "-T", "commit_id", "@-", "--no-patch" }, repo.dir) + ) + local right = adapter.Rev(RevType.LOCAL) + local args = adapter:rev_to_args(left, right) + + local err, files = await( + adapter:tracked_files( + left, + right, + args, + "working", + { default_layout = Diff2, merge_layout = Diff2 } + ) + ) + + assert.is_nil(err) + assert.equals(1, #files) + assert.equals("R", files[1].status) + assert.equals("a{y}/f.txt", files[1].path) + assert.equals("a{x}/f.txt", files[1].oldpath) + end) + ) + + it( + "reports the full old and new paths when a name contains ` => `", + helpers.async_test(function() + if not jj_available() then + pending("jj not installed") + return + end + + -- Unlike the `{`/`}` case above, this one is provably unrecoverable + -- from `--summary` output, not just hard to parse: renaming a file + -- named `p => q` to `z`, and separately renaming a file named `p` + -- to `q => z`, both render as the identical line `R {p => q => z}` + -- -- no parser can tell which rename produced it. `-T + -- TRACKED_FILES_TEMPLATE` never renders that ambiguous form in the + -- first place, so this resolves correctly. + repo.write("p => q", "hi\n") + repo.jj({ "describe", "-m", "add" }) + repo.jj({ "new" }) + + os.rename(repo.dir .. "/p => q", repo.dir .. "/z") + + local adapter = repo.adapter() + local left = adapter.Rev( + RevType.COMMIT, + run({ "jj", "show", "-T", "commit_id", "@-", "--no-patch" }, repo.dir) + ) + local right = adapter.Rev(RevType.LOCAL) + local args = adapter:rev_to_args(left, right) + + local err, files = await( + adapter:tracked_files( + left, + right, + args, + "working", + { default_layout = Diff2, merge_layout = Diff2 } + ) + ) + + assert.is_nil(err) + assert.equals(1, #files) + assert.equals("R", files[1].status) + assert.equals("z", files[1].path) + assert.equals("p => q", files[1].oldpath) + end) + ) + it( "shows file content at a revision without errors", helpers.async_test(function() @@ -1823,6 +1965,47 @@ describe("diffview.vcs.adapters.jj", function() end) end) + describe("parse_tracked_files_line", function() + local parse_tracked_files_line = + require("diffview.vcs.adapters.jj")._test.parse_tracked_files_line + + local US = "\x1f" -- field separator + + it("parses a modified/added/deleted line with no old path", function() + local status, path, oldpath = parse_tracked_files_line("M" .. US .. "src/main.lua" .. US) + assert.equals("M", status) + assert.equals("src/main.lua", path) + assert.is_nil(oldpath) + end) + + it("parses a renamed line with the old path in the 3rd field", function() + local status, path, oldpath = + parse_tracked_files_line("R" .. US .. "common/new.txt" .. US .. "common/old.txt") + assert.equals("R", status) + assert.equals("common/new.txt", path) + assert.equals("common/old.txt", oldpath) + end) + + it("parses a copied line the same way as renamed", function() + local status, path, oldpath = parse_tracked_files_line("C" .. US .. "b.txt" .. US .. "a.txt") + assert.equals("C", status) + assert.equals("b.txt", path) + assert.equals("a.txt", oldpath) + end) + + it("treats an empty 3rd field as nil, not an empty string", function() + local _, _, oldpath = parse_tracked_files_line("M" .. US .. "f.txt" .. US .. "") + assert.is_nil(oldpath) + end) + + it("returns nil for a line with no path (e.g. a stray blank line)", function() + local status, path, oldpath = parse_tracked_files_line("") + assert.is_nil(status) + assert.is_nil(path) + assert.is_nil(oldpath) + end) + end) + describe("parse_fh_data", function() local Diff2 = require("diffview.scene.layouts.diff_2").Diff2 diff --git a/lua/diffview/vcs/adapters/jj/init.lua b/lua/diffview/vcs/adapters/jj/init.lua index b59a241e..20e1f549 100644 --- a/lua/diffview/vcs/adapters/jj/init.lua +++ b/lua/diffview/vcs/adapters/jj/init.lua @@ -1661,6 +1661,55 @@ JjAdapter._query_merge_context = async.wrap(function(self, callback) callback(nil, ctx) end) +---Template fed to `jj diff -T ...` by `tracked_files` to list the status, +---path, and (for renames/copies) old path of each changed file. +--- +---Each file produces exactly one line terminated by `\n`. Fields are +---separated by `\x1f` (ASCII US) and are, in order: +--- 1. status_char (`M`, `A`, `D`, `R`, `C`, ...) +--- 2. path (the target/right path -- the only path for non-renames) +--- 3. old path, or an empty string when the entry isn't a rename/copy +--- +---`-T` is used instead of `--summary`: `--summary`'s built-in rename/copy +---display (`display_diff_path()`) factors out the common prefix/suffix of +---the two paths into `prefix{old => new}suffix`, unescaped. That's lossy, +---not just awkward to parse: renaming `p => q` to `z`, and separately +---renaming `p` to `q => z`, both render as `R {p => q => z}` -- no parser +---can recover which happened from that string alone. Calling +---`self.source().path()` / `self.target().path()` directly instead always +---yields the full, unabbreviated paths. +--- +---The old-path field is always present (empty when unused) rather than +---omitted, so every line has exactly 3 fields and parsing never has to +---branch on how many separators showed up. +local TRACKED_FILES_TEMPLATE = table.concat({ + [[ self.status_char() ++ "\x1f" ++ self.path() ++ "\x1f" ]], + [[ ++ if(self.status() == "renamed" || self.status() == "copied", ]], + [[ self.source().path(), "") ]], + [[ ++ "\n" ]], +}, "") + +---Parse one line of `TRACKED_FILES_TEMPLATE` output. +--- +---Returns `nil` for a line with no path (e.g. a stray blank line), so +---callers can `if not status then goto continue end`-style skip it. +---@param line string +---@return string? status +---@return string? path +---@return string? oldpath +local function parse_tracked_files_line(line) + local fields = vim.split(line, "\x1f", { plain = true }) + local status, path = fields[1], fields[2] + + if not status or not path or path == "" then + return nil + end + + local oldpath = fields[3] ~= "" and fields[3] or nil + + return status, path, oldpath +end + ---@param self JjAdapter ---@param left Rev ---@param right Rev @@ -1671,7 +1720,7 @@ end) JjAdapter.tracked_files = async.wrap(function(self, left, right, args, kind, opt, callback) local job = Job({ command = self:bin(), - args = utils.vec_join(self:args(), "diff", "--summary", args), + args = utils.vec_join(self:args(), "diff", "-T", TRACKED_FILES_TEMPLATE, args), cwd = self.ctx.toplevel, retry = 2, log_opt = { label = "JjAdapter:tracked_files()" }, @@ -1706,27 +1755,18 @@ JjAdapter.tracked_files = async.wrap(function(self, left, right, args, kind, opt local files = {} for _, line in ipairs(job.stdout) do - local status, path = line:match("^(%u)%s+(.*)$") - - if status and path then - local oldpath - if status == "R" or status == "C" then - local from_path, to_path = path:match("^(.-)%s+=>%s+(.-)$") - oldpath = from_path - path = to_path or path - end + local status, path, oldpath = parse_tracked_files_line(line) - if not conflicting[path] then - files[#files + 1] = FileEntry.with_layout(opt.default_layout, { - adapter = self, - path = path, - oldpath = oldpath, - status = status, - stats = {}, - kind = kind, - revs = { a = left, b = right }, - }) - end + if status and path and not conflicting[path] then + files[#files + 1] = FileEntry.with_layout(opt.default_layout, { + adapter = self, + path = path, + oldpath = oldpath, + status = status, + stats = {}, + kind = kind, + revs = { a = left, b = right }, + }) end end @@ -2002,7 +2042,7 @@ M.JjAdapter = JjAdapter -- adapter; the shape is unstable. M._test = { structure_fh_data = structure_fh_data, - FH_TEMPLATE = FH_TEMPLATE, + parse_tracked_files_line = parse_tracked_files_line, is_non_literal_pathspec = is_non_literal_pathspec, is_ambiguous_literal_path = is_ambiguous_literal_path, quote_path_args = quote_path_args,