| Message ID | 20180223021541.15416-4-lukeshu@lukeshu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Misc touchup | expand |
On 02/22/2018 09:15 PM, Luke Shumaker wrote: > From: Luke Shumaker <lukeshu@parabola.nu> > > These are things that were (IMO) missed in 5afac1e. I found them using: > > git 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 than > the usual "slap %s in there, and move the ${...} to the right". > > v2: > - arch_repo_add, arch_repo_remove: Use Bash 4.4 parameter > transformation to avoid having to introduce temporary variables. > --- a/db-functions > +++ b/db-functions > @@ -450,7 +450,7 @@ arch_repo_add() { > # package files might be relative to repo dir > pushd "${FTP_BASE}/${repo}/os/${arch}" >/dev/null > /usr/bin/repo-add -q "${repo}${DBEXT}" ${pkgs[@]} \ > - || error "repo-add ${repo}${DBEXT} ${pkgs[@]}" > + || error 'repo-add %q %s' "${repo}${DBEXT}" "${pkgs[*]@Q}" > popd >/dev/null > set_repo_permission "${repo}" "${arch}" > > @@ -468,7 +468,7 @@ arch_repo_remove() { > return 1 > fi > /usr/bin/repo-remove -q "${dbfile}" ${pkgs[@]} \ > - || error "repo-remove ${dbfile} ${pkgs[@]}" > + || error 'repo-remove %q %s' "$dbfile" "${pkgs[*]@Q}" > set_repo_permission "${repo}" "${arch}" I think for consistency we should use the same style which means using "${dbfile@Q}" Currently we'll get the dbfile backslash-escaped, followed by the package names unequivocally quoted on the same line of error output. It makes more sense to ensure that they are all quoted. While in theory one could bikeshed over which is the better way to consistently print them, there is only one option for us since we're trying to interject all the ${pkgs[*]} as one argument... > > 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 > >
On Mon, 26 Feb 2018 00:46:48 -0500, Eli Schwartz wrote: > > +++ b/db-functions > > @@ -450,7 +450,7 @@ arch_repo_add() { > > # package files might be relative to repo dir > > pushd "${FTP_BASE}/${repo}/os/${arch}" >/dev/null > > /usr/bin/repo-add -q "${repo}${DBEXT}" ${pkgs[@]} \ > > - || error "repo-add ${repo}${DBEXT} ${pkgs[@]}" > > + || error 'repo-add %q %s' "${repo}${DBEXT}" "${pkgs[*]@Q}" > > popd >/dev/null > > set_repo_permission "${repo}" "${arch}" > > > > @@ -468,7 +468,7 @@ arch_repo_remove() { > > return 1 > > fi > > /usr/bin/repo-remove -q "${dbfile}" ${pkgs[@]} \ > > - || error "repo-remove ${dbfile} ${pkgs[@]}" > > + || error 'repo-remove %q %s' "$dbfile" "${pkgs[*]@Q}" > > set_repo_permission "${repo}" "${arch}" > > I think for consistency we should use the same style which means using > "${dbfile@Q}" I was going for consistency with the repo-add version, which doesn't have a single dbfile variable to @Q. Would you have me introduce a dbfile variable in arch_repo_add?
On 02/26/2018 05:14 PM, Luke Shumaker wrote: > On Mon, 26 Feb 2018 00:46:48 -0500, > Eli Schwartz wrote: >>> +++ b/db-functions >>> @@ -450,7 +450,7 @@ arch_repo_add() { >>> # package files might be relative to repo dir >>> pushd "${FTP_BASE}/${repo}/os/${arch}" >/dev/null >>> /usr/bin/repo-add -q "${repo}${DBEXT}" ${pkgs[@]} \ >>> - || error "repo-add ${repo}${DBEXT} ${pkgs[@]}" >>> + || error 'repo-add %q %s' "${repo}${DBEXT}" "${pkgs[*]@Q}" >>> popd >/dev/null >>> set_repo_permission "${repo}" "${arch}" >>> >>> @@ -468,7 +468,7 @@ arch_repo_remove() { >>> return 1 >>> fi >>> /usr/bin/repo-remove -q "${dbfile}" ${pkgs[@]} \ >>> - || error "repo-remove ${dbfile} ${pkgs[@]}" >>> + || error 'repo-remove %q %s' "$dbfile" "${pkgs[*]@Q}" >>> set_repo_permission "${repo}" "${arch}" >> >> I think for consistency we should use the same style which means using >> "${dbfile@Q}" > > I was going for consistency with the repo-add version, which doesn't > have a single dbfile variable to @Q. Would you have me introduce a > dbfile variable in arch_repo_add? Well, we basically use it hardcoded three times, so why not. :p Just like the repo-add version, except with pushd "${dbfile%/*}" Actually, this decreases the difference between the two enough that we may want to just have one call the other... or do this: https://lists.archlinux.org/pipermail/arch-projects/2018-February/004832.html
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..fb45513 100644 --- a/db-functions +++ b/db-functions @@ -450,7 +450,7 @@ arch_repo_add() { # package files might be relative to repo dir pushd "${FTP_BASE}/${repo}/os/${arch}" >/dev/null /usr/bin/repo-add -q "${repo}${DBEXT}" ${pkgs[@]} \ - || error "repo-add ${repo}${DBEXT} ${pkgs[@]}" + || error 'repo-add %q %s' "${repo}${DBEXT}" "${pkgs[*]@Q}" popd >/dev/null set_repo_permission "${repo}" "${arch}" @@ -468,7 +468,7 @@ arch_repo_remove() { return 1 fi /usr/bin/repo-remove -q "${dbfile}" ${pkgs[@]} \ - || error "repo-remove ${dbfile} ${pkgs[@]}" + || error 'repo-remove %q %s' "$dbfile" "${pkgs[*]@Q}" 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 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 than the usual "slap %s in there, and move the ${...} to the right". v2: - arch_repo_add, arch_repo_remove: Use Bash 4.4 parameter transformation to avoid having to introduce temporary variables. --- cron-jobs/ftpdir-cleanup | 8 ++++---- cron-jobs/integrity-check | 2 +- cron-jobs/sourceballs | 8 ++++---- db-functions | 4 ++-- db-move | 4 ++-- db-remove | 2 +- db-repo-add | 2 +- db-repo-remove | 2 +- db-update | 2 +- testing2x | 2 +- 10 files changed, 18 insertions(+), 18 deletions(-)