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

Git gutter shows everything changed with automatic end-of-line conversion (.gitattributes contains * text=auto) #8145

Closed
chtenb opened this issue Sep 2, 2023 · 9 comments · Fixed by #9503
Labels
A-vcs Area: Version control system interaction C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR
Milestone

Comments

@chtenb
Copy link
Contributor

chtenb commented Sep 2, 2023

Summary

Git has a feature to check out text files with native line endings, something that is often used for crossplatform development.
When viewing a file on windows which has been converted to crlf this way, the git gutter shows everything changed:

image

However, when setting :line-ending lf it doesn't anymore:

image

This is no workaround, because this actually changed the line endings of the file to lf, where crlf is desired.
Note that git diff works correctly, and does not report any changes.

Reproduction Steps

No response

Helix log

No response

Platform

Windows

Terminal Emulator

wezterm 20230828-220414-99c96139

Helix Version

helix 23.05 (19b73bc)

@chtenb chtenb added the C-bug Category: This is a bug label Sep 2, 2023
@pascalkuthe
Copy link
Member

helix currently only supports core.autocrlf, when the diff gutter was implemented gitoxide did not support git attributes. I think there was some movement on this but not sure if this is possible yet.

CC @Byron

@Byron
Copy link
Contributor

Byron commented Sep 2, 2023

Indeed, times have changed. Now gitoxide supports attributes fully and makes it easy to filter an object buffer just like git would. It comes pre-configured according to all the options and will do exactly what git does.

It works in both directions, and one can also convert the worktree representation back to what's stored in git.

Probably that should now be used instead of the manual implementation used in helix currently.

@pascalkuthe
Copy link
Member

Thanks, that's great news! The next release is right around the corner so a fix won't land before that but in the next cycle we should definitely swap. The API seems fairly straightforward. The only thing I am not sure how to deal with the Delayed variant.

@Byron
Copy link
Contributor

Byron commented Sep 2, 2023

When converting to worktree there is a can_delay parameter, which allows this delay feature of some filters to be deactivated. When done, the 'Delayed' variant will never happen. Note that the ToWorktreeOutcome struct also implements Read, so you wouldn't have to care about it if delays are forbidden.

Please note that gix-worktree doesn't even use Read though as it reduces throughput roughly by half. This is due to the (previously) inefficient implementation of std::io::copy which didn't specialise for if Read was a slice, I think. Anyway, best to match on it for best performance.

Talking about performance, on typical repository going through the filter pipeline is as good as free when no filter is actually applied, and even if this is the case, the built-in filters costs nearly nothing. So should fit right into helix :).

@pascalkuthe
Copy link
Member

Thanks! Forbidding delays seems reasonable here since we can't progress until the file filter finished

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR A-vcs Area: Version control system interaction and removed C-bug Category: This is a bug labels Sep 2, 2023
@pascalkuthe pascalkuthe added this to the next milestone Sep 2, 2023
@Byron
Copy link
Contributor

Byron commented Sep 2, 2023

Thanks! Forbidding delays seems reasonable here since we can't progress until the file filter finished

Yes, absolutely! It's not worth the complexity and I don't think that most filters will benefit enough from it to do that. gitoxide supports it 'just because', but I don't think helix should until there is a proven benefit. For context, git-lfs is a filter that supports delays, but files dealing with git-lfs are also typically not the ones you would want to edit, so even less reason to deal with delayed filtering.

@McDonnellJoseph
Copy link

Hey I noticed this is labeled as easy I would be happy to work on it if that's ok for you 😄 I love using Helix I would like to start contributing

@A-Walrus
Copy link
Contributor

I tried updating the gix dependency, but got this error:

error: package `time-core v0.1.2` cannot be built because it requires rustc 1.67.0 or newer, while the currently active rustc version is 1.65.0
Either upgrade to rustc 1.67.0 or newer, or use
cargo update -p [email protected] --precise ver
where `ver` is the latest version of `time-core` supporting rustc 1.65.0

seems a bit strange considering the MSRV of gix should be 1.65.0 https://github.com/Byron/gitoxide/blob/a12e4a88d5f5636cd694c72ce45a8b75aa754d28/clippy.toml#L1

@gabydd
Copy link
Member

gabydd commented Sep 26, 2023

See #7863

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 C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants