diff --git a/doc/diffview.txt b/doc/diffview.txt index 04a35c7c..340b1a85 100644 --- a/doc/diffview.txt +++ b/doc/diffview.txt @@ -2099,6 +2099,10 @@ toggle_stage_entry *diffview-actions-toggle_stage_e Stage / unstage the subject. If files are selected (via `toggle_select_entry`), operates on all selected files. + On `jj` (no staging index), a conflict view instead writes the + current entry's MERGED buffer and advances to the next conflict; + pair with `auto_close_on_empty` to close after the last resolve. + unstage_all *diffview-actions-unstage_all* Contexts: `diff_view`, `file_panel` @@ -2441,6 +2445,9 @@ C Clear all file selections. *diffview-maps-toggle_stage_entry* - Stage/unstage the selected file entry. If files are selected, operates on all selected files as a batch. + On a `jj` conflict, writes the MERGED buffer to disk + and advances to the next entry instead (see + |diffview-actions-toggle_stage_entry|). *diffview-maps-stage_all* S Stage all entries. diff --git a/doc/diffview_defaults.txt b/doc/diffview_defaults.txt index bca779e1..21e8674f 100644 --- a/doc/diffview_defaults.txt +++ b/doc/diffview_defaults.txt @@ -266,8 +266,8 @@ DEFAULT CONFIG *diffview.defaults* { "n", "<2-LeftMouse>", actions.select_entry, { desc = "Open the diff for the selected entry" } }, { { "n", "x" }, "w", actions.toggle_select_entry, { desc = "Toggle file selection for multi-file operations" } }, { "n", "C", actions.clear_select_entries, { desc = "Clear all file selections" } }, - { "n", "-", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry" } }, - { "n", "s", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry" } }, + { "n", "-", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry (jj: save & advance)" } }, + { "n", "s", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry (jj: save & advance)" } }, { "n", "S", actions.stage_all, { desc = "Stage all entries" } }, { "n", "U", actions.unstage_all, { desc = "Unstage all entries" } }, { "n", "X", actions.restore_entry, { desc = "Restore entry to the state on the left side" } }, diff --git a/lua/diffview/actions.lua b/lua/diffview/actions.lua index cd884078..249da24e 100644 --- a/lua/diffview/actions.lua +++ b/lua/diffview/actions.lua @@ -118,7 +118,7 @@ M.compat = {} ---suppress entries that would be no-ops in the current view. Weak keys let ---closures returned by action factories be collected once no keymap references ---them anymore. ----@alias diffview.ActionTag "merge_only" | "working_tree_only" +---@alias diffview.ActionTag "merge_only" | "working_tree_only" | "stage_or_resolve" ---@type table local action_meta = setmetatable({}, { __mode = "k" }) @@ -153,10 +153,10 @@ function M._is_applicable(fn, view) return view ~= nil and (view --[[@as DiffView]]).merge_ctx ~= nil end if t == "working_tree_only" then - -- Index-vs-worktree actions (staging, etc.) are only meaningful when the - -- diff compares STAGE against LOCAL. `left`/`right` exist on `DiffView` - -- and `FileHistoryView`; in the latter, `left.type` is `COMMIT`, so the - -- predicate correctly evaluates to false there. + -- Strict index-vs-worktree actions (`stage_all`, `unstage_all`) are only + -- meaningful when the diff compares STAGE against LOCAL. `left`/`right` + -- exist on `DiffView` and `FileHistoryView`; in the latter, `left.type` + -- is `COMMIT`, so the predicate correctly evaluates to false there. if view == nil then return false end @@ -166,6 +166,24 @@ function M._is_applicable(fn, view) and v.left.type == RevType.STAGE and v.right.type == RevType.LOCAL end + if t == "stage_or_resolve" then + -- `toggle_stage_entry` covers index-based staging (git/hg) and + -- content-based resolution (index-less adapters, e.g., jj). Split by + -- adapter capability, not just rev shape, so a staging adapter in a + -- mid-merge non-STAGE view (e.g., `:DiffviewOpen HEAD` during a git + -- merge) doesn't get the resolve treatment. + if view == nil then + return false + end + local v = view --[[@as DiffView]] + if v.left == nil or v.right == nil then + return false + end + if v.adapter:has_staging() then + return v.left.type == RevType.STAGE and v.right.type == RevType.LOCAL + end + return v.right.type == RevType.LOCAL and v.merge_ctx ~= nil + end return true end @@ -1261,7 +1279,7 @@ local action_names = { ---stub site) because the stubs are generated in the loop below. ---@type table local action_tags = { - toggle_stage_entry = "working_tree_only", + toggle_stage_entry = "stage_or_resolve", stage_all = "working_tree_only", unstage_all = "working_tree_only", } diff --git a/lua/diffview/config.lua b/lua/diffview/config.lua index 48c23a22..d026b8ce 100644 --- a/lua/diffview/config.lua +++ b/lua/diffview/config.lua @@ -716,8 +716,8 @@ M.defaults = { file_panel = utils.vec_join(common_panel_keymaps, common_nav_keymaps, { { { "n", "x" }, "w", actions.toggle_select_entry, { desc = "Toggle file selection for multi-file operations" } }, { "n", "C", actions.clear_select_entries, { desc = "Clear all file selections" } }, - { "n", "-", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry" } }, - { "n", "s", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry" } }, + { "n", "-", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry (jj: save & advance)" } }, + { "n", "s", actions.toggle_stage_entry, { desc = "Stage / unstage the selected entry (jj: save & advance)" } }, { "n", "S", actions.stage_all, { desc = "Stage all entries" } }, { "n", "U", actions.unstage_all, { desc = "Unstage all entries" } }, { "n", "X", actions.restore_entry, { desc = "Restore entry to the state on the left side" } }, diff --git a/lua/diffview/scene/views/diff/listeners.lua b/lua/diffview/scene/views/diff/listeners.lua index c194f717..a69c76bf 100644 --- a/lua/diffview/scene/views/diff/listeners.lua +++ b/lua/diffview/scene/views/diff/listeners.lua @@ -18,11 +18,21 @@ return function(view) -- guarded close aborts (dirty stage buffer); cleared when the retry either -- succeeds or no longer applies (working/conflicting non-empty). local auto_close_pending = false - - ---Run the `auto_close_on_empty` policy. Closes the view when there are no - ---working/conflicting entries left and stage buffers are clean. If the - ---guarded close aborts, set the retry flag so the next stage save (via - ---`buf_write_post`) re-evaluates. + -- Latched once this view surfaces a conflict, so index-less adapters (where + -- a resolved file lands in `working` rather than leaving the view) only + -- auto-close after a merge was actually resolved here. + local had_conflicts = false + + ---Run the `auto_close_on_empty` policy. Closes the view when the + ---`conflicting` bucket is empty AND stage buffers are clean AND either: + --- (a) staging adapters (git/hg): the `working` bucket is also empty, + --- i.e., every change has been staged; or + --- (b) index-less adapters (jj): the view previously surfaced + --- conflicts, i.e., a merge was actually resolved here (a resolved + --- file stays in `working` as the resolution artifact, so an + --- empty `working` bucket cannot be the gate). + ---If the guarded close aborts, set the retry flag so the next stage + ---save (via `buf_write_post`) re-evaluates. --- ---When `silent` is true, the dirty-stage gate is pre-checked and the close ---call is skipped if it would fail. The BufWritePost retry path uses this @@ -35,10 +45,24 @@ return function(view) auto_close_pending = false return end - if #view.files.working ~= 0 or #view.files.conflicting ~= 0 then + if #view.files.conflicting ~= 0 then auto_close_pending = false return end + -- See docstring: staging adapters gate on `working` empty; index-less + -- adapters gate on `had_conflicts` so an unrelated diff view doesn't + -- auto-close on the first save. + if view.adapter:has_staging() then + if #view.files.working ~= 0 then + auto_close_pending = false + return + end + else + if not had_conflicts then + auto_close_pending = false + return + end + end if silent and #view:_modified_stage_paths() > 0 then auto_close_pending = true return @@ -145,6 +169,10 @@ return function(view) view.initialized = true -- File entries may be replaced on update; prune stale selections. view.panel:prune_selections() + -- Latch for the index-less branch of `maybe_auto_close`. + if #view.files.conflicting > 0 then + had_conflicts = true + end end, close = function() if view.panel:is_focused() then @@ -322,7 +350,110 @@ return function(view) end end, toggle_stage_entry = function() - if not (view.left.type == RevType.STAGE and view.right.type == RevType.LOCAL) then + local right_is_local = view.right.type == RevType.LOCAL + + -- Index-less adapters (jj): resolve by content instead of by staging. + -- Write the current entry's MERGED buffer to disk and advance; the + -- refresh observes it leave `conflicting` once the markers are gone, + -- and `auto_close_on_empty` handles the final close. + if not view.adapter:has_staging() then + if not (right_is_local and view.merge_ctx ~= nil) then + return + end + -- Use `cur_entry`, not `infer_cur_file`: the main window's MERGED + -- buffer belongs to the displayed entry, and a stale panel cursor + -- must not let us save one path while advancing from another. + local item = view.cur_entry + if not item or item.kind ~= "conflicting" then + return + end + -- Panel cursor on a different entry: no buffer for it is displayed, + -- so warn instead of silently writing `cur_entry`'s MERGED buffer. + if view.panel:is_focused() then + local hovered = view.panel:get_item_at_cursor() + if hovered and hovered ~= item then + utils.warn("Open the file to resolve its conflicts.") + return + end + end + + local main = view.cur_layout and view.cur_layout:get_main_win() + if main and main:is_valid() and main.file and main.file:is_valid() then + local bufnr = main.file.bufnr + if bufnr and vim.bo[bufnr].modified then + api.nvim_buf_call(bufnr, function() + vim.cmd("silent update") + end) + end + end + + -- Defer advance/close until after the refresh: keying off pre-refresh + -- state (like the git branch's `next_file`) would race with the close + -- policy when the resolved file was the last conflict. + local saved_path = item.path + -- Record the successor before refresh so mid-list resolution advances + -- downward rather than snapping to `conflicting[1]`. + local next_after_path + do + local seen_self = false + for _, e in ipairs(view.files.conflicting) do + if seen_self then + next_after_path = e.path + break + end + if e.path == saved_path then + seen_self = true + end + end + end + view:update_files( + nil, + vim.schedule_wrap(function() + local still_conflicting = false + for _, e in ipairs(view.files.conflicting) do + if e.path == saved_path then + still_conflicting = true + break + end + end + if not still_conflicting then + -- Prefer the pre-refresh successor; fall back to `conflicting[1]` + -- if it was resolved out-of-band or the resolved file was last. + local next_entry + if next_after_path then + for _, e in ipairs(view.files.conflicting) do + if e.path == next_after_path then + next_entry = e + break + end + end + end + next_entry = next_entry or view.files.conflicting[1] + if next_entry and view.cur_entry ~= next_entry then + view:set_file(next_entry, false, true) + end + end + view.panel:highlight_cur_file() + -- Wait for any in-flight `set_file` (`update_files` may schedule + -- one) to finish before letting `maybe_auto_close` run: tearing + -- down the layout mid-flight crashes `sync_scroll` on the freed + -- window ids. + local function close_when_idle() + local in_flight = view:set_file_in_flight() + if in_flight and not in_flight:is_done() then + vim.defer_fn(close_when_idle, 20) + return + end + maybe_auto_close() + end + close_when_idle() + end) + ) + view.emitter:emit(EventName.FILES_STAGED, view) + return + end + + if not (view.left.type == RevType.STAGE and right_is_local) then return end diff --git a/lua/diffview/scene/views/standard/standard_view.lua b/lua/diffview/scene/views/standard/standard_view.lua index ce040660..96d51a2b 100644 --- a/lua/diffview/scene/views/standard/standard_view.lua +++ b/lua/diffview/scene/views/standard/standard_view.lua @@ -445,6 +445,14 @@ function StandardView:_set_file(file) return self._set_file_in_flight end +---The currently-running `_set_file` worker, or `nil` if none is outstanding. +---Exposed so external policy code (e.g., `maybe_auto_close`) can defer teardown +---until a queued file swap has finished, without reaching into `_set_file_in_flight`. +---@return Future? +function StandardView:set_file_in_flight() + return self._set_file_in_flight +end + ---@param self StandardView StandardView._drain_set_file_pending = async.void(function(self) while self._set_file_pending do diff --git a/lua/diffview/tests/functional/actions_spec.lua b/lua/diffview/tests/functional/actions_spec.lua index 9bf292d7..c8c3d000 100644 --- a/lua/diffview/tests/functional/actions_spec.lua +++ b/lua/diffview/tests/functional/actions_spec.lua @@ -239,7 +239,19 @@ describe("diffview.actions._is_applicable", function() local make_view = function(opts) opts = opts or {} - local v = { merge_ctx = opts.merge_ctx and {} or nil } + -- Defaults to a staging-model adapter; pass `has_staging = false` for jj-like. + local has_staging = opts.has_staging + if has_staging == nil then + has_staging = true + end + local v = { + merge_ctx = opts.merge_ctx and {} or nil, + adapter = { + has_staging = function() + return has_staging + end, + }, + } if opts.left then v.left = { type = opts.left } end @@ -289,35 +301,84 @@ describe("diffview.actions._is_applicable", function() assert.is_false(actions._is_applicable(actions.next_conflict, nil)) end) - it("shows `working_tree_only` actions when comparing STAGE against LOCAL", function() + it("shows `stage_or_resolve` actions when comparing STAGE against LOCAL", function() local view = make_view({ left = RevType.STAGE, right = RevType.LOCAL }) assert.is_true(actions._is_applicable(actions.toggle_stage_entry, view)) + end) + + it("shows `working_tree_only` actions when comparing STAGE against LOCAL", function() + local view = make_view({ left = RevType.STAGE, right = RevType.LOCAL }) assert.is_true(actions._is_applicable(actions.stage_all, view)) assert.is_true(actions._is_applicable(actions.unstage_all, view)) end) - it("hides `working_tree_only` actions when comparing two commits", function() + it("hides staging actions when comparing two commits", function() local view = make_view({ left = RevType.COMMIT, right = RevType.COMMIT }) assert.is_false(actions._is_applicable(actions.toggle_stage_entry, view)) assert.is_false(actions._is_applicable(actions.stage_all, view)) assert.is_false(actions._is_applicable(actions.unstage_all, view)) end) - it("hides `working_tree_only` actions when right side is not LOCAL", function() + it("hides `stage_or_resolve` (staging adapter) when right side is not LOCAL", function() local view = make_view({ left = RevType.STAGE, right = RevType.COMMIT }) assert.is_false(actions._is_applicable(actions.toggle_stage_entry, view)) end) - it("hides `working_tree_only` actions when left side is not STAGE", function() - local view = make_view({ left = RevType.COMMIT, right = RevType.LOCAL }) + it("hides `stage_or_resolve` (staging adapter) when left side is not STAGE", function() + -- Staging adapter in a mid-merge non-STAGE view (e.g., `:DiffviewOpen HEAD` + -- during a git merge) must not fall into the resolve branch. + local view = make_view({ left = RevType.COMMIT, right = RevType.LOCAL, merge_ctx = true }) assert.is_false(actions._is_applicable(actions.toggle_stage_entry, view)) end) - it("hides `working_tree_only` actions when view lacks left/right (non-DiffView)", function() + it("hides staging actions when view lacks left/right (non-DiffView)", function() assert.is_false(actions._is_applicable(actions.toggle_stage_entry, make_view())) + assert.is_false(actions._is_applicable(actions.stage_all, make_view())) end) - it("returns false when view is nil and the action is `working_tree_only`", function() + it("returns false when view is nil and the action needs view state", function() assert.is_false(actions._is_applicable(actions.toggle_stage_entry, nil)) + assert.is_false(actions._is_applicable(actions.stage_all, nil)) + end) + + it("shows `stage_or_resolve` on no-staging adapter with LOCAL right + merge_ctx", function() + local view = make_view({ + left = RevType.COMMIT, + right = RevType.LOCAL, + merge_ctx = true, + has_staging = false, + }) + assert.is_true(actions._is_applicable(actions.toggle_stage_entry, view)) + end) + + it("hides `stage_or_resolve` on no-staging adapter without merge_ctx", function() + local view = make_view({ + left = RevType.COMMIT, + right = RevType.LOCAL, + has_staging = false, + }) + assert.is_false(actions._is_applicable(actions.toggle_stage_entry, view)) + end) + + it("hides `stage_or_resolve` on no-staging adapter when right is not LOCAL", function() + local view = make_view({ + left = RevType.COMMIT, + right = RevType.COMMIT, + merge_ctx = true, + has_staging = false, + }) + assert.is_false(actions._is_applicable(actions.toggle_stage_entry, view)) + end) + + it("hides `working_tree_only` (stage_all/unstage_all) on no-staging adapters", function() + -- Even in a mid-merge no-staging view, index-only actions have no target. + local view = make_view({ + left = RevType.COMMIT, + right = RevType.LOCAL, + merge_ctx = true, + has_staging = false, + }) + assert.is_false(actions._is_applicable(actions.stage_all, view)) + assert.is_false(actions._is_applicable(actions.unstage_all, view)) end) end) diff --git a/lua/diffview/tests/functional/diff_view_spec.lua b/lua/diffview/tests/functional/diff_view_spec.lua index e2b4dd4a..16fbcb92 100644 --- a/lua/diffview/tests/functional/diff_view_spec.lua +++ b/lua/diffview/tests/functional/diff_view_spec.lua @@ -450,6 +450,9 @@ describe("diffview.scene.views.diff.DiffView", function() has_local = function() return true end, + has_staging = function() + return true + end, }, left = {}, right = {}, diff --git a/lua/diffview/tests/functional/jj_adapter_spec.lua b/lua/diffview/tests/functional/jj_adapter_spec.lua index 914e956f..ad66d17a 100644 --- a/lua/diffview/tests/functional/jj_adapter_spec.lua +++ b/lua/diffview/tests/functional/jj_adapter_spec.lua @@ -23,9 +23,12 @@ describe("diffview.vcs.adapters.jj", function() JjAdapter.get_dir = old_get_dir + -- `parse_revs` queries `latest(@-)` (merge-safe); `file_restore` queries + -- bare `@-`. Map both to the same commit for the non-merge case. adapter._rev_map = { ["@"] = "head_hash", ["@-"] = "parent_hash", + ["latest(@-)"] = "parent_hash", ["root()"] = "root_hash", ["main"] = "main_hash", ["master"] = "master_hash", @@ -132,7 +135,8 @@ describe("diffview.vcs.adapters.jj", function() local adapter = new_adapter() local left, right = adapter:parse_revs(nil, {}) - adapter._rev_map["@-"] = "next_parent_hash" + -- `parse_revs`/`refresh_revs` query the wrapped form. + adapter._rev_map["latest(@-)"] = "next_parent_hash" local new_left, new_right = adapter:refresh_revs(nil, left, right) eq("next_parent_hash", new_left.commit) @@ -477,6 +481,8 @@ describe("diffview.vcs.adapters.jj", function() eq(true, ok) eq(":!jj op undo", undo) + -- Bare `@-` (not `latest(@-)`) so a merge working copy fails loudly + -- instead of silently discarding content from one parent. eq({ "restore", "--from", "@-", "--", 'file:"src/main.lua"' }, captured_args) end) ) @@ -1314,6 +1320,59 @@ describe("diffview.vcs.adapters.jj", function() assert.equals(adapter.Rev.NULL_TREE_SHA, d_rev:object_name()) end) ) + + it( + "returns nil when @ has a rebase-edit conflict with only a resolved merge in ancestry", + helpers.async_test(function() + if not jj_available() then + pending("jj not installed") + return + end + + -- Build history: + -- root - A ----- M(resolved) - post - X + -- \- B ----/ \- Y (rebased onto X) + -- @ = Y', which is a single-parent commit with a rebase-edit + -- conflict on `other.txt`. M is a merge in ancestors(@) but its + -- conflict was resolved, so it is not the source of @'s conflict. + -- Without the `& conflicts()` guard on the merge query, the code + -- would pick M and populate OURS/THEIRS with A/B (unrelated to + -- `other.txt`). + repo.write("file.txt", "root\n") + repo.jj({ "describe", "-m", "root" }) + local root = repo.jj({ "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }) + + repo.jj({ "new", root, "-m", "A" }) + repo.write("file.txt", "A\n") + local a = repo.jj({ "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }) + + repo.jj({ "new", root, "-m", "B" }) + repo.write("file.txt", "B\n") + local b = repo.jj({ "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }) + + repo.jj({ "new", a, b, "-m", "merge" }) + repo.write("file.txt", "resolved\n") + + repo.jj({ "new", "-m", "post" }) + local post = repo.jj({ "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }) + + repo.jj({ "new", post, "-m", "X" }) + repo.write("other.txt", "X\n") + local x = repo.jj({ "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }) + + repo.jj({ "new", post, "-m", "Y" }) + repo.write("other.txt", "Y\n") + local y = repo.jj({ "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }) + + repo.jj({ "rebase", "-s", y, "-d", x }) + repo.jj({ "edit", y }) + + local adapter = repo.adapter() + local err, ctx = await(adapter:_query_merge_context()) + assert.is_nil(err) + assert.is_nil(ctx) + end) + ) end) describe("file_history_worker", function() diff --git a/lua/diffview/tests/functional/jj_save_and_advance_spec.lua b/lua/diffview/tests/functional/jj_save_and_advance_spec.lua new file mode 100644 index 00000000..16ca508c --- /dev/null +++ b/lua/diffview/tests/functional/jj_save_and_advance_spec.lua @@ -0,0 +1,251 @@ +-- End-to-end: `s`/`-` on a jj working-copy conflict resolves the current +-- entry and either advances or drains the `conflicting` bucket. Exercises +-- the full listener chain (refresh, callback, advance) that unit tests +-- in `actions_spec.lua` / `jj_adapter_spec.lua` do not cover. +local config = require("diffview.config") +local helpers = require("diffview.tests.helpers") + +local DiffView = require("diffview.scene.views.diff.diff_view").DiffView +local EventEmitter = require("diffview.events").EventEmitter +local JjAdapter = require("diffview.vcs.adapters.jj").JjAdapter + +local function jj_available() + return vim.fn.executable("jj") == 1 +end + +local function run(cmd, cwd) + local res = vim.system(cmd, { cwd = cwd, text = true }):wait() + assert.equals(0, res.code, (table.concat(cmd, " ") .. "\n" .. (res.stderr or ""))) + return vim.trim(res.stdout or "") +end + +-- Build a 2-parent merge that conflicts on every path in `filenames`. Returns +-- the workspace directory and a `write(relpath, content)` helper. +local function make_conflict_repo(filenames) + local repo = vim.fn.tempname() + vim.fn.mkdir(repo, "p") + run({ "jj", "git", "init" }, repo) + run({ "jj", "config", "set", "--repo", "user.name", "Test" }, repo) + run({ "jj", "config", "set", "--repo", "user.email", "test@test.com" }, repo) + + local function write(relpath, content) + local dir = vim.fn.fnamemodify(repo .. "/" .. relpath, ":h") + vim.fn.mkdir(dir, "p") + local f = assert(io.open(repo .. "/" .. relpath, "w")) + f:write(content) + f:close() + end + + for _, name in ipairs(filenames) do + write(name, "base\n") + end + run({ "jj", "describe", "-m", "initial" }, repo) + local base_id = run({ "jj", "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }, repo) + + run({ "jj", "new", "-m", "left" }, repo) + for _, name in ipairs(filenames) do + write(name, "left\n") + end + local ours_id = run({ "jj", "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }, repo) + + run({ "jj", "new", base_id, "-m", "right" }, repo) + for _, name in ipairs(filenames) do + write(name, "right\n") + end + local theirs_id = run({ "jj", "log", "-r", "@", "--no-graph", "-T", "change_id.short()" }, repo) + + run({ "jj", "new", ours_id, theirs_id, "-m", "merge" }, repo) + + return repo, write +end + +describe("`toggle_stage_entry` on a jj working-copy conflict", function() + local orig_emitter, original_config, saved_bootstrap + + before_each(function() + if not jj_available() then + pending("jj not installed") + return + end + orig_emitter = DiffviewGlobal.emitter + DiffviewGlobal.emitter = EventEmitter() + original_config = vim.deepcopy(config.get_config()) + config.get_config().use_icons = false + -- Disable auto-close so the tab survives long enough for post-resolve assertions. + config.get_config().auto_close_on_empty = false + saved_bootstrap = vim.deepcopy(JjAdapter.bootstrap) + JjAdapter.bootstrap.done = true + JjAdapter.bootstrap.ok = true + end) + + after_each(function() + if orig_emitter then + DiffviewGlobal.emitter = orig_emitter + end + if original_config then + config.setup(original_config) + end + if saved_bootstrap then + JjAdapter.bootstrap = saved_bootstrap + end + end) + + local function open_view(repo) + local adapter = JjAdapter({ toplevel = repo, path_args = {} }) + local left, right = adapter:parse_revs(nil, {}) + assert.is_not_nil(left) + assert.is_not_nil(right) + + local view = DiffView({ + adapter = adapter, + rev_arg = nil, + path_args = {}, + left = left, + right = right, + options = {}, + }) + view:open() + return view + end + + it("drains the `conflicting` bucket after the sole conflict is resolved", function() + local repo, write = make_conflict_repo({ "file.txt" }) + local view + + local ok, err = pcall(function() + view = open_view(repo) + -- `view.ready` flips after the initial `post_open` dispatch, not after + -- `update_files` populates the panel. Wait for the conflict entries + -- (which arrive via the async `tracked_files` -> `_query_merge_context` + -- pipeline) before asserting on them. + assert.is_true( + vim.wait(5000, function() + return view.ready and #view.files.conflicting > 0 + end), + "view never surfaced conflict entries" + ) + assert.is_not_nil(view.cur_entry) + assert.is_not_nil(view.merge_ctx) + assert.equals(1, #view.files.conflicting) + assert.equals("file.txt", view.files.conflicting[1].path) + assert.equals("conflicting", view.cur_entry.kind) + + -- Resolve on disk; MERGED buffer stays unmodified so this covers the + -- "already clean" branch where refresh alone must drop the entry. + write("file.txt", "resolved\n") + + view.emitter:emit("toggle_stage_entry") + + local drained = vim.wait(5000, function() + return #view.files.conflicting == 0 + end) + assert.is_true(drained, "conflicting bucket did not drain after `toggle_stage_entry`") + assert.is_nil(view.merge_ctx) + end) + + helpers.close_view(view) + vim.schedule(function() + pcall(vim.fn.delete, repo, "rf") + end) + if not ok then + error(err) + end + end) + + it("resolves a propagated conflict where `@` is a linear descendant of the merge", function() + local repo, write = make_conflict_repo({ "file.txt" }) + -- Step off the merge so `@` inherits the conflict via a single parent. + -- This is the shape produced by `jj rebase` or a manual `jj new` after a + -- merge, and the case that regressed `merge_ctx` detection before the + -- nearest-ancestor-merge lookup landed. + run({ "jj", "new", "-m", "child" }, repo) + local view + + local ok, err = pcall(function() + view = open_view(repo) + -- `view.ready` flips after the initial `post_open` dispatch, not after + -- `update_files` populates the panel. Wait for the conflict entries + -- (which arrive via the async `tracked_files` -> `_query_merge_context` + -- pipeline) before asserting on them. + assert.is_true( + vim.wait(5000, function() + return view.ready and #view.files.conflicting > 0 + end), + "view never surfaced conflict entries" + ) + assert.is_not_nil( + view.merge_ctx, + "merge_ctx must resolve for a propagated conflict on a single-parent working copy" + ) + assert.equals(1, #view.files.conflicting) + assert.equals("file.txt", view.files.conflicting[1].path) + assert.equals("conflicting", view.cur_entry.kind) + + write("file.txt", "resolved\n") + view.emitter:emit("toggle_stage_entry") + + local drained = vim.wait(5000, function() + return #view.files.conflicting == 0 + end) + assert.is_true(drained, "conflicting bucket did not drain after `toggle_stage_entry`") + end) + + helpers.close_view(view) + vim.schedule(function() + pcall(vim.fn.delete, repo, "rf") + end) + if not ok then + error(err) + end + end) + + it("advances `cur_entry` to the remaining conflict after resolving one of two", function() + local repo, write = make_conflict_repo({ "a.txt", "b.txt" }) + local view + + local ok, err = pcall(function() + view = open_view(repo) + -- `view.ready` flips after the initial `post_open` dispatch, not after + -- `update_files` populates the panel. Wait for the conflict entries + -- (which arrive via the async `tracked_files` -> `_query_merge_context` + -- pipeline) before asserting on them. + assert.is_true( + vim.wait(5000, function() + return view.ready and #view.files.conflicting > 0 + end), + "view never surfaced conflict entries" + ) + assert.is_not_nil(view.cur_entry) + assert.equals(2, #view.files.conflicting) + + -- Resolve whichever entry `cur_entry` picked; avoids depending on FileDict order. + local resolved_path = view.cur_entry.path + local other_path = (resolved_path == "a.txt") and "b.txt" or "a.txt" + write(resolved_path, "resolved\n") + + view.emitter:emit("toggle_stage_entry") + + local drained = vim.wait(5000, function() + if #view.files.conflicting ~= 1 then + return false + end + return view.files.conflicting[1].path == other_path + end) + assert.is_true(drained, "resolved entry did not leave the conflicting bucket") + + -- `set_file` is async, so poll for the advance rather than asserting synchronously. + local advanced = vim.wait(2000, function() + return view.cur_entry ~= nil and view.cur_entry.path == other_path + end) + assert.is_true(advanced, "cur_entry did not advance to the remaining conflict") + end) + + helpers.close_view(view) + vim.schedule(function() + pcall(vim.fn.delete, repo, "rf") + end) + if not ok then + error(err) + end + end) +end) diff --git a/lua/diffview/vcs/adapter.lua b/lua/diffview/vcs/adapter.lua index 4d3ed681..6b7bbe93 100644 --- a/lua/diffview/vcs/adapter.lua +++ b/lua/diffview/vcs/adapter.lua @@ -540,6 +540,14 @@ function VCSAdapter:restore_file(path, kind, commit) oop.abstract_stub() end +---Whether the adapter has a staging index (git/hg: true, jj: false). Consumers +---gate stage/unstage UI and choose between index-based staging and content-based +---resolution off this. +---@return boolean +function VCSAdapter:has_staging() + return true +end + ---Add file(s) ---@param paths string[] ---@return boolean # add was successful diff --git a/lua/diffview/vcs/adapters/jj/init.lua b/lua/diffview/vcs/adapters/jj/init.lua index 20e1f549..5897c9d8 100644 --- a/lua/diffview/vcs/adapters/jj/init.lua +++ b/lua/diffview/vcs/adapters/jj/init.lua @@ -519,7 +519,10 @@ function JjAdapter:parse_revs(rev_arg, opt) local right if not rev_arg then - local parent_hash = self:resolve_rev_arg("@-") or self:resolve_rev_arg("root()") + -- Wrap `@-` in `latest(...)` so a merge working copy (2+ parents) picks + -- the most-recent parent instead of erroring on "resolved to more than + -- one revision". Non-merge cases collapse back to `@-`. + local parent_hash = self:resolve_rev_arg("latest(@-)") or self:resolve_rev_arg("root()") left = parent_hash and JjRev(RevType.COMMIT, parent_hash) or JjRev.new_null_tree() right = JjRev(RevType.LOCAL) elseif rev_arg:match("%.%.%.") then @@ -1588,10 +1591,15 @@ local function filter_conflict_paths(ctx, path_args) } end ----Query the working copy for a 2-sided merge conflict context. Returns nil ----for non-conflicts and for merges with 0, 1, or 3+ parents (the ----OURS/THEIRS/BASE slots in `vcs.MergeContext` can't represent them); N-way ----conflicts log a warning to `:DiffviewLog` before returning nil. +---Query the working copy for a 2-sided merge conflict context. Locates the +---nearest still-conflicted ancestor merge of `@` (which is `@` itself when +---the working copy is the merge) and derives OURS/THEIRS from its parents; +---this covers the common propagated-conflict shape where `@` is a linear +---descendant of a conflicted merge (e.g., after `jj new` on top of an +---unresolved merge). Returns nil for non-conflicts, for conflicts with no +---conflicted merge ancestor (rebase-only edit conflicts, where no ancestor +---merge is the source), and for merges with 3+ parents; the last two log a +---warning to `:DiffviewLog`. ---@param self JjAdapter ---@param callback fun(err: string[]?, ctx: JjAdapter.MergeContextData?) JjAdapter._query_merge_context = async.wrap(function(self, callback) @@ -1617,27 +1625,52 @@ JjAdapter._query_merge_context = async.wrap(function(self, callback) return end - -- `parents.map(...)` iterates in positional order, so index 1 is OURS, - -- index 2 is THEIRS. - local parents_job = log_job( + -- Locate the nearest still-conflicted ancestor merge and read its parents + -- in one call. `ancestors(@)` includes `@` itself, so a working-copy-as- + -- merge still resolves to itself; `& conflicts()` restricts to merges whose + -- conflicts are the source of `@`'s (a resolved ancient merge in ancestry + -- wouldn't propagate conflicts, so its OURS/THEIRS would be unrelated to + -- the current conflict and mislead the 3-way layout); `latest(..., 1)` + -- picks the most recent such merge and guards against multi-commit output + -- clobbering the fixed layout below. Template line 1 is the merge's + -- `commit_id`; lines 2+ are its parents in positional order (index 1 + -- OURS, index 2 THEIRS). + local merge_job = log_job( self, - "@", - [[parents.map(|c| c.commit_id()).join("\n")]], - "JjAdapter:_query_merge_context() parents" + "latest(ancestors(@) & merges() & conflicts(), 1)", + [[commit_id ++ "\n" ++ parents.map(|c| c.commit_id()).join("\n")]], + "JjAdapter:_query_merge_context() merge" ) - if not await(parents_job) or parents_job.code ~= 0 then - callback(parents_job.stderr or {}, nil) + if not await(merge_job) or merge_job.code ~= 0 then + callback(merge_job.stderr or {}, nil) return end - local parents = vim.tbl_filter(function(s) + local lines = vim.tbl_filter(function(s) return s ~= "" - end, parents_job.stdout) + end, merge_job.stdout) + if #lines == 0 then + logger:warn( + fmt( + "[JjAdapter] Skipping merge-tool layout for %d conflicted file(s): " + .. "no ancestor merge found (conflict has no 2-sided source).", + #paths + ) + ) + callback(nil, nil) + return + end + + local merge_id = lines[1] + local parents = {} + for i = 2, #lines do + parents[#parents + 1] = lines[i] + end if #parents ~= 2 then logger:warn( fmt( "[JjAdapter] Skipping merge-tool layout for %d conflicted file(s): " - .. "working copy has %d parent(s), only 2-sided merges are supported.", + .. "nearest ancestor merge has %d parents, only 2-sided merges are supported.", #paths, #parents ) @@ -1648,12 +1681,19 @@ JjAdapter._query_merge_context = async.wrap(function(self, callback) local ctx = { paths = paths, ours = parents[1], theirs = parents[2] } - -- `latest(fork_point(@-), 1)` matches the sibling pattern at - -- `symmetric_diff_revs`: bare `fork_point(@-)` can resolve to multiple - -- commits in a criss-cross merge, and `commit_id` templates would - -- concatenate them into a garbage 80+ char string without `latest(..., 1)`. - local base_job = - log_job(self, "latest(fork_point(@-), 1)", "commit_id", "JjAdapter:_query_merge_context() base") + -- Anchor `fork_point` at the merge's parents, not `@-`: when `@` is a + -- descendant of the merge, `@-` is a single commit and `fork_point(@-)` + -- yields that commit itself, not the merge's base. `latest(..., 1)` + -- matches the sibling pattern at `symmetric_diff_revs`: bare + -- `fork_point(...)` can resolve to multiple commits in a criss-cross + -- merge, and `commit_id` templates would concatenate them into a garbage + -- 80+ char string without the guard. + local base_job = log_job( + self, + fmt("latest(fork_point(%s-), 1)", merge_id), + "commit_id", + "JjAdapter:_query_merge_context() base" + ) if await(base_job) and base_job.code == 0 and base_job.stdout[1] and base_job.stdout[1] ~= "" then ctx.base = base_job.stdout[1] end @@ -1900,9 +1940,14 @@ JjAdapter.file_restore = async.wrap(function(self, path, kind, commit, callback) return end - -- `jj restore --from -- ` rewrites the working copy from the - -- source commit. When `commit` is nil this defaults to `@-` (the parent), - -- which matches "discard local changes" semantics. + -- `jj restore --from -- ` rewrites the working copy from + -- the source commit. When `commit` is nil this defaults to `@-` (the + -- parent). Do NOT wrap in `latest(...)` here: on a merge working copy + -- `@-` is ambiguous by design, and picking one parent silently would + -- discard content from the other side, potentially destroying an + -- in-progress conflict resolution. Let jj surface the ambiguity as a + -- loud error instead; callers that need a specific side must pass + -- `commit` explicitly. local from = commit or "@-" local abs_path = pl:join(self.ctx.toplevel, path) local _, code, stderr = self:exec_sync( @@ -1950,6 +1995,11 @@ function JjAdapter:stage_index_file(file) ---@diagnostic disable-line: unused-lo return true end +---@return boolean +function JjAdapter:has_staging() + return false +end + ---@param paths string[]? ---@return boolean function JjAdapter:reset_files(paths) ---@diagnostic disable-line: unused-local