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 configuring diff gutter source #8056

Closed
wants to merge 4 commits into from

Conversation

doy
Copy link
Contributor

@doy doy commented Aug 25, 2023

following up from #7805

this implementation removes the diff provider cascading fallback behavior in favor of allowing the user to configure which diff provider to use (currently just vcs, file, or none). this should allow maintaining the current behavior in the normal case, but also allow using :set diff-source file to see a diff against the state of the file on disk.

this does not yet support per-buffer configuration of diff sources because helix doesn't really have a good mechanism for per-buffer configuration in general at the moment - it feels like something that would be easier to add once the more general feature exists.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thabks fir working on this! Please try to keep your PRs as minimal as possible that nkes them easier to review and merge. This contains multie changes at once.

Some if them (see my review comments) are entirely orthogonal and also not the direction I want to go.

helix-view/src/macros.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-vcs/src/lib.rs Outdated Show resolved Hide resolved
helix-vcs/src/git.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe added E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-vcs Area: Version control system interaction labels Aug 31, 2023
@doy
Copy link
Contributor Author

doy commented Aug 31, 2023

i think this addresses the comments you had so far, let me know if there's anything else!

Comment on lines +300 to +305
#[serde(default)]
/// What the diff gutter should diff against
pub diff_source: DiffSource,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be just editor.diff-source since the diff is also used for the reset-diff-change command and ]g and also for a side by side diff in the future so it doesn't just pertain to the gutter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually is just editor.diff-source, i guess the comment is a bit misleading. i'll clean up the docs etc once the feature itself is more finalized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry github collapsed the code so that it looked like it was a gutter config field and the comment reinforced that idea 😅

image

@doy
Copy link
Contributor Author

doy commented Sep 1, 2023

alright, how's this?

@pascalkuthe
Copy link
Member

I think this is getting pretty good now. I want the git diff provider to be configurable (so you can diff against any revision like upstream/master or v23.5.0 and even stage. You don' t necessarily have to implement that in this PR (you can if you want to) but I think we should think about how that will be handled in the config to avoid future breaking changes

@doy
Copy link
Contributor Author

doy commented Sep 1, 2023

i thiiiink the current config should be extensible? like right now we have

[editor]
diff-source = "git" # or "file" etc

but this would probably still allow

[editor.diff-source]
type = "git"
rev = "main"
staged = true

or whatever in the future? i think it'll be easier to do that in a followup pr, but it seems pretty doable to me.

@doy
Copy link
Contributor Author

doy commented Sep 1, 2023

(the test failures look like upstream repositories having problems)

pascalkuthe
pascalkuthe previously approved these changes Sep 1, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now 👍

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2023
@pascalkuthe pascalkuthe added this to the next milestone Sep 1, 2023
@poliorcetics
Copy link
Contributor

I strongly disagree with forcing people to have a repo-local config each time they want to change the diff provider.

I want to configure helix once and have that config be enough for 99.99% of my usage, not have every repo on which I use JJ/git/no VCS (file) have a different configuration file and have to remember to copy some cargo-culted file from somewhere else.

Not only is it repetitive for very little gain, it also pollutes repos with local files, which mean adding git worktrees will not work the same for example, because the ignored files won't be copied over. Plus, Helix doesn't respect the .config/ format for local configs, which also pollutes listings with "ls -la" for example

@poliorcetics
Copy link
Contributor

poliorcetics commented Mar 15, 2024

Some stats for example:

Currently on my personal machine, I have 14 repos using JJ, two using no VCS (playgrounds to try things out in Python and Rust) and 22 using git (I think I counted them all, I may have missed some in some deep folder tree)

By this PR, if I want to minimize configs, I have to write 16 different files for JJ/no VCS and set git at the default. That's not a good state.

In comparison, the fallback behavior means I can set "JJ, Git, File" once and just move on, for all my repos at once

@pascalkuthe
Copy link
Member

the assumption is that you very much will be working with a single vcs system 99% of the time (really always git) and if you are forced to use something niche like mecurial adding a config file isn't too bad.

The fact that you seem evenly split between two different vcs is incredibly niche.

If anything I would not allow this to be configured at all and instead just hardcode the detection with the option to explicitly overwrite (which would be the same as this PR. We weren't planning to support vcs other than git when this was created which would mean the autodetectin would always return git)

@poliorcetics
Copy link
Contributor

If autodetection is a thing then yes, this PR will work well. Is there an issue for it ?

@pascalkuthe
Copy link
Member

clsoign thi onein favor of #9951, thanks for working on this on!

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 E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants