fetchpatch: quote excludes

[timokau]

The excludes argument of fetchpatch is based on the -x flag of
filterdiff. 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 excludes yet. 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 includes that 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

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Fits CONTRIBUTING.md.

---

[dotlambda]

This shouldn't break anything, since nobody actually uses anything but plain paths in excludes yet.

I used a * just a few days ago: af1313e

[Ericson2314]

@volth Good call. @timokau It was part of #41420 which was revert. Feel free to revert the revert, but the newer version of that PR doesn't use it so it will be dead code.

[timokau]

@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.

[vcunat]

Seems good. Personally I'd use an assert instead of meta.broken for this.

[vcunat]

Perhaps we might additionally block empty result of fetchpatch somehow, either make the derivation fail or recognize the hash at evaluation time.

[timokau]

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.

[timokau]

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 ${args.postFetch or ""} right now though and could be considered feature creep. What do you think?

[vcunat]

The ability to revert a patch sounds natural, though it seems rarely useful in nixpkgs to me.

[timokau]

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.

[timokau]

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.

[vcunat]

Borg reported this changes the sage* builds, but I suppose @timokau knows about that.

[dotlambda]

Borg reported this changes the sage* builds

That's because @timokau immediately made use of the new revert feature: https://github.com/NixOS/nixpkgs/pull/43538/files#diff-5bd819821ddc9ac4a654e7ea45752448.