lib.cli.escapeShellArg{,s}: Only escape when necessary#333744
lib.cli.escapeShellArg{,s}: Only escape when necessary#333744Gabriella439 merged 6 commits intoNixOS:stagingfrom
Conversation
These utilities are like `escapeShellArg{,s}` except that they will
leave the string undisturbed if it doesn't need to be quoted (because
it doesn't have any special characters). This can help generate
nicer-looking command lines.
ofborg
bot
added
10.rebuild-darwin: 0
|
Oh this is nice! I don't think we need a separate function for this actually. While the existing ones have
And even if we assume |
|
Also a minor nit for the commit message: This function is not in |
|
I'll get to your feedback soon. Also, just to clarify: are you saying it's okay to make this the new behavior of |
Yes I think that would be okay :) |
ofborg
bot
added
10.rebuild-darwin-stdenv
|
This would need to target |
Gabriella439
changed the title
escapeBashArg{,s}
|
Alright, I think this is ready for re-review (and rebased against |
emilazy
left a comment
emilazy
left a comment
There was a problem hiding this comment.
LGTM. Disadvantaging non‐ASCII alphabetical characters is unfortunate, but probably better than trying to correctly handle Unicode in a language that doesn’t really support it. This seems like it should be a big improvement for standard CLI cases. Hard to say if there’ll be any Hyrum’s law type breakage, but I think it’s worth a try at least.
I’ll let @infinisil do another round of reviews, but could you squash the commits together before merge? Right now this PR would break git bisect.
|
@emilazy: I typically squash merge (and clean up the commit message in the process of doing so) |
This needs to be a single‐quoted string; #333744 broke this incorrect use. Hyrum’s law strikes again!
|
This also caused a regression in the dokuwiki module. |
|
|
Fixes tests that are affected by NixOS/nixpkgs#333744.
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
|
Another Hyrum’s law issue: nix-darwin/nix-darwin#1068. (I don’t think it was necessarily wrong to do the PR this way or anything; just feel it is interesting/useful have a reference for the kinds of ways semantics‐preserving changes like this can result in downstream users breaking.) |
PHP strings don't obey shell quoting rules. See #333744.
|
Yeah, this is why my original change didn't make this behavior the default. |
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Changes to escapeShellArg introduced in NixOS/nixpkgs#333744 made different versions of nixpkgs behave differently. If current nix-darwin is used with nixpkgs before that change, labels end up having labels quoted twice (see nix-darwin#1085), but without changes from nix-darwin#1055, with new nixpkgs, labels end up not quoted at all, and ShellCheck ends up complaining that commas might have been used as array item separator (see https://www.shellcheck.net/wiki/SC2054). Use the old version of escapeShellArg to always escape the list of labels and make nix-darwin work with both old and new versions of nixpkgs. Fixes nix-darwin#1085
Changes to escapeShellArg introduced in NixOS/nixpkgs#333744 made different versions of nixpkgs behave differently. If current nix-darwin is used with nixpkgs before that change, labels end up having labels quoted twice (see nix-darwin#1085), but without changes from nix-darwin#1055, with new nixpkgs, labels end up not quoted at all, and ShellCheck ends up complaining that commas might have been used as array item separator (see https://www.shellcheck.net/wiki/SC2054). Use the old version of escapeShellArg to always escape the list of labels and make nix-darwin work with both old and new versions of nixpkgs. Fixes nix-darwin#1085
Changes to escapeShellArg introduced in NixOS/nixpkgs#333744 made different versions of nixpkgs behave differently. If current nix-darwin is used with nixpkgs before that change, labels end up having labels quoted twice (see nix-darwin#1085), but without changes from nix-darwin#1055, with new nixpkgs, labels end up not quoted at all, and ShellCheck ends up complaining that commas might have been used as array item separator (see https://www.shellcheck.net/wiki/SC2054). Use the old version of escapeShellArg to always escape the list of labels and make nix-darwin work with both old and new versions of nixpkgs. Fixes nix-darwin#1085
Regression introduced in 94bc0f5, because I developed against unstable, which has NixOS/nixpkgs#333744 merged, while 24.05 doesn't. Partly shellchecks mistake, see koalaman/shellcheck#2891 but whatever, we can't do much more about it than ignore the warning. Fixes #868
Regression introduced in 94bc0f5, because I developed against unstable, which has NixOS/nixpkgs#333744 merged, while 24.05 doesn't. Partly shellchecks mistake, see koalaman/shellcheck#2891 but whatever, we can't do much more about it than ignore the warning. Fixes #868
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
Yup: #367288 |
|
Sorry for not pushing back harder on repurposing the name here. I think it’s pointless to revert at this point now that it’s in a stable release though (but it’s a useful lesson for next time). |
Fixes tests that are affected by <NixOS/nixpkgs#333744>.
6 participants
These utilities will now leave the string undisturbed if it doesn't need to be quoted (because it doesn't have any special characters). This can help generate nicer-looking command lines.
Description of changes
Things done
Add a 👍 reaction to pull requests you find important.