Changed file picker

[xJonathanLEI]

I just started learning Git internals so I might be doing things wrong. Please feel free to correct me :)

See known issues here

Resolves #5362.

This PR adds a "Git status" file picker that's intended to mimic the list of files from a git status command invocation.

I find this feature very useful for picking up previous work when launching a new editing session. In fact, it's useful even when you're in the middle of a session - as you open more buffers from LSP commands etc. it gets harder and harder to find the files you're actually editing, as the number of buffers opened grows.

I set the default key binding of this picker to <SPACE>g and moved the deubgging menu to <SPACE>G because I think it makes the most sense:

• we already have ]g and [g for navigating through the VCS changes; and
• <SPACE>f and <SPACE>g are close both in location and functionality.

I added the sha1 dependency as SHA1 performance matters a lot. This is the same crate gitoxide uses if we enable the fast-sha1 feature on the crate (we didn't). I could have enabled the feature and use the Sha1 from gitoxide instead of importing it myself. I didn't do it beacuse unfortuantely gitoxide uses a wrapper around Sha1 and the wrapper doesn't implement Write, making it impossible to utilize the std::io::copy method.

This PR is obviously in no way complete. I'm still submitting it first as I believe this feature has a lot of value even in its current form. Missing stuffs (at least):

• parallelization of working tree traversal and SHA1 calculation
• detection of renaming w/ minor edits (which git status is able to do)
• handling of CRLF transformation of huge files (see code comments)

With that said, the feature itself works fine. It's been tested on Ubuntu, Windows 11 (with autocrlf of course, not WSL), macOS. All working perfectly.

Screenshot on macOS with stock config:

[haikyuu]

This will definitely become my most used picker right away. Thanks for working on this @xJonathanLEI 🙏

[Byron]

I didn't do it beacuse unfortuantely gitoxide uses a wrapper around Sha1 and the wrapper doesn't implement Write, making it impossible to utilize the std::io::copy method.

Good news, what you were looking for actually exists :)

• parallelization of working tree traversal and SHA1 calculation
• detection of renaming w/ minor edits (which git status is able to do)
• handling of CRLF transformation of huge files (see code comments)

Even though nothing speaks against having a first version of these in this codebase, especially if you have the time and fun doing it, I truly think the right place for these capabilities is gitoxide itself. @pascalkuthe and I will be putting our heads together to find a good solution for this so that everybody is happy.

Please note that right now I am super busy with the cargo integration and thus can't jump on this myself, but I will support the efforts together with you to be part of getting the best possible solution both for helix-editor and for gitoxide :).

Exciting!

[xJonathanLEI]

First off thank you for taking your time looking at this! @Byron

Good news, what you were looking for actually exists :)

Thanks for thr pointer! I somehow missed this type when looking at that file. However, it looks like a write multiplxer: it writes to an internal writer and the hasher at the same time. I don't really have the second writer to write to and actually only want the Sha1 wrapper to implement Write (which is reasonable to do I assume given it's just a newtype wrapper). Should I submit the Write implementation patch to gitoxide and wait for a new release for this PR?

I truly think the right place for these capabilities is gitoxide itself.

I agree. I started out building this PR by looking for similar features in gitoxide but no luck. IMO having the majority (other than the UI part of course) of this PR implemented in gitoxide makes the most sense simply because of specialization - Helix gets incremental improvements over time for free.

That's why I stopped after having this (reasonably) working POC - leaving the hard part (perf & edge cases) to Git masters like yourself and @pascalkuthe. I do see Helix abandoning this implementation in favor of a native out-of-the-box solution from gitoxide in the future.

On the other hand, I do believe this would be a highly wanted feature itself. That's why I'm still submitting it depsite things not being perfect.

@pascalkuthe and I will be putting our heads together to find a good solution for this so that everybody is happy.

Amazing! We're in good hands!

[xJonathanLEI]

While using this on one my of other repos, I realized that it's wrong that I thought a Commit entry cannot appear in the HEAD tree - turns out it can when it's a submodule. As a result, the diff provider chokes whenever there's a submodule. I just pushed a patch that simply ignores all submodules for now.

What do maintainers think? Should I complete the support for change detection in submodules in this PR or do we leave it for future work?

I have a feeling that the UX here is kinda hard to get right. If we just extend the current changed file list to include the submodule files, things are actually pretty messy: files are listed together but you can't really commit them together - they belong to different repositories. We probably need some kind of grouping when submodules exist.

[xJonathanLEI]

Just realized there's a small bug: on Linux if a file is was committed as CRLF then it would be incorrectly detected as modified. This is actually not a new bug per ser. I followed the CRLF handling logic in #4995 by @pascalkuthe. Unsurprisingly, the file that is getting incorrectly flagged also has Git gutters on all lines.

I had a feeling that our CRLF handling was too naive and it turns out to be true. I guess we need to mimic how Git handles the option to get rid of this issue (along with the gutters).

[xJonathanLEI]

Update on the CRLF issue: after investigating more I realized it's not related to the CRLF handling logic itself. The project I encountered this issue on has a .gitattributes file:

# Default.
*           text eol=lf

# Treat patch files as binaries but let diff'ing them
# as normal text.
*.diff      binary diff
*.patch     binary diff
*.patch32   binary diff
*.patch64   binary diff
*.patch.*   binary diff

# Powershell scripts.
*.ps1       text eol=crlf

# Binaries.
*.gpg       binary
*.gz        binary
*.jpg       binary
*.png       binary
*.tar       binary
*.tar.*     binary

To resolve this we need to be able to respect this file instead of blindly following core.autocrlf. I already implemented binary file detection but still need to add support for this one.

Again I'm not sure if this should be left as future work. Input from maintainers would be nice. Thanks!

Edit: just read #4995 more carefully and turns out it's a known issue

[pascalkuthe]

I am pretty sure the diff gutter doesn't work for submodules either so I would be fine with leaving that as future work for. Especially because git status also doesn't report submodule changes I believe

Update on the CRLF issue: after investigating more I realized it's not related to the CRLF handling logic itself. The project I encountered this issue on has a .gitattributes file:

# Default.
*           text eol=lf

# Treat patch files as binaries but let diff'ing them
# as normal text.
*.diff      binary diff
*.patch     binary diff
*.patch32   binary diff
*.patch64   binary diff
*.patch.*   binary diff

# Powershell scripts.
*.ps1       text eol=crlf

# Binaries.
*.gpg       binary
*.gz        binary
*.jpg       binary
*.png       binary
*.tar       binary
*.tar.*     binary

To resolve this we need to be able to respect this file instead of blindly following core.autocrlf. I already implemented binary file detection but still need to add support for this one.

Again I'm not sure if this should be left as future work. Input from maintainers would be nice. Thanks!

Edit: just read #4995 more carefully and turns out it's a known issue

I believe attributes aren't fully supported yet in gitoxide. The diff gutter doesn't support then for this reason either so its fine if you ignore them to now

[xJonathanLEI]

PR conflicts with master now due to Cargo.lock. I just rebased and squashed.

[poliorcetics]

Resolves https://github.com/helix-editor/helix/issues/5362.

This PR doesn't resolve it, that's not the (only) picker I had in mind for the issue

[xJonathanLEI]

Rebased on master due to merge conflicts.

[xJonathanLEI]

Rebased on master due to merge conflicts.

[xJonathanLEI]

There are a few known issues on this PR base on my own usage:

• the diff provider fails when HEAD does not exist (e.g. right after git init)
• the picker would show the same file 3 times when resolving Git conflicts
• the diff provider fails when the process doesn't have permission to read certain folders/files

Technically I should fix these, but as confirmed by @pascalkuthe, this PR will be replaced with the upstream gitoxide implementation when it becomes available. So I'll simply keep the PR as is and only perform master rebase should conflicts arise to make it easier to those who want to daily drive with this PR to pick the branch.

[Byron]

Thanks for the summary, please let me share some thoughts.

the diff provider fails when HEAD does not exist (e.g. right after git init)

This would have to be adjusted here to work as expected. I presume not having git initialized would mark all present files as new (as there also is no .git/index then).

the diff provider fails when the process doesn't have permission to read certain folders/files

@pascalkuthe already made me aware of an issue on Windows where the repository couldn't be opened without admin rights, and with admin rights it could be opened but some tests failed probably indicating that the git repository didn't actually yield any information to provide a base for diffing. That should probably be fixed upstream but I wasn't able to reproduce it.

To my mind it should be possible to fix the remaining issues here (except the permission issue which needs repro to be fixed in gitoxide) and make the PR usable that way, without waiting for the upstream implementation. If the git status implementation here is correct enough, the upstream version of it should 'just' bring performance and correctness in all cases. Thus the work done here should mostly be reusable.

[xJonathanLEI]

Hi @Byron thanks for the input! But I was under the impression that the whole thing will be replaced by an upstream impl. aka I will just call something like repo.status() and get the whole list of diffs that my impl here works to dig out. The PR would then only be about the UI part. Did I misunderstand something? Input from @pascalkuthe would be nice!

[Byron]

Yes, this is my understanding as well. To me it felt the PR was blocked waiting for something that is potentially quite far away, and I thought that maybe a 'good enough' version could fill the gap meanwhile. Ultimately me chiming in might not have been the best idea as it caused confusion and I am sorry for that - @pascalkuthe has a plan and I don't mean to change it 😅.

[xJonathanLEI]

Lol no worries. I think @pascalkuthe's plan is to leave the PR open until gitoxide has it. In that case, I'll just leave the PR as is cuz it's good enough for me despite those issues (those are non-issues in most cases anyways). Should the plan changes and we want to merge this one soon, I'm also happy to polish it up :)

[pascalkuthe]

A little update here: An inital implementation of an index/worktree diff is about the merged upstream https://github.com/Byron/gitoxide/pull/805 (tomorrow if all goes to plan). This is still missing support for untracked files and filters. I will add those in followup PRs. Untracked files shouldn't be too hard and I will restrict myself to some basic git filters (like line endings) for now as implementing full git filter support would take quite a while. Once that is ready then this PR can start using the upstream functionality.

[xJonathanLEI]

Looks like the removal of file picker from #7264 has introduced some conflicts here.

[xJonathanLEI]

Studied that PR a bit and conflict resolved. Git diff plugin™ works again.

[poliorcetics]

Most (all?) or my comments earlier in the pr are still unaddressed.

Also, this still doesn't fully resolve #5362, please don't mark the issue as closed by this MR

[pascalkuthe]

Most (all?) or my comments earlier in the pr are still unaddressed.

Also, this still doesn't fully resolve #5362, please don't mark the issue as closed by this MR

It's waiting for the upstream implemention in gitoxide to finish. Implementing git status correctly is complex and out of scope for helix

[xJonathanLEI]

Look I'm not obligated to resolve your comments, and I don't have time nor care enough to. As already stated in the convo above, there is currently no plan to merge this PR and it's still maintained only for people (myself included) who daily drives with a "juiced up" version of Helix with pending PRs incorporated.

[Byron]

It's waiting for the upstream implemention in gitoxide to finish. Implementing git status correctly is complex and out of scope for helix

And I truly hope that by the end of the year git status-like features will be ready. Right now I am on track to finish the checkout of files (with submodule support), probably followed by something like git reset. When work related to changing the state of the worktree is done, I will focus on everything related to obtaining the state of the worktree in comparison to something else, e.g. git status. Interestingly, with all that done, the integration into cargo will finally be (mostly) done, and while this PR should have everything it will need. And I am very much looking forward to that day :)!

[pascalkuthe]

It's waiting for the upstream implemention in gitoxide to finish. Implementing git status correctly is complex and out of scope for helix

And I truly hope that by the end of the year git status-like features will be ready. Right now I am on track to finish the checkout of files (with submodule support), probably followed by something like git reset. When work related to changing the state of the worktree is done, I will focus on everything related to obtaining the state of the worktree in comparison to something else, e.g. git status. Interestingly, with all that done, the integration into cargo will finally be (mostly) done, and while this PR should have everything it will need. And I am very much looking forward to that day :)!

Thanks for all your hard work :) IIRC the PR I did a while ago contained almost everything necessary for this PR (index to worktree diff, although submodule support was missing and I probably missed some niche features). Only untracked file detection is missing but I got sidetracked and just have so much backlog it's hard to find the time to finish that right now 😅

[xJonathanLEI]

Hey it's one year anniversary of this PR! 🎉🎉

Just resolved gix breaking changes from gix-v0.55.0..gix-v0.57.1. For those (if any) that still daily drive with this branch, enjoy!

[Byron]

Just chiming in to mention the gix status related work over at gitoxide which might be relevant here when done.

[daedroza]

Would something like git blame current line can also be added for juicing up further?

[the-mikedavis]

That's https://github.com/helix-editor/helix/issues/3035, it's not related to this PR

[pascalkuthe]

@Byron I saw you made some great progress on the gutoxide front (with more work still underway). Is gitlxide Boe at a point where we could use it to implement a prototype of this feature in helix?

[Byron]

In this this PR over at gitoxide is a complete implementation of index-worktree changes including rename tracking. It lacks the HEAD - index diff still, but you seem to not be needing this here.

I plan to first integrate what's there into the higher-level gix crate which is probably what you would rather use here, and I will definitely be back here to let you know when it's available :). Definitely closing in… .

(Note the my reply is unrelated to git blame which was marked off-topic)