Step to the next change under the cursor - #321
Conversation
Stepping to the next change follows the file under its former name once the walk passes the commit that renamed it. A pure rename touches no line, so it is passed over like any other commit that leaves the line alone; a rename that also edits the line is where the walk stops. Before, the walk opened the renaming commit without a mapped line, because older entries list the file under a name it no longer matched on. Git reports a rename only when both names lie inside the history's pathspec, so wherever a rename shows up, the older entries are in the list too, and nothing is left to guard against.
There was a problem hiding this comment.
🔵 Needs a closer look
It adds subtle async history-walking logic with intricate rename, multi-file, and cursor-carry interactions that warrant human review despite good test coverage.
Pull request overview
This PR implements step 2 of #313 (building on the cursor-carry work in #314): two new file-history actions, select_next_change_here and select_prev_change_here, that walk the history to the next older/newer commit which actually changed the code under the cursor, skipping commits that only shifted the line or touched the file elsewhere. To decide "did this commit change the line?", it reads each revision's text with adapter:show (without opening the commit) and reuses the line-mapping logic from #314, which is extracted into a new standalone diffview.line_map module and extended to also report whether a line was rewritten. The walk handles renames (following the file under its former name) and multi-file commits (resolving by the path under the cursor rather than files[1]), and hands the walked line to the cursor carry so a multi-commit skip doesn't misplace the cursor.
Changes:
- Extract
_map_lnum/diff helpers fromStandardViewinto a newline_mapmodule returning(lnum, touched), and let a caller supply a pre-resolved carry line viaset_carry_lnum. - Add
FileHistoryView:select_change_here(dir)plus thepick_change_here_target/FileEntry:main_filehelpers, wire the two actions, and register/document them (no default keymap). - Add thorough functional tests for the walk (shift/rename/multi-file/identical-body cases) and update
line_mapunit tests.
File summaries
| File | Description |
|---|---|
lua/diffview/line_map.lua |
New module holding the pure line-mapping/diff logic, now also returning a touched flag. |
lua/diffview/scene/views/standard/standard_view.lua |
Delegates to line_map, adds winview_at, _carry_lnum, and a caller-supplied line override in restore_main_view. |
lua/diffview/scene/views/file_history/file_history_view.lua |
Implements the change-here walk and its rename/multi-file target resolution. |
lua/diffview/scene/file_entry.lua |
Adds main_file() to read an entry's main-window file without opening it. |
lua/diffview/scene/views/file_history/listeners.lua |
Wires the two new actions to select_change_here. |
lua/diffview/actions.lua |
Registers the new action names and type annotations. |
doc/diffview.txt |
Documents the two actions and expands the cursor-carry section. |
lua/diffview/tests/functional/change_here_spec.lua |
New end-to-end tests across shift/rename/multi-file/identical-body histories. |
lua/diffview/tests/functional/cursor_carry_spec.lua |
Repoints unit tests to line_map and adds touched/between coverage. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for the review. I pushed my corrections as fixups, for an easier review. |
There was a problem hiding this comment.
🔵 Needs a closer look
Pinned/reversed histories, non-linear commit graphs, binary revisions, and concurrent navigation can produce incorrect results.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
lua/diffview/scene/views/file_history/file_history_view.lua:255
- This assumes increasing indices always mean older commits, but Git supports
--reverseand Mercurial supports--reversed. In those histories+1walks newer, and the streaming frontier is also at the newer end, so both actions use the wrong chronology andstill_loadingis computed for the wrong boundary. Keep chronological direction separate from the index step and derive the latter from the active log options.
This issue also appears in the following locations of the same file:
- line 294
- line 296
- line 309
- line 318
lua/diffview/scene/views/file_history/file_history_view.lua:294
main_file()returns the displayed b-side, not necessarily this entry's commit snapshot. In--pin-localthat file is the shared LOCAL buffer (vcs/adapter.lua:339-370), and with Git--baseit is the fixed base (vcs/adapters/git/init.lua:1450-1462), so every candidate read can compare the same pinned text and miss or misattribute all changes. Resolve the revision represented by the candidate commit (while handling the synthetic working-tree entry separately) instead of reading the layout's main side.
local file = candidate:main_file()
lua/diffview/scene/views/file_history/file_history_view.lua:311
adapter:showyields, but this guard only detects closure or a tab switch. If the user selects another entry/file in the same tab while the scan is running, this stale coroutine resumes and eventually opens its old result, overriding the newer navigation. Re-check that the panel still has the entry/file captured at the start before continuing.
if self:swap_cancelled() then
return
end
lua/diffview/scene/views/file_history/file_history_view.lua:320
- Adjacent log entries are not guaranteed to be parent/child (for example, default Git history can traverse both sides of a merge, and
--allmakes this more common). Diffing their snapshots and assigning that difference toprev/candidatecan therefore stop on a commit that did not change the line relative to its own parent. Determine whether each commit touched the line from that entry's own before/after revisions, or constrain the walk to an actual ancestry chain.
local touched
local before = lnum
lnum, touched = line_map.between(lines, next_lines, lnum)
lua/diffview/scene/views/file_history/file_history_view.lua:300
- For unopened entries
File.binaryis stillnil; it is populated lazily byFile:create_buffer(vcs/file.lua:289-297). Since this walk deliberately does not open candidates, this branch does not actually detect binary revisions, and a failedvim.diffis then reported byline_map.betweenas an untouched line, allowing the walk to skip the commit despite the documented fallback. Explicitly probe comparability or propagate a comparison-failed result so the candidate is opened.
-- Nothing to compare against: a binary or unreadable revision has no
-- lines. Open it and let the reader judge.
if not file or file.binary then
found = candidate
break
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Pinned histories, binary revisions, and asynchronous navigation races can produce incorrect traversal behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
lua/diffview/scene/views/file_history/file_history_view.lua:310
- This cancellation check only detects closing or switching tabpages. If the user navigates to another entry in the same tab while
showis pending, the stale walk resumes and eventually overrides that newer selection. Also verify thatpanel.cur_itemis still the captured entry/file (or use a walk generation token) after every yield.
await(async.scheduler())
if self:swap_cancelled() then
return
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| FileHistoryView.select_change_here = async.void(function(self, dir) | ||
| local cur_entry, cur_file = self.panel.cur_item[1], self.panel.cur_item[2] |
| if candidate then | ||
| local file = candidate:main_file() |
| -- Nothing to compare against: a binary or unreadable revision has no | ||
| -- lines. Open it and let the reader judge. | ||
| if not file or file.binary then | ||
| found = candidate |
Closes #313
Two new actions for the file history,
select_next_change_hereandselect_prev_change_here, open the next older or newer commit that changed the code under the cursor. Commits that touched the file somewhere else are skipped, and the cursor stays on the line.Whether a commit changed the line is decided with the mapping from #314, extended to tell whether a line is part of rewritten text or was only shifted by changes above it. The walk applies that test to revision text read with
adapter:show, one revision per step, so it never opens the commits it passes and should work on every adapter.The walk handles renames and moves. Past the renaming commit it looks for the file under its former name. A pure rename touches no line and is skipped like any other commit that leaves the line alone; a rename that also edits the line is where the walk stops. In a history spanning several files, each commit is read at the file under the cursor.
If no commit ahead changes the line, the view does not move and a message reports that. While the log is still loading, the message reports the loading instead; pressing again once more commits are in continues the walk.
Neither action has a default keymap. They work from the panel as well as the view.