# [PATCH 01/20] check_bindir: avoid "test <cond> -a/-o <cond>"

35 messages from 2014-06-06 to 2014-06-10. Participants: Elia Pinto, Torsten Bögershausen, Matthieu Moy, Junio C Hamano, Johannes Sixt, David Aguilar, René Scharfe.
Thread: https://gitlist.dev/t/36857

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 00/20] avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-1-git-send-email-gitter.spiros%40gmail.com

```
These patch series  convert test -a/-o to && and ||.

This is the the third version.

Changes:

- Change the commit message for consistency with the comment to the patch to the CodingGuideline (Junio proposed patch
 http://www.spinics.net/lists/git/msg232199.html) expressing even better the meaning 
 of the patches ( in particular, the phrase "Better known" vs. "error prone").
 The previous comment was "convert test -a/-o to && and ||"


- Therefore also includes the Julio patch to the CodingGuideline


I have done the rebase with "master" and the patches also apply to "next".

I hope I have done the right thing in  resending the patch series.
I am Sorry if not.


Elia Pinto (20):
  check_bindir: avoid "test <cond> -a/-o <cond>"
  contrib/examples/git-clone.sh: avoid "test <cond> -a/-o <cond>"
  contrib/examples/git-commit.sh: avoid "test <cond> -a/-o <cond>"
  contrib/examples/git-merge.sh: avoid "test <cond> -a/-o <cond>"
  contrib/examples/git-repack.sh: avoid "test <cond> -a/-o <cond>"
  contrib/examples/git-resolve.sh: avoid "test <cond> -a/-o <cond>"
  git-bisect.sh: avoid "test <cond> -a/-o <cond>"
  git-mergetool.sh: avoid "test <cond> -a/-o <cond>"
  git-rebase--interactive.sh: avoid "test <cond> -a/-o <cond>"
  git-submodule.sh: avoid "test <cond> -a/-o <cond>"
  t/lib-httpd.sh: avoid "test <cond> -a/-o <cond>"
  t/t0025-crlf-auto.sh: avoid "test <cond> -a/-o <cond>"
  t/t0026-eol-config.sh: avoid "test <cond> -a/-o <cond>"
  t/t4102-apply-rename.sh: avoid "test <cond> -a/-o <cond>"
  t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"
  t/t5403-post-checkout-hook.sh: avoid "test <cond> -a/-o <cond>"
  t/t5538-push-shallow.sh: avoid "test <cond> -a/-o <cond>"
  t/t9814-git-p4-rename.sh: avoid "test <cond> -a/-o <cond>"
  t/test-lib-functions.sh: avoid "test <cond> -a/-o <cond>"
  CodingGuidelines: avoid "test <cond> -a/-o <cond>"

 Documentation/CodingGuidelines  |   12 ++++++++++++
 check_bindir                    |    2 +-
 contrib/examples/git-clone.sh   |    2 +-
 contrib/examples/git-commit.sh  |    4 ++--
 contrib/examples/git-merge.sh   |    4 ++--
 contrib/examples/git-repack.sh  |    4 ++--
 contrib/examples/git-resolve.sh |    2 +-
 git-bisect.sh                   |    2 +-
 git-mergetool.sh                |    4 ++--
 git-rebase--interactive.sh      |    2 +-
 git-submodule.sh                |   32 ++++++++++++++++++++------------
 t/lib-httpd.sh                  |    2 +-
 t/t0025-crlf-auto.sh            |    6 +++---
 t/t0026-eol-config.sh           |    8 ++++----
 t/t4102-apply-rename.sh         |    2 +-
 t/t5000-tar-tree.sh             |    2 +-
 t/t5403-post-checkout-hook.sh   |    8 ++++----
 t/t5538-push-shallow.sh         |    2 +-
 t/t9814-git-p4-rename.sh        |    4 ++--
 t/test-lib-functions.sh         |    4 ++--
 20 files changed, 64 insertions(+), 44 deletions(-)

-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 01/20] check_bindir: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-2-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-2-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 check_bindir |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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, 2014-06-06 14:55

Subject: [PATCH 02/20] contrib/examples/git-clone.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-3-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-3-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 contrib/examples/git-clone.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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, 2014-06-06 14:55

Subject: [PATCH 03/20] contrib/examples/git-commit.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-4-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-4-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 contrib/examples/git-commit.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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, 2014-06-06 14:55

Subject: [PATCH 04/20] contrib/examples/git-merge.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-5-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-5-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 contrib/examples/git-merge.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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, 2014-06-06 14:55

Subject: [PATCH 05/20] contrib/examples/git-repack.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-6-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-6-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 contrib/examples/git-repack.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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, 2014-06-06 14:55

Subject: [PATCH 06/20] contrib/examples/git-resolve.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-7-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-7-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 contrib/examples/git-resolve.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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, 2014-06-06 14:55

Subject: [PATCH 07/20] git-bisect.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-8-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-8-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 git-bisect.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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, 2014-06-06 14:55

Subject: [PATCH 08/20] git-mergetool.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-9-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-9-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 git-mergetool.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index d08dc92..9a046b7 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

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 09/20] git-rebase--interactive.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-10-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-10-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 git-rebase--interactive.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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, 2014-06-06 14:55

Subject: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-11-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-11-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 git-submodule.sh |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index b55d83a..1e3a5a6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -396,7 +396,7 @@ cmd_add()
 			sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
 	fi
 
-	if test -z "$repo" -o -z "$sm_path"; then
+	if test -z "$repo" || test -z "$sm_path"; then
 		usage
 	fi
 
@@ -453,7 +453,7 @@ Use -f if you really want to add it." >&2
 	# perhaps the path exists and is already a git repo, else clone it
 	if test -e "$sm_path"
 	then
-		if test -d "$sm_path"/.git -o -f "$sm_path"/.git
+		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
 		then
 			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
 		else
@@ -835,7 +835,7 @@ Maybe you want to use 'update --init'?")"
 			continue
 		fi
 
-		if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
+		if ! test -d "$sm_path"/.git || test -f "$sm_path"/.git
 		then
 			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
 			cloned_modules="$cloned_modules;$name"
@@ -860,11 +860,11 @@ Maybe you want to use 'update --init'?")"
 			die "$(eval_gettext "Unable to find current ${remote_name}/${branch} revision in submodule path '\$sm_path'")"
 		fi
 
-		if test "$subsha1" != "$sha1" -o -n "$force"
+		if test "$subsha1" != "$sha1" || test -n "$force"
 		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
@@ -1034,7 +1034,7 @@ cmd_summary() {
 	then
 		head=$rev
 		test $# = 0 || shift
-	elif test -z "$1" -o "$1" = "HEAD"
+	elif test -z "$1" || test "$1" = "HEAD"
 	then
 		# before the first commit: compare with an empty tree
 		head=$(git hash-object -w -t tree --stdin </dev/null)
@@ -1059,13 +1059,17 @@ 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
+			case "$status" in
+			[DT])
+				printf '%s\n' "$sm_path" &&
+				continue
+			esac
 			# 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 +1129,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 +1166,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 +1237,11 @@ 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 +1410,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, 2014-06-06 14:55

Subject: [PATCH 11/20] t/lib-httpd.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-12-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-12-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/lib-httpd.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 252cbf1..38a47fe 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,7 +132,7 @@ prepare_httpd() {
 	HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST
 	HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST
 
-	if test -n "$LIB_HTTPD_DAV" -o -n "$LIB_HTTPD_SVN"
+	if test -n "$LIB_HTTPD_DAV" || test -n "$LIB_HTTPD_SVN"
 	then
 		HTTPD_PARA="$HTTPD_PARA -DDAV"
 
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 12/20] t/t0025-crlf-auto.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-13-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-13-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t0025-crlf-auto.sh |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index b0e5694..28102de 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -36,7 +36,7 @@ test_expect_success 'default settings cause no changes' '
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
 	threediff=$(git diff three) &&
-	test -z "$onediff" -a -z "$twodiff" -a -z "$threediff"
+	test -z "$onediff" && test -z "$twodiff" && test -z "$threediff"
 '
 
 test_expect_success 'crlf=true causes a CRLF file to be normalized' '
@@ -111,7 +111,7 @@ test_expect_success 'autocrlf=true does not normalize CRLF files' '
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
 	threediff=$(git diff three) &&
-	test -z "$onediff" -a -z "$twodiff" -a -z "$threediff"
+	test -z "$onediff" && test -z "$twodiff" && test -z "$threediff"
 '
 
 test_expect_success 'text=auto, autocrlf=true _does_ normalize CRLF files' '
@@ -126,7 +126,7 @@ test_expect_success 'text=auto, autocrlf=true _does_ normalize CRLF files' '
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
 	threediff=$(git diff three) &&
-	test -z "$onediff" -a -n "$twodiff" -a -z "$threediff"
+	test -z "$onediff" && test -n "$twodiff" && test -z "$threediff"
 '
 
 test_expect_success 'text=auto, autocrlf=true does not normalize binary files' '
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 13/20] t/t0026-eol-config.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-14-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-14-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t0026-eol-config.sh |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t0026-eol-config.sh b/t/t0026-eol-config.sh
index e1126aa..4807b0f 100755
--- a/t/t0026-eol-config.sh
+++ b/t/t0026-eol-config.sh
@@ -36,7 +36,7 @@ test_expect_success 'eol=lf puts LFs in normalized file' '
 	! has_cr two &&
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
-	test -z "$onediff" -a -z "$twodiff"
+	test -z "$onediff" && test -z "$twodiff"
 '
 
 test_expect_success 'eol=crlf puts CRLFs in normalized file' '
@@ -49,7 +49,7 @@ test_expect_success 'eol=crlf puts CRLFs in normalized file' '
 	! has_cr two &&
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
-	test -z "$onediff" -a -z "$twodiff"
+	test -z "$onediff" && test -z "$twodiff"
 '
 
 test_expect_success 'autocrlf=true overrides eol=lf' '
@@ -63,7 +63,7 @@ test_expect_success 'autocrlf=true overrides eol=lf' '
 	has_cr two &&
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
-	test -z "$onediff" -a -z "$twodiff"
+	test -z "$onediff" && test -z "$twodiff"
 '
 
 test_expect_success 'autocrlf=true overrides unset eol' '
@@ -77,7 +77,7 @@ test_expect_success 'autocrlf=true overrides unset eol' '
 	has_cr two &&
 	onediff=$(git diff one) &&
 	twodiff=$(git diff two) &&
-	test -z "$onediff" -a -z "$twodiff"
+	test -z "$onediff" && test -z "$twodiff"
 '
 
 test_done
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 14/20] t/t4102-apply-rename.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-15-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-15-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t4102-apply-rename.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4102-apply-rename.sh b/t/t4102-apply-rename.sh
index 49e2d6c..fae3059 100755
--- a/t/t4102-apply-rename.sh
+++ b/t/t4102-apply-rename.sh
@@ -52,6 +52,6 @@ EOF
 
 test_expect_success 'apply copy' \
     'git apply --index --stat --summary --apply test-patch &&
-     test "$(cat bar)" = "This is bar" -a "$(cat foo)" = "This is foo"'
+     test "$(cat bar)" = "This is bar" && test "$(cat foo)" = "This is foo"'
 
 test_done
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-16-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-16-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t5000-tar-tree.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 74fc5a8..ad6fa0d 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -72,7 +72,7 @@ check_tar() {
 			for header in *.paxheader
 			do
 				data=${header%.paxheader}.data &&
-				if test -h $data -o -e $data
+				if test -h $data || test -e $data
 				then
 					path=$(get_pax_header $header path) &&
 					if test -n "$path"
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:55

Subject: [PATCH 16/20] t/t5403-post-checkout-hook.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-17-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-17-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t5403-post-checkout-hook.sh |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 1753ef2..fc898c9 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -39,7 +39,7 @@ test_expect_success 'post-checkout receives the right arguments with HEAD unchan
 	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
-	test $old = $new -a $flag = 1
+	test $old = $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout runs as expected ' '
@@ -52,7 +52,7 @@ test_expect_success 'post-checkout args are correct with git checkout -b ' '
 	old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
-	test $old = $new -a $flag = 1
+	test $old = $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout receives the right args with HEAD changed ' '
@@ -60,7 +60,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
 	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
-	test $old != $new -a $flag = 1
+	test $old != $new && test $flag = 1
 '
 
 test_expect_success 'post-checkout receives the right args when not switching branches ' '
@@ -68,7 +68,7 @@ test_expect_success 'post-checkout receives the right args when not switching br
 	old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
 	new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
 	flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
-	test $old = $new -a $flag = 0
+	test $old = $new && test $flag = 0
 '
 
 if test "$(git config --bool core.filemode)" = true; then
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:56

Subject: [PATCH 17/20] t/t5538-push-shallow.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-18-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-18-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t5538-push-shallow.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5538-push-shallow.sh b/t/t5538-push-shallow.sh
index 8e54ac5..63d9ca9 100755
--- a/t/t5538-push-shallow.sh
+++ b/t/t5538-push-shallow.sh
@@ -121,7 +121,7 @@ EOF
 	)
 '
 
-if test -n "$NO_CURL" -o -z "$GIT_TEST_HTTPD"; then
+if test -n "$NO_CURL" || test -z "$GIT_TEST_HTTPD"; then
 	say 'skipping remaining tests, git built without http support'
 	test_done
 fi
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:56

Subject: [PATCH 18/20] t/t9814-git-p4-rename.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-19-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-19-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/t9814-git-p4-rename.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index be802e0..1fc1f5f 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -177,7 +177,7 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
 		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-		test "$src" = file10 -o "$src" = file11 &&
+		test "$src" = file10 || test "$src" = file11 &&
 		git config git-p4.detectCopies $(($level + 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file12 &&
@@ -191,7 +191,7 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
 		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-		test "$src" = file10 -o "$src" = file11 -o "$src" = file12 &&
+		test "$src" = file10 || test "$src" = file11 || test "$src" = file12 &&
 		git config git-p4.detectCopies $(($level - 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file13 &&
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:56

Subject: [PATCH 19/20] t/test-lib-functions.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-20-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-20-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 t/test-lib-functions.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 158e10a..0681003 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -542,7 +542,7 @@ test_must_fail () {
 	if test $exit_code = 0; then
 		echo >&2 "test_must_fail: command succeeded: $*"
 		return 1
-	elif test $exit_code -gt 129 -a $exit_code -le 192; then
+	elif test $exit_code -gt 129 && test $exit_code -le 192; then
 		echo >&2 "test_must_fail: died by signal: $*"
 		return 1
 	elif test $exit_code = 127; then
@@ -569,7 +569,7 @@ test_must_fail () {
 test_might_fail () {
 	"$@"
 	exit_code=$?
-	if test $exit_code -gt 129 -a $exit_code -le 192; then
+	if test $exit_code -gt 129 && test $exit_code -le 192; then
 		echo >&2 "test_might_fail: died by signal: $*"
 		return 1
 	elif test $exit_code = 127; then
-- 
1.7.10.4

```

## Elia Pinto, 2014-06-06 14:56

Subject: [PATCH 20/20] CodingGuidelines: avoid "test <cond> -a/-o <cond>"
Message-ID: <1402066563-28519-21-git-send-email-gitter.spiros@gmail.com>
URL: https://gitlist.dev/e/1402066563-28519-21-git-send-email-gitter.spiros%40gmail.com
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
The construct is error-prone; "test" being built-in in most modern
shells, the reason to avoid "test <cond> && test <cond>" spawning
one extra process by using a single "test <cond> -a <cond>" no
longer exists.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 Documentation/CodingGuidelines |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index f424dbd..2d426bc 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -106,6 +106,18 @@ For shell scripts specifically (not exhaustive):
    interface translatable. See "Marking strings for translation" in
    po/README.
 
+ - We do not write our "test" command with "-a" and "-o" and use "&&"
+   or "||" to concatenate multiple "test" commands instead, because
+   the use of "-a/-o" is often error-prone.  E.g.
+
+     test -n "$x" -a "$a" = "$b"
+
+   is buggy and breaks when $x is "=", but
+
+     test -n "$x" && test "$a" = "$b"
+
+   does not have such a problem.
+
 For C programs:
 
  - We use tabs to indent, and interpret tabs as taking up to
-- 
1.7.10.4

```

## Torsten Bögershausen, 2014-06-06 19:04

Subject: Re: [PATCH 20/20] CodingGuidelines: avoid "test <cond> -a/-o <cond>"
Message-ID: <539210A8.5040003@web.de>
URL: https://gitlist.dev/e/539210A8.5040003%40web.de
In-Reply-To: <1402066563-28519-21-git-send-email-gitter.spiros@gmail.com>

```
On 2014-06-06 16.56, Elia Pinto wrote:
> + - We do not write our "test" command with "-a" and "-o" and use "&&"
That "and" "and" "and" could be somewhat confusing.
How about:

We do not write our "test" command with "-a" and/or "-o".
Instead we use "&&" or "||" to concatenate multiple "test" commands instead,
because the use of "-a/-o" is often error-prone.  E.g.
(The rest looks OK)

```

## Matthieu Moy, 2014-06-09 13:31

Subject: Re: [PATCH 00/20] avoid "test <cond> -a/-o <cond>"
Message-ID: <vpq1tuyw6nt.fsf@anie.imag.fr>
URL: https://gitlist.dev/e/vpq1tuyw6nt.fsf%40anie.imag.fr
In-Reply-To: <1402066563-28519-1-git-send-email-gitter.spiros@gmail.com>

```
Elia Pinto <gitter.spiros@gmail.com> writes:

> These patch series  convert test -a/-o to && and ||.

Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

```

## Junio C Hamano, 2014-06-09 23:23

Subject: Re: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <xmqqoay1d5vi.fsf@gitster.dls.corp.google.com>
URL: https://gitlist.dev/e/xmqqoay1d5vi.fsf%40gitster.dls.corp.google.com
In-Reply-To: <1402066563-28519-11-git-send-email-gitter.spiros@gmail.com>

```
Elia Pinto <gitter.spiros@gmail.com> writes:

> @@ -1059,13 +1059,17 @@ 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
> +			case "$status" in
> +			[DT])
> +				printf '%s\n' "$sm_path" &&
> +				continue
> +			esac

I think this conversion is wrong and causes parse error.  The
surrounding code cannot be seen in the context of thsi patch, but
looks somewhat like this:

	modules=$( ....
                   case "$status" in
                   [DT])
                           ...
                   esac
                   .... )

Perhaps you would need to spell it with the extra opening
parenthesis, like so:

	case string in
        ([DT])
        	...
	esac

or something.

```

## Junio C Hamano, 2014-06-09 23:23

Subject: Re: [PATCH 02/20] contrib/examples/git-clone.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <xmqqha3td5v5.fsf@gitster.dls.corp.google.com>
URL: https://gitlist.dev/e/xmqqha3td5v5.fsf%40gitster.dls.corp.google.com
In-Reply-To: <1402066563-28519-3-git-send-email-gitter.spiros@gmail.com>

```
Elia Pinto <gitter.spiros@gmail.com> writes:

> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  contrib/examples/git-clone.sh |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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

Hmph.  If we want to see them both empty, 

	test "$quiet,$no_progress" = ,

would have been a better way to spell this, but that is outside the
scope of this series.

But I wonder if we really want to update the contrib/examples/,
which is a record of how historically we have implemented various
scripted Porcelains using lower level plumbing commands.

```

## Johannes Sixt, 2014-06-10 06:52

Subject: Re: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <5396AB10.8020603@viscovery.net>
URL: https://gitlist.dev/e/5396AB10.8020603%40viscovery.net
In-Reply-To: <xmqqoay1d5vi.fsf@gitster.dls.corp.google.com>

```
Am 6/10/2014 1:23, schrieb Junio C Hamano:
> Elia Pinto <gitter.spiros@gmail.com> writes:
> 
>> @@ -1059,13 +1059,17 @@ 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
>> +			case "$status" in
>> +			[DT])
>> +				printf '%s\n' "$sm_path" &&
>> +				continue
>> +			esac
> 
> I think this conversion is wrong and causes parse error.  The
> surrounding code cannot be seen in the context of thsi patch, but
> looks somewhat like this:
> 
> 	modules=$( ....
>                    case "$status" in
>                    [DT])
>                            ...
>                    esac
>                    .... )
> 
> Perhaps you would need to spell it with the extra opening
> parenthesis, like so:
> 
> 	case string in
>         ([DT])
>         	...
> 	esac
> 
> or something.

Do you just think that it causes parse errors or did you actually observe
them? Because I think that no parse error should occur.

-- Hannes

```

## David Aguilar, 2014-06-10 07:37

Subject: Re: [PATCH 08/20] git-mergetool.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <CAJDDKr5dvsChQDk+0qJS8qJq+jr6XggWFaveFkaUj0T2OiGSpA@mail.gmail.com>
URL: https://gitlist.dev/e/CAJDDKr5dvsChQDk%2B0qJS8qJq%2Bjr6XggWFaveFkaUj0T2OiGSpA%40mail.gmail.com
In-Reply-To: <1402066563-28519-9-git-send-email-gitter.spiros@gmail.com>

```
On Fri, Jun 6, 2014 at 7:55 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:
> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---

This looks good to me.  Thanks Elia,

Acked-by: David Aguilar <davvid@gmail.com>

>  git-mergetool.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index d08dc92..9a046b7 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
David

```

## Johannes Sixt, 2014-06-10 08:19

Subject: Re: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <5396BFAA.3000003@viscovery.net>
URL: https://gitlist.dev/e/5396BFAA.3000003%40viscovery.net
In-Reply-To: <5396AB10.8020603@viscovery.net>

```
Am 6/10/2014 8:52, schrieb Johannes Sixt:
> Am 6/10/2014 1:23, schrieb Junio C Hamano:
>> Elia Pinto <gitter.spiros@gmail.com> writes:
>>
>>> @@ -1059,13 +1059,17 @@ 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
>>> +			case "$status" in
>>> +			[DT])
>>> +				printf '%s\n' "$sm_path" &&
>>> +				continue
>>> +			esac
>>
>> I think this conversion is wrong and causes parse error.  The
>> surrounding code cannot be seen in the context of thsi patch, but
>> looks somewhat like this:
>>
>> 	modules=$( ....
>>                    case "$status" in
>>                    [DT])
>>                            ...
>>                    esac
>>                    .... )
>>
>> Perhaps you would need to spell it with the extra opening
>> parenthesis, like so:
>>
>> 	case string in
>>         ([DT])
>>         	...
>> 	esac
>>
>> or something.
> 
> Do you just think that it causes parse errors or did you actually observe
> them? Because I think that no parse error should occur.

(I should not talk, but test...) bash and zsh get it wrong, dash and ksh
get it right.
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_03
item 5 does leave some leeway for interpretation. So it's better to adjust
as you suggest.

-- Hannes
-- 
"Atomic objects are neither active nor radioactive." --
Programming Languages -- C++, Final Committee Draft (Doc.N3092)

```

## David Aguilar, 2014-06-10 18:36

Subject: Re: [PATCH 03/20] contrib/examples/git-commit.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <20140610183638.GA72751@gmail.com>
URL: https://gitlist.dev/e/20140610183638.GA72751%40gmail.com
In-Reply-To: <1402066563-28519-4-git-send-email-gitter.spiros@gmail.com>

```
On Fri, Jun 06, 2014 at 07:55:46AM -0700, Elia Pinto wrote:
> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  contrib/examples/git-commit.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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

It might be worth moving the "then" to the next line so that it's
consistent with the preferred sh style and with the rest of the script.

If we do that then there's one less line that would need to be touched
by a future style-fix patch.

> @@ -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

-- 
David

```

## David Aguilar, 2014-06-10 18:39

Subject: Re: [PATCH 05/20] contrib/examples/git-repack.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <20140610183900.GB72751@gmail.com>
URL: https://gitlist.dev/e/20140610183900.GB72751%40gmail.com
In-Reply-To: <1402066563-28519-6-git-send-email-gitter.spiros@gmail.com>

```
On Fri, Jun 06, 2014 at 07:55:48AM -0700, Elia Pinto wrote:
> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  contrib/examples/git-repack.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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

I do not think the traling "\" is needed anymore; the "&&" takes care of it.
-- 
David

```

## David Aguilar, 2014-06-10 18:43

Subject: Re: [PATCH 09/20] git-rebase--interactive.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <20140610184346.GC72751@gmail.com>
URL: https://gitlist.dev/e/20140610184346.GC72751%40gmail.com
In-Reply-To: <1402066563-28519-10-git-send-email-gitter.spiros@gmail.com>

```
On Fri, Jun 06, 2014 at 07:55:52AM -0700, Elia Pinto wrote:
> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  git-rebase--interactive.sh |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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

This line is getting pretty long.
I wonder whether it should be wrapped at the && to keep it shorter?

Also, it looks like the last half of the expression can be simplified to,

	test -z "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)"

rather than comparing against "".
-- 
David

```

## David Aguilar, 2014-06-10 18:49

Subject: Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <20140610184913.GD72751@gmail.com>
URL: https://gitlist.dev/e/20140610184913.GD72751%40gmail.com
In-Reply-To: <1402066563-28519-16-git-send-email-gitter.spiros@gmail.com>

```
On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  t/t5000-tar-tree.sh |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 74fc5a8..ad6fa0d 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -72,7 +72,7 @@ check_tar() {
>  			for header in *.paxheader
>  			do
>  				data=${header%.paxheader}.data &&
> -				if test -h $data -o -e $data
> +				if test -h $data || test -e $data
>  				then

This looks okay, but it raises a question for the original author
(René, I think that's you so I've added you to the To: line).

Should that be "test -f" instead of "test -e"?

This is a very minor note and should not block this patch.
-- 
David

```

## David Aguilar, 2014-06-10 19:21

Subject: Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <20140610192110.GA72977@gmail.com>
URL: https://gitlist.dev/e/20140610192110.GA72977%40gmail.com
In-Reply-To: <20140610184913.GD72751@gmail.com>

```
On Tue, Jun 10, 2014 at 11:49:15AM -0700, David Aguilar wrote:
> On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
> > The construct is error-prone; "test" being built-in in most modern
> > shells, the reason to avoid "test <cond> && test <cond>" spawning
> > one extra process by using a single "test <cond> -a <cond>" no
> > longer exists.
> > 
> > Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> > ---
> >  t/t5000-tar-tree.sh |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> > index 74fc5a8..ad6fa0d 100755
> > --- a/t/t5000-tar-tree.sh
> > +++ b/t/t5000-tar-tree.sh
> > @@ -72,7 +72,7 @@ check_tar() {
> >  			for header in *.paxheader
> >  			do
> >  				data=${header%.paxheader}.data &&
> > -				if test -h $data -o -e $data
> > +				if test -h $data || test -e $data
> >  				then
> 
> This looks okay, but it raises a question for the original author
> (René, I think that's you so I've added you to the To: line).

Just following up -- I got a bounce from René's email address.

> 
> Should that be "test -f" instead of "test -e"?

It does seem like this should be "test -f" nonetheless.
Sorry for the noise.

> This is a very minor note and should not block this patch.
-- 
David

```

## David Aguilar, 2014-06-10 20:08

Subject: Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <20140610200804.GD72977@gmail.com>
URL: https://gitlist.dev/e/20140610200804.GD72977%40gmail.com
In-Reply-To: <1402066563-28519-16-git-send-email-gitter.spiros@gmail.com>

```
[Resent using René's correct email address this time, sorry for the noise]

On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
> The construct is error-prone; "test" being built-in in most modern
> shells, the reason to avoid "test <cond> && test <cond>" spawning
> one extra process by using a single "test <cond> -a <cond>" no
> longer exists.
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  t/t5000-tar-tree.sh |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 74fc5a8..ad6fa0d 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -72,7 +72,7 @@ check_tar() {
>  			for header in *.paxheader
>  			do
>  				data=${header%.paxheader}.data &&
> -				if test -h $data -o -e $data
> +				if test -h $data || test -e $data
>  				then

This looks okay, but it raises a question for the original author
(René, I think that's you so I've added you to the To: line).

Should that be "test -f" instead of "test -e"?

This is a very minor note and should not block this patch.
It's probably a change that's better made in a follow-up patch.

>  					path=$(get_pax_header $header path) &&
>  					if test -n "$path"
-- 
David

```

## René Scharfe, 2014-06-10 21:47

Subject: Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"
Message-ID: <53977D05.5070505@web.de>
URL: https://gitlist.dev/e/53977D05.5070505%40web.de
In-Reply-To: <20140610200804.GD72977@gmail.com>

```
Am 10.06.2014 22:08, schrieb David Aguilar:
> [Resent using René's correct email address this time, sorry for the noise]
>
> On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
>> The construct is error-prone; "test" being built-in in most modern
>> shells, the reason to avoid "test <cond> && test <cond>" spawning
>> one extra process by using a single "test <cond> -a <cond>" no
>> longer exists.
>>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>> ---
>>   t/t5000-tar-tree.sh |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
>> index 74fc5a8..ad6fa0d 100755
>> --- a/t/t5000-tar-tree.sh
>> +++ b/t/t5000-tar-tree.sh
>> @@ -72,7 +72,7 @@ check_tar() {
>>   			for header in *.paxheader
>>   			do
>>   				data=${header%.paxheader}.data &&
>> -				if test -h $data -o -e $data
>> +				if test -h $data || test -e $data
>>   				then
>
> This looks okay, but it raises a question for the original author
> (René, I think that's you so I've added you to the To: line).
>
> Should that be "test -f" instead of "test -e"?

With -f instead of -e the function would ignore pax path headers for 
directories and special files.  The latter is not relevant for git at 
all and we don't currently have a test for long directory names, but why 
restrict the code to handle only regular files?

A better change would be adding tests for symlinks and directories with 
long names.

>
> This is a very minor note and should not block this patch.
> It's probably a change that's better made in a follow-up patch.
>
>>   					path=$(get_pax_header $header path) &&
>>   					if test -n "$path"

```