While this is most likely the result of user error, as repo-remove doesn't accept globs, using 'package*' as the pkgname will result in an endless loop of the following message being printed:
-> Removing existing entry 'package**'...
This happens because find_pkgentry() fails to account the case where globbing fails and the expression is taken literally. Fix that by checking the existence of the file before doing anything else.
Signed-off-by: Rafael Ascensão rafa.almas@gmail.com --- scripts/repo-add.sh.in | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 57413df5..123bb796 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -120,10 +120,12 @@ find_pkgentry() { local pkgentry
for pkgentry in "$tmpdir/db/$pkgname"*; do - name=${pkgentry##*/} - if [[ ${name%-*-*} = $pkgname ]]; then - echo $pkgentry - return 0 + if [[ -e $pkgentry ]]; then + name=${pkgentry##*/} + if [[ ${name%-*-*} = $pkgname ]]; then + echo $pkgentry + return 0 + fi fi done return 1
On Fri, Apr 26, 2019 at 10:04 PM Rafael Ascensão rafa.almas@gmail.com wrote:
This happens because find_pkgentry() fails to account the case where globbing fails and the expression is taken literally.
Maybe we should use failglob instead to provoke an error?
Hi Jan,
This happens because find_pkgentry() fails to account the case where globbing fails and the expression is taken literally.
Maybe we should use failglob instead to provoke an error?
Wouldn't nullglob be better to skip the loop's body?
$ touch foo bar $ ls bar foo $ $ shopt -u nullglob failglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done = * $ $ touch foo bar $ shopt -s nullglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done $
On 4/27/19 4:38 AM, Ralph Corderoy wrote:
Hi Jan,
This happens because find_pkgentry() fails to account the case where globbing fails and the expression is taken literally.
Maybe we should use failglob instead to provoke an error?
Wouldn't nullglob be better to skip the loop's body?
$ touch foo bar $ ls bar foo $ $ shopt -u nullglob failglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done = * $ $ touch foo bar $ shopt -s nullglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done $
All three of you are wrong, or at least missing the point. :)
[[ foo = f* ]] is defined to have pattern-matching context, and in this case, [[ pkgname* = pkgname** ]] matches true. If we do not want this behavior, we need to quote this -- and by properly quoting where we are supposed to quote, we don't need to stat for file existence, we don't need to change the way the whole script handles globs, and we still get the failing action we want. To be more precise, we get the exact behavior and method of acquiring that behavior, which was originally intended.
There are also other cases where this could matter, some of which are long shots.
The right-hand side of the [[ ... = ... ]] keyword is an exception to the general rule that quoting is unnecessary with [[
This is usually not a problem, e.g. in libmakepkg, lint_one_pkgname will already fail if pkgname has an asterisk, but it certainly doesn't hurt to be "more proper" and go with the spec; it is more dangerous in repo-add, which can get caught in an infinite loop instead of safely asserting there is no package named 'foo*'.
Reported-by: Rafael Ascensão rafa.almas@gmail.com Signed-off-by: Eli Schwartz eschwartz@archlinux.org --- scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/depends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/provides.sh.in | 2 +- scripts/libmakepkg/source/git.sh.in | 2 +- scripts/libmakepkg/tidy/zipman.sh.in | 2 +- scripts/pacman-db-upgrade.sh.in | 2 +- scripts/repo-add.sh.in | 4 ++-- 10 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in b/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in index 0a9ddf67..df754d7e 100644 --- a/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in @@ -44,7 +44,7 @@ lint_checkdepends() { for checkdepend in "${checkdepends_list[@]}"; do name=${checkdepend%%@(<|>|=|>=|<=)*} lint_one_pkgname checkdepends "$name" || ret=1 - if [[ $name != $checkdepend ]]; then + if [[ $name != "$checkdepend" ]]; then ver=${checkdepend##$name@(<|>|=|>=|<=)} check_fullpkgver "$ver" checkdepends || ret=1 fi diff --git a/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in b/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in index b61459e1..ee0e6f50 100644 --- a/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in @@ -44,7 +44,7 @@ lint_conflicts() { for conflict in "${conflicts_list[@]}"; do name=${conflict%%@(<|>|=|>=|<=)*} lint_one_pkgname conflicts "$name" || ret=1 - if [[ $name != $conflict ]]; then + if [[ $name != "$conflict" ]]; then ver=${conflict##$name@(<|>|=|>=|<=)} check_fullpkgver "$ver" conflicts || ret=1 fi diff --git a/scripts/libmakepkg/lint_pkgbuild/depends.sh.in b/scripts/libmakepkg/lint_pkgbuild/depends.sh.in index aba43825..3fe9614f 100644 --- a/scripts/libmakepkg/lint_pkgbuild/depends.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/depends.sh.in @@ -44,7 +44,7 @@ lint_depends() { for depend in "${depends_list[@]}"; do name=${depend%%@(<|>|=|>=|<=)*} lint_one_pkgname depends "$name" || ret=1 - if [[ $name != $depend ]]; then + if [[ $name != "$depend" ]]; then ver=${depend##$name@(<|>|=|>=|<=)} # Don't validate empty version because of https://bugs.archlinux.org/task/58776 if [[ -n $ver ]]; then diff --git a/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in b/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in index 20c7f7dc..ed1c1120 100644 --- a/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in @@ -44,7 +44,7 @@ lint_makedepends() { for makedepend in "${makedepends_list[@]}"; do name=${makedepend%%@(<|>|=|>=|<=)*} lint_one_pkgname makedepends "$name" || ret=1 - if [[ $name != $makedepend ]]; then + if [[ $name != "$makedepend" ]]; then ver=${makedepend##$name@(<|>|=|>=|<=)} check_fullpkgver "$ver" makedepends || ret=1 fi diff --git a/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in b/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in index 505ee848..ef7078d1 100644 --- a/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in @@ -44,7 +44,7 @@ lint_optdepends() { for optdepend in "${optdepends_list[@]%%:[[:space:]]*}"; do name=${optdepend%%@(<|>|=|>=|<=)*} lint_one_pkgname optdepends "$name" || ret=1 - if [[ $name != $optdepend ]]; then + if [[ $name != "$optdepend" ]]; then ver=${optdepend##$name@(<|>|=|>=|<=)} check_fullpkgver "$ver" optdepends || ret=1 fi diff --git a/scripts/libmakepkg/lint_pkgbuild/provides.sh.in b/scripts/libmakepkg/lint_pkgbuild/provides.sh.in index 5a529728..41b4c6b9 100644 --- a/scripts/libmakepkg/lint_pkgbuild/provides.sh.in +++ b/scripts/libmakepkg/lint_pkgbuild/provides.sh.in @@ -49,7 +49,7 @@ lint_provides() { fi name=${provide%=*} lint_one_pkgname provides "$name" || ret=1 - if [[ $name != $provide ]]; then + if [[ $name != "$provide" ]]; then ver=${provide##$name=} check_fullpkgver "$ver" provides || ret=1 fi diff --git a/scripts/libmakepkg/source/git.sh.in b/scripts/libmakepkg/source/git.sh.in index 96d79623..ccf4642b 100644 --- a/scripts/libmakepkg/source/git.sh.in +++ b/scripts/libmakepkg/source/git.sh.in @@ -117,7 +117,7 @@ extract_git() {
if [[ ${fragment%%=*} = tag ]]; then tagname="$(git tag -l --format='%(tag)' "$ref")" - if [[ -n $tagname && $tagname != $ref ]]; then + if [[ -n $tagname && $tagname != "$ref" ]]; then error "$(gettext "Failure while checking out version %s, the git tag has been forged")" "$ref" plain "$(gettext "Aborting...")" exit 1 diff --git a/scripts/libmakepkg/tidy/zipman.sh.in b/scripts/libmakepkg/tidy/zipman.sh.in index 43f66730..772a2ea2 100644 --- a/scripts/libmakepkg/tidy/zipman.sh.in +++ b/scripts/libmakepkg/tidy/zipman.sh.in @@ -40,7 +40,7 @@ tidy_zipman() { while read -r link ; do if [[ "${file}" -ef "${link}" ]] ; then rm -f "$link" "${link}.gz" - if [[ ${file%/*} = ${link%/*} ]]; then + if [[ ${file%/*} = "${link%/*}" ]]; then ln -s -- "${file##*/}.gz" "${link}.gz" else ln -s -- "/${file}.gz" "${link}.gz" diff --git a/scripts/pacman-db-upgrade.sh.in b/scripts/pacman-db-upgrade.sh.in index a831f181..75a108bc 100644 --- a/scripts/pacman-db-upgrade.sh.in +++ b/scripts/pacman-db-upgrade.sh.in @@ -182,7 +182,7 @@ if [[ -z "$db_version" ]]; then realdir="$(resolve_dir "$dir")"
# verify realdir is inside root - if [[ ${realdir:0:${#pacroot}} != $pacroot ]]; then + if [[ ${realdir:0:${#pacroot}} != "$pacroot" ]]; then warning "$(gettext "symlink '%s' points outside pacman root, manual repair required")" "$dir" continue fi diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 57413df5..e23f47c9 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -121,7 +121,7 @@ find_pkgentry() {
for pkgentry in "$tmpdir/db/$pkgname"*; do name=${pkgentry##*/} - if [[ ${name%-*-*} = $pkgname ]]; then + if [[ ${name%-*-*} = "$pkgname" ]]; then echo $pkgentry return 0 fi @@ -464,7 +464,7 @@ remove() { error "$(gettext "Package matching '%s' not found.")" "$pkgname" return 1 fi - + return 0 }
On 28/4/19 12:54 pm, Eli Schwartz wrote:
The right-hand side of the [[ ... = ... ]] keyword is an exception to the general rule that quoting is unnecessary with [[
This is usually not a problem, e.g. in libmakepkg, lint_one_pkgname will already fail if pkgname has an asterisk, but it certainly doesn't hurt to be "more proper" and go with the spec; it is more dangerous in repo-add, which can get caught in an infinite loop instead of safely asserting there is no package named 'foo*'.
Reported-by: Rafael Ascensão rafa.almas@gmail.com Signed-off-by: Eli Schwartz eschwartz@archlinux.org
scripts/libmakepkg/lint_pkgbuild/checkdepends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/conflicts.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/depends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/makedepends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/optdepends.sh.in | 2 +- scripts/libmakepkg/lint_pkgbuild/provides.sh.in | 2 +- scripts/libmakepkg/source/git.sh.in | 2 +- scripts/libmakepkg/tidy/zipman.sh.in | 2 +- scripts/pacman-db-upgrade.sh.in | 2 +- scripts/repo-add.sh.in | 4 ++-- 10 files changed, 11 insertions(+), 11 deletions(-)
Looks good. (I'll allow the formatting fix that slipped in at the end there!)
A
On 28/4/19 12:49 pm, Eli Schwartz wrote:
On 4/27/19 4:38 AM, Ralph Corderoy wrote:
Hi Jan,
This happens because find_pkgentry() fails to account the case where globbing fails and the expression is taken literally.
Maybe we should use failglob instead to provoke an error?
Wouldn't nullglob be better to skip the loop's body?
$ touch foo bar $ ls bar foo $ $ shopt -u nullglob failglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done = * $ $ touch foo bar $ shopt -s nullglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done $All three of you are wrong, or at least missing the point. :)
[[ foo = f* ]] is defined to have pattern-matching context, and in this case, [[ pkgname* = pkgname** ]] matches true. If we do not want this behavior, we need to quote this -- and by properly quoting where we are supposed to quote, we don't need to stat for file existence, we don't need to change the way the whole script handles globs, and we still get the failing action we want. To be more precise, we get the exact behavior and method of acquiring that behavior, which was originally intended.
There are also other cases where this could matter, some of which are long shots.
We probably need to work on the enforcement of pkgname rules from makepkg into repo-add as well:
pkgname (array) Either the name of the package or an array of names for split packages. Valid characters for members of this array are alphanumerics, and any of the following characters: “@ . _ + -”. Additionally, names are not allowed to start with hyphens or dots.
Can probably link in libmakepkg into here to do so. Although I'd like to move repo-add to using libalpm, and we need to enforce rules here too...
Allan
On 4/27/19 11:00 PM, Allan McRae wrote:
On 28/4/19 12:49 pm, Eli Schwartz wrote:
On 4/27/19 4:38 AM, Ralph Corderoy wrote:
Hi Jan,
This happens because find_pkgentry() fails to account the case where globbing fails and the expression is taken literally.
Maybe we should use failglob instead to provoke an error?
Wouldn't nullglob be better to skip the loop's body?
$ touch foo bar $ ls bar foo $ $ shopt -u nullglob failglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done = * $ $ touch foo bar $ shopt -s nullglob $ for f in *; do echo = $f; done = bar = foo $ rm foo $ for f in *; do echo = $f; done = bar $ rm bar $ for f in *; do echo = $f; done $All three of you are wrong, or at least missing the point. :)
[[ foo = f* ]] is defined to have pattern-matching context, and in this case, [[ pkgname* = pkgname** ]] matches true. If we do not want this behavior, we need to quote this -- and by properly quoting where we are supposed to quote, we don't need to stat for file existence, we don't need to change the way the whole script handles globs, and we still get the failing action we want. To be more precise, we get the exact behavior and method of acquiring that behavior, which was originally intended.
There are also other cases where this could matter, some of which are long shots.
We probably need to work on the enforcement of pkgname rules from makepkg into repo-add as well:
pkgname (array) Either the name of the package or an array of names for split packages. Valid characters for members of this array are alphanumerics, and any of the following characters: “@ . _ + -”. Additionally, names are not allowed to start with hyphens or dots.
Can probably link in libmakepkg into here to do so. Although I'd like to move repo-add to using libalpm, and we need to enforce rules here too...
Moving to libalpm would be nice, also because repo-add is some hairy code and I'm almost afraid to touch it. :p
I'm unsure what some of the logic in there is doing. Spent like 15 minutes trying to figure out why db_remove_entry is even an infinite loop at all (but then Andrew pointed out even if we don't support multiple versions of a pkgname in a database, it probably makes sense to not fail to remove them if they get created some other way).
pacman-dev@lists.archlinux.org