Skip to content

Step to the next change under the cursor - #321

Open
seflue wants to merge 6 commits into
dlyongemallo:mainfrom
seflue:feat/change-here-navigation
Open

Step to the next change under the cursor#321
seflue wants to merge 6 commits into
dlyongemallo:mainfrom
seflue:feat/change-here-navigation

Conversation

@seflue

@seflue seflue commented Sep 11, 2026

Copy link
Copy Markdown

Closes #313

Two new actions for the file history, select_next_change_here and select_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.

  • The carry from fix(view): keep the cursor on its code line #314 places the cursor by diffing the revision being left against the one arriving. A walk can skip many commits in one swap, and over that span the diff may align identical code the wrong way and lose the line. The walk already mapped the line through every revision it read, so it passes that result to the carry instead.
  • A binary or unreadable revision cannot be compared. The walk opens that commit.

Neither action has a default keymap. They work from the panel as well as the view.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 from StandardView into a new line_map module returning (lnum, touched), and let a caller supply a pre-resolved carry line via set_carry_lnum.
  • Add FileHistoryView:select_change_here(dir) plus the pick_change_here_target / FileEntry:main_file helpers, 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_map unit 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.

Comment thread lua/diffview/scene/views/file_history/file_history_view.lua Outdated
Comment thread lua/diffview/scene/views/file_history/file_history_view.lua Outdated
Comment thread lua/diffview/scene/views/file_history/file_history_view.lua
@seflue

seflue commented Sep 12, 2026

Copy link
Copy Markdown
Author

Thanks for the review. I pushed my corrections as fixups, for an easier review.
We can autosquash them as soon as you approve the PR.

@seflue
seflue requested review from dlyongemallo and a balanced review from Copilot September 12, 2026 09:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 --reverse and Mercurial supports --reversed. In those histories +1 walks newer, and the streaming frontier is also at the newer end, so both actions use the wrong chronology and still_loading is 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-local that file is the shared LOCAL buffer (vcs/adapter.lua:339-370), and with Git --base it 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:show yields, 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 --all makes this more common). Diffing their snapshots and assigning that difference to prev/candidate can 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.binary is still nil; it is populated lazily by File: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 failed vim.diff is then reported by line_map.between as 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 show is pending, the stale walk resumes and eventually overrides that newer selection. Also verify that panel.cur_item is 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

Comment on lines +255 to +256
FileHistoryView.select_change_here = async.void(function(self, dir)
local cur_entry, cur_file = self.panel.cur_item[1], self.panel.cur_item[2]
Comment on lines +293 to +294
if candidate then
local file = candidate:main_file()
Comment on lines +296 to +299
-- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hard to follow one code block through a file's history

3 participants