diff --git a/lua/diffview/scene/views/standard/standard_view.lua b/lua/diffview/scene/views/standard/standard_view.lua index e969b0d4..ce040660 100644 --- a/lua/diffview/scene/views/standard/standard_view.lua +++ b/lua/diffview/scene/views/standard/standard_view.lua @@ -17,6 +17,17 @@ local await, pawait = async.await, async.pawait local M = {} +---Predicate matching `DiffView.update_files_impl`'s cancellation guard. +---True when the view is closing or the user has navigated to another +---tabpage while a yielded coroutine was suspended. Keep this identical +---to the inline check in `update_files_impl` so the two async pipelines +---can't drift apart on the definition. +---@param view StandardView +---@return boolean +local function swap_cancelled(view) + return view.closing:check() or view.tabpage ~= api.nvim_get_current_tabpage() +end + ---@class StandardView : View ---@field panel Panel ---@field winopts table @@ -298,12 +309,27 @@ StandardView.use_entry = async.void(function(self, entry) end local old_layout = self.cur_layout + local old_entry = self.cur_entry local panel_was_focused = self.panel:is_focused() self.cur_entry = entry if entry.layout.class == self.cur_layout.class then self.cur_layout.emitter = entry.layout.emitter await(self.cur_layout:use_entry(entry)) + + -- Bail if the view closed or the user navigated to a different + -- tabpage while `use_entry` was yielded. The caller (the drain + -- loop) also re-checks after this returns; the guard here keeps + -- any post-await work in the same-class branch from touching + -- editor state that no longer belongs to us. + -- + -- Do NOT revert `cur_entry` here: `Layout.use_entry` binds + -- `win.file` for the new entry synchronously (before its yield), + -- so `cur_layout` is already showing the new file; keeping + -- `cur_entry` advanced preserves consistency. + if swap_cancelled(self) then + return + end else -- Atomic swap: `self.cur_layout` must always expose a valid, -- fully-created layout, since observers can read it at any point @@ -329,13 +355,53 @@ StandardView.use_entry = async.void(function(self, entry) -- `self.layouts` for the next swap to reuse. Tear it down and drop -- the cache entry before rethrowing so `_stage_layout` re-clones -- next time. - local ok, err = pawait(async.void(function() - await(new_layout:use_entry(entry)) - await(new_layout:create()) - end)) - if not ok then + -- + -- Cancellation cleanup (also below) mirrors the error path: + -- destroy the staged layout and drop the cache entry so the next + -- real swap re-clones from `entry.layout` instead of inheriting + -- whatever partial state the cancelled attempt left behind. + -- Deliberately does NOT destroy `old_layout`: if `closing` fired, + -- `DiffView:close` is already tearing down the diff tabpage; if + -- only `tabpage` flipped, the diff tabpage is still there and its + -- outgoing layout should stay intact until the user comes back. + -- Also skips the post-swap `wincmd =` / focus restore, which would + -- otherwise run against someone else's tabpage. + -- Also reverts `self.cur_entry`: neither the cancellation nor the + -- error path publishes `new_layout`, so `cur_layout` is still the + -- outgoing one and `cur_entry` must follow it, or later actions + -- (e.g., `view.cur_entry.layout:files()`) would act on the wrong + -- entry. + local function stage_cleanup() new_layout:destroy() self.layouts[entry.layout.class] = nil + self.cur_entry = old_entry + end + + -- Check between `use_entry` and `create`. `Layout.use_entry` on a + -- cached layout with still-valid windows takes the + -- `await(open_files())` branch and yields; the pre-`create` guard + -- catches a close/tabnew that lands during that yield so + -- `create()` never runs `pivot_producer` / `create_wins` against + -- the wrong tabpage. Cancellation is checked before the error + -- path so a close that races with a `pawait` failure is swallowed + -- silently (the user asked for the close, not for an error). + local ok, err = pawait(new_layout:use_entry(entry)) + if swap_cancelled(self) then + stage_cleanup() + return + end + if not ok then + stage_cleanup() + error(err) + end + + ok, err = pawait(new_layout:create()) + if swap_cancelled(self) then + stage_cleanup() + return + end + if not ok then + stage_cleanup() error(err) end @@ -382,6 +448,14 @@ end ---@param self StandardView StandardView._drain_set_file_pending = async.void(function(self) while self._set_file_pending do + -- Bail if the view closed while a previous drain iteration was + -- suspended. Supersession itself is already handled by the loop + -- re-reading `_set_file_pending` at the top; this predicate only + -- covers the close-during-supersession case. + if swap_cancelled(self) then + break + end + local target = self._set_file_pending --[[@as FileEntry]] self._set_file_pending = nil @@ -403,6 +477,15 @@ StandardView._drain_set_file_pending = async.void(function(self) -- foldmethod=diff already folds unchanged regions in the diff. -- See: sindrets/diffview.nvim#552 + -- The critical guard: `use_entry` yields on file loads and git + -- blob fetches, so the view may have closed or the user may have + -- switched tabpages by the time it returns. Skip `file_open_post` + -- (listeners assume a live view) and the `file_open_new` emit + -- (which would mark a superseded entry as opened) in that case. + if swap_cancelled(self) then + break + end + self.emitter:emit("file_open_post", target, cur_entry) if not self.cur_entry.opened then diff --git a/lua/diffview/tests/functional/standard_view_spec.lua b/lua/diffview/tests/functional/standard_view_spec.lua index 083a2e04..be7f0f3d 100644 --- a/lua/diffview/tests/functional/standard_view_spec.lua +++ b/lua/diffview/tests/functional/standard_view_spec.lua @@ -32,6 +32,21 @@ local function drain_remaining_cbs(cbs) end end +-- Fields consumed by the `swap_cancelled` guard in +-- `_drain_set_file_pending`. A duck-typed `closing` avoids pulling in +-- the real `Signal`; setting `tabpage` to the current tabpage keeps the +-- guard's `nvim_get_current_tabpage()` check quiet. +local function alive_view_stubs() + return { + closing = { + check = function() + return false + end, + }, + tabpage = vim.api.nvim_get_current_tabpage(), + } +end + describe("diffview.standard_view panel cursor", function() local orig_win_is_valid, orig_win_get_cursor, orig_win_set_cursor @@ -400,19 +415,22 @@ describe("diffview.standard_view _set_file serialization", function() local done_future = async.void(function() end)() ---@diagnostic disable-next-line: missing-fields - local view = setmetatable({ - panel = { render = function() end, redraw = function() end }, - cur_layout = { detach_files = function() end }, - emitter = { emit = function() end }, - cur_entry = { opened = true }, - nulled = false, - _set_file_in_flight = done_future, - _set_file_pending = nil, - use_entry = async.wrap(function(_, target, cb) - table.insert(opened, target) - table.insert(cbs, cb) - end, 3), - }, { __index = StandardView }) + local view = setmetatable( + vim.tbl_extend("error", { + panel = { render = function() end, redraw = function() end }, + cur_layout = { detach_files = function() end }, + emitter = { emit = function() end }, + cur_entry = { opened = true }, + nulled = false, + _set_file_in_flight = done_future, + _set_file_pending = nil, + use_entry = async.wrap(function(_, target, cb) + table.insert(opened, target) + table.insert(cbs, cb) + end, 3), + }, alive_view_stubs()), + { __index = StandardView } + ) local ok, err = pcall(function() StandardView._set_file(view, "file_A") @@ -435,10 +453,13 @@ describe("diffview.standard_view _set_file serialization", function() it("does not queue when no _set_file is in-flight", function() ---@diagnostic disable-next-line: missing-fields - local view = setmetatable({ - _set_file_in_flight = nil, - _set_file_pending = nil, - }, { __index = StandardView }) + local view = setmetatable( + vim.tbl_extend("error", { + _set_file_in_flight = nil, + _set_file_pending = nil, + }, alive_view_stubs()), + { __index = StandardView } + ) -- Without an in-flight worker `_set_file` must proceed past the -- queuing branch and start a worker. We don't care about the @@ -464,19 +485,22 @@ describe("diffview.standard_view _set_file serialization", function() local cbs = {} ---@diagnostic disable-next-line: missing-fields - local view = setmetatable({ - panel = { render = function() end, redraw = function() end }, - cur_layout = { detach_files = function() end }, - emitter = { emit = function() end }, - cur_entry = { opened = true }, - nulled = false, - -- Controllable use_entry: record the target and stash its - -- callback so the test resumes the worker on demand. - use_entry = async.wrap(function(_, target, cb) - table.insert(opened, target) - table.insert(cbs, cb) - end, 3), - }, { __index = StandardView }) + local view = setmetatable( + vim.tbl_extend("error", { + panel = { render = function() end, redraw = function() end }, + cur_layout = { detach_files = function() end }, + emitter = { emit = function() end }, + cur_entry = { opened = true }, + nulled = false, + -- Controllable use_entry: record the target and stash its + -- callback so the test resumes the worker on demand. + use_entry = async.wrap(function(_, target, cb) + table.insert(opened, target) + table.insert(cbs, cb) + end, 3), + }, alive_view_stubs()), + { __index = StandardView } + ) local saved_vim_cmd = vim.cmd vim.cmd = function() end @@ -533,6 +557,9 @@ describe("diffview.standard_view _detach_files_for_next", function() view.nulled = false view._set_file_in_flight = nil view._set_file_pending = nil + for k, v in pairs(alive_view_stubs()) do + view[k] = v + end view.use_entry = async.wrap(function(_, t, cb) table.insert(opened, t) table.insert(cbs, cb) diff --git a/lua/diffview/tests/functional/swap_cancellation_spec.lua b/lua/diffview/tests/functional/swap_cancellation_spec.lua new file mode 100644 index 00000000..e169c653 --- /dev/null +++ b/lua/diffview/tests/functional/swap_cancellation_spec.lua @@ -0,0 +1,270 @@ +local async = require("diffview.async") +local config = require("diffview.config") +local helpers = require("diffview.tests.helpers") + +local DiffView = require("diffview.scene.views.diff.diff_view").DiffView +local Diff1Raw = require("diffview.scene.layouts.diff_1_raw").Diff1Raw +local Diff2Hor = require("diffview.scene.layouts.diff_2_hor").Diff2Hor +local EventEmitter = require("diffview.events").EventEmitter +local GitAdapter = require("diffview.vcs.adapters.git").GitAdapter +local GitRev = require("diffview.vcs.adapters.git.rev").GitRev +local RevType = require("diffview.vcs.rev").RevType +local Window = require("diffview.scene.window").Window + +local await = async.await +local eq = helpers.eq +local run = helpers.run +local cleanup_repo = helpers.cleanup_repo +local close_view = helpers.close_view + +-- Same fixture shape as `restore_focus_after_layout_swap_spec`: two files +-- that resolve to different layout classes under +-- `view.one_sided_layout = "raw"` so that navigation between them exercises +-- the layout-class swap branch in `StandardView.use_entry`. +local function make_repo() + local repo = helpers.init_repo() + + local existing = repo .. "/existing.txt" + local f = assert(io.open(existing, "w")) + f:write("line one\n") + f:close() + run({ "git", "add", "existing.txt" }, repo) + run({ "git", "-c", "commit.gpgsign=false", "commit", "-q", "-m", "init" }, repo) + + f = assert(io.open(existing, "a")) + f:write("line two\n") + f:close() + + local newfile = repo .. "/newfile.txt" + f = assert(io.open(newfile, "w")) + f:write("new file content\n") + f:close() + + return repo +end + +---Set up a fresh DiffView against `repo` and wait for its initial load. +---@param repo string +---@return DiffView, FileEntry, FileEntry # view, modified_entry (Diff2Hor), raw_entry (Diff1Raw) +local function open_view_and_wait(repo) + local adapter = GitAdapter({ toplevel = repo, cpath = repo, path_args = {} }) + local view = DiffView({ + adapter = adapter, + rev_arg = nil, + path_args = {}, + left = GitRev(RevType.STAGE, 0), + right = GitRev(RevType.LOCAL), + options = { show_untracked = true }, + }) + assert.is_true(view:is_valid()) + + view:open() + local loaded = vim.wait(3000, function() + return view.initialized + end, 10) + assert.is_true(loaded, "view did not finish loading within 3s") + + if view._set_file_in_flight then + await(view._set_file_in_flight) + end + + local modified_entry, raw_entry + for _, f in view.files:iter() do + if f.path == "existing.txt" then + modified_entry = f + elseif f.path == "newfile.txt" then + raw_entry = f + end + end + assert.is_not_nil(modified_entry, "expected a FileEntry for existing.txt") + assert.is_not_nil(raw_entry, "expected a FileEntry for newfile.txt") + eq(Diff2Hor, modified_entry.layout.class) + eq(Diff1Raw, raw_entry.layout.class) + + return view, modified_entry, raw_entry +end + +describe("StandardView layout-swap cancellation (integration)", function() + local orig_emitter, original_config, orig_load_file + + before_each(function() + orig_emitter = DiffviewGlobal.emitter + DiffviewGlobal.emitter = EventEmitter() + original_config = vim.deepcopy(config.get_config()) + orig_load_file = Window.load_file + end) + + after_each(function() + Window.load_file = orig_load_file + DiffviewGlobal.emitter = orig_emitter + config.setup(original_config) + end) + + ---Install a `Window.load_file` shim that yields via `async.scheduler` + ---before delegating to the real implementation. The extra scheduler + ---tick is the deterministic seam a mid-swap injection (close signal or + ---`:tabnew`) needs: the drain coroutine parks in load_file, control + ---returns to the test, we mutate view/tab state, and the drain resumes + ---to find `swap_cancelled` true. + local function install_load_file_seam() + local orig = orig_load_file + Window.load_file = async.wrap(function(self, callback) + await(async.scheduler()) + local ok = await(orig(self)) + callback(ok) + end) + end + + -- Cancellation via `self.closing:send()` must: + -- * Drop the staged layout from `self.layouts` (so the next swap + -- re-clones from `entry.layout` rather than reusing the partial + -- one). + -- * Not publish `new_layout` to `self.cur_layout`. + -- * Skip the `file_open_post` emit (listeners assume a live view). + it( + "aborts a mid-yield layout swap when the view starts closing", + helpers.async_test(function() + config.setup({ + use_icons = false, + view = { + default = { layout = "diff2_horizontal", focus_diff = false }, + one_sided_layout = "raw", + }, + }) + + local repo = make_repo() + local view + + local ok, err = pcall(function() + local modified_entry, raw_entry + view, modified_entry, raw_entry = open_view_and_wait(repo) + + await(view:set_file(modified_entry, false, false)) + eq(modified_entry, view.cur_entry) + eq(Diff2Hor, view.cur_layout.class) + + local file_open_post_calls = 0 + view.emitter:on("file_open_post", function() + file_open_post_calls = file_open_post_calls + 1 + end) + + install_load_file_seam() + + -- Fire `closing:send()` on a scheduler tick so it lands while + -- the drain is parked in the load_file seam. `vim.schedule` + -- runs its callback on the next event-loop pump, which happens + -- while we're awaiting the swap Future below. + vim.schedule(function() + view.closing:send() + end) + + await(view:_set_file(raw_entry)) + + -- The staged Diff1Raw layout must have been torn down and its + -- cache slot cleared so a follow-up swap will re-stage rather + -- than reuse the partial state left by the cancelled attempt. + eq(nil, view.layouts[Diff1Raw]) + + -- The published layout was never replaced, and `cur_entry` + -- follows it: leaving `cur_entry` advanced would put the + -- outgoing layout under a selection for the incoming entry. + eq(Diff2Hor, view.cur_layout.class) + eq(modified_entry, view.cur_entry) + + -- The post-swap emit is skipped so listeners don't run against + -- a closing view. + eq(0, file_open_post_calls) + end) + + close_view(view) + cleanup_repo(repo) + if not ok then + error(err) + end + end) + ) + + -- Cancellation via `self.tabpage ~= nvim_get_current_tabpage()` must: + -- * Also drop the staged cache entry and skip the publish. + -- * Leave the diff view usable once the user returns to the diff + -- tabpage (a subsequent `set_file` completes normally). + -- * Skip the `wincmd =` that would otherwise run in the intruding + -- tabpage. + it( + "aborts a mid-yield layout swap when the user switches tabpages", + helpers.async_test(function() + config.setup({ + use_icons = false, + view = { + default = { layout = "diff2_horizontal", focus_diff = false }, + one_sided_layout = "raw", + }, + }) + + local repo = make_repo() + local view + local other_tab + + local ok, err = pcall(function() + local modified_entry, raw_entry + view, modified_entry, raw_entry = open_view_and_wait(repo) + + await(view:set_file(modified_entry, false, false)) + eq(modified_entry, view.cur_entry) + eq(Diff2Hor, view.cur_layout.class) + + local file_open_post_calls = 0 + view.emitter:on("file_open_post", function() + file_open_post_calls = file_open_post_calls + 1 + end) + + install_load_file_seam() + + -- Schedule the tab switch to fire while the drain is parked in + -- the load_file seam. + vim.schedule(function() + vim.cmd("tabnew") + other_tab = vim.api.nvim_get_current_tabpage() + end) + + await(view:_set_file(raw_entry)) + + assert.is_not_nil(other_tab, "tabnew should have run before the swap resolved") + assert.is_true( + vim.api.nvim_tabpage_is_valid(other_tab), + "intruding tabpage should still exist" + ) + + -- The intruding tabpage keeps exactly the one window `tabnew` + -- opened it with: no stray `pivot_producer` split, no `wincmd =` + -- reflow. + eq(1, #vim.api.nvim_tabpage_list_wins(other_tab)) + + -- Same cache/publish invariants as the close case, plus + -- `cur_entry` matches `cur_layout`. + eq(nil, view.layouts[Diff1Raw]) + eq(Diff2Hor, view.cur_layout.class) + eq(modified_entry, view.cur_entry) + eq(0, file_open_post_calls) + + -- Returning to the diff tabpage and swapping again completes + -- normally: the view is not permanently wedged by the + -- cancellation. + vim.api.nvim_set_current_tabpage(view.tabpage) + Window.load_file = orig_load_file + await(view:set_file(raw_entry, false, false)) + eq(raw_entry, view.cur_entry) + eq(Diff1Raw, view.cur_layout.class) + end) + + if other_tab and vim.api.nvim_tabpage_is_valid(other_tab) then + pcall(vim.api.nvim_command, "tabclose " .. vim.api.nvim_tabpage_get_number(other_tab)) + end + close_view(view) + cleanup_repo(repo) + if not ok then + error(err) + end + end) + ) +end)