[arch-projects] [dbscripts] [PATCH v2 3/3] Update messages to make fuller use of printf formatters
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.
---
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(-)
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##*/} "
+ die "usage: %s " "${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
p
[arch-projects] [dbscripts] [PATCH v2 2/3] test: Fixup glob matching
From: Luke Shumaker
- ftpdir-cleanup.bats: Glob expansion does not occur in [[ -f ]] tests.
The [[ ! -f .../${pkgname}-*${PKGEXT} ]] checks were checking that there
were no files containing a literal '*' for that part of their name.
Obviously, this isn't what was intended.
- sourceballs.bats: [ -r ] checks explode if the glob returns >1 file.
Avoid using them if the path being checked contains a glob.
- 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.
---
test/cases/ftpdir-cleanup.bats | 4 ++--
test/cases/sourceballs.bats| 4 ++--
test/lib/common.bash | 12 +++-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/test/cases/ftpdir-cleanup.bats b/test/cases/ftpdir-cleanup.bats
index fd485f3..8c713c6 100644
--- a/test/cases/ftpdir-cleanup.bats
+++ b/test/cases/ftpdir-cleanup.bats
@@ -13,8 +13,8 @@ __checkRepoRemovedPackage() {
local pkgname
for pkgname in $(__getPackageNamesFromPackageBase ${pkgbase}); do
- [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT} ]]
- [[ ! -f
${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT} ]]
+ ! __isGlobfile "${FTP_BASE}/${PKGPOOL}/${pkgname}"-*"${PKGEXT}"
+ ! __isGlobfile
"${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}"-*"${PKGEXT}"
done
}
diff --git a/test/cases/sourceballs.bats b/test/cases/sourceballs.bats
index a0a2999..df7ddd4 100644
--- a/test/cases/sourceballs.bats
+++ b/test/cases/sourceballs.bats
@@ -2,12 +2,12 @@ load ../lib/common
__checkSourcePackage() {
local pkgbase=$1
- [ -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ]
+ __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}"
}
__checkRemovedSourcePackage() {
local pkgbase=$1
- [ ! -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ]
+ ! __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}"
}
@test "create simple package sourceballs" {
diff --git a/test/lib/common.bash b/test/lib/common.bash
index 5411641..6e2b3df 100644
--- a/test/lib/common.bash
+++ b/test/lib/common.bash
@@ -14,6 +14,16 @@ __getCheckSum() {
echo "${result%% *}"
}
+# Proxy function to check if a file exists. Using [[ -f ... ]] directly is not
+# always wanted because we might want to expand bash globs first. This way we
+# can pass unquoted globs to __isGlobfile() and have them expanded as function
+# arguments before being checked.
+#
+# This is a copy of db-functions is_globfile
+__isGlobfile() {
+ [[ -f $1 ]]
+}
+
__buildPackage() {
local pkgdest=${1:-.}
local p
@@ -203,7 +213,7 @@ checkPackageDB() {
for repoarch in ${repoarches[@]}; do
# Only 'any' packages can be found in repos of
both arches
if [[ $pkgarch != any ]]; then
- if [[ $pkgarch != ${repoarch} ]]; then
+ if [[ $pkgarch != "$repoarch" ]]; then
continue
fi
fi
--
2.16.1
[arch-projects] [dbscripts] [PATCH v2 1/3] test: common.bash:__getCheckSum: Don't rely on IFS
From: Luke Shumaker
I managed to stumble across a bug in BATS where the run() function
screwed with the global IFS. The bug has been fixed in git, but isn't
in a release yet.
https://github.com/sstephenson/bats/issues/89
Anyway, this bug breaks __getCheckSum(). Fortunately, we have avoided
tripping it so far because luck has it that we never call
__getCheckSum() after run() in the same test.
So, there's nothing actually broken here, but it makes me nervous. So
go ahead and modify __getCheckSum to not rely on IFS.
And, while we're at it: declare the result variable and set it as
separate commands. Doing both in the same command masks the exit code
of the subshell expansion. We don't explicitly check the exit code,
but BATS runs the test suite with `set -e`, so splitting it does mean
that BATS will now detect errors from sha1sum. We don't really expect
that to happen, but if BATS will give us error checking on it for
free, why not?
v2:
- commit message: Mention spliting declare/set into separate commands.
---
test/lib/common.bash | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/test/lib/common.bash b/test/lib/common.bash
index 568a541..5411641 100644
--- a/test/lib/common.bash
+++ b/test/lib/common.bash
@@ -9,8 +9,9 @@ __updatePKGBUILD() {
}
__getCheckSum() {
- local result=($(sha1sum $1))
- echo ${result[0]}
+ local result
+ result="$(sha1sum "$1")"
+ echo "${result%% *}"
}
__buildPackage() {
--
2.16.1
[arch-projects] [dbscripts] [PATCH v2 0/3] Misc touchup
From: Luke Shumaker These are a few minor unrelated touch-ups that I noticed when doing the PKGEXT work. There's no real theme to them. v2: - Use Bash 4.4 feature to avoid introducing temporary variables in the printf commit - Mention splitting declaring and setting the result variable in the __getCheckSum commit message. Luke Shumaker (3): test: common.bash:__getCheckSum: Don't rely on IFS test: Fixup glob matching Update messages to make fuller use of printf formatters 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 +- test/cases/ftpdir-cleanup.bats | 4 ++-- test/cases/sourceballs.bats| 4 ++-- test/lib/common.bash | 17 ++--- testing2x | 2 +- 13 files changed, 36 insertions(+), 25 deletions(-) -- 2.16.1
Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
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 1/3] test: common.bash:__getCheckSum: Don't rely on IFS
On Thu, 22 Feb 2018 19:27:04 -0500,
Eli Schwartz wrote:
>
> [1 Re: [arch-projects] [dbscripts] [PATCH 1/3] test:
> common.bash:__getCheckSum: Don't rely on IFS ]
> [1.1 ]
> On 02/22/2018 06:43 PM, Luke Shumaker wrote:
> > On Thu, 22 Feb 2018 16:43:36 -0500,
> > Eli Schwartz wrote:
> >>> __getCheckSum() {
> >>> - local result=($(sha1sum $1))
> >>> - echo ${result[0]}
> >>> + local result
> >>> + result="$(sha1sum "$1")"
> >>> + echo "${result%% *}"
> >>
> >> Why are you moving over to declaring the variable and assigning it on
> >> different lines?
> >
> > Because shellcheck complains about it, so it's a habit I've gotten in
> > to :) Even in cases where it doesn't really make a difference.
> >
> > https://github.com/koalaman/shellcheck/wiki/SC2155
> >
> > However, BATS does run the test suite with `set -e`, so splitting it
> > does mean that BATS will now detect errors from sha1sum. We don't
> > really expect that to happen, but if BATS will give us error checking
> > on it for free, why not?
>
> Then the commit message should say so...
Honestly, I didn't even think about it. Like I said, I've just made
it a habit.
--
Happy hacking,
~ Luke Shumaker
Re: [arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
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 1/3] test: common.bash:__getCheckSum: Don't rely on IFS
On 02/22/2018 06:43 PM, Luke Shumaker wrote:
> On Thu, 22 Feb 2018 16:43:36 -0500,
> Eli Schwartz wrote:
>>> __getCheckSum() {
>>> - local result=($(sha1sum $1))
>>> - echo ${result[0]}
>>> + local result
>>> + result="$(sha1sum "$1")"
>>> + echo "${result%% *}"
>>
>> Why are you moving over to declaring the variable and assigning it on
>> different lines?
>
> Because shellcheck complains about it, so it's a habit I've gotten in
> to :) Even in cases where it doesn't really make a difference.
>
> https://github.com/koalaman/shellcheck/wiki/SC2155
>
> However, BATS does run the test suite with `set -e`, so splitting it
> does mean that BATS will now detect errors from sha1sum. We don't
> really expect that to happen, but if BATS will give us error checking
> on it for free, why not?
Then the commit message should say so...
--
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
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
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 1/3] test: common.bash:__getCheckSum: Don't rely on IFS
On Thu, 22 Feb 2018 16:43:36 -0500,
Eli Schwartz wrote:
> > __getCheckSum() {
> > - local result=($(sha1sum $1))
> > - echo ${result[0]}
> > + local result
> > + result="$(sha1sum "$1")"
> > + echo "${result%% *}"
>
> Why are you moving over to declaring the variable and assigning it on
> different lines?
Because shellcheck complains about it, so it's a habit I've gotten in
to :) Even in cases where it doesn't really make a difference.
https://github.com/koalaman/shellcheck/wiki/SC2155
However, BATS does run the test suite with `set -e`, so splitting it
does mean that BATS will now detect errors from sha1sum. We don't
really expect that to happen, but if BATS will give us error checking
on it for free, why not?
--
Happy hacking,
~ Luke Shumaker
Re: [arch-projects] [dbscripts] [PATCH 2/3] test: Fixup glob matching
On 02/22/2018 03:43 PM, Luke Shumaker wrote:
> From: Luke Shumaker
>
> - ftpdir-cleanup.bats: Glob expansion does not occur in [[ -f ]] tests.
>The [[ ! -f .../${pkgname}-*${PKGEXT} ]] checks were checking that there
>were no files containing a literal '*' for that part of their name.
>Obviously, this isn't what was intended.
Good catch, thanks!
> - sourceballs.bats: [ -r ] checks explode if the glob returns >1 file.
>Avoid using them if the path being checked contains a glob.
Arguably I'd like to upgrade the whole testsuite to use [[ ... ]]
Thanks.
> - 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.
No we don't, we only "need" to quote the ones that (can) contain globs.
In the general case, there is a school of thought that says you should
only quote what you explicitly need to quote. :p
Also not really surprising. This is a bash feature, you can do regex
comparisons too.
> ---
> test/cases/ftpdir-cleanup.bats | 4 ++--
> test/cases/sourceballs.bats| 4 ++--
> test/lib/common.bash | 12 +++-
> 3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/test/cases/ftpdir-cleanup.bats b/test/cases/ftpdir-cleanup.bats
> index fd485f3..8c713c6 100644
> --- a/test/cases/ftpdir-cleanup.bats
> +++ b/test/cases/ftpdir-cleanup.bats
> @@ -13,8 +13,8 @@ __checkRepoRemovedPackage() {
> local pkgname
>
> for pkgname in $(__getPackageNamesFromPackageBase ${pkgbase}); do
> - [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT} ]]
> - [[ ! -f
> ${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT} ]]
> + ! __isGlobfile "${FTP_BASE}/${PKGPOOL}/${pkgname}"-*"${PKGEXT}"
> + ! __isGlobfile
> "${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}"-*"${PKGEXT}"
> done
> }
>
> diff --git a/test/cases/sourceballs.bats b/test/cases/sourceballs.bats
> index a0a2999..df7ddd4 100644
> --- a/test/cases/sourceballs.bats
> +++ b/test/cases/sourceballs.bats
> @@ -2,12 +2,12 @@ load ../lib/common
>
> __checkSourcePackage() {
> local pkgbase=$1
> - [ -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ]
> + __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}"
> }
>
> __checkRemovedSourcePackage() {
> local pkgbase=$1
> - [ ! -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ]
> + ! __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}"
> }
>
> @test "create simple package sourceballs" {
> diff --git a/test/lib/common.bash b/test/lib/common.bash
> index 5411641..6e2b3df 100644
> --- a/test/lib/common.bash
> +++ b/test/lib/common.bash
> @@ -14,6 +14,16 @@ __getCheckSum() {
> echo "${result%% *}"
> }
>
> +# Proxy function to check if a file exists. Using [[ -f ... ]] directly is
> not
> +# always wanted because we might want to expand bash globs first. This way we
> +# can pass unquoted globs to __isGlobfile() and have them expanded as
> function
> +# arguments before being checked.
> +#
> +# This is a copy of db-functions is_globfile
> +__isGlobfile() {
> + [[ -f $1 ]]
> +}
> +
> __buildPackage() {
> local pkgdest=${1:-.}
> local p
> @@ -203,7 +213,7 @@ checkPackageDB() {
> for repoarch in ${repoarches[@]}; do
> # Only 'any' packages can be found in repos of
> both arches
> if [[ $pkgarch != any ]]; then
> - if [[ $pkgarch != ${repoarch} ]]; then
> + if [[ $pkgarch != "$repoarch" ]]; then
> continue
> fi
> fi
>
--
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
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
Re: [arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS
On 02/22/2018 03:43 PM, Luke Shumaker wrote:
> From: Luke Shumaker
>
> I managed to stumble across a bug in BATS where the run() function
> screwed with the global IFS. The bug has been fixed in git, but isn't
> in a release yet.
>
> https://github.com/sstephenson/bats/issues/89
>
> Anyway, this bug breaks __getCheckSum(). Fortunately, we have avoided
> tripping it so far because luck has it that we never call
> __getCheckSum() after run() in the same test.
>
> So, there's nothing actually broken here, but it makes me nervous. So
> go ahead and modify __getCheckSum to not rely on IFS.
> ---
> test/lib/common.bash | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/test/lib/common.bash b/test/lib/common.bash
> index 568a541..5411641 100644
> --- a/test/lib/common.bash
> +++ b/test/lib/common.bash
> @@ -9,8 +9,9 @@ __updatePKGBUILD() {
> }
>
> __getCheckSum() {
> - local result=($(sha1sum $1))
> - echo ${result[0]}
> + local result
> + result="$(sha1sum "$1")"
> + echo "${result%% *}"
Why are you moving over to declaring the variable and assigning it on
different lines?
--
Eli Schwartz
Bug Wrangler and Trusted User
signature.asc
Description: OpenPGP digital signature
[arch-projects] [dbscripts] [PATCH 1/3] test: common.bash:__getCheckSum: Don't rely on IFS
From: Luke Shumaker
I managed to stumble across a bug in BATS where the run() function
screwed with the global IFS. The bug has been fixed in git, but isn't
in a release yet.
https://github.com/sstephenson/bats/issues/89
Anyway, this bug breaks __getCheckSum(). Fortunately, we have avoided
tripping it so far because luck has it that we never call
__getCheckSum() after run() in the same test.
So, there's nothing actually broken here, but it makes me nervous. So
go ahead and modify __getCheckSum to not rely on IFS.
---
test/lib/common.bash | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/test/lib/common.bash b/test/lib/common.bash
index 568a541..5411641 100644
--- a/test/lib/common.bash
+++ b/test/lib/common.bash
@@ -9,8 +9,9 @@ __updatePKGBUILD() {
}
__getCheckSum() {
- local result=($(sha1sum $1))
- echo ${result[0]}
+ local result
+ result="$(sha1sum "$1")"
+ echo "${result%% *}"
}
__buildPackage() {
--
2.16.1
[arch-projects] [dbscripts] [PATCH 0/3] Misc touchup
From: Luke Shumaker These are a few minor unrelated touch-ups that I noticed when doing the PKGEXT work. There's no real theme to them. Luke Shumaker (3): test: common.bash:__getCheckSum: Don't rely on IFS test: Fixup glob matching Update messages to make fuller use of printf formatters 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 +- test/cases/ftpdir-cleanup.bats | 4 ++-- test/cases/sourceballs.bats| 4 ++-- test/lib/common.bash | 17 ++--- testing2x | 2 +- 13 files changed, 40 insertions(+), 25 deletions(-) -- Happy hacking, ~ Luke Shumaker
[arch-projects] [dbscripts] [PATCH 2/3] test: Fixup glob matching
From: Luke Shumaker
- ftpdir-cleanup.bats: Glob expansion does not occur in [[ -f ]] tests.
The [[ ! -f .../${pkgname}-*${PKGEXT} ]] checks were checking that there
were no files containing a literal '*' for that part of their name.
Obviously, this isn't what was intended.
- sourceballs.bats: [ -r ] checks explode if the glob returns >1 file.
Avoid using them if the path being checked contains a glob.
- 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.
---
test/cases/ftpdir-cleanup.bats | 4 ++--
test/cases/sourceballs.bats| 4 ++--
test/lib/common.bash | 12 +++-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/test/cases/ftpdir-cleanup.bats b/test/cases/ftpdir-cleanup.bats
index fd485f3..8c713c6 100644
--- a/test/cases/ftpdir-cleanup.bats
+++ b/test/cases/ftpdir-cleanup.bats
@@ -13,8 +13,8 @@ __checkRepoRemovedPackage() {
local pkgname
for pkgname in $(__getPackageNamesFromPackageBase ${pkgbase}); do
- [[ ! -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-*${PKGEXT} ]]
- [[ ! -f
${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}-*${PKGEXT} ]]
+ ! __isGlobfile "${FTP_BASE}/${PKGPOOL}/${pkgname}"-*"${PKGEXT}"
+ ! __isGlobfile
"${FTP_BASE}/${repo}/os/${repoarch}/${pkgname}"-*"${PKGEXT}"
done
}
diff --git a/test/cases/sourceballs.bats b/test/cases/sourceballs.bats
index a0a2999..df7ddd4 100644
--- a/test/cases/sourceballs.bats
+++ b/test/cases/sourceballs.bats
@@ -2,12 +2,12 @@ load ../lib/common
__checkSourcePackage() {
local pkgbase=$1
- [ -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ]
+ __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}"
}
__checkRemovedSourcePackage() {
local pkgbase=$1
- [ ! -r ${FTP_BASE}/${SRCPOOL}/${pkgbase}-*${SRCEXT} ]
+ ! __isGlobfile "${FTP_BASE}/${SRCPOOL}/${pkgbase}"-*"${SRCEXT}"
}
@test "create simple package sourceballs" {
diff --git a/test/lib/common.bash b/test/lib/common.bash
index 5411641..6e2b3df 100644
--- a/test/lib/common.bash
+++ b/test/lib/common.bash
@@ -14,6 +14,16 @@ __getCheckSum() {
echo "${result%% *}"
}
+# Proxy function to check if a file exists. Using [[ -f ... ]] directly is not
+# always wanted because we might want to expand bash globs first. This way we
+# can pass unquoted globs to __isGlobfile() and have them expanded as function
+# arguments before being checked.
+#
+# This is a copy of db-functions is_globfile
+__isGlobfile() {
+ [[ -f $1 ]]
+}
+
__buildPackage() {
local pkgdest=${1:-.}
local p
@@ -203,7 +213,7 @@ checkPackageDB() {
for repoarch in ${repoarches[@]}; do
# Only 'any' packages can be found in repos of
both arches
if [[ $pkgarch != any ]]; then
- if [[ $pkgarch != ${repoarch} ]]; then
+ if [[ $pkgarch != "$repoarch" ]]; then
continue
fi
fi
--
2.16.1
[arch-projects] [dbscripts] [PATCH 3/3] Update messages to make fuller use of printf formatters
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) "[^"]*\$'
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(-)
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##*/} "
+ die "usage: %s " "${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 r
[arch-projects] [dbscripts] [GIT] Official repo DB scripts branch master updated. 20131102-69-ge53cad6
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "Official repo DB scripts".
The branch, master has been updated
via e53cad6e4a8284165c6d0b2c7c86f6c077be693b (commit)
via c82f4372440b42f879344f105e12fb358c4c0ccf (commit)
via 3e01ba9a82234c1d78a14be095c18a626591ad71 (commit)
via 8757c459089c0db12e57db66561f640f657e34ac (commit)
via 07e6a91da1331ae61b667d878092c5b03aa49ced (commit)
via b45650c3e3870b975620dd89bb6aa77585c8ff02 (commit)
from 134aff74d16331231ed94f34d8417d1325d4600f (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -
commit e53cad6e4a8284165c6d0b2c7c86f6c077be693b
Author: Eli Schwartz
Date: Thu Feb 15 22:34:36 2018 -0500
Globally set $PKGEXT to a bash extended glob representing valid choices.
The current glob `*.pkg.tar.?z` is both less restrictive and more
restrictive than makepkg, as it accepts any valid unicode character.
To be more exact, it's almost completely orthogonal to the one in makepkg.
makepkg only accepts .tar.gz, .tar.bz2, .tar.xz, .tar.lzo, .tar.lrz, and
.tar.Z and most of those fail to match against a two-char compression type.
dbscripts accepts .pkg.tar.💩z which incidentally is what I think of
cherry-picking xz and gz as supported methods.
Since this can be anything makepkg.conf accepts, it needs to be able to
match all that, unless we decide to perform additional restrictions in
which case we should still explicitly list each allowed extension. Using
bash extended globbing allows us to do this relatively painlessly.
Document the fact that this has *always* been some sort of glob, and
update the two cases where this was (not!) being evaluated by bash
[[ ... ]], to use a not-elegant-at-all proxy function is_globfile() to
evaluate globs *before* testing if they exist.
commit c82f4372440b42f879344f105e12fb358c4c0ccf
Author: Eli Schwartz
Date: Thu Feb 15 22:32:04 2018 -0500
ftpdir-cleanup,sourceballs: replace external find command with bash globbing
This fully removes the use of find from the codebase, leads to a
micro-optimization in a couple cases, and ensures that $PKGEXT is
consistently treated as a shell globbing character (which is important
because it is used as one).
Of the eight instances in these files:
- One was unnecessary as `cat` can natively consume all files passed to
it and no directory traversal was in use.
- Two were unnecessary as they were hardcoded to read a single file
- Another four were only being used to strip leading directory paths,
and can be replaced by globstar and ${filepath##*/}
- The final two were checking the modification time of the files, and
can be replaced with touch(1) and [[ -nt ]]. Although this introduces
an additional temporary file, this is not such a big deal.
commit 3e01ba9a82234c1d78a14be095c18a626591ad71
Author: Eli Schwartz
Date: Thu Feb 15 22:26:24 2018 -0500
db-update: replace external find command with bash globbing
Don't bother emitting errors. bash doesn't show globbing errors if it
cannot read a directory to try globbing there. And the former code never
aborted on errors anyway, as without `set -o pipefail` the sort command
swallowed the return code.
commit 8757c459089c0db12e57db66561f640f657e34ac
Author: Luke Shumaker
Date: Fri Feb 16 12:10:25 2018 -0500
test: db-update: verify that PKGEXT(S) is treated as a glob
Commit b61a714 introduced a regression that in a db-functions function
called by db-update, config:PKGEXT is treated like a fixed string, instead
of as a glob. Because of inadequacy of the test-suite, this did not cause
a test failure.
https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.html
The broken function checks if the repo already contains a package that
matches the one being released. This did not trigger a test-suite failure
because right after the broken check there is another check that checks for
the exact filename of the new package file. In the test-suite, all
package files have the same extension, so the latter check always "saved"
us.
So, modify a relevant test case (a test case that verifies that we can't
release the same package twice) such that the second time we release the
package, we set PKGEXT to something different (.gz, instead of the normal
.xz). This way, we can be sure that when db-update rejects the operation,
it's because of the glob check, not the exact-filename check.
This is made slightl
