Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
On 02/22/2018 08:52 PM, Luke Shumaker wrote: > I guess I *should* have explained it a bit more; the escaping of the > package list happens when assigning pkgs_str: > > printf -v pkgs_str -- '%q ' "${pkgs[@]}" Hmm, true. But the version without the additional variable wins IMO. > Anyway, your simpler version LGTM. Shall I submit a v2 patchset? Yes please. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature
Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
On Thu, 22 Feb 2018 19:23:17 -0500, Eli Schwartz wrote: > > 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. I guess I *should* have explained it a bit more; the escaping of the package list happens when assigning pkgs_str: printf -v pkgs_str -- '%q ' "${pkgs[@]}" > 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". Oohh, a new Bash 4.4 feature that I missed. (It's been >1 year, I don't have an excuse). And, to be fair, I had written those ~4 lines of code (f338cb0 in Parabola's dbscripts) before Bash 4.4 existed! (I knew that I had written code to escape the package list before, and just grabbed those lines from our dbscripts.) @Q generally escapes things by wrapping them in single-quotes; %q generally escapes them by inserting backslashes. Anyway, your simpler version LGTM. Shall I submit a v2 patchset? -- Happy hacking, ~ Luke Shumaker
Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
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 signature.asc Description: OpenPGP digital signature
Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
On Thu, 22 Feb 2018 16:44:20 -0500, Eli Schwartz wrote: > > I went a little above-and-beyond for escaping strings for the error > > messages in db-functions' arch_repo_add and arch_repo_remove. The > > code should explain itself, but I wanted to point it out, as it's more > > complex than the "slap %s in there, and move the ${...} to the right" > > that is used everywhere else. > > > > [...] > > index 8b71cae..6d02c50 100644 > > --- a/db-functions > > +++ b/db-functions > > @@ -446,11 +446,13 @@ arch_repo_add() { > > local repo=$1 > > local arch=$2 > > local pkgs=(${@:3}) > > + local pkgs_str > > > > # package files might be relative to repo dir > > pushd "${FTP_BASE}/${repo}/os/${arch}" >/dev/null > > + printf -v pkgs_str -- '%q ' "${pkgs[@]}" > > /usr/bin/repo-add -q "${repo}${DBEXT}" ${pkgs[@]} \ > > - || error "repo-add ${repo}${DBEXT} ${pkgs[@]}" > > + || error 'repo-add %q %s' "${repo}${DBEXT}" "${pkgs_str% }" > > popd >/dev/null > > set_repo_permission "${repo}" "${arch}" > > > > @@ -462,13 +464,15 @@ arch_repo_remove() { > > local arch=$2 > > local pkgs=(${@:3}) > > local dbfile="${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" > > + local pkgs_str > > > > if [[ ! -f ${dbfile} ]]; then > > error "No database found at '%s'" "$dbfile" > > return 1 > > fi > > + printf -v pkgs_str -- '%q ' "${pkgs[@]}" > > /usr/bin/repo-remove -q "${dbfile}" ${pkgs[@]} \ > > - || error "repo-remove ${dbfile} ${pkgs[@]}" > > + || error 'repo-remove %q %s' "$dbfile" "${pkgs_str% }" > > set_repo_permission "${repo}" "${arch}" > > 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. 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. -- Happy hacking, ~ Luke Shumaker
Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
On 02/22/2018 03:43 PM, Luke Shumaker wrote: > From: Luke Shumaker > > These are things that were (IMO) missed in 5afac1e. I found them using: > > git ls-files|xargs grep -E '(plain|msg|msg2|warning|error|die) "[^"]*\$' Consider using git grep next time :p rather than piping through both xargs and the inferior GNU grep. > I went a little above-and-beyond for escaping strings for the error > messages in db-functions' arch_repo_add and arch_repo_remove. The > code should explain itself, but I wanted to point it out, as it's more > complex than the "slap %s in there, and move the ${...} to the right" > that is used everywhere else. > > [...] > index 8b71cae..6d02c50 100644 > --- a/db-functions > +++ b/db-functions > @@ -446,11 +446,13 @@ arch_repo_add() { > local repo=$1 > local arch=$2 > local pkgs=(${@:3}) > + local pkgs_str > > # package files might be relative to repo dir > pushd "${FTP_BASE}/${repo}/os/${arch}" >/dev/null > + printf -v pkgs_str -- '%q ' "${pkgs[@]}" > /usr/bin/repo-add -q "${repo}${DBEXT}" ${pkgs[@]} \ > - || error "repo-add ${repo}${DBEXT} ${pkgs[@]}" > + || error 'repo-add %q %s' "${repo}${DBEXT}" "${pkgs_str% }" > popd >/dev/null > set_repo_permission "${repo}" "${arch}" > > @@ -462,13 +464,15 @@ arch_repo_remove() { > local arch=$2 > local pkgs=(${@:3}) > local dbfile="${FTP_BASE}/${repo}/os/${arch}/${repo}${DBEXT}" > + local pkgs_str > > if [[ ! -f ${dbfile} ]]; then > error "No database found at '%s'" "$dbfile" > return 1 > fi > + printf -v pkgs_str -- '%q ' "${pkgs[@]}" > /usr/bin/repo-remove -q "${dbfile}" ${pkgs[@]} \ > - || error "repo-remove ${dbfile} ${pkgs[@]}" > + || error 'repo-remove %q %s' "$dbfile" "${pkgs_str% }" > set_repo_permission "${repo}" "${arch}" 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. -- Eli Schwartz Bug Wrangler and Trusted User signature.asc Description: OpenPGP digital signature