Skip to content
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

Allow VCS diffing to ignore staged changes #5825

Open
xJonathanLEI opened this issue Feb 4, 2023 · 4 comments
Open

Allow VCS diffing to ignore staged changes #5825

xJonathanLEI opened this issue Feb 4, 2023 · 4 comments
Labels
A-helix-term Area: Helix term improvements A-vcs Area: Version control system interaction C-enhancement Category: Improvements

Comments

@xJonathanLEI
Copy link
Contributor

Allowing VCS diffing to ignore staged changes (aka. diffing against the staged index instead of HEAD) in Git gutters and the (hopefully) upcoming changed file picker (#5645) is very helpful for code review.

Today we can already use Helix for performing simple code reviews:

  1. Rebase PR branch on top of merge base;
  2. Soft reset branch to merge base;
  3. Review changed file one by one (to be made EASY by Changed file picker #5645), and stage the reviewed file;
  4. Check git status to see if there's any unstaged changes, and if so, repeat step 3.

We use staging to mark files as reviewed. All good, except two issues:

  1. We have to rely on an external tool git status for checking review status. Even with Changed file picker #5645 merged, there's still no way to only see the list of unstaged changes;
  2. Inside a changed file, there's no way to only focuse on unstaged (i.e. unreviewed) hunks. It's a problem when there are many changes inside a single file.

These issues can be resolved simply by allowing VCS diffing to set staged tree as diff base. Doing this will make Helix a much more viable option for code review. We would only still need gitui for staging individual hunks.

In fact, I already have a (wip) working impl as POC. It's obviously not complete as it hard-codes to always diff against the index tree (no way to diff against HEAD). I'm submitting this issue first instead of getting the PR ready because I'm not sure what's the optimal UX for this feature (and also because #5645 is not merged yet).

I see 2 options:

  1. Add yet another picker and yet another gutter with the only difference being the diff base.
  2. Make it a config option. Users can switch between the two on the fly with :set xxxx.

I'd prefer option 2. WDYT?

@xJonathanLEI xJonathanLEI added the C-enhancement Category: Improvements label Feb 4, 2023
@pascalkuthe
Copy link
Member

My longterm vision (specifically for the diffmode) is to adds come command to change the document diff provider.
For example you could do :diff-base git HEAD diff-base git INDEX diff-base git origin/master diff-base git origin/master:/foo.rs or diff-base file foo.rs. This setting would be stored as an Option on the document and would be set by this setting. diff-base reset could set that setting back to None. If the setting is None then we would fallback to the global lockup we use right now. Right now diff providers are not configurable at all as there is only git. In the future I want the default diff provider to be a config option. Both to support multiple different VCS aswell as allowing configuration like what you requested here. That config option would accept similar strings as the diff-provder command. For example:

diff_providers = ["git", "pejul"]

would diff against git HEAD or pejul HEAD (or whatever the equivalent is called there) in that order *meaning in the weird case that both are available git would be used).

diff_providers = ["git INDEX"]

would diff against the index instead of head. It might seem unnecessary to set any value other then HEAD or INDEX here (and hence a simple bool switch would suffice) but with code-review you could :set diff_providers ["git origin/master"] (or with #5748 set it per workspace).

Implementing all of this should be possible but is a decent amount of work,

To simplify we couldconsider moving away from the list based lookup (that makes things a lot more difficult) as only git is implemented right now (falling back to different git revisions seems niche/unnecessary) and if other VCS systems are added you can simply use a .helix/config.toml file to set the diff_provider for the specific workspace.

@xJonathanLEI
Copy link
Contributor Author

@pascalkuthe I think that makes a lot of sense. I would think that having multiple active diff providers is kinda not needed right now (and maybe in the near future).

So maybe we can just:

  • go with the single diff base provider approach;
  • change provider parsing logic to allow supplying a diff base like git INDEX, with the default being HEAD?
  • add a helper command :diff-base xxx xxx although technically you can use the :set command to achieve the same thing.

@pascalkuthe
Copy link
Member

@pascalkuthe I think that makes a lot of sense. I would think that having multiple active diff providers is kinda not needed right now (and maybe in the near future).

So maybe we can just:

* go with the single diff base provider approach;

* change provider parsing logic to allow supplying a diff base like `git INDEX`, with the default being `HEAD`?

* add a helper command `:diff-base xxx xxx` although technically you can use the `:set` command to achieve the same thing.

the point of :diff-base is that you can change the diff base per document. For example to compare one file to a different file. That is important to have for the diffmode in particular if you want to compare two files you don't want to comapre every other file to that file aswell. It's probably more useful for the diffmode so you could leave that to future work.

The parsing is quite tricky but luckily gitoxide already has a full implementaiton of rev-parse so you only need to call that. You just need to a speceical case for the index as you can not refer to the index directly in git AFAIK

@xJonathanLEI
Copy link
Contributor Author

Got it. I will work on supporting the gutters first for this feature unless #5645 gets merged first.

@kirawi kirawi added A-helix-term Area: Helix term improvements A-vcs Area: Version control system interaction labels Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements A-vcs Area: Version control system interaction C-enhancement Category: Improvements
Projects
None yet
Development

No branches or pull requests

3 participants