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

[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