From cef31fde6ac77404f778627f1adcca68f905618e Mon Sep 17 00:00:00 2001 From: Evgeni Chasnovski Date: Sat, 27 Jun 2026 21:35:19 +0300 Subject: [PATCH] feat(pack): ensure order of `PackChanged{Pre,}` events #40455 Problem: due to totally async install/update/checkout there is no guaranteed order of `PackChanged{Pre,}` events across different plugins. This might lead to conflicts when callback for some "main" plugin relies on features from "dependency" plugin: i.e. callback for "main" plugin can trigger before installing/updating "dependency" plugin. The installation order can be enforced by separate vim.pack.add() calls, but update/checkout can not. Solution: Trigger events in bulk independently of async execution: - `PackChangedPre` before any action for all input plugins in order they are supplied. It will also trigger even if an action will fail. - `PackChanged` after all actions finished for all sucessfully affected plugins in order they are supplied. This also comes with a couple of side effect changes: - `PackChangedPre kind=delete` is now also triggered even if the delete won't be done. This makes it more aligned with `kind=install` and `kind=update`. - Force update (`:packupdate!`) and "udpate LSP action" now do two async steps: download/compute updates and apply them. This also results in two progress reports. This is mostly a by-product of the implementation (there has to be a pre-computation of target revision for all plugins before doing `PackChangedPre` in bulk before possibly applying an update), but I kind of like it more this way as it is more explicit of what's going on. If absolutely not acceptable, there might be some hacks to mitigate it at least in code action, but I'd keep it like this. --- runtime/doc/pack.txt | 17 ++-- runtime/lua/vim/pack.lua | 119 +++++++++++++---------- test/functional/plugin/pack_spec.lua | 139 +++++++++++++-------------- 3 files changed, 148 insertions(+), 127 deletions(-) diff --git a/runtime/doc/pack.txt b/runtime/doc/pack.txt index 762caee682..fb355d88b5 100644 --- a/runtime/doc/pack.txt +++ b/runtime/doc/pack.txt @@ -227,12 +227,12 @@ The latest state of all managed plugins is stored inside a *vim.pack-lockfile* located at `$XDG_CONFIG_HOME/nvim/nvim-pack-lock.json`. It is a JSON file that is used to persistently track data about plugins. For a more robust config treat lockfile like its part: put under version control, etc. In this case all -plugins from the lockfile will be installed at once and at lockfile's revision -(instead of inferring from `version`). This is done on the very first -`vim.pack` function call to ensure that lockfile is aligned with what is -actually on the disk. Lockfile should not be edited by hand. Corrupted data -for installed plugins is repaired (including after deleting whole file), but -`version` fields will be missing for not yet added plugins. +plugins from the lockfile will be installed at once (in alphabetical order) +and at lockfile's revision (instead of inferring from `version`). This is done +on the very first `vim.pack` function call to ensure that lockfile is aligned +with what is actually on the disk. Lockfile should not be edited by hand. +Corrupted data for installed plugins is repaired (including after deleting +whole file), but `version` fields will be missing for not yet added plugins. *vim.pack-examples* @@ -383,6 +383,11 @@ Performing actions via `vim.pack` functions can trigger these events: • *PackChangedPre* - before trying to change plugin's state. • *PackChanged* - after plugin's state has changed. +Events are triggered in bulk respecting order of plugins in which they are +supplied. First all `PackChangedPre`, then perform all actions, and only after +- all `PackChanged` for successful actions. This provides more control for +connected plugins, like by specifying dependencies before the plugin itself. + The |event-data| has these keys (type: `vim.event.packchanged.data`): • `active` - whether plugin was added via |vim.pack.add()| to current session. • `kind` - one of "install" (install on disk; before loading), "update" diff --git a/runtime/lua/vim/pack.lua b/runtime/lua/vim/pack.lua index aad11e0915..4b83a87fdc 100644 --- a/runtime/lua/vim/pack.lua +++ b/runtime/lua/vim/pack.lua @@ -19,9 +19,9 @@ ---located at `$XDG_CONFIG_HOME/nvim/nvim-pack-lock.json`. It is a JSON file that ---is used to persistently track data about plugins. ---For a more robust config treat lockfile like its part: put under version control, etc. ----In this case all plugins from the lockfile will be installed at once and at lockfile's revision ----(instead of inferring from `version`). This is done on the very first `vim.pack` function call ----to ensure that lockfile is aligned with what is actually on the disk. +---In this case all plugins from the lockfile will be installed at once (in alphabetical order) and +---at lockfile's revision (instead of inferring from `version`). This is done on the very first +---`vim.pack` function call to ensure that lockfile is aligned with what is actually on the disk. ---Lockfile should not be edited by hand. Corrupted data for installed plugins is repaired ---(including after deleting whole file), but `version` fields will be missing ---for not yet added plugins. @@ -192,6 +192,11 @@ ---- [PackChangedPre]() - before trying to change plugin's state. ---- [PackChanged]() - after plugin's state has changed. --- +---Events are triggered in bulk respecting order of plugins in which they are supplied. +---First all `PackChangedPre`, then perform all actions, and only after - all +---`PackChanged` for successful actions. This provides more control for connected +---plugins, like by specifying dependencies before the plugin itself. +--- ---The |event-data| has these keys (type: `vim.event.packchanged.data`): ---- `active` - whether plugin was added via |vim.pack.add()| to current session. ---- `kind` - one of "install" (install on disk; before loading), @@ -431,6 +436,7 @@ end --- @field err string The latest error when working on plugin. If non-empty, --- all further actions should not be done (including triggering events). --- @field installed? boolean Whether plugin was successfully installed. +--- @field updated? boolean Whether plugin was successfully updated. --- @field version_str? string `spec.version` with resolved version range. --- @field version_ref? string Resolved version as Git reference (if different --- from `version_str`). @@ -514,13 +520,15 @@ end local active_plugins = {} local n_active_plugins = 0 ---- @param p vim.pack.Plug +--- @param plugs vim.pack.Plug[] --- @param event_name 'PackChangedPre'|'PackChanged' --- @param kind 'install'|'update'|'delete' -local function trigger_event(p, event_name, kind) - local active = active_plugins[p.path] ~= nil - local data = { active = active, kind = kind, spec = vim.deepcopy(p.spec), path = p.path } - api.nvim_exec_autocmds(event_name, { pattern = p.path, data = data }) +local function trigger_events(plugs, event_name, kind) + for _, p in ipairs(plugs) do + local active = active_plugins[p.path] ~= nil + local data = { active = active, kind = kind, spec = vim.deepcopy(p.spec), path = p.path } + api.nvim_exec_autocmds(event_name, { pattern = p.path, data = data }) + end end --- @param action string @@ -741,8 +749,6 @@ local function install_list(plug_list, confirm) --- @async --- @param p vim.pack.Plug local function do_install(p) - trigger_event(p, 'PackChangedPre', 'install') - git_clone(p.spec.src, p.path) plugin_lock.plugins[p.spec.name].src = p.spec.src @@ -752,13 +758,16 @@ local function install_list(plug_list, confirm) checkout(p, timestamp, true) p.info.installed = true - - trigger_event(p, 'PackChanged', 'install') end -- Install possibly after user confirmation if not confirm or confirm_install(plug_list) then + trigger_events(plug_list, 'PackChangedPre', 'install') run_list(plug_list, do_install, 'Installing plugins') + local installed = vim.tbl_filter(function(p) --- @param p vim.pack.Plug + return p.info.installed + end, plug_list) + trigger_events(installed, 'PackChanged', 'install') end -- Ensure that not fully installed plugins are absent on disk and in lockfile @@ -1246,6 +1255,26 @@ local function get_update_map(bufnr) return res end +--- Checkout plugins to the update target +--- @param plug_list vim.pack.Plug[] +local function update_list(plug_list) + trigger_events(plug_list, 'PackChangedPre', 'update') + + local timestamp = get_timestamp() + --- @async + --- @param p vim.pack.Plug + local function do_update(p) + checkout(p, timestamp) + p.info.updated = true + end + run_list(plug_list, do_update, 'Applying updates') + + local updated = vim.tbl_filter(function(p) --- @param p vim.pack.Plug + return p.info.updated + end, plug_list) + trigger_events(updated, 'PackChanged', 'update') +end + --- @class vim.pack.keyset.update --- @inlinedoc --- @field force? boolean Whether to skip confirmation and make updates immediately. Default `false`. @@ -1311,13 +1340,12 @@ function M.update(names, opts) git_ensure_exec() lock_read() - -- Perform update - local timestamp = get_timestamp() + -- Infer update details local needs_lock_write = opts.force --- @type boolean --- @async --- @param p vim.pack.Plug - local function do_update(p) + local function infer_details(p) local l_data = plugin_lock.plugins[p.spec.name] -- Ensure proper `origin` if needed if l_data.src ~= p.spec.src then @@ -1337,17 +1365,17 @@ function M.update(names, opts) p.info.sha_target = l_data.rev end infer_update_details(p) - - -- Checkout immediately if no need to confirm - if opts.force and p.info.sha_head ~= p.info.sha_target then - trigger_event(p, 'PackChangedPre', 'update') - checkout(p, timestamp) - trigger_event(p, 'PackChanged', 'update') - end end - local progress_title = opts.force and (opts.offline and 'Applying updates' or 'Updating') - or (opts.offline and 'Computing updates' or 'Downloading updates') - run_list(plug_list, do_update, progress_title) + local infer_title = opts.offline and 'Computing updates' or 'Downloading updates' + run_list(plug_list, infer_details, infer_title) + + -- Update and show report + if opts.force then + local plugs_to_update = vim.tbl_filter(function(p) --- @param p vim.pack.Plug + return p.info.sha_head ~= p.info.sha_target + end, plug_list) + update_list(plugs_to_update) + end if needs_lock_write then lock_write() @@ -1368,22 +1396,13 @@ function M.update(names, opts) end --- @param p vim.pack.Plug - local plugs_to_checkout = vim.tbl_filter(function(p) + local plugs_to_update = vim.tbl_filter(function(p) return to_update[p.spec.name] end, plug_list) - - local timestamp2 = get_timestamp() - --- @async - --- @param p vim.pack.Plug - local function do_checkout(p) - trigger_event(p, 'PackChangedPre', 'update') - checkout(p, timestamp2) - trigger_event(p, 'PackChanged', 'update') - end - run_list(plugs_to_checkout, do_checkout, 'Applying updates') + update_list(plugs_to_update) lock_write() - feedback_log(plugs_to_checkout) + feedback_log(plugs_to_update) end) end @@ -1410,33 +1429,35 @@ function M.del(names, opts) lock_read() - local successful_delete = {} --- @type string[] - local fail_to_delete = {} --- @type string[] + trigger_events(plug_list, 'PackChangedPre', 'delete') + + local deleted = {} --- @type vim.pack.Plug[] + local deleted_names = {} --- @type string[] + local not_deleted_names = {} --- @type string[] for _, p in ipairs(plug_list) do if not active_plugins[p.path] or opts.force then - trigger_event(p, 'PackChangedPre', 'delete') - vim.fs.rm(p.path, { recursive = true, force = true }) active_plugins[p.path] = nil plugin_lock.plugins[p.spec.name] = nil - trigger_event(p, 'PackChanged', 'delete') - successful_delete[#successful_delete + 1] = p.spec.name + deleted[#deleted + 1] = p + deleted_names[#deleted_names + 1] = p.spec.name else - fail_to_delete[#fail_to_delete + 1] = p.spec.name + not_deleted_names[#not_deleted_names + 1] = p.spec.name end end + trigger_events(deleted, 'PackChanged', 'delete') lock_write() - if #successful_delete > 0 then - local suffix = #successful_delete == 1 and '' or 's' - local plugs = table.concat(successful_delete, ', ') + if #deleted_names > 0 then + local suffix = #deleted_names == 1 and '' or 's' + local plugs = table.concat(deleted_names, ', ') notify(('Removed plugin%s: %s'):format(suffix, plugs), 'INFO') end - if #fail_to_delete > 0 then - local plugs = table.concat(fail_to_delete, ', ') + if #not_deleted_names > 0 then + local plugs = table.concat(not_deleted_names, ', ') if opts._ex then util.echo_err(N_('E5810: Some plugins are active and were not deleted: %s'):format(plugs)) return diff --git a/test/functional/plugin/pack_spec.lua b/test/functional/plugin/pack_spec.lua index 94f4cf1ad3..24fd938970 100644 --- a/test/functional/plugin/pack_spec.lua +++ b/test/functional/plugin/pack_spec.lua @@ -280,26 +280,18 @@ local function watch_events(event) end) end ---- @param log table[] -local function make_find_packchanged(log) - --- @param suffix string - return function(suffix, kind, name, version, active, user_data) - local path = pack_get_plug_path(name) - local spec = { name = name, src = repos_src[name], version = version, data = user_data } - local data = { active = active, kind = kind, path = path, spec = spec } - local entry = { event = 'PackChanged' .. suffix, match = vim.fs.abspath(path), data = data } - - local res = 0 - for i, tbl in ipairs(log) do - if vim.deep_equal(tbl, entry) then - res = i - break - end - end - eq(true, res > 0) - - return res +--- Input is array of { suffix, kind, name, version, active, user_data } +--- @param log_compact ([string,string,string,string,boolean,any])[] +local function assert_packchanged(log_compact) + local log = exec_lua('return _G.event_log') + local expected = {} --- @type table[] + for i, l in ipairs(log_compact) do + local path = pack_get_plug_path(l[3]) + local spec = { name = l[3], src = repos_src[l[3]], version = l[4], data = l[6] } + local data = { active = l[5], kind = l[2], path = path, spec = spec } + expected[i] = { event = 'PackChanged' .. l[1], match = vim.fs.abspath(path), data = data } end + eq(expected, log) end local function track_nvim_echo() @@ -314,9 +306,9 @@ local function track_nvim_echo() end) end -local function assert_progress_report(action, step_names) - -- NOTE: Assume that `nvim_echo` mocked log has only progress report messages - local echo_log = exec_lua('return _G.echo_log') ---@type table[] +--- @param echo_log table[]? +local function assert_progress_report(echo_log, action, step_names) + echo_log = echo_log or exec_lua('return _G.echo_log') local n_steps = #step_names eq(n_steps + 2, #echo_log) @@ -653,16 +645,13 @@ describe('vim.pack', function() eq(true, pack_exists('defbranch')) eq(false, exec_lua('return pcall(require, "defbranch")')) - -- Should trigger `kind=install` events - local log = exec_lua('return _G.event_log') - local find_event = make_find_packchanged(log) - local installpre_basic = find_event('Pre', 'install', 'basic', 'main', false, { 'd' }) - local installpre_defbranch = find_event('Pre', 'install', 'defbranch', nil, false) - local install_basic = find_event('', 'install', 'basic', 'main', false, { 'd' }) - local install_defbranch = find_event('', 'install', 'defbranch', nil, false) - eq(4, #log) - eq(true, installpre_basic < install_basic) - eq(true, installpre_defbranch < install_defbranch) + -- Should trigger `kind=install` events in a pre-defined order + assert_packchanged({ + { 'Pre', 'install', 'basic', 'main', false, { 'd' } }, + { 'Pre', 'install', 'defbranch', nil, false }, + { '', 'install', 'basic', 'main', false, { 'd' } }, + { '', 'install', 'defbranch', nil, false }, + }) -- Running `update()` should still update to use `main` exec_lua(function() @@ -920,26 +909,21 @@ describe('vim.pack', function() it('shows progress report during installation', function() track_nvim_echo() vim_pack_add({ repos_src.basic, repos_src.defbranch }) - assert_progress_report('Installing plugins', { 'basic', 'defbranch' }) + assert_progress_report(nil, 'Installing plugins', { 'basic', 'defbranch' }) end) it('triggers relevant events', function() watch_events({ 'PackChangedPre', 'PackChanged' }) -- Should provide event-data respecting manual `version` without inferring default - vim_pack_add({ { src = repos_src.basic, version = 'feat-branch' }, repos_src.defbranch }) - - local log = exec_lua('return _G.event_log') - local find_event = make_find_packchanged(log) - local installpre_basic = find_event('Pre', 'install', 'basic', 'feat-branch', false) - local installpre_defbranch = find_event('Pre', 'install', 'defbranch', nil, false) - local install_basic = find_event('', 'install', 'basic', 'feat-branch', false) - local install_defbranch = find_event('', 'install', 'defbranch', nil, false) - eq(4, #log) - - -- NOTE: There is no guaranteed installation order among separate plugins (as it is async) - eq(true, installpre_basic < install_basic) - eq(true, installpre_defbranch < install_defbranch) + -- Should order events as they were supplied in `vim.pack.add` + vim_pack_add({ repos_src.defbranch, { src = repos_src.basic, version = 'feat-branch' } }) + assert_packchanged({ + { 'Pre', 'install', 'defbranch', nil, false }, + { 'Pre', 'install', 'basic', 'feat-branch', false }, + { '', 'install', 'defbranch', nil, false }, + { '', 'install', 'basic', 'feat-branch', false }, + }) end) it('recognizes several `version` types', function() @@ -1687,13 +1671,19 @@ describe('vim.pack', function() exec_lua('_G.echo_log = {}') ref_lockfile.plugins.fetch.rev = git_get_hash('main', 'fetch') + git_cmd({ 'checkout', 'main' }, 'fetch') repo_write_file('fetch', 'lua/fetch.lua', 'return "fetch new 3"') git_add_commit('Commit to be added 3', 'fetch') assert_action({ 3, 0 }, fetch_actions, 1) pack_assert_content('fetch', 'return "fetch new 2"') - assert_progress_report('Applying updates', { 'fetch' }) + + local echo_log = exec_lua('return _G.echo_log') + assert_progress_report(vim.list_slice(echo_log, 1, 3), 'Computing updates', { 'fetch' }) + assert_progress_report(vim.list_slice(echo_log, 4, 6), 'Applying updates', { 'fetch' }) + eq(6, #echo_log) + line_match(1, '^# Update') eq(1, api.nvim_buf_line_count(0)) @@ -1943,7 +1933,7 @@ describe('vim.pack', function() end) -- There should be no progress report about downloading updates - assert_progress_report('Computing updates', { 'defbranch' }) + assert_progress_report(nil, 'Computing updates', { 'defbranch' }) n.exec('write') pack_assert_content('defbranch', 'return "defbranch main"') @@ -1956,22 +1946,28 @@ describe('vim.pack', function() exec_lua('vim.pack.update()') -- During initial download - assert_progress_report('Downloading updates', { 'fetch', 'defbranch', 'semver' }) + assert_progress_report(nil, 'Downloading updates', { 'fetch', 'defbranch', 'semver' }) exec_lua('_G.echo_log = {}') -- During application (only for plugins that have updates) n.exec('write') - assert_progress_report('Applying updates', { 'fetch' }) + assert_progress_report(nil, 'Applying updates', { 'fetch' }) -- During force update n.clear() track_nvim_echo() + git_cmd({ 'checkout', 'main' }, 'fetch') repo_write_file('fetch', 'lua/fetch.lua', 'return "fetch new 3"') git_add_commit('Commit to be added 3', 'fetch') vim_pack_add({ repos_src.fetch, repos_src.defbranch }) exec_lua('vim.pack.update(nil, { force = true })') - assert_progress_report('Updating', { 'fetch', 'defbranch', 'semver' }) + + local echo_log = exec_lua('return _G.echo_log') + local all_plugs = { 'fetch', 'defbranch', 'semver' } + assert_progress_report(vim.list_slice(echo_log, 1, 5), 'Downloading updates', all_plugs) + assert_progress_report(vim.list_slice(echo_log, 6, 8), 'Applying updates', { 'fetch' }) + eq(8, #echo_log) end) it('triggers relevant events', function() @@ -1983,11 +1979,10 @@ describe('vim.pack', function() -- Should trigger relevant events only for actually updated plugins n.exec('write') - local log = exec_lua('return _G.event_log') - local find_event = make_find_packchanged(log) - eq(1, find_event('Pre', 'update', 'fetch', nil, true)) - eq(2, find_event('', 'update', 'fetch', nil, true)) - eq(2, #log) + assert_packchanged({ + { 'Pre', 'update', 'fetch', nil, true }, + { '', 'update', 'fetch', nil, true }, + }) end) it('stashes before applying changes', function() @@ -2332,21 +2327,23 @@ describe('vim.pack', function() local msg = 'vim.pack: Removed plugins: basic, plugindirs' eq(msg, n.exec_capture('messages')) + -- Should trigger relevant events in order as specified in `vim.pack.add()` + assert_packchanged({ + { 'Pre', 'delete', 'basic', 'feat-branch', false }, + { 'Pre', 'delete', 'defbranch', nil, true }, + { 'Pre', 'delete', 'plugindirs', nil, false }, + { '', 'delete', 'basic', 'feat-branch', false }, + { '', 'delete', 'plugindirs', nil, false }, + }) + exec_lua('_G.event_log = {}') + -- `:packdel` should output E5810 instead of the normal error eq( 'Vim(packdel):E5810: Some plugins are active and were not deleted: defbranch', pcall_err(n.command, 'packdel defbranch') ) assert_on_disk({ defbranch = true }) - - -- Should trigger relevant events in order as specified in `vim.pack.add()` - local log = exec_lua('return _G.event_log') - local find_event = make_find_packchanged(log) - eq(1, find_event('Pre', 'delete', 'basic', 'feat-branch', false)) - eq(2, find_event('', 'delete', 'basic', 'feat-branch', false)) - eq(3, find_event('Pre', 'delete', 'plugindirs', nil, false)) - eq(4, find_event('', 'delete', 'plugindirs', nil, false)) - eq(4, #log) + assert_packchanged({ { 'Pre', 'delete', 'defbranch', nil, true } }) -- Should be possible to force delete active plugins n.exec('messages clear') @@ -2355,15 +2352,13 @@ describe('vim.pack', function() vim.pack.del({ 'defbranch' }, { force = true }) end) - assert_on_disk({ basic = false, defbranch = false, plugindirs = false }) - eq('vim.pack: Removed plugin: defbranch', n.exec_capture('messages')) - log = exec_lua('return _G.event_log') - find_event = make_find_packchanged(log) - eq(1, find_event('Pre', 'delete', 'defbranch', nil, true)) - eq(2, find_event('', 'delete', 'defbranch', nil, false)) - eq(2, #log) + assert_on_disk({ basic = false, defbranch = false, plugindirs = false }) + assert_packchanged({ + { 'Pre', 'delete', 'defbranch', nil, true }, + { '', 'delete', 'defbranch', nil, false }, + }) end) it('works without prior `add()`', function()