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

feat: Diff source autodetection and caching #9951

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Mar 20, 2024

Following discussions in both #9892 and #8056, this PR adds diff source autodetection and caching.

Supported diff sources

  • None
  • Git (when the relevant feature is active)

Work left for another PR because it would clutter this one (it's possible not all of this will be implemented, those are ideas):

  • Adding "File" Diff source
  • Adding command to select diff source
  • Adding config for diff source detection order
  • Adding a statusline element to show the currently active diff source
  • Adding support for JJ as an example of another vcs based diff source (see feat: vcs: support Jujutsu as a diff-provider #12022)

I did not find any issue about diff source autodetection, if there is one I missed tell me or edit the PR desc to mark it as closing the issue if relevant :)

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. A-vcs Area: Version control system interaction labels Mar 20, 2024
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 5 times, most recently from 4a9baf9 to de43777 Compare March 27, 2024 12:47
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 6 times, most recently from 2f9deec to 475b847 Compare April 4, 2024 20:14
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 2 times, most recently from 6c08b04 to fc7c8c3 Compare April 9, 2024 20:32
@pascalkuthe
Copy link
Member

if I see this right this does "autodetection" per document and turns diff provider into a per-document thing right?

I want this to be a config option in the future that can be set for the document with :set buffer::diff_source git for the buffer and :set diff.provider git globally.

All this is part of the config refactor I am working on. I also want to add an :unset option so you could do :unset buffer diff.provider to reset to the global diff source (otherwise if you change the global setting the global setting doesn't change).

If no explicit value is provided autodetection would be used.

This would be particularly relevant to this PR because I don't want the file diff source to be enabled by default. It's a neat feature but should be opt-in. That would mean its precluded from the auto-detection.

I was only thinking of VCS with the autodection. I thought that there would usually only be one vcs per workspace (we use .git for root detection anyway) so we would just do the autodetection once at startup/when changing directories.

@poliorcetics
Copy link
Contributor Author

That's a lot of ideas in one comment :) !

This would be particularly relevant to this PR because I don't want the file diff source to be enabled by default. It's a neat feature but should be opt-in. That would mean its precluded from the auto-detection.

Sure I can do that easily, I don't have a strong opinion and copied the previous MR (1)

I thought that there would usually only be one vcs per workspace (we use .git for root detection anyway) so we would just do the autodetection once at startup/when changing directories.

I thought about that but for now I find it useless since the git repo is not kept open between diff calls. Ideally we would do it yes but I'm unsure it should be done in this MR just yet

There are also issues with updating this from external events and even just removing the git dir for example, but I think the proper solution will be to tell people to reload the diffs

I want this to be a config option in the future that can be set for the document with :set buffer::diff_source git for the buffer and :set diff.provider git globally.

that can be done in another MR without issues I feel


I'll do (1) today, for the rest, especially keeping track of workspaces will need more work

@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from fc7c8c3 to ef87854 Compare April 10, 2024 18:26
@pascalkuthe
Copy link
Member

I thought about that but for now I find it useless since the git repo is not kept open between diff calls. Ideally we would do it yes but I'm unsure it should be done in this MR just yet

I originally was thinking in the direction of just having a global config option that would be controlled by autodetection by default. I was hoping to go that direction since I wanted to avoid adding :diffsource (since it would be covered by :set buffer diff.source in the future) But on second thought I like having a typable command for this from a ui standpoint. We can alias that to :set buffer diff source in the future (I will probably do the same for other commands which set stuff per document right now which will become proper config options in the future).

There are also issues with updating this from external events and even just removing the git dir for example, but I think the proper solution will be to tell people to reload the diff

Yeah I think this should only update if people reload. Maybe something fancier once we have a filewatcher But that is still in the more distant future.

helix-vcs/src/lib.rs Outdated Show resolved Hide resolved
helix-vcs/src/lib.rs Outdated Show resolved Hide resolved
helix-view/src/document.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Apr 11, 2024
@pascalkuthe pascalkuthe added this to the next milestone Apr 14, 2024
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 2 times, most recently from 9aeead5 to de1c13b Compare April 28, 2024 20:51
@poliorcetics poliorcetics changed the title feat: Diff source autodetection feat: Diff source autodetection and caching Apr 28, 2024
@poliorcetics
Copy link
Contributor Author

I changed the approach completely:

  • Removed the "File" diff source, I'll do it in a subsequent PR
  • Removed the "diffsource" command since it's now useless
  • Added diff provider caching, to avoid recomputing a git repo on each diff-related data gathering

@poliorcetics
Copy link
Contributor Author

I'll add the "File" diff source and the "diffsource" command in a PR on top when this one is merged since it's an orthogonal issue and would make reviewing this harder

@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from 9a2adf6 to 06c861b Compare May 24, 2024 18:36
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 2 times, most recently from ece35b6 to af70135 Compare June 2, 2024 21:26
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 3 times, most recently from b6c8095 to 933f8fe Compare July 16, 2024 19:12
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 4 times, most recently from c5b7c81 to 0d4c665 Compare July 28, 2024 14:36
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 3 times, most recently from 43c9c5f to a8515c3 Compare August 7, 2024 19:43
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from a8515c3 to 246ad3b Compare August 10, 2024 14:12
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from 246ad3b to 4648abf Compare August 30, 2024 22:27
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from 4648abf to 4554e0e Compare September 8, 2024 14:31
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 3 times, most recently from 38b9700 to 68b7b1e Compare October 22, 2024 20:17
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 2 times, most recently from a63236d to 6892770 Compare November 2, 2024 15:35
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 2 times, most recently from f10169c to f34ba78 Compare November 11, 2024 11:38
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from f34ba78 to ec7b62e Compare November 14, 2024 20:46
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch 2 times, most recently from 52c4a28 to d4f31f6 Compare November 27, 2024 19:09
@poliorcetics poliorcetics force-pushed the ab/autodetect-diff-source branch from d4f31f6 to fd604c2 Compare December 7, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-vcs Area: Version control system interaction S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants