[arch-projects] [dbscripts] [PATCH v2 0/3] Misc touchup
From: Luke Shumaker
From: Luke Shumaker
From: Luke Shumaker
On 02/22/2018 09:15 PM, Luke Shumaker wrote:
- common.bash: Globbing happens on the RHS of a [[ = ]] test. This means that we must quote variables on the RHS that are to be taken verbatim. This is surprising, because we don't need to quote the LHS.
Unless we intend to do a general style cleanup, it is unnecessary to "fix" cases where variables with known static content aren't quoted. -- Eli Schwartz Bug Wrangler and Trusted User
From: Luke Shumaker
On 02/22/2018 09:15 PM, Luke Shumaker wrote:
From: Luke Shumaker
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>
..." + msg "usage: %s <repo-from> <repo-to> ..." "${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>
..." + msg "usage: %s <repo> <arch> ..." "${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##*/}
..." + msg "usage: %s ..." "${0##*/}" exit 1 fi
-- Eli Schwartz Bug Wrangler and Trusted User
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? -- Happy hacking, ~ Luke Shumaker
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.htm... -- Eli Schwartz Bug Wrangler and Trusted User
participants (2)
-
Eli Schwartz
-
Luke Shumaker