(Git) diff gutter implementation

[pascalkuthe]

Supercedes #1623.

I took this PR up as the original PR was tagged as waiting-for-author/inactive.

This PR is rebased on the current master to avoid conflicts.
Additionally, I have updated the PR with some improvements.
The design is now much more similar to the design suggested by @p-e-w.

The original author already did most of the work by implementing incremental diffing.
However, I have now streamlined the git interface.
The incremental diff only requires that git provides the head state of a file while the rest is handled by helix.
Therefore, I have simplified the git interface down to one function: get_file_head.
This interface is now provided by a trait, that is implemented for git but is trivial to implement for other diff sources as well and could be exposed to the plugin system in the future.

The original PR cached the git repositories in memory.
This has been removed because it did not handle nested repositories correctly (example: $HOME as git repo for dotfile management).
It's also not required anymore as the git repo is only accessed when a file is open instead of every keystroke.

Finally, I have improved the git implementation by switching to gitoxide (as suggested in the original PR).
During this switch, I found a few edge cases that were not handled correctly by the previous implementation:

• Non utf-8 encoded files were not supported (now uses document encoding)
• Directories/symlinks that were replaced by files would use the directory children/link path as the diff base (now the diff is not shown)

I have purposefully not added more features to this PR to keep it somewhat small and reviewable.
In the future, I would like to expand on this PR.
In particular, I would like to implement a mechanism to reload the diff base after a file is first opened.
This avoids having to call :reload helix after every commit, to continue receiving correct diffs.
However, this feature is more complex (requires watching .git) so I choose not to include it here.

Currently, all commits are separate to make review easier.
I would like to squash the commits before this is merged, as the implementation changed quite a bit.

Note that there is currently one test failing in helix-core but that seems completely unrelated as the PR does not touch anything in helix-core

[pascalkuthe]

It seems the pipeline is failing because gitoxide has MSVR of 1.59 but helix currently targets 1.57. Gitoxide has been targeting newer versions for quite a while so downgrading doesn't seem like an option. I am not sure what the policy on MSRV is for helix, perhaps a MSVR bump to 1.59 could be possible. Even Ubuntu has updated to 1.59 at this point so it seems a reasonable requirement

[the-mikedavis]

We set the MSRV a while back, we're probably ok to update now (though that should be done separately from this PR). I think we set it to 1.57 on account of Void linux.

Is gitoxide really necessary though? It adds a very significant number of transitive dependencies (285 crates to build where master has 179 and with git2 we have 189) and compile time. The transitive dependencies also seem to add new compile-time dependencies on cmake and make (libz-sys).

[pascalkuthe]

Whether gitoxide or git2-rs is used doesn't make too much difference for this PR.
I was able to convert my implementation to git2-rs fairly quickly

I was able to remove the zlib-ng native dependency by disabling the gitoxide default features (this costs some performance but is hardly relevant for our use-case), git2-rs can be compiled in vendored mode. This means no matter what implementation we use, no dependency on system libraries is introduced.
In that case the compile times are similar (clean debug build 27s git2-rs vs 27.5s gitoxide on my machine).

I think compile times are the main concern here, because both projects are currently used in cargo/will be used in cargo in the near future and are maintained by trusted members of the rust community so supply/maintenance concerns should not be a problem. Furthermore comparing number of dependencies between the project does not really make sense because git2-rs contains 350k LOC of C in a single crate, while gitoxide simply spreads its functionality across multiple crates.

I believe gitoxide is preferable because its development is much more active and its written in rust.
Contributing to libgit2 is more difficult than contributing to gitoxide, because contributing to such a large C project is very difficult (the usual problems with C) compared to rust.

I hope that git support is expanded upon in helix in the future.
If we run into issues with gi2 in during that process, we can not do much about it because even longstanding issues of much larger projects (like cargo) are undressed. By comparison contributing to gitoxide is much easier.

I personally believe that trading 0,5 seconds of compile time for a cleaner/easier to fix git implementation is worth but for this PR it doesn't make a difference so I could easily change the git implementation if that is preferred.

[pascalkuthe]

I have discovered that the current diffing implementation I reused from #1623 has major shortcomings. I am working on improving this but it will take a while

[poliorcetics]

Very much in support of gitoxide too, because 1) it would help @Byron know if there are any missing features that would be useful for helix and have more real-life testing 2) it's very easy to contribute to 3) less C to build means users will have much less unexpected problems because of unrelated-to-helix causes

[Byron]

Thanks for pinging me! It's exciting to see gitoxide in helix, the editor 'of the new kind' that I liked most thus far and want to use ❤️!

Just now I was thinking about adding a line-counter to ein tool hours -p (now 15% faster than git at 1/10th of the peak memory) which would probably be the moment where this plumbing-level implementation of diffing I did when switching crates-index-diff (powering docs.rs) to gitoxide would turn into an easy-to-use API for consumption by everyone without sacrificing performance. I wanted to move on and postpone once again as to me this is 'candy' even though I have a lot of cargo related work to do… but now I think it's worth to see how fast I can put a higher-level API around the plumbing code that would hopefully also work here one day.

@pascalkuthe Thanks for this PR and for running the numbers, I never did and was positively surprised the compile times are only marginally different. I also took a look at the code and understand gitoxide is currently used to pull the base-object bytes to compare to out of the object database, with the diffing happening on top of that.
I also hope that in future iterations it will be possible to re-use the Repository instance as discovering a git repository is costly in terms of IOps.
Additionally I recommend to set a ceiling-directory for the discovery to assure that files that aren't in a git-repo don't cause it to look up to the root of the filesystem, if that such a ceiling is known at all - maybe the CWD could work initially? Since only the ODB is required here, one should probably adjust the open-options to not load global git configuration, but rely only on the repository-local one to avoid waste. The code for that is a bit cumbersome, but starship already went through the trouble and the result could be copied from there. Also note how it uses the git:: namespace to access many of the types, which helps to see more meaningful type names.

That said, I think it's safe to say that if helix-editor chooses to use gitoxide, I'd be there for instant-support as far as my sleep-cycle permits :D, and would make sure you get the fastest and most efficient git implementation available.

[pascalkuthe]

In the previous PR diffing was implemented using the helix internal changeset after a suggestion by @archseer.
While using this PR in practice I noticed that this approach does not work well.
The helix internal diffs are meant to allow helix to efficiently apply edits to the text and not to produce a human readable diff.
It is possible to fix this up somewhat (as the original PR did) but the fixup in the original PR was pretty naive and did not produce good results.
For example removing a character produced a removed line symbol, adding it back did not change that, inserting new test after that character adds more removed line symbols.

I have invested quite a bit of effort into improving this and have fixed the most horrible cases.
However this still does not work well. The problem essentially boils down to the fact that a linediff is fundamentally different from a character diff and trying to construct one from the other doesn't really work.

Therefore I decided to abandon this approach and instead use a more traditional async diff approach that is used in other editors (I looked at nvim-gitsgins and micro mainly).
This implementation simply spawns an async task for each file that computes the diff on each file change.
I have spend quite a bit of time optimizing this to address to comment made in the original PR regarding duplicate allocations.
While this approach might not scale as-well to huge files as the other approach it is still plenty fast enough for any reasonable sized file. I have added a check that disables the diffing in case files get too huge to ensure helix is still able to deal large files.

The implementation (even with optimizations) is much simpler and more maintainable than the UI fixup code I had developed for the changset based approach while providing much better results. I did not notice any significant different between the async-diff and changset implementation for normal-sized files. Furthermore the performance could be improved further by optimizing the line iterator cessen/ropey#25 and potentially using a different diffing algorithm (histogram diffs should outperform patience and mayer for sourcecode: mitsuhiko/similar#9).

@Byron thanks for reaching out/the review/the support. I had my hands full with this and work today but I will take a look at the git implementation again tomorrow :)

[kirawi]

There are quite a few grammar mistakes in the comments. Should these be addressed in a follow-up PR so as to not clog up this one?

[pascalkuthe]

There are quite a few grammar mistakes in the comments. Should these be addressed in a follow-up PR so as to not clog up this one?

I am still working on incorporating the suggestions from @Byron, once that is done I will quickly go trough all my comments again with a spellchecker. I hope that catches most things. I also think that further grammer fixes might then be better bundeled into a follow up PRs.

[pascalkuthe]

Ropey has released a new prerelease to crates.io with the necessary fixes. I have opened a separate PR for bumping helix to use that version and rebase on that PR for now. That means that there are no more upstream issues blocking this PR.

[the-mikedavis]

just some small comment typos

[archseer]

Do we still need all the strategies? If not I'd rather just keep a single one to simplify the code

[pascalkuthe]

I think they are all still required, as they serve different orhogonal usecases:

• SyncWithTimeout: Is the standard strategy used in apply_impl this will perform the diff in parallel and block the rendering until it is completed or the timout is expired. If the timeout expires before the diff completes a new redraw will be requsted
• Sync: Same as SyncWithTimeout but without the timeout (so the rendering always waits for the diff to complete). This will be required for side by side diffing where it would look extremely jarring if the the virtual text updates asynchronously and used instead of SyncWithTimeout in apply_impl.
• Async: An update of the diff_base trigger by something externally (for example when something is staged/a commit is updated). This strategy is required because it will not be triggered during the rendering. Therefore it always needs to request a new redraw event.

Note that I found a slight shortcoming in the locking code that required some small refactors (explained below) and removed this enum in the process but the three cases remain

[pascalkuthe]

I tested how often a delay of 12 is actually reached.
It turns out that the even if the timeout is reached there is rarely noticable delay on my PC with treesitter enabled because for files that are large enough to slow down the diff, the treesittter parser is often slower then the diff anyway (in that case the async redraw request is automatically canceled) so I had to test with :lang text.

However on my machine the rendering then happened so fast that the render occurs before the DiffWorker was scheduled by tokio and aquired the lock. I have refactored the code a bit so that the lock is acquired synchronously to prevent that.

All changes were included in a separate commit so you don't need to re-review the existing commits (I only force pushed because I rebased on master to resolve the Cargo.lock conflict)). Ideally this branch should be squashed before it's merged.

[archseer]

Says it defaults to 100 but the code uses 200. We should set this to DIFF_DEBOUNCE_TIME_ASYNC maybe? Or double that.

I also think this shouldn't be configurable by end users because it's easy to misconfigure (same as idle-timeout which really shouldn't be 0 but it's set to 0 by a lot of users). If complaints come in we can always make it smarter or configurable at that point.

[pascalkuthe]

That makes a lot of sense. I was trying to follow the precedent of the idle-timeout.
I hard-coded this to 96ms (DIFF_DEBOUNCE_TIME_ASYNC)

[Mathspy]

Sorry for hijacking with something unrelated to this PR. I want to mention that the current documentation explicitly states “set to 0 for instant” so if 0 is a bad idea that section could be altered to explain why 0 ought to be avoided and what’d be recommended instead

[archseer]

Merged! 🎉 Thanks for all the hard work on this, including patching ropey and developing imara-diff!

[poliorcetics]

I concur, incredible work, thanks for this a lot !

[Proful]

Looking great. Thank you!

[PORTALSURFER]

this features does not work well on windows where line endings use crlf
#4989

[xJonathanLEI]

Maybe submit a separate issue instead?