fix: properly parse pure renames in git diffs#4
Conversation
remimimimimi
left a comment
remimimimimi
left a comment
There was a problem hiding this comment.
Just couple of small comments, otherwise looks good to me!
src/patch/parse.rs
Outdated
| Option<(Cow<'a, [u8]>, Option<LineEnd>)>, | ||
| )> { | ||
| skip_header_preamble(parser)?; | ||
| let (git_original, git_modified) = skip_header_preamble(parser)?; |
There was a problem hiding this comment.
Maybe then it would make sense to rename skip_header_preamble to header_preamble?
| Patch { | ||
| original: Some( | ||
| Filename( | ||
| "tinygrad/frontend/__init__.py", | ||
| ), | ||
| ), | ||
| modified: Some( | ||
| Filename( | ||
| "tinygrad/frontend/__init__.py", | ||
| ), | ||
| ), | ||
| hunks: [], | ||
| }, | ||
| Patch { | ||
| original: Some( | ||
| Filename( | ||
| "tinygrad/frontend/onnx.py", | ||
| ), | ||
| ), | ||
| modified: Some( | ||
| Filename( | ||
| "tinygrad/nn/onnx.py", | ||
| ), | ||
| ), | ||
| hunks: [], | ||
| }, |
There was a problem hiding this comment.
Won't it cause problems if we will leave filenames without their usual a/b prefix?
There was a problem hiding this comment.
It won't in our code in rattler-build (we are stripping them later anyways).
I also made the code more robust because git diff --no-prefix can actually also produce files without that prefix. So now we are checking that both files start/end with a/ or b/).
Now I am realizing that this logic might not work great for /dev/null...
There was a problem hiding this comment.
Don't we have special handling for /dev/null in rattler-build? @wolfv
There was a problem hiding this comment.
We just discussed this on Discord. The reasoning for stripping a/ and b/ here is that we have more information during patch parsing (e.g. we can observe if the patch starts with diff --git ....
So stripping a/ and b/ is easier here.
Additionally, it is more necessary for these operations because they usually map from "/dev/null" -> "a/foobar". Now it's ambigous whether we need to strip or create a a/ directory.
2 participants
A pure rename has no hunks, and this made the parser error out before.
Now we treat it more carefully.
Should fix: prefix-dev/rattler-build#1960
AI Disclaimer:
Most changes made by Claude :)