On 02/22/2018 06:54 PM, Luke Shumaker wrote:
I do see what you're doing, I'm just not sure why. Is the whole idea with this extra variable floating around, to avoid tokenizing "${pkgs[@]}" as separate messages? That's why "${pkgs[*]}" tokenizes the members of an array as one word by gluing the members together using the first IFS character (which is a space). You'll note I used this in testing2x.
As for using %q for filepaths that can theoretically contain spaces... good point I guess.
It's all about %q. (I did use ${ary[*]} somewhere else in the commit). The separate variable applies %q escaping to each package filename separately. Without it, if I did something like:
+ || error 'repo-remove %q %q' "$dbfile" "${pkgs[*]}"
Then it would also escape the spaces that separate them.
But, you're using error 'repo-remove %q %s' "$dbfile" "${pkgs_str% }" with a %s which works just as well as it ever did. And you might as well do that with "${pkgs[*]}" since that also works as well as it ever did. Maybe, you should update that to work properly %q then... Or maybe, you should skip the temporary variable and use error 'repo-remove %s %s' "${dbfile@Q}" "${pkgs[*]@Q}" This will result in the variables being passed into error, after being suitably '/path quoted/rather/than/escaped/' See the bash documentation on "Parameter transformation".
Anyway, correctly applying %q escaping probably isn't super-important. But, we don't really expect repo-add or repo-remove to fail; if something is wrong, any of the numerous checks leading up to actually calling them should have already caught that. If we trigger one of these error messages, something *weird* is going on, and I'd like the most precise error message possible.
No, I agree we might as well be careful here! -- Eli Schwartz Bug Wrangler and Trusted User