Skip to content

Improved diff popup with fzf-lua & advanced range selections #1748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Aitai
Copy link
Contributor

@Aitai Aitai commented May 30, 2025

This PR brings a bunch of new commands to the diff popup that I've been missing personally.

  • New diffing options:
    • Diff between branch ranges (b), commit/ref ranges (c), and tag ranges (t).
    • Diff HEAD to a specific commit/ref (h).
    • Diff specific files (f) or paths/globs (p) against HEAD.
    • The Stash diff (S) is also updated to use the new picker.
  • Integrating fzf-lua:
    • Uses fzf-lua (if available) for selecting branches, commits, tags, and stashes.
  • Internal changes:
    • diffview.open can now accept an array of arguments, simplifying how diffs with multiple file arguments are invoked.
    • Removed --literal-pathspecs from default git arguments to allow globs in paths.

Testing:
I've tested these changes on Neovim v0.12.0-dev-461+gb28bbee539. The fzf-lua integration works as expected, and the fallback to another fuzzy finder (which I've tested with Telescope) works too.

@CKolkey
Copy link
Member

CKolkey commented May 30, 2025

Is the existing fzf-lua integration not working?

elseif config.check_integration("fzf_lua") then
local fzf_lua = require("fzf-lua")
fzf_lua.fzf_exec(self.entries, {
prompt = string.format("%s> ", self.opts.prompt_prefix),
fzf_opts = fzf_opts(self.opts),
winopts = {
height = self.opts.layout_config.height,
},
actions = fzf_actions(on_select, self.opts.allow_multi, self.opts.refocus_status),
})

Anyways, super cool you're taking the initiative here :)

As a general note, the fzf specific stuff doesn't belong in the action, it's an implementation detail of the FuzzyFinderBuffer (and underlying Finder class). The action doesn't need to know/care which finder engine is being used.

Diffing between branch ranges, tag ranges, and commit ranges already works in the 'diff range' action. the input can be any valid refspec. However, the UX could be improved, especially when a range is selected in the log/reflog buffer. We could do more to pre-populate selections.

Diffing a specific commit against HEAD is a good idea - "diff this" just does a refspec to it's parent. Adding something like this to HEAD would make sense.

So to summarize:

  • Remove all fzf specific stuff from action
  • branch range, tag range already exists
  • commit range exists (same as above), but UX could be improved.
  • diffing paths would be a good addition
  • diffing against HEAD would be a good addition

@Aitai
Copy link
Contributor Author

Aitai commented May 30, 2025

The existing fzf-lua integration works well for general list fuzzy finding, but I've tried to enhance the diff menu by using fzf-lua's specific providers (like fzf_lua.git_branches(), fzf_lua.git_commits(), etc.) when available for actions like branch/commit/stash/tag ranges. This offers richer previews and native fzf-lua handling for those specific object types. If these specific fzf-lua methods aren't available or for other generic lists, it falls back to Neogit's FuzzyFinderBuffer (which then uses the existing generic fzf-lua integration if enabled).

@CKolkey
Copy link
Member

CKolkey commented May 30, 2025

I edited my above comment before seeing you had replied already, so have a look.

The existing fzf-lua integration works well for general list fuzzy finding, but I've tried to enhance the diff menu by using fzf-lua's specific providers (like fzf_lua.git_branches(), fzf_lua.git_commits(), etc.) when available for actions like branch/commit/stash/tag ranges. This offers richer previews and native fzf-lua handling for those specific object types. If these specific fzf-lua methods aren't available or for other generic lists, it falls back to Neogit's FuzzyFinderBuffer (which then uses the existing generic fzf-lua integration if enabled).

Ah, ok, that's going to be a no-go then. I'm not interested in having fzf specific stuff at this layer, because that would mean snacks and telescope specific stuff would also belong here... which quickly gets out of hand. Honestly, I've considered removing all integrations, and just implementing my own fuzzy picker that is more tailored to whats needed here, but... not a high priority.

@Aitai
Copy link
Contributor Author

Aitai commented May 30, 2025

First of all, thanks for this amazing project and your guidance!

I understand the concern about scalability if every integration (telescope, snacks, etc.) required its own handling at that level. My primary goal with the changes was to improve the user experience for diffing ranges, especially when fzf-lua (or potentially Telescope with its built-ins) is available, as they offer much richer, context-specific previews for git objects compared to a generic list.

Let me address the specific points and explain the rationale behind my initial approach:

  1. The old M.range function and commit diffing:
    You mentioned that "Diffing between branch ranges, tag ranges, and commit ranges already works in the 'diff range' action."
    The previous M.range function:

    function M.range(popup)
      local options = util.deduplicate(
        util.merge(
          { git.branch.current() or "HEAD" },
          git.branch.get_all_branches(false),
          git.tag.list(),
          git.refs.heads()
        )
      )
      -- ... then used FuzzyFinderBuffer.new(options) for both 'from' and 'to' ...
    end

    This options list primarily contained branches, tags, and symbolic refs like HEAD. It did not include a list of actual commits (e.g., from git log). Consequently:

    • No commit discovery: Users couldn't browse their commit history to select a commit for the range.
    • Manual commit SHA: Even when I tried to manually enter a commit SHA (that I knew existed) into the prompt for this old M.range function or for the M.commit function, I found it didn't work for creating commit diffs.
  2. Rationale for specific branch_range, commit_range, tag_range actions:
    Given the limitations of the old M.range (especially for commits) and the desire to leverage the rich UIs of specialized pickers, I split the functionality:

    • When a user intends to diff, for example, two branches, presenting them with a dedicated branch picker (like fzf_lua.git_branches() or telescope.builtin.git_branches()) is a much more intuitive and powerful experience than a generic list of all refs. These specialized pickers show relevant information (tracking status, last commit message for branches; commit details for commits) that is lost if Neogit just passes a flat list of names/SHAs to a generic fuzzy finder.
    • Pickers like fzf-lua and telescope have their own optimized ways to fetch and display branches, commits, and tags. My aim was to tap into these native capabilities for the best possible UX when these integrations are enabled.

Path Forward:
I understand the "no picker-specific logic in actions" constraint. To achieve the desired UX improvements (like rich previews for specific git object types) without putting picker-specific calls in actions.lua, the FuzzyFinderBuffer (or the underlying Finder) would indeed need to be enhanced.

Perhaps FuzzyFinderBuffer could accept an item_type hint (e.g., 'branch', 'commit', 'tag'). Then, within Finder, if fzf-lua is active and an item_type like 'branch' is specified, Finder could internally decide to call fzf_lua.git_branches() instead of fzf_lua.fzf_exec(self.entries). A similar logic could apply if Telescope is the active picker and it has a corresponding built-in for that item_type. If no specialized provider exists in the active picker for that item_type, it would fall back to Neogit collecting the entries and using the generic fuzzy finding method (fzf_lua.fzf_exec or telescope over self.entries).

This would keep the actions.lua layer clean – it would just tell FuzzyFinderBuffer "I need to pick a branch" or "I need to pick a commit."

Even if we abstract the picker logic, I believe presenting distinct branch, commit and tag actions in the diff popup menu itself is more user-friendly than a single "diff range" that then prompts "What type of range?" This directness in the menu improves discoverability and reduces an interaction step for the user. The M.paths and M.head_to_commit_ref additions also seem well-received, and they naturally fit this more granular approach.

If enhancing Finder to support specialized picker providers isn't immediately feasible, would a compromise be to keep the separate actions for now, but ensure the fzf-lua path within actions.lua is very cleanly isolated and primarily just for dispatching to the correct fzf-lua method, with a clear fallback to the existing FuzzyFinderBuffer for everything else (including other pickers)? This would at least allow users to benefit from the improved fzf-lua experience for these common tasks immediately.

Thanks again!

@CKolkey
Copy link
Member

CKolkey commented May 30, 2025

Really great write up - lets dig into it.

  1. One of the ideas with this plugin is that the entire UI is very interactive. For example, the reason that the fuzzy finder doesn't list commits is because there's already a view for that - the log and reflog views. When you have a commit under the cursor in the log view, it's (almost always) passed in some way to the action invoked. Now, you've correctly spotted that this isn't wired up for the diff#this and diff#range actions - but it should be.

As for why it wouldn't work with manual entry... I don't know. That should 😅

Regardless, it's quite a simple fix, so I'll merge that now. I know discoverability of this "everything is interactive" isn't great - in fact I think it's mostly hidden unless you happen to notice it working by accident. Open to ideas on improving that.

  1. I think there are two main points here: showing more specialized information in the picker, and having individual actions for different "types" of refs.

To the first point, maybe I'm weird, but I don't personally see the value-add of that information. I can understand that others would, but IMO having a single list that supports commits/branches/tags/refs/stashes is ideal.

That said, I know not everyone will agree with me 😄

The second point, having separate actions per type, I think boils down to either providing a more "user friendly" and "rich" UI with specific actions for specific "types" of objects, or a UI that recognizes that all those "types" are all just revisions under the surface, and providing one interface that doesn't discriminate (but is therefore less "rich").

I think I see an issue, though, with more bespoke pickers like you describe. Items like HEAD and such no longer have a place, but are quite valuable. If you wanted to diff a tag against CHERRY_PICK_HEAD, for example, how would that be accomplished?

(the above issue aside, I think your "item_type" solution is reasonable)

There's probably some points I missed, but that's the meat of it :)

@Aitai
Copy link
Contributor Author

Aitai commented May 30, 2025

My initial observation that the old 'diff range' didn't list commits makes much more sense now. If 'diff range' can be enhanced to seamlessly use commits selected from the log view, that would indeed cover a significant part of the commit diffing use case. Thanks for merging the fix related to manual entry!

I appreciate your perspective on preferring a single, unified list for all ref types in the picker. You raised an excellent point: if we have, say, a "Diff Tag Range" action that only shows tags in its picker, how would a user diff a tag against HEAD, CHERRY_PICK_HEAD, etc.? Those generic/symbolic refs wouldn't naturally appear in a tag-only list (or any picker plugin with git actions as far as I can see). This is a clear advantage of a unified picker.
My drive for specialized pickers (like fzf_lua.git_commits) stemmed from wanting to provide users (especially those heavily using fzf-lua, telescope, or snacks) with the familiar, rich previews their tools' native git integrations offer (e.g., commit details, branch topology with tag/commit list). And personally, I love the syntax highlighting inside the popups and live previews to confirm I'm selecting the right thing. A generic list passed to fzf_lua.fzf_exec can't provide this.

For power-user scenarios like diffing CHERRY_PICK_HEAD against a specific tag, one could resolve the tag to its SHA and then use the "Diff Commit Range" action with the SHA of CHERRY_PICK_HEAD and SHA of the tag. Admittedly, this is quite cumbersome and would definitely benefit from the more generic range action.

Enhancing Finder with an item_type hint (e.g., 'branch', 'commit', 'tag', 'any_ref') seems like the way to achieve this abstraction. Finder could then internally decide to use a specialized picker provider if available for that item_type, or fall back to Neogit's generic list (which would include symbolic refs like HEAD for the 'any_ref' type).

Even with an enhanced Finder, I still lean towards having separate "Diff Branch Range," "Diff Commit Range," and "Diff Tag Range" actions visible in the popup menu. This makes common operations more discoverable.

  • Those actions would call Finder with the appropriate item_type.
  • We would still need a more generic "Diff Custom Range" action using Finder with item_type: 'any_ref' (or direct refspec input which would allow for things like <rev>^{<type>}, branch@{yesterday} or :/commit message) to handle arbitrary combinations like "tag vs. HEAD".

This hybrid UI (discoverable common actions + a flexible generic one), backed by an enhanced Finder, could offer a good balance.

I'm happy to work on refactoring towards the Finder enhancement with item_type if that aligns with your vision.

Let me know your thoughts on this direction. Thanks again for the great discussion!

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