Conversation
|
This is somewhat quick-and-dirty solution. Perhaps, we we shouldn't run |
cd10de9 to
86ef851
Compare
There was a problem hiding this comment.
Overall the approach here looks totally sensible, thanks for taking the time to write a PR!
I do think it needs a test: perhaps the easiest thing would be to write a unit test that e.g. "package.json\0diff\0unspecified\0" parses to the value you expect, and use that function after getting the raw string from git check-attr.
Otherwise, I think we should support at least the binary attribute too, and this is good to go. Thanks again, and let me know if you need any help :)
src/git.rs
Outdated
|
|
||
| /// Corresponds to the diff attribute. See man gitattribute. | ||
| pub(crate) enum DiffAttribute { | ||
| Set, |
There was a problem hiding this comment.
Would you mind adding doc comments to these? Even a brief phrase copied from the man page would be a big help. I think Other is something you've added, and isn't defined in git?
src/git.rs
Outdated
| "set" => Self::Set, | ||
| "unset" => Self::Unset, | ||
| "unspecified" => Self::Unspecified, | ||
| _ => Self::Other, |
There was a problem hiding this comment.
I guess the other option would be to use TryFrom so you don't need this catch-all Other value.
There was a problem hiding this comment.
I suppose this is no longer relevant since I changed this variant to Other(String)
src/git.rs
Outdated
| } | ||
| } | ||
| Err(err) => { | ||
| warn!("failed to execute git: {err}"); |
There was a problem hiding this comment.
These should be debug logging I think, especially this one. You can run difft in a directory that isn't a git repository, or even on a machine that doesn't have git installed.
There was a problem hiding this comment.
Running git outside of git repository is already handled by output.status.success() check, which gets logged with debug level.
I erroneously claimed that git being unavailable also goes there, but yeah, it actually ends up as a warning here.
Changed it to debug.
src/git.rs
Outdated
| /// Runs `git check-attr diff` to get the diff attribute of the path. Returns | ||
| /// [`Option::None`] when either `git` is not available, file is not inside git | ||
| /// directory, or something else went wrong. | ||
| pub(crate) fn check_attr(path: &Path) -> Option<DiffAttribute> { |
There was a problem hiding this comment.
I think should either be check_diff_attr, or take an attribute name as an argument.
There was a problem hiding this comment.
I changed this to check_diff_attr. It also checks binary attribute under the hood, but I decided against complicating thing to the caller too much
src/main.rs
Outdated
| let (mut lhs_src, mut rhs_src) = match ( | ||
| guess_content(&lhs_bytes, lhs_path, binary_overrides), | ||
| guess_content(&rhs_bytes, rhs_path, binary_overrides), | ||
| check_attr(Path::new(display_path)), |
There was a problem hiding this comment.
This covers the case when someone writes *.ext -diff, but not *.ext binary, which is also mentioned in #466.
Apparently binary means -text -diff -merge according to https://git-scm.com/docs/gitattributes#_using_macro_attributes
I think we should support -text and binary too, and also treat them as forcing binary.
There was a problem hiding this comment.
Yeah, that's probably a good idea, since specifying both binary and an external diff driver with diff= is valid.
There was a problem hiding this comment.
To avoid calling git check-attr twice, and to hide this complexity in main, I decided to make binary essentially force -diff. Please tell if you're against this implicit behaviour
There was a problem hiding this comment.
Alternatively, we could return something like Option<(DiffAttribute, BinaryAttribute)>. That would be more explicit, but the patterm matching expression in main is already complicated as it is, and it will become even more ridiculous.
I don't really have a strong preference here. I can implement it either way, please tell me, how :)
86ef851 to
1fcd21d
Compare
246876e to
a120746
Compare
Runs git check-attr to get the files git attributes, and treats the file as binary if the attribute value is "unset", matching the built-in git diff. Fixes Wilfred#466 Idea-by: @anuramat (Wilfred#466 (comment))
a120746 to
982edb0
Compare
Runs
git check-attrto get the files git attributes, and treats the file as binary if the attribute value is"unset", matching the built-in git diff.Fixes #466
Idea-by: @anuramat (#466 (comment))