[pacman-dev] [PATCH] scripts: replace test builtin [ with shell keywords [[ and ((
FS#16623 suggested this change for makepkg; this patch applies it to the
remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski
Cedric Staniewski wrote:
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski
--- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) <snip>
I went through every single change and they all look correct to me. I would appreciate someone else also doing a review here are there are a lot of changes and I could have overlooked something. The only caution I have is this: - [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE" The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ] and not [ $CLEAN_LOCK -eq 1 ]. In this case it does not matter as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that changing. But it may pay to be careful. As an aside. Bash tests are confusing... (( 0 )) returns 1 or "false" (( 1 )) returns 0 or "true" Does that not confuse other people too? Allan
On Fri, Nov 06, 2009 at 03:18:23PM +0000, Allan McRae wrote:
Cedric Staniewski wrote:
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski
--- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) <snip> I went through every single change and they all look correct to me. I would appreciate someone else also doing a review here are there are a lot of changes and I could have overlooked something.
The only caution I have is this:
- [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ] and not [ $CLEAN_LOCK -eq 1 ]. In this case it does not matter as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that changing. But it may pay to be careful.
As an aside. Bash tests are confusing... (( 0 )) returns 1 or "false" (( 1 )) returns 0 or "true" Does that not confuse other people too?
Allan
In my original patch for makepkg there were a number of instances where I replaced a test for -eq 1 with a non-zero test. If the variable is assigned always to 0 or 1, this shouldn't be a problem, should it? With the notable exception of SOURCEONLY a lot of variables are boolean. (( 0 )) and (( 1 )) are very much like how C treats integer testing. I just think of (( )) as a C-style test. - Isaac
Isaac Good wrote:
On Fri, Nov 06, 2009 at 03:18:23PM +0000, Allan McRae wrote:
Cedric Staniewski wrote:
FS#16623 suggested this change for makepkg; this patch applies it to the remaining files in the scripts directory.
Signed-off-by: Cedric Staniewski
--- scripts/pacman-optimize.sh.in | 22 ++++++------ scripts/pkgdelta.sh.in | 22 ++++++------ scripts/rankmirrors.sh.in | 12 +++--- scripts/repo-add.sh.in | 74 ++++++++++++++++++++-------------------- 4 files changed, 65 insertions(+), 65 deletions(-) <snip> I went through every single change and they all look correct to me. I would appreciate someone else also doing a review here are there are a lot of changes and I could have overlooked something.
The only caution I have is this:
- [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE" + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ] and not [ $CLEAN_LOCK -eq 1 ]. In this case it does not matter as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that changing. But it may pay to be careful.
As an aside. Bash tests are confusing... (( 0 )) returns 1 or "false" (( 1 )) returns 0 or "true" Does that not confuse other people too?
Allan
In my original patch for makepkg there were a number of instances where I replaced a test for -eq 1 with a non-zero test. If the variable is assigned always to 0 or 1, this shouldn't be a problem, should it? With the notable exception of SOURCEONLY a lot of variables are boolean.
Well, SOURCEONLY used to only have values 0 and 1 but then got extended to have 2 as well. This was more flagging that we will need to be careful if we ever expand other variables. Allan
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
If the command is a bash function, is that not good enough? (ie why the -p) type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then $ type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $? 1 1 0 Definitely worth its own patch, though. - Isaac
Isaac Good wrote:
On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
If the command is a bash function, is that not good enough? (ie why the -p) type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then
$ type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $? 1 1 0
Definitely worth its own patch, though.
- Isaac
It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
On Tue, Nov 10, 2009 at 2:20 PM, Cedric Staniewski
Isaac Good wrote:
On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
Forgot to point out that the 'type -p anything' tests might not work as intended.
- if [ ! "$(type -p xdelta3)" ]; then + if ! type xdelta3 &>/dev/null; then
$ type asdf bash: type: asdf: not found $ type -p asdf; echo $? 1 $ $ $ touch ~/bin/asdf; chmod +x ~/bin/asdf $ type asdf asdf is /home/makepkg/bin/asdf $ type -p asdf; echo $? /home/makepkg/bin/asdf 0 $ rm ~/bin/asdf $ $ $ asdf() {
echo 1 } $ type asdf asdf is a function asdf () { echo 1 } $ type -p asdf; echo $? 0
That means you cannot rely on the return code when you want to emulate which. You have to test the length of the resulting string instead.
If the command is a bash function, is that not good enough? (ie why the -p) type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then
$ type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $? 1 1 0
Definitely worth its own patch, though.
- Isaac
It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
dmcgee@galway ~/projects/pacman (gpg-more) $ type -p xdelta3; echo $? 1 dmcgee@galway ~/projects/pacman (gpg-more) $ type xdelta3; echo $? -bash: type: xdelta3: not found 1 Note that -p suppresses error messages that you may not want; it's output is only the return code. -Dan
On Tue, Nov 10, 2009 at 02:23:25PM -0600, Dan McGee wrote:
On Tue, Nov 10, 2009 at 2:20 PM, Cedric Staniewski
wrote: It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
dmcgee@galway ~/projects/pacman (gpg-more) $ type -p xdelta3; echo $? 1
dmcgee@galway ~/projects/pacman (gpg-more) $ type xdelta3; echo $? -bash: type: xdelta3: not found 1
Note that -p suppresses error messages that you may not want; it's output is only the return code.
-Dan
The -p only suppresses error messages (STDERR) when it fails. On success it still emits the path to the file on disk (STDOUT). If you want to do the test like this: $ if [ -n "$(type -p xdelta3)" ] ; then then that is the desired behaviour. Output on success, nothing on failure. If you want to just use the return status, you still need to do something with the STDOUT. $ if type -p xdelta3 >/dev/null ; then or $ if type xdelta3 >/dev/null 2>&1 ; then
From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001 From: Isaac Good
Date: Wed, 11 Nov 2009 16:47:43 -0500 Subject: [PATCH 1/2] Changing [ to [[ and ((
First half of makepkg
This replaces any prior patches of mine
Includes stuff like -o to || and -a to && etc
if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps
Signed-off-by: Isaac Good
Isaac Good wrote:
From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001 From: Isaac Good
Date: Wed, 11 Nov 2009 16:47:43 -0500 Subject: [PATCH 1/2] Changing [ to [[ and (( First half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps
Signed-off-by: Isaac Good
---
I hope your patch do not get lost in this thread.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
We could remove the quotes here, too.
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done
What about "while (( $i < $((${#integ}+6)) )); do"?
@@ -521,8 +521,8 @@ generate_checksums() { for netfile in "${source[@]}"; do local file="$(get_filename "$netfile")"
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" plain "$(gettext "Aborting...")" exit 1
Same as above. Quotes are not necessary.
@@ -533,10 +533,10 @@ generate_checksums() {
local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - [ $ct -gt 0 ] && echo -n "$indent" + (( ct )) && echo -n $indent echo -n "'$sum'"
I do not think we want to remove the quotes here. $ indent=" " $ echo -n $indent; echo asdf asdf $ echo -n "$indent"; echo asdf asdf $
@@ -566,8 +566,8 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then echo "$(gettext "NOT FOUND")" >&2 errors=1 found=0 @@ -622,8 +622,8 @@ extract_sources() { continue fi
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s for extraction.")" "$file" plain "$(gettext "Aborting...")" exit 1
Quotes are not necessary.
@@ -662,31 +662,31 @@ extract_sources() { ... fi done
- if [ $EUID -eq 0 ]; then + if (( EUID == 0 )); then # change perms of all source files to root user & root group chown -R 0:0 "$srcdir" fi }
...
(( ! EUID )) ?
@@ -713,12 +713,12 @@ run_function() { ... local i=1 while true; do - if [ -f "$BUILDLOG.$i" ]; then + if [[ -f "$BUILDLOG.$i" ]]; then i=$(($i +1)) else break
Quotes again? There are several other places where quotes or the dollar signs (in e.g. (( $found )) ) are not needed. Maybe you want to remove them, maybe not. Cedric
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
We could remove the quotes here, too.
I was lax with some of the quotes when doing concatenation and parameter expansion...
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done
What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop. for (( i = 0; i < ${#integ} + 6; i++ ))
@@ -521,8 +521,8 @@ generate_checksums() { for netfile in "${source[@]}"; do local file="$(get_filename "$netfile")"
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s to generate checksum.")" "$file" plain "$(gettext "Aborting...")" exit 1
Same as above. Quotes are not necessary.
@@ -533,10 +533,10 @@ generate_checksums() {
local sum="$(openssl dgst -${integ} "$file")" sum=${sum##* } - [ $ct -gt 0 ] && echo -n "$indent" + (( ct )) && echo -n $indent echo -n "'$sum'"
I do not think we want to remove the quotes here.
$ indent=" " $ echo -n $indent; echo asdf asdf $ echo -n "$indent"; echo asdf asdf $
That'd be a careless use of s/"// in a macro on my behalf. Thanks for catching.
@@ -566,8 +566,8 @@ check_checksums() { file="$(get_filename "$file")" echo -n " $file ... " >&2
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then echo "$(gettext "NOT FOUND")" >&2 errors=1 found=0 @@ -622,8 +622,8 @@ extract_sources() { continue fi
- if [ ! -f "$file" ] ; then - if [ ! -f "$SRCDEST/$file" ] ; then + if [[ ! -f $file ]] ; then + if [[ ! -f "$SRCDEST/$file" ]] ; then error "$(gettext "Unable to find source file %s for extraction.")" "$file" plain "$(gettext "Aborting...")" exit 1
Quotes are not necessary.
@@ -662,31 +662,31 @@ extract_sources() { ... fi done
- if [ $EUID -eq 0 ]; then + if (( EUID == 0 )); then # change perms of all source files to root user & root group chown -R 0:0 "$srcdir" fi }
...
(( ! EUID )) ?
As above
@@ -713,12 +713,12 @@ run_function() { ... local i=1 while true; do - if [ -f "$BUILDLOG.$i" ]; then + if [[ -f "$BUILDLOG.$i" ]]; then i=$(($i +1)) else break
Quotes again? There are several other places where quotes or the dollar signs (in e.g. (( $found )) ) are not needed. Maybe you want to remove them, maybe not.
Cedric
Should I implement these changes and resend this patch? /me goes looking for git help Thanks for the feedback! - Isaac
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
I guess you hit the reply button instead of writing a new mail to pacman-dev. At least, there is the In-Reply-To header field in your mail header.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
I am fine with that, but would it not makes sense to use (( $# > 0 )) instead of just (( $# )) then?
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop. for (( i = 0; i < ${#integ} + 6; i++ ))
In this case, I would either get rid of -lt by converting to a for loop directly or not touch that line at all in this patch.
Should I implement these changes and resend this patch? /me goes looking for git help
If you like, but you could also wait for at least Allan's comment. You can use "git commit --amend" to alter your existing commit in git by the way.
On Thu, Nov 12, 2009 at 10:37:31PM +0000, Cedric Staniewski wrote:
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
I guess you hit the reply button instead of writing a new mail to pacman-dev. At least, there is the In-Reply-To header field in your mail header.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
I am fine with that, but would it not makes sense to use (( $# > 0 )) instead of just (( $# )) then?
I am not sure I understand what you are saying. In general there are no negative values. With boolean flags either it is set or not set. So you get : if (( FLAG )) and if (( ! FLAG )) With $# (and EUID) the code is set up to test for zero, not non-zero. (( $# > 0 )) and (( $# )) are the reverse of what the code checks for. All of these do the same thing: $ (( $# == 0 )) && return $R_DEPS_SATISFIED $ (( $# > 0 )) || return $R_DEPS_SATISFIED $ (( ! $# )) && return $R_DEPS_SATISFIED $ (( $# )) || return $R_DEPS_SATISFIED I left the test as && because there was no compelling reason to change it. That aside, I am in favor of either of the first two for $# as opposed to the latter two for boolean flags.
@@ -512,7 +512,7 @@ generate_checksums() {
local i=0; local indent='' - while [ $i -lt $((${#integ}+6)) ]; do + while [[ $i -lt $((${#integ}+6)) ]]; do indent="$indent " i=$(($i+1)) done What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop. for (( i = 0; i < ${#integ} + 6; i++ ))
In this case, I would either get rid of -lt by converting to a for loop directly or not touch that line at all in this patch.
amended/reversed
Should I implement these changes and resend this patch? /me goes looking for git help
If you like, but you could also wait for at least Allan's comment. You can use "git commit --amend" to alter your existing commit in git by the way.
Thanks. I'm learning a lot of git today ;) - Isaac
Isaac Good wrote:
On Thu, Nov 12, 2009 at 10:37:31PM +0000, Cedric Staniewski wrote:
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
I hope your patch do not get lost in this thread. Erm. Yeah, I hope so too. How does it get threaded?
I guess you hit the reply button instead of writing a new mail to pacman-dev. At least, there is the In-Reply-To header field in your mail header.
@@ -345,26 +345,26 @@ handle_deps() { local R_DEPS_SATISFIED=0 local R_DEPS_MISSING=1
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
Is there a reason why you do not use (( ! $# )) here? Some variables are used as a boolean flag in which case testing if ((VAR)) and if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it struck me as more understandable or readable in this form.
I am fine with that, but would it not makes sense to use (( $# > 0 )) instead of just (( $# )) then?
I am not sure I understand what you are saying. In general there are no negative values. With boolean flags either it is set or not set. So you get : if (( FLAG )) and if (( ! FLAG )) With $# (and EUID) the code is set up to test for zero, not non-zero. (( $# > 0 )) and (( $# )) are the reverse of what the code checks for. All of these do the same thing: $ (( $# == 0 )) && return $R_DEPS_SATISFIED $ (( $# > 0 )) || return $R_DEPS_SATISFIED $ (( ! $# )) && return $R_DEPS_SATISFIED $ (( $# )) || return $R_DEPS_SATISFIED
I left the test as && because there was no compelling reason to change it. That aside, I am in favor of either of the first two for $# as opposed to the latter two for boolean flags.
Sorry, I meant another line from your patch where you use (( $# )).
}
check_deps() { - [ $# -gt 0 ] || return + (( $# )) || return
pmout=$(pacman $PACMAN_OPTS -T "$@") ret=$? - if [ $ret -eq 127 ]; then #unresolved deps + if (( ret == 127 )); then #unresolved deps echo "$pmout" - elif [ $ret -ne 0 ]; then + elif (( ret )); then error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout" exit 1 fi
ret is another non-boolean variable by the way. I think some kind of code style guide is a good idea. :)
On Thu, Nov 12, 2009 at 06:29:11PM -0500, Isaac Good wrote:
I left the test as && because there was no compelling reason to change it.
I've been reading the various [ ( vs [[ (( threads and still cannot find a compelling reason for any of it. It makes the code much less readable to the people who have been writing portable or semi-portable scripts for years. Makepkg doesn't require sh portability, but seeing all the back and forth questions, I'd say it doesn't require the extra confusion and opacity. It seems clear that even the majority of respondents have different ideas of what the proper syntax should be whereas with the old way, people read it and just know what it is doing. If I am missing something, could someone please enlighten me? Thanks! -- Jeff My other computer is an abacus.
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
What about something like this?
if [[ "$(check_option makeflags)" = "n" ]]; then
None of the quotes are needed. I'm inclined to replace it with:
if [[ $(check_option makeflags) = "n" ]]; then
While the quotes around n are not needed, they highlight that n is a string which appeals to me since it looks like a string as found in other languages. Or would you say to drop all the quotes. Maybe some style guidelines are needed for the bash language... - Isaac
Isaac Good wrote:
On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
What about something like this?
if [[ "$(check_option makeflags)" = "n" ]]; then
None of the quotes are needed. I'm inclined to replace it with:
if [[ $(check_option makeflags) = "n" ]]; then
While the quotes around n are not needed, they highlight that n is a string which appeals to me since it looks like a string as found in other languages. Or would you say to drop all the quotes.
Aren't the quotes around the string on the right required if it does contain spaces? Anyway, I would prefer all such strings to be quoted so I am happy with your suggestion. Allan
Cedric Staniewski wrote:
Isaac Good wrote:
From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001 From: Isaac Good
Date: Wed, 11 Nov 2009 16:47:43 -0500 Subject: [PATCH 1/2] Changing [ to [[ and (( First half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps
Signed-off-by: Isaac Good
--- I hope your patch do not get lost in this thread.
I have the patches marked so that they will not be lost.
@@ -450,12 +450,12 @@ download_sources() { for netfile in "${source[@]}"; do local file=$(get_filename "$netfile") local url=$(get_url "$netfile") - if [ -f "$startdir/$file" ]; then + if [[ -f "$startdir/$file" ]]; then msg2 "$(gettext "Found %s in build dir")" "$file" rm -f "$srcdir/$file" ln -s "$startdir/$file" "$srcdir/" continue - elif [ -f "$SRCDEST/$file" ]; then + elif [[ -f "$SRCDEST/$file" ]]; then msg2 "$(gettext "Using cached copy of %s")" "$file" rm -f "$srcdir/$file" ln -s "$SRCDEST/$file" "$srcdir/"
We could remove the quotes here, too.
Hmmm... can we. I know we can when there are spaces in a single variable, but for some reason I thought quotes were needed when joining multiple variables like that. I could be wrong... Allan
Allan McRae wrote:
Cedric Staniewski wrote:
We could remove the quotes here, too.
Hmmm... can we. I know we can when there are spaces in a single variable, but for some reason I thought quotes were needed when joining multiple variables like that. I could be wrong...
[[ behaves quite different compared to [ in this aspect, but as far as I know, you do not need quotes to join variables as long as there are no spaces between the variables ;). There are several exceptions though, like cd for example, which require quotes even for single variables when they contain spaces. I think this has something to do with the type of the command. Builtins/external commands (usually) need quotes whereas everything else should work without. In my opinion, it is often not obvious whether they are required or not which is why I tended to use more quotes than actually were needed. I am fairly familiar with quoting by now and can use both "quoting styles", but I think it would probably be a good idea to decide for one. Using just as much quotes as required would be a little bit shorter, but using quotes `where possible` may be a little bit more foolproof. $ a=" a d" $ b=" e" $ c=$a:$b $ echo "$c" a d: e $ $ $ filename="a d e" $ suffix=".f" $ [ -e ${filename}${suffix} ] && echo 1 bash: [: too many arguments $ [[ -e ${filename}${suffix} ]] && echo 1 $ touch "${filename}${suffix}" $ [[ -e ${filename}${suffix} ]] && echo 1 1 $ [[ -e a d e.f ]] && echo 1 bash: syntax error in conditional expression bash: syntax error near `d' $ [[ -e "a d e.f" ]] && echo 1 1
On Thu, Nov 12, 2009 at 11:52:31PM +0000, Cedric Staniewski wrote:
Allan McRae wrote:
Cedric Staniewski wrote:
We could remove the quotes here, too.
Hmmm... can we. I know we can when there are spaces in a single variable, but for some reason I thought quotes were needed when joining multiple variables like that. I could be wrong...
[[ behaves quite different compared to [ in this aspect, but as far as I know, you do not need quotes to join variables as long as there are no spaces between the variables ;). There are several exceptions though, like cd for example, which require quotes even for single variables when they contain spaces. I think this has something to do with the type of the command. Builtins/external commands (usually) need quotes whereas everything else should work without. In my opinion, it is often not obvious whether they are required or not which is why I tended to use more quotes than actually were needed. I am fairly familiar with quoting by now and can use both "quoting styles", but I think it would probably be a good idea to decide for one. Using just as much quotes as required would be a little bit shorter, but using quotes `where possible` may be a little bit more foolproof.
$ a=" a d" $ b=" e" $ c=$a:$b $ echo "$c" a d: e $ $ $ filename="a d e" $ suffix=".f" $ [ -e ${filename}${suffix} ] && echo 1 bash: [: too many arguments $ [[ -e ${filename}${suffix} ]] && echo 1 $ touch "${filename}${suffix}" $ [[ -e ${filename}${suffix} ]] && echo 1 1 $ [[ -e a d e.f ]] && echo 1 bash: syntax error in conditional expression bash: syntax error near `d' $ [[ -e "a d e.f" ]] && echo 1 1
The [[ ]] and (( )) are the exception to everything else in bash (cd, [, touch, grep). I know of nothing else that acts like it (except array indices). With all bash commands, the variable is expanded early enough that a space will split the result into multiple parameters (aka word splitting). Ergo these two are the same: $ cd a b $ var="a b"; cd $var cd will see two arguments. Compare to this, which has one parameter: $ cd "a b" $ var="a b"; cd "$var" The same applies to [, which can make stuff fun, eg: $ [ -n a -a -z a ] -> returns false because the second condition is false and there is an and (-a) $ var="a -a -z a"; [ -n $var ] -> false, too! Go figure... $ var="a -a -z a"; [ -n "$var" ] -> works right The [[ and (( are "special". Word splitting will not occur inside. (Array indices are computed in arithmetic mode, ie like inside ((: ${arr[stuff]}). The entire parsing is set up different. Word splitting does not occur. And (mostly as an aside,) bash parses them over double as fast. Concatenating variables has no special rules. It acts identical to one variable with both the contents. Assuming there is no whitespace between them, it is just like one variable. Whitespace inside causes word splitting but not in (( and [[. Once discussing word splitting, this also applies to arrays, specifically in this context: $ for var in ${arr[@]} ; do Without quotes, elements with a space will be turned into multiple elements for the loop. - Isaac
From f29298f8d9b947ce64d33d40d91f485396bb54eb Mon Sep 17 00:00:00 2001 From: Isaac Good
Date: Thu, 12 Nov 2009 15:07:34 -0500 Subject: [PATCH 1/2] Changing [ to [[ and (( - Part One - Amended
First half of makepkg
This replaces any prior patches of mine
Includes stuff like -o to || and -a to && etc
if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR traps
Signed-off-by: Isaac Good
From e3e5bb93cb2b33315a8d6eff8956aa5b6c74d28d Mon Sep 17 00:00:00 2001 From: Isaac Good
Date: Thu, 12 Nov 2009 15:09:05 -0500 Subject: [PATCH 2/2] Changing [ to [[ and (( - Part Two - Amended
Second half of makepkg
This replaces any prior patches of mine
Includes stuff like -o to || and -a to && etc
if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR trap
Signed-off-by: Isaac Good
Isaac Good wrote:
From e3e5bb93cb2b33315a8d6eff8956aa5b6c74d28d Mon Sep 17 00:00:00 2001 From: Isaac Good
Date: Thu, 12 Nov 2009 15:09:05 -0500 Subject: [PATCH 2/2] Changing [ to [[ and (( - Part Two - Amended Second half of makepkg This replaces any prior patches of mine Includes stuff like -o to || and -a to && etc if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR trap
Signed-off-by: Isaac Good
<snip> I have pushed these two patches and Cedric's one for the other scripts to the "bashtest" branch in my repo [1]. I am going through these now and will get back with any queries. Allan [1] http://projects.archlinux.org/users/allan/pacman.git/log/?h=bashtest
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss. I would like to get some more consistency in quoting. e.g. here are some varied tests: - if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then + if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then - if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then - if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then - if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then - if [ ${1:0:2} = '--' ]; then + if [[ ${1:0:2} = '--' ]]; then - if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then -if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this. There should be quotes kept in the gettext calls in this test: - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then Why is EUID tested against 0 explicitly when all other tests for zero just use ! EUID? e.g. - if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then + if (( EUID == 0 && ! ASROOT )); then In fact, I quite like that things are explicitly tested in this case. I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code. - if [ $ret -gt 0 ]; then + if (( ret )); then - elif [ $ret -ne 0 ]; then + elif (( ret )); then Note that these explicitly test for "== 0" or "> 0" and I think that is much clearer: - [ $# -gt 0 ] || return + (( $# > 0 )) || return - [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED Allan
On Sun, Nov 15, 2009 at 02:40:47PM +0000, Allan McRae wrote:
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss.
I would like to get some more consistency in quoting. e.g. here are some varied tests:
- if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then
+ if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then
- if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then
- if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then
- if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then
- if [ ${1:0:2} = '--' ]; then + if [[ ${1:0:2} = '--' ]]; then
- if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then
-if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then
My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this.
That "n" and 'any' slipped my radar. I tried removing quotes for simple strings. ? looks like a bash pattern so I left it quotes. -- is also "different".
There should be quotes kept in the gettext calls in this test: - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then
Perhaps? Around the arguments passed to gettext or around the $( )?
Why is EUID tested against 0 explicitly when all other tests for zero just use ! EUID? e.g.
- if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then + if (( EUID == 0 && ! ASROOT )); then
In fact, I quite like that things are explicitly tested in this case.
I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code.
- if [ $ret -gt 0 ]; then + if (( ret )); then
- elif [ $ret -ne 0 ]; then + elif (( ret )); then
Note that these explicitly test for "== 0" or "> 0" and I think that is much clearer:
- [ $# -gt 0 ] || return + (( $# > 0 )) || return
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
As mentioned earlier, most the variables tested are being used as booleans. Either TRUE or FALSE. Hence: if (( FLAG )) then the flag is set. Similar to C/Java/Perl/etc The EUID and $# are not flags. They are integer values. So I left the test explicit. - Isaac
On Sun, Nov 15, 2009 at 8:40 AM, Allan McRae
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss.
I would like to get some more consistency in quoting. e.g. here are some varied tests:
- if [ "$ret" != '?' ]; then + if [[ $ret != '?' ]]; then
+ if [[ $ret != "?" ]]; then + if [[ $ret = y ]]; then
- if [ "$cmd" = "bsdtar" ]; then + if [[ $cmd = bsdtar ]]; then
- if [ "$(check_option makeflags)" = "n" ]; then + if [[ $(check_option makeflags) = "n" ]]; then
- if [ "$arch" != 'any' ]; then + if [[ $arch != 'any' ]]; then
- if [ ${1:0:2} = '--' ]; then + if [[ ${1:0:2} = '--' ]]; then
- if [ -d ./src/$_hgrepo ] ; then + if [[ -d ./src/$_hgrepo ]] ; then
-if [ -r ~/.makepkg.conf ]; then +if [[ -r ~/.makepkg.conf ]]; then
My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this.
There should be quotes kept in the gettext calls in this test: - if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext "Y")" ]; then + if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then
Why is EUID tested against 0 explicitly when all other tests for zero just use ! EUID? e.g.
- if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then + if (( EUID == 0 && ! ASROOT )); then
In fact, I quite like that things are explicitly tested in this case.
I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code.
- if [ $ret -gt 0 ]; then + if (( ret )); then
- elif [ $ret -ne 0 ]; then + elif (( ret )); then
Note that these explicitly test for "== 0" or "> 0" and I think that is much clearer:
- [ $# -gt 0 ] || return + (( $# > 0 )) || return
- [ $# -eq 0 ] && return $R_DEPS_SATISFIED + (( $# == 0 )) && return $R_DEPS_SATISFIED
As a side note, in the pacman C code we try to stick to a convention where if testing for 0 makes sense, do it explicitly. This comes up often in the case of strcmp() == 0, which means "these two strings match". e.g. if(strcmp(pkgname, name) == 0) { instead of if(!strcmp(pkgname, name)) { I like the logic here of doing explicit declarations of 0 in the EUID case. For a true boolean in the other cases, I'm fine with dropping the zero check. -Dan
Allan McRae wrote:
I have looked at the two patches and the good news is that I can spot nothing wrong! I just have a few comments about style that I would like to discuss.
[...]
My suggestion is that any thing with text (i.e. not a pure variable) is quoted. I know this is excessive in some cases (e.g. the last case) but the only exception I would be happy with is tests that are pure paths with only added "/" (e.g. $startdir/$file). Even then, maybe quotes would be nicer... I am happy to be debated on this.
To quote myself [1]:
In my opinion, it is often not obvious whether they are required or not which is why I tended to use more quotes than actually were needed. I am fairly familiar with quoting by now and can use both "quoting styles", but I think it would probably be a good idea to decide for one. Using just as much quotes as required would be a little bit shorter, but using quotes `where possible` may be a little bit more foolproof.
Meaning that I am fine with quoting strings generally. [1] http://mailman.archlinux.org/pipermail/pacman-dev/2009-November/010115.html
I wonder if the tests of return values should explicitly test for "== 0" or "!= 0". e.g. these test have become less clear to me when I read the code.
- if [ $ret -gt 0 ]; then + if (( ret )); then
- elif [ $ret -ne 0 ]; then + elif (( ret )); then
Yep, sounds reasonable, especially since we already explicitly test all (or at least many) other non-boolean variables.
participants (5)
-
Allan McRae
-
Cedric Staniewski
-
Dan McGee
-
Isaac Good
-
Jeff