Use gix pipeline filter instead of manual crlf implementation #9503
Conversation
the-mikedavis
added
S-waiting-on-review
helix-vcs/src/git.rs
Outdated
| // Get the actual data that git would make out of the git object. | ||
| // This will apply the user's git config or attributes like crlf conversions. | ||
| if let Some(work_dir) = repo.work_dir() { | ||
| let rela_path = file.strip_prefix(work_dir)?.to_string_lossy(); |
There was a problem hiding this comment.
Doing lossy conversion is incorrect. Gix provides some utilities for proper path -> byte conversion.
helix-vcs/src/git.rs
Outdated
| let rela_path = file.strip_prefix(work_dir)?.to_string_lossy(); | ||
| let (mut pipeline, _) = repo.filter_pipeline(None)?; | ||
| let mut worktree_outcome = | ||
| pipeline.convert_to_worktree(&data, rela_path.as_ref().into(), Delay::Forbid)?; |
There was a problem hiding this comment.
@Byron how does this handle encoding? Git stores everything as utf-8 internally IIRC and only converts back on checkout.
I guess that this function would do that? Could we instead checkout utf-8 somehow. (My memory is very cloudy here so I may be missremebering)
There was a problem hiding this comment.
That's not the case, but it's true that Git has an 'internal' form that it wants to store in the object database. This is merely a form that passed through the filter pipeline though, which on Unix is most often a no-op.
It's true though that this method takes care of transforming data which is supposed to be directly from the object database and turn it into what would be checked out, applying all necessary transformations.
| @@ -448,6 +448,18 @@ dependencies = [ | |||
| "winapi", | |||
| ] | |||
|
|
|||
| [[package]] | |||
| name = "filetime" | |||
There was a problem hiding this comment.
It comes from new feature flag gix = { features = ["attributes"], .. }, which is needed for https://docs.rs/gix/0.58.0/gix/struct.Repository.html#method.filter_pipeline 😢
There was a problem hiding this comment.
I reviewed how this dependency is used and it seems 'convenience' is the answer. No feature is used that wouldn't be covered by the doing what the crate does.
I added a task to the helix tracking issue to represent that possibility, and help would definitely be welcome.
There was a problem hiding this comment.
I took my time to study the PR and believe that't the way to do it! Good work :)!
Additionally I was thinking that using the worktree-version as base for a diff is also exactly would should be done given how the user opens and edits files in the worktree.
For posterity, let me mention that by now there is also the repo.diff_recource_cache() method which allows to perform diffs just like diff does which, you guessed it, may apply even more transformations 😁. Doing so would be wrong here, but if there ever is a 'show me the git diff' function in helix, that would be the way to do it (and that's particularly interesting if the user configured diff-filters to turn binary files into something diffable).
| if let Some(work_dir) = repo.work_dir() { | ||
| let rela_path = file.strip_prefix(work_dir)?; | ||
| let rela_path = gix::path::try_into_bstr(rela_path)?; | ||
| let (mut pipeline, _) = repo.filter_pipeline(None)?; |
There was a problem hiding this comment.
Just as a note: It's a bit sad that the filter pipeline can't be reused under the circumstances. if this should ever be an issue that can be achieved by dropping down to the internal plumbing types.
However, I wouldn't go there as this will seriously complicate the code - I just wanted you to know that you can to shave off some cycles.
helix-vcs/src/git.rs
Outdated
| let rela_path = file.strip_prefix(work_dir)?.to_string_lossy(); | ||
| let (mut pipeline, _) = repo.filter_pipeline(None)?; | ||
| let mut worktree_outcome = | ||
| pipeline.convert_to_worktree(&data, rela_path.as_ref().into(), Delay::Forbid)?; |
There was a problem hiding this comment.
That's not the case, but it's true that Git has an 'internal' form that it wants to store in the object database. This is merely a form that passed through the filter pipeline though, which on Unix is most often a no-op.
It's true though that this method takes care of transforming data which is supposed to be directly from the object database and turn it into what would be checked out, applying all necessary transformations.
There was a problem hiding this comment.
Thanks!
This fixes the diff gutter for me in a repo where I use git-crypt 🚀
Fixes #8145