Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters

2018-02-22 Thread Eli Schwartz via arch-projects 
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

2018-02-22 Thread Luke Shumaker 
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

2018-02-22 Thread Eli Schwartz via arch-projects 
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

2018-02-22 Thread Luke Shumaker 
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

2018-02-22 Thread Eli Schwartz via arch-projects 
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