| Message ID | 20180222204331.10802-4-lukeshu@lukeshu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Misc touchup | expand |
On 02/22/2018 03:43 PM, Luke Shumaker wrote: > From: Luke Shumaker <lukeshu@parabola.nu> > > 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.
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.
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!
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?
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.
diff --git a/cron-jobs/ftpdir-cleanup b/cron-jobs/ftpdir-cleanup index ff65d46..e24e614 100755 --- a/cron-jobs/ftpdir-cleanup +++ b/cron-jobs/ftpdir-cleanup @@ -50,7 +50,7 @@ for repo in ${PKGREPOS[@]}; do if (( ${#missing_pkgs[@]} >= 1 )); then error "Missing packages in [%s] (%s)..." "$repo" "$arch" for missing_pkg in ${missing_pkgs[@]}; do - msg2 "${missing_pkg}" + msg2 '%s' "${missing_pkg}" done fi @@ -58,7 +58,7 @@ for repo in ${PKGREPOS[@]}; do if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old packages from [%s] (%s)..." "$repo" "$arch" for old_pkg in ${old_pkgs[@]}; do - msg2 "${old_pkg}" + msg2 '%s' "${old_pkg}" clean_pkg "${FTP_BASE}/${repo}/os/${arch}/${old_pkg}" done fi @@ -76,7 +76,7 @@ old_pkgs=($(comm -23 "${WORKDIR}/pool" "${WORKDIR}/db")) if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old packages from package pool..." for old_pkg in ${old_pkgs[@]}; do - msg2 "${old_pkg}" + msg2 '%s' "${old_pkg}" clean_pkg "$FTP_BASE/${PKGPOOL}/${old_pkg}" done fi @@ -91,7 +91,7 @@ done if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do - msg2 "${old_pkg}" + msg2 '%s' "${old_pkg}" if ! ${CLEANUP_DRYRUN}; then rm -f "${CLEANUP_DESTDIR}/${old_pkg}" rm -f "${CLEANUP_DESTDIR}/${old_pkg}.sig" diff --git a/cron-jobs/integrity-check b/cron-jobs/integrity-check index f1e75ab..47cf856 100755 --- a/cron-jobs/integrity-check +++ b/cron-jobs/integrity-check @@ -8,7 +8,7 @@ dirname="$(dirname $0)" script_lock if (( $# != 1 )); then - die "usage: ${0##*/} <mailto>" + die "usage: %s <mailto>" "${0##*/}" fi mailto=$1 diff --git a/cron-jobs/sourceballs b/cron-jobs/sourceballs index 8f089b3..7370594 100755 --- a/cron-jobs/sourceballs +++ b/cron-jobs/sourceballs @@ -109,13 +109,13 @@ for repo in ${PKGREPOS[@]}; do if [ ${#newpkgs[@]} -ge 1 ]; then msg "Adding source packages for [%s]..." "$repo" for new_pkg in ${newpkgs[@]}; do - msg2 "${new_pkg}" + msg2 '%s' "${new_pkg}" done fi if [ ${#failedpkgs[@]} -ge 1 ]; then msg "Failed to create source packages for [%s]..." "$repo" for failed_pkg in ${failedpkgs[@]}; do - msg2 "${failed_pkg}" + msg2 '%s' "${failed_pkg}" done fi done @@ -129,7 +129,7 @@ if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old source packages..." ${SOURCE_CLEANUP_DRYRUN} && warning 'dry run mode is active' for old_pkg in ${old_pkgs[@]}; do - msg2 "${old_pkg}" + msg2 '%s' "${old_pkg}" if ! ${SOURCE_CLEANUP_DRYRUN}; then mv_acl "$FTP_BASE/${SRCPOOL}/${old_pkg}" "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}" touch "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}" @@ -148,7 +148,7 @@ done if (( ${#old_pkgs[@]} >= 1 )); then msg "Removing old source packages from the cleanup directory..." for old_pkg in ${old_pkgs[@]}; do - msg2 "${old_pkg}" + msg2 '%s' "${old_pkg}" ${SOURCE_CLEANUP_DRYRUN} || rm -f "${SOURCE_CLEANUP_DESTDIR}/${old_pkg}" done fi diff --git a/db-functions b/db-functions 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}" REPO_MODIFIED=1 diff --git a/db-move b/db-move index fb7ebac..806ad9a 100755 --- a/db-move +++ b/db-move @@ -4,7 +4,7 @@ . "$(dirname $0)/db-functions" if (( $# < 3 )); then - msg "usage: ${0##*/} <repo-from> <repo-to> <pkgname|pkgbase> ..." + msg "usage: %s <repo-from> <repo-to> <pkgname|pkgbase> ..." "${0##*/}" exit 1 fi @@ -74,7 +74,7 @@ for pkgbase in ${args[@]:2}; do else tarches=("${pkgarch}") fi - msg2 "${pkgbase} ($(echo ${tarches[@]}))" + msg2 "%s (%s)" "$pkgbase" "${tarches[*]}" pkgnames=($(. "${svnrepo_from}/PKGBUILD"; echo ${pkgname[@]})) pkgver=$(. "${svnrepo_from}/PKGBUILD"; get_full_version) diff --git a/db-remove b/db-remove index 70502bc..e7aed31 100755 --- a/db-remove +++ b/db-remove @@ -4,7 +4,7 @@ . "$(dirname $0)/db-functions" if (( $# < 3 )); then - msg "usage: ${0##*/} <repo> <arch> <pkgname|pkgbase> ..." + msg "usage: %s <repo> <arch> <pkgname|pkgbase> ..." "${0##*/}" exit 1 fi diff --git a/db-repo-add b/db-repo-add index d7be302..4ea758f 100755 --- a/db-repo-add +++ b/db-repo-add @@ -4,7 +4,7 @@ . "$(dirname $0)/db-functions" if (( $# < 3 )); then - msg "usage: ${0##*/} <repo> <arch> <pkgfile> ..." + msg "usage: %s <repo> <arch> <pkgfile> ..." "${0##*/}" exit 1 fi diff --git a/db-repo-remove b/db-repo-remove index 32d167e..2f24edd 100755 --- a/db-repo-remove +++ b/db-repo-remove @@ -4,7 +4,7 @@ . "$(dirname $0)/db-functions" if (( $# < 3 )); then - msg "usage: ${0##*/} <repo> <arch> <pkgname> ..." + msg "usage: %s <repo> <arch> <pkgname> ..." "${0##*/}" exit 1 fi diff --git a/db-update b/db-update index 4e17184..5077389 100755 --- a/db-update +++ b/db-update @@ -78,7 +78,7 @@ for repo in ${repos[@]}; do arch_pkgs=($(getpkgfiles "${STAGING}/${repo}/"*-${pkgarch}${PKGEXTS} 2>/dev/null)) for pkg in ${arch_pkgs[@]} ${any_pkgs[@]}; do pkgfile="${pkg##*/}" - msg2 "${pkgfile} (${pkgarch})" + msg2 '%s (%s)' "$pkgfile" "$pkgarch" # any packages might have been moved by the previous run if [[ -f ${pkg} ]]; then mv "${pkg}" "$FTP_BASE/${PKGPOOL}" diff --git a/testing2x b/testing2x index f0d77cb..d68e405 100755 --- a/testing2x +++ b/testing2x @@ -4,7 +4,7 @@ . "$(dirname $0)/db-functions" if (( $# < 1 )); then - msg "usage: ${0##*/} <pkgname|pkgbase> ..." + msg "usage: %s <pkgname|pkgbase> ..." "${0##*/}" exit 1 fi
From: Luke Shumaker <lukeshu@parabola.nu> 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) "[^"]*\$' 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. --- cron-jobs/ftpdir-cleanup | 8 ++++---- cron-jobs/integrity-check | 2 +- cron-jobs/sourceballs | 8 ++++---- db-functions | 8 ++++++-- db-move | 4 ++-- db-remove | 2 +- db-repo-add | 2 +- db-repo-remove | 2 +- db-update | 2 +- testing2x | 2 +- 10 files changed, 22 insertions(+), 18 deletions(-)