Skip to content

Conversation

@txemaotero
Copy link
Contributor

This PR addresses the feature requested on issue #1239

What was implemented?

Two new functionalities on the CommitView buffer. Placing the cursor on a line inside a hunk:

  • Pressing <cr>: Jump file at that specific location in the corresponding commit, i.e., if the cursor is at a line added or at a context line, jump to the commit being viewed. If it is on a deleted line, jump to the parent commit. The view of the commit is done in an hidden read-only buffer and pressing q exits it and put you back on the CommitView.
  • Pressing a (the default for the new map OpenFileInWorktree) tries to open the file the hunk belongs to but in its current state (current worktree).

Inspiration and design decisions

The <cr> behavior mimics what magit does (see magit-diff-visit-file). Basically, it opens the view of the file in the current CommitView window.

I didn't find an equivalent for the other feature on magit so I took some decisions when opening the file in the current worktree:

  • If there is any window managed by the user (not a Neogit handled one), the buffer is opened there.
  • If not, open a new tab.

Initially I opted to implement a more destructive approach, i.e., opening the file closes neogit. However, I realized that you can also open a CommitView while rebasing and you don't want to quit neogit in that location. That was the main motivation to change the approach and be more conservative.

Why a by default and not <s-cr> as mentioned in the issue? Mainly because <s-cr> is a problematic map in some shells/environments. I haven't a strong position about a so any suggestion is welcome (note that it shouldn't collide with other mappings, e.g., we cannot use c).

Some implementation notes

Some general notes for the review:

  • I found the Hunk class declaration missed some members so I added them in a separate commit
  • I've added some documentation for the neogit_commit_buffer section
  • Isolated utilities in a new file jump.lua
  • Add a test spec for one of the key new utility

Note: I run the test locally and I've got a failure:

Randomized with seed 39550
 0/469 |>                                                                                                                                                                                                                 |  ETA: ??:??:??

  1) Remote Popup prune_branches with a remote configured can launch remote config popup
     Failure/Error: expect(nvim.screen.last).to start_with("Pruned remote origin")
       expected "                                                                                " to start with "Pruned remote origin"
     # ./spec/popups/remote_popup_spec.rb:145:in `block (5 levels) in <top (required)>'
     # ./spec/support/helpers.rb:17:in `block in await'
     # ./spec/support/helpers.rb:16:in `await'
     # ./spec/popups/remote_popup_spec.rb:144:in `block (4 levels) in <top (required)>'
     # ./spec/spec_helper.rb:59:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:58:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:50:in `block (5 levels) in <top (required)>'
     # ./spec/spec_helper.rb:47:in `chdir'
     # ./spec/spec_helper.rb:47:in `block (4 levels) in <top (required)>'
     # ./spec/spec_helper.rb:44:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:43:in `block (2 levels) in <top (required)>'

 469/469 |===================================================================================================== 100 =====================================================================================================>| Time: 00:04:16

However, the same failure is reproducible on the master branch so, not related with the changes.

@txemaotero txemaotero force-pushed the feat-1239-goto-hunk-locations-from-commit-view branch from 116dfe2 to 76cddb3 Compare November 21, 2025 20:29
@CKolkey
Copy link
Member

CKolkey commented Nov 22, 2025

I think this is really great! Thank you for taking the time to work on this. I'll test-drive this over the week to see if any issues arise, but I don't really have anything I'd want changed (besides linting stuff). Really good stuff.

- Allow jumping from commit view hunks to worktree or committed file content
- Share jump logic via new lib/jump helpers and extra tests
- Make commit-view "open file" mapping configurable
- Document commit view mappings and behavior
@txemaotero txemaotero force-pushed the feat-1239-goto-hunk-locations-from-commit-view branch from 76cddb3 to 19a64e1 Compare November 23, 2025 20:10
@txemaotero
Copy link
Contributor Author

Thank you! I've pushed the fixes for the linting errors (I knew I was missing something 😅 )

I feel quite confortable now navigating the code so I don't discard contributing again with more fixes, features, documentation or whatever. I really like this plugin and I think this is good a way of expressing my gratitude 😄

@CKolkey
Copy link
Member

CKolkey commented Nov 25, 2025

Anything you want to contribute will be appreciated :D As you can see from the commit graph, I haven't had time for this project in about a year, and I don't foresee that changing any time soon. Let me know if you have any questions, and I'll be happy to assist as much as I can.

@CKolkey CKolkey merged commit bf4e4bd into NeogitOrg:master Nov 25, 2025
6 checks passed
txemaotero added a commit to txemaotero/neogit that referenced this pull request Nov 25, 2025
- Fix a bug introduced in PR NeogitOrg#1862.
- hunk.disk_from was used before that PR to calculate row to jump. After
  that, hunk.index_from was used. This commit recovers the previous
  behavior.
@txemaotero
Copy link
Contributor Author

Hey @CKolkey , I'm really sorry 🙏 but there was a bug introduced by this PR. I catch it some minutes ago using the feature branch at work. Then I saw you already merged 😓 I've opened a new PR to fix it. Take a look and I recommend to merge ASAP because it can be really annoying.

CKolkey pushed a commit that referenced this pull request Nov 25, 2025
- Fix a bug introduced in PR #1862.
- hunk.disk_from was used before that PR to calculate row to jump. After
  that, hunk.index_from was used. This commit recovers the previous
  behavior.
@CKolkey
Copy link
Member

CKolkey commented Nov 25, 2025

No worries, thanks for catching and fixing it so quickly :)

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.

2 participants