threads / patch / 36665

patch, 10 partscheck_bindir: don't use the -a or -b option with the test command

Subject: [PATCH 01/10] check_bindir: don't use the -a or -b option with the test command

## tl;dr

14 messages between May 15, 2014 and May 16, 2014. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Elia Pinto· May 15, 2014, 14:17 UTC · lore

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 check_bindir |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to check_bindir +1 −1
diff --git a/check_bindir b/check_bindir
index a1c4c3e..623eadc 100755
--- a/check_bindir
+++ b/check_bindir
@@ -2,7 +2,7 @@
 bindir="$1"
 gitexecdir="$2"
 gitcmd="$3"
-if test "$bindir" != "$gitexecdir" -a -x "$gitcmd"
+if test "$bindir" != "$gitexecdir" && test -x "$gitcmd"
 then
 	echo
 	echo "!! You have installed git-* commands to new gitexecdir."

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 02/10] contrib/examples/git-clone.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 contrib/examples/git-clone.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/examples/git-clone.sh +1 −1
diff --git a/contrib/examples/git-clone.sh b/contrib/examples/git-clone.sh
index b4c9376..08cf246 100755
--- a/contrib/examples/git-clone.sh
+++ b/contrib/examples/git-clone.sh
@@ -516,7 +516,7 @@ then
 
 	case "$no_checkout" in
 	'')
-		test "z$quiet" = z -a "z$no_progress" = z && v=-v || v=
+		test "z$quiet" = z && test "z$no_progress" = z && v=-v || v=
 		git read-tree -m -u $v HEAD HEAD
 	esac
 fi

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 03/10] contrib/examples/git-commit.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 contrib/examples/git-commit.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/examples/git-commit.sh +2 −2
diff --git a/contrib/examples/git-commit.sh b/contrib/examples/git-commit.sh
index 5cafe2e..934505b 100755
--- a/contrib/examples/git-commit.sh
+++ b/contrib/examples/git-commit.sh
@@ -51,7 +51,7 @@ run_status () {
 		export GIT_INDEX_FILE
 	fi
 
-	if test "$status_only" = "t" -o "$use_status_color" = "t"; then
+	if test "$status_only" = "t" || test "$use_status_color" = "t"; then
 		color=
 	else
 		color=--nocolor
@@ -296,7 +296,7 @@ t,,,[1-9]*)
 	die "No paths with -i does not make sense." ;;
 esac
 
-if test ! -z "$templatefile" -a -z "$log_given"
+if test ! -z "$templatefile" && test -z "$log_given"
 then
 	if test ! -f "$templatefile"
 	then

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 04/10] contrib/examples/git-merge.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 contrib/examples/git-merge.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/examples/git-merge.sh +2 −2
diff --git a/contrib/examples/git-merge.sh b/contrib/examples/git-merge.sh
index 7e40f40..52f2aaf 100755
--- a/contrib/examples/git-merge.sh
+++ b/contrib/examples/git-merge.sh
@@ -161,7 +161,7 @@ merge_name () {
 			return
 		fi
 	fi
-	if test "$remote" = "FETCH_HEAD" -a -r "$GIT_DIR/FETCH_HEAD"
+	if test "$remote" = "FETCH_HEAD" && test -r "$GIT_DIR/FETCH_HEAD"
 	then
 		sed -e 's/	not-for-merge	/		/' -e 1q \
 			"$GIT_DIR/FETCH_HEAD"
@@ -527,7 +527,7 @@ do
 		git diff-files --name-only
 		git ls-files --unmerged
 	    } | wc -l`
-	    if test $best_cnt -le 0 -o $cnt -le $best_cnt
+	    if test $best_cnt -le 0 || test $cnt -le $best_cnt
 	    then
 		best_strategy=$strategy
 		best_cnt=$cnt

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 05/10] contrib/examples/git-repack.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 contrib/examples/git-repack.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/examples/git-repack.sh +2 −2
diff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh
index f312405..96e3fed 100755
--- a/contrib/examples/git-repack.sh
+++ b/contrib/examples/git-repack.sh
@@ -76,8 +76,8 @@ case ",$all_into_one," in
 				existing="$existing $e"
 			fi
 		done
-		if test -n "$existing" -a -n "$unpack_unreachable" -a \
-			-n "$remove_redundant"
+		if test -n "$existing" && test -n "$unpack_unreachable" && \
+			test -n "$remove_redundant"
 		then
 			# This may have arbitrary user arguments, so we
 			# have to protect it against whitespace splitting

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 06/10] contrib/examples/git-resolve.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 contrib/examples/git-resolve.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/examples/git-resolve.sh +1 −1
diff --git a/contrib/examples/git-resolve.sh b/contrib/examples/git-resolve.sh
index 48d0fc9..70fdc27 100755
--- a/contrib/examples/git-resolve.sh
+++ b/contrib/examples/git-resolve.sh
@@ -76,7 +76,7 @@ case "$common" in
 			2>/dev/null || continue
 		# Count the paths that are unmerged.
 		cnt=$(GIT_INDEX_FILE=$G git ls-files --unmerged | wc -l)
-		if test $best_cnt -le 0 -o $cnt -le $best_cnt
+		if test $best_cnt -le 0 || test $cnt -le $best_cnt
 		then
 			best=$c
 			best_cnt=$cnt

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 07/10] git-bisect.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 git-bisect.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to git-bisect.sh +1 −1
diff --git a/git-bisect.sh b/git-bisect.sh
index af4d04c..1e0d602 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -408,7 +408,7 @@ bisect_replay () {
 	bisect_reset
 	while read git bisect command rev
 	do
-		test "$git $bisect" = "git bisect" -o "$git" = "git-bisect" || continue
+		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
 		if test "$git" = "git-bisect"
 		then
 			rev="$command"

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 08/10] git-mergetool.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 git-mergetool.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to git-mergetool.sh +2 −2
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 332528f..88e853f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -205,7 +205,7 @@ checkout_staged_file () {
 		"$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \
 		: '\([^	]*\)	')
 
-	if test $? -eq 0 -a -n "$tmpfile"
+	if test $? -eq 0 && test -n "$tmpfile"
 	then
 		mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
 	else
@@ -256,7 +256,7 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test -z "$local_mode" -o -z "$remote_mode"
+	if test -z "$local_mode" || test -z "$remote_mode"
 	then
 		echo "Deleted merge conflict for '$MERGED':"
 		describe_file "$local_mode" "local" "$LOCAL"

-- 
1.7.10.4
David Aguilar· May 16, 2014, 08:24 UTC · re: Elia Pinto · lore

Re: [PATCH 08/10] git-mergetool.sh: don't use the -a or -b option with the test command

On Thu, May 15, 2014 at 07:17:35AM -0700, Elia Pinto wrote:
Show 50 quoted lines
> Even though POSIX.1 lists -a/-o as options to "test", they are
> marked "Obsolescent XSI". Scripts using these expressions
> should be converted  as follow:
> 
> test "$1" -a "$2"
> 
> should be written as:
> 
> test "$1" && test "$2"
> 
> Likewise
> 
> test "$1" -o "$2"
> 
> should be written as:
> 
> test "$1"  test "$2"
> 
> But note that, in test, -a has higher precedence than -o while
> "&&" and "||" have equal precedence in the shell.
> 
> The reason for this is that the precedence rules were never well
> specified, and this made many sane-looking uses of "test -a/-o" problematic.
> 
> For example, if $x is "=", these work according to POSIX (it's not
> portable, but in practice it's okay):
> 
>    $ test -z "$x"
>    $ test -z "$x" && test a = b
> 
> but this doesn't
> 
>    $ test -z "$x" -a a = b
>    bash: test: too many arguments
> 
> because it groups "test -n = -a" and is left with "a = b".
> 
> Similarly, if $x is "-f", these
> 
>    $ test "$x"
>    $ test "$x" || test c = d
> 
> correctly adds an implicit "-n", but this fails:
> 
>    $ test "$x" -o c = d
>    bash: test: too many arguments
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
Looks good, thanks.
Acked-by: David Aguilar <davvid@gmail.com>
Show 27 quoted lines
>  git-mergetool.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 332528f..88e853f 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -205,7 +205,7 @@ checkout_staged_file () {
>  		"$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \
>  		: '\([^	]*\)	')
>  
> -	if test $? -eq 0 -a -n "$tmpfile"
> +	if test $? -eq 0 && test -n "$tmpfile"
>  	then
>  		mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
>  	else
> @@ -256,7 +256,7 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> -	if test -z "$local_mode" -o -z "$remote_mode"
> +	if test -z "$local_mode" || test -z "$remote_mode"
>  	then
>  		echo "Deleted merge conflict for '$MERGED':"
>  		describe_file "$local_mode" "local" "$LOCAL"
> -- 
> 1.7.10.4
-- 
David
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 09/10] git-rebase--interactive.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 git-rebase--interactive.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to git-rebase--interactive.sh +1 −1
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6ec9d3c..797571f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1013,7 +1013,7 @@ then
 	git rev-list $revisions |
 	while read rev
 	do
-		if test -f "$rewritten"/$rev -a "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = ""
+		if test -f "$rewritten"/$rev && test "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = ""
 		then
 			# Use -f2 because if rev-list is telling us this commit is
 			# not worthwhile, we don't want to track its multiple heads,

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:17 UTC · re: Elia Pinto · lore

[PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command

Even though POSIX.1 lists -a/-o as options to "test", they are marked "Obsolescent XSI". Scripts using these expressions should be converted as follow:

test "$1" -a "$2"
should be written as:
test "$1" && test "$2"
Likewise
test "$1" -o "$2"
should be written as:
test "$1"  test "$2"

But note that, in test, -a has higher precedence than -o while "&&" and "||" have equal precedence in the shell.

The reason for this is that the precedence rules were never well specified, and this made many sane-looking uses of "test -a/-o" problematic.

For example, if $x is "=", these work according to POSIX (it's not portable, but in practice it's okay):

   $ test -z "$x"
   $ test -z "$x" && test a = b
but this doesn't
   $ test -z "$x" -a a = b
   bash: test: too many arguments
because it groups "test -n = -a" and is left with "a = b".
Similarly, if $x is "-f", these
   $ test "$x"
   $ test "$x" || test c = d
correctly adds an implicit "-n", but this fails:
   $ test "$x" -o c = d
   bash: test: too many arguments
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
 git-submodule.sh |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Show changes to git-submodule.sh +7 −7
diff --git a/git-submodule.sh b/git-submodule.sh
index b55d83a..d89e1d0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -864,7 +864,7 @@ Maybe you want to use 'update --init'?")"
 		then
 			subforce=$force
 			# If we don't already have a -f flag and the submodule has never been checked out
-			if test -z "$subsha1" -a -z "$force"
+			if test -z "$subsha1" || test -z "$force"
 			then
 				subforce="-f"
 			fi
@@ -1059,13 +1059,13 @@ cmd_summary() {
 		while read mod_src mod_dst sha1_src sha1_dst status sm_path
 		do
 			# Always show modules deleted or type-changed (blob<->module)
-			test $status = D -o $status = T && echo "$sm_path" && continue
+                        ( test $status = D || test $status = T ) && echo "$sm_path" && continue
 			# Respect the ignore setting for --for-status.
 			if test -n "$for_status"
 			then
 				name=$(module_name "$sm_path")
 				ignore_config=$(get_submodule_config "$name" ignore none)
-				test $status != A -a $ignore_config = all && continue
+				test $status != A && test $ignore_config = all && continue
 			fi
 			# Also show added or modified modules which are checked out
 			GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
@@ -1125,7 +1125,7 @@ cmd_summary() {
 		*)
 			errmsg=
 			total_commits=$(
-			if test $mod_src = 160000 -a $mod_dst = 160000
+			if test $mod_src = 160000 && test $mod_dst = 160000
 			then
 				range="$sha1_src...$sha1_dst"
 			elif test $mod_src = 160000
@@ -1162,7 +1162,7 @@ cmd_summary() {
 			# i.e. deleted or changed to blob
 			test $mod_dst = 160000 && echo "$errmsg"
 		else
-			if test $mod_src = 160000 -a $mod_dst = 160000
+			if test $mod_src = 160000 && test $mod_dst = 160000
 			then
 				limit=
 				test $summary_limit -gt 0 && limit="-$summary_limit"
@@ -1233,7 +1233,7 @@ cmd_status()
 			say "U$sha1 $displaypath"
 			continue
 		fi
-		if test -z "$url" || ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
+		if test -z "$url" || ! test -d "$sm_path"/.git || test -f "$sm_path"/.git
 		then
 			say "-$sha1 $displaypath"
 			continue;
@@ -1402,7 +1402,7 @@ then
 fi
 
 # "--cached" is accepted only by "status" and "summary"
-if test -n "$cached" && test "$command" != status -a "$command" != summary
+if test -n "$cached" && test "$command" != status && test "$command" != summary
 then
 	usage
 fi

-- 
1.7.10.4
Elia Pinto· May 15, 2014, 14:19 UTC · re: Elia Pinto · lore

Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command

I am very sorry : the comment is wrong. I will repost shortly.
Best regards
2014-05-15 16:17 GMT+02:00 Elia Pinto <gitter.spiros@gmail.com>:
Show 122 quoted lines
> Even though POSIX.1 lists -a/-o as options to "test", they are
> marked "Obsolescent XSI". Scripts using these expressions
> should be converted  as follow:
>
> test "$1" -a "$2"
>
> should be written as:
>
> test "$1" && test "$2"
>
> Likewise
>
> test "$1" -o "$2"
>
> should be written as:
>
> test "$1"  test "$2"
>
> But note that, in test, -a has higher precedence than -o while
> "&&" and "||" have equal precedence in the shell.
>
> The reason for this is that the precedence rules were never well
> specified, and this made many sane-looking uses of "test -a/-o" problematic.
>
> For example, if $x is "=", these work according to POSIX (it's not
> portable, but in practice it's okay):
>
>    $ test -z "$x"
>    $ test -z "$x" && test a = b
>
> but this doesn't
>
>    $ test -z "$x" -a a = b
>    bash: test: too many arguments
>
> because it groups "test -n = -a" and is left with "a = b".
>
> Similarly, if $x is "-f", these
>
>    $ test "$x"
>    $ test "$x" || test c = d
>
> correctly adds an implicit "-n", but this fails:
>
>    $ test "$x" -o c = d
>    bash: test: too many arguments
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056
>
>  git-submodule.sh |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index b55d83a..d89e1d0 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -864,7 +864,7 @@ Maybe you want to use 'update --init'?")"
>                 then
>                         subforce=$force
>                         # If we don't already have a -f flag and the submodule has never been checked out
> -                       if test -z "$subsha1" -a -z "$force"
> +                       if test -z "$subsha1" || test -z "$force"
>                         then
>                                 subforce="-f"
>                         fi
> @@ -1059,13 +1059,13 @@ cmd_summary() {
>                 while read mod_src mod_dst sha1_src sha1_dst status sm_path
>                 do
>                         # Always show modules deleted or type-changed (blob<->module)
> -                       test $status = D -o $status = T && echo "$sm_path" && continue
> +                        ( test $status = D || test $status = T ) && echo "$sm_path" && continue
>                         # Respect the ignore setting for --for-status.
>                         if test -n "$for_status"
>                         then
>                                 name=$(module_name "$sm_path")
>                                 ignore_config=$(get_submodule_config "$name" ignore none)
> -                               test $status != A -a $ignore_config = all && continue
> +                               test $status != A && test $ignore_config = all && continue
>                         fi
>                         # Also show added or modified modules which are checked out
>                         GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
> @@ -1125,7 +1125,7 @@ cmd_summary() {
>                 *)
>                         errmsg=
>                         total_commits=$(
> -                       if test $mod_src = 160000 -a $mod_dst = 160000
> +                       if test $mod_src = 160000 && test $mod_dst = 160000
>                         then
>                                 range="$sha1_src...$sha1_dst"
>                         elif test $mod_src = 160000
> @@ -1162,7 +1162,7 @@ cmd_summary() {
>                         # i.e. deleted or changed to blob
>                         test $mod_dst = 160000 && echo "$errmsg"
>                 else
> -                       if test $mod_src = 160000 -a $mod_dst = 160000
> +                       if test $mod_src = 160000 && test $mod_dst = 160000
>                         then
>                                 limit=
>                                 test $summary_limit -gt 0 && limit="-$summary_limit"
> @@ -1233,7 +1233,7 @@ cmd_status()
>                         say "U$sha1 $displaypath"
>                         continue
>                 fi
> -               if test -z "$url" || ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
> +               if test -z "$url" || ! test -d "$sm_path"/.git || test -f "$sm_path"/.git
>                 then
>                         say "-$sha1 $displaypath"
>                         continue;
> @@ -1402,7 +1402,7 @@ then
>  fi
>
>  # "--cached" is accepted only by "status" and "summary"
> -if test -n "$cached" && test "$command" != status -a "$command" != summary
> +if test -n "$cached" && test "$command" != status && test "$command" != summary
>  then
>         usage
>  fi
> --
> 1.7.10.4
>
Jonathan Nieder· May 15, 2014, 15:58 UTC · re: Elia Pinto · lore

Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command

Elia Pinto wrote:
> Even though POSIX.1 lists -a/-o as options to "test", they are
> marked "Obsolescent XSI". Scripts using these expressions
> should be converted  as follow:
[... many lines snipped ...]

This is a very long description, and it doesn't leave me excited by the change.

Is there some potential bug this prevents, or is it just a style fix? If the latter, do we have a way of checking for new examples of the same thing to avoid having to repeat the same patch again in the future?

Are there any platforms that were broken which this fixes? Even posh seems to understand -a and -o.

Nowadays Documentation/CodingGuidelines says
 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.
   "Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up."
   Cf. http://article.gmane.org/gmane.linux.kernel/943020

which I think goes too far (some patterns really are error prone or distracting and it can be worth fixing them tree-wide), but it makes a reasonable case that an idiom not being preferred in the style guide is not *on its own* enough reason to change it.

Perhaps something like the following would work?
	tree-wide: convert test -a/-o to && and ||
	The interaction with unary operators and operator precedence
	for && and || are better known than -a and -o, and for that
	reason we prefer them.  Replace all existing instances in git
	of -a and -o to save readers from the burden of thinking
	about such things.
	Add a check-non-portable-shell.pl to avoid more instances of
	test -a and -o arising in the future.
[...]
> -			test $status = D -o $status = T && echo "$sm_path" && continue
> +			 ( test $status = D || test $status = T ) && echo "$sm_path" && continue
There's no need for a subshell for this.  Better to use a block:
			{
				test "$status" = D ||
				test "$status" = T
			} &&
			echo "$sm_path" &&
			continue
or an if statement:
			if test "$status" = D || test "$status" = T
			then
				echo "$sm_path"
				continue
			fi
or case:
			case $status in
			D|T)
				echo "$sm_path"
				continue
				;;
			esac

Hope that helps, Jonathan

Junio C Hamano· May 16, 2014, 19:09 UTC · re: Jonathan Nieder · lore

Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> Perhaps something like the following would work?
>
> 	tree-wide: convert test -a/-o to && and ||
>
> 	The interaction with unary operators and operator precedence
> 	for && and || are better known than -a and -o, and for that
> 	reason we prefer them.  Replace all existing instances in git
> 	of -a and -o to save readers from the burden of thinking
> 	about such things.
>
> 	Add a check-non-portable-shell.pl to avoid more instances of
> 	test -a and -o arising in the future.

Yeah, the title is certainly better than "-a or -b option" I see above ;-) and a single tree-wide fix may be OK while the tree is quiescent.

I however do think "better known" is much less of an issue than that "-a/-o" is more error prone e.g. 'test -n "$x" -a a = b' is buggy because it does not consider that $x could be "=".

Show 12 quoted lines
> [...]
>> -			test $status = D -o $status = T && echo "$sm_path" && continue
>> +			 ( test $status = D || test $status = T ) && echo "$sm_path" && continue
>
> There's no need for a subshell for this.  Better to use a block:
>
> 			{
> 				test "$status" = D ||
> 				test "$status" = T
> 			} &&
> 			echo "$sm_path" &&
> 			continue
Yes.

← back to recent threads