fetchpatch: quote excludes #43538
fetchpatch: quote excludes #43538
Conversation
I used a |
…h not be non-empty. This commit was originally introduced as part of NixOS#41420 and then reverted with the rest of that PR. However there was no reason to revert his particular commit.
Excludes and includes are implemented by passing the parameters to the respective flags of `filterdiff`. Those were passed unescaped until now. Since those flags expect patterns (similar to shell globs), something like `/some/path/*` might be used to exclude or include all files in some path. Without escaping the shell would expand the `*`, leading to unexpected behaviour.
|
@dotlambda Oh I only grepped through master. But the patch fetched in that commit actually ends up to be empty. That has happened to me more often than I'd like and I recognize the hash by now 😁 So that shows that we should probably apply this to 18.03 too. @volth Good point. Changed. @Ericson2314 I think it makes sense to offer both, so I included that. |
|
Perhaps we might additionally block empty result of |
Since this is probably never the desired case and has led to actual issues, see the comments at: NixOS@af1313e This might also happen when pulling a patch from GitHub or a similar web interface without explicitly selecting the "raw" format.
|
I have added another commit that checks for empty files. It is a bit verbose because I check at every step, but I think the improved error messages are worth it. |
|
While we're at it: I think it would be neat to add an option to reverse a patch. That can already be done with |
|
The ability to revert a patch sounds natural, though it seems rarely useful in nixpkgs to me. |
|
It would be useful if a bug was introduced by some commit and the commit isn't reversed yet upstream. I've needed that at least once for sage and ended up downloading & reversing the patch. But its not all that common. |
|
Since its usually easier to get feedback about something after implementing than it is to get feedback for ideas, I went ahead and implemented it. |
|
Borg reported this changes the |
That's because @timokau immediately made use of the new revert feature: https://github.com/NixOS/nixpkgs/pull/43538/files#diff-5bd819821ddc9ac4a654e7ea45752448. |
(cherry picked from commit fddd90e) This seems safe enough. It solves a bug in a conservative way; it also adds features, possibly easing cherry-picks of fixes from master.
vcunat
added
the
8.has: port to stable
The excludes argument of fetchpatch is based on the
-xflag offilterdiff. That flag takes a "pattern" as an argument, which is similar
to a bash glob. For example 'some/path/*' would be acceptable and
exclude all files in some/path.
Currently those excludes are passed without quoting, leading to
unexpected things (the shell expanding the glob).
This shouldn't break anything, since nobody actually uses anything but plain paths in
excludesyet. It shouldn't cause any rebuilds either, since the result of fetchpatch doesn't change and the hashes are fixed.@vcunat @globin
There used to also be a
includesthat had the same error, but that patch (by @Ericson2314) was shortly after reverted (by mistake? the commit message doesn't indicate why).Motivation for this change
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)