[PATCH 00/19] convert test -a/-o to && and || patch series

[PATCH 00/19] convert test -a/-o to && and || patch series

[Elia Pinto]

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

This is the second version.

Changes:

- Modified commit comment based on Jonathan Nieder suggestions
(was "don't use the -a or -o option with the test command")

- Modified patch on git-submodule.sh based on Jonathan Nieder suggestions


Elia Pinto (19):
  check_bindir: convert test -a/-o to && and ||
  contrib/examples/git-clone.sh: convert test -a/-o to && and ||
  contrib/examples/git-commit.sh: convert test -a/-o to && and ||
  contrib/examples/git-merge.sh: convert test -a/-o to && and ||
  contrib/examples/git-repack.sh: convert test -a/-o to && and ||
  contrib/examples/git-resolve.sh: convert test -a/-o to && and ||
  git-bisect.sh: convert test -a/-o to && and ||
  git-mergetool.sh: convert test -a/-o to && and ||
  git-rebase--interactive.sh: convert test -a/-o to && and ||
  git-submodule.sh: convert test -a/-o to && and ||
  t/t0025-crlf-auto.sh: convert test -a/-o to && and ||
  t/t0026-eol-config.sh: convert test -a/-o to && and ||
  t/t4102-apply-rename.sh: convert test -a/-o to && and ||
  t/t5000-tar-tree.sh: convert test -a/-o to && and ||
  t/t5403-post-checkout-hook.sh: convert test -a/-o to && and ||
  t/t5537-fetch-shallow.sh: convert test -a/-o to && and ||
  t/t5538-push-shallow.sh: convert test -a/-o to && and ||
  t/t9814-git-p4-rename.sh: convert test -a/-o to && and ||
  t/test-lib-functions.sh: convert test -a/-o to && and ||

 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                |   29 +++++++++++++++++------------
 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/t5537-fetch-shallow.sh        |    2 +-
 t/t5538-push-shallow.sh         |    2 +-
 t/t9814-git-p4-rename.sh        |    4 ++--
 t/test-lib-functions.sh         |    4 ++--
 19 files changed, 49 insertions(+), 44 deletions(-)

-- 
1.7.10.4

[PATCH 01/19] check_bindir: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

Re: [PATCH 01/19] check_bindir: convert test -a/-o to && and ||

[Junio C Hamano]

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

> 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
> of -a and -o to save readers from the burden of thinking
> about such things.
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---

Thanks.

As I already said, I think "better known" is much less of an issue
than that "-a/-o" is "more error prone", and that is the reason why
we may want to do this rewrite.

I do not know offhand how busy the tree would be when we can apply
these patches post-release without them getting rebased, but the
zero-th step before this series may want to be a patch like this.

 Documentation/CodingGuidelines | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index ef67b53..7864c5b 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -106,6 +106,19 @@ 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

Re: [PATCH 01/19] check_bindir: convert test -a/-o to && and ||

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As I already said, I think "better known" is much less of an issue
> than that "-a/-o" is "more error prone", and that is the reason why
> we may want to do this rewrite.
>
> I do not know offhand how busy the tree would be when we can apply
> these patches post-release without them getting rebased, but the
> zero-th step before this series may want to be a patch like this.

... and this time with a proposed log message.

-- >8 --
Subject: [PATCH] CodingGuidelines: avoid "test <cond> -a/-o <cond>"

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>
---
 Documentation/CodingGuidelines | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 3d08671..4d90c77 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -151,6 +151,19 @@ 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
-- 
2.0.0-rc3-438-g36dae77

[PATCH 02/19] contrib/examples/git-clone.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 03/19] contrib/examples/git-commit.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 04/19] contrib/examples/git-merge.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 05/19] contrib/examples/git-repack.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 06/19] contrib/examples/git-resolve.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 07/19] git-bisect.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 08/19] git-mergetool.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 09/19] git-rebase--interactive.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 10/19] git-submodule.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

diff --git a/git-submodule.sh b/git-submodule.sh
index b55d83a..4b57b96 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,18 @@ 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 +1130,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 +1167,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 +1238,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 +1407,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

Re: [PATCH 10/19] git-submodule.sh: convert test -a/-o to && and ||

[Matthieu Moy]

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

> -			test $status = D -o $status = T && echo "$sm_path" && continue
> +			{
> +			test "$status" = D ||
> +			test "$status" = T
> +			} &&
> +			echo "$sm_path"
> +			&& continue

We usually write the && at the end of the line, hence that would be

echo "$sm_path" &&
continue

(shouldn't block the patch, but you may change if you need to resend)

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

Re: [PATCH 10/19] git-submodule.sh: convert test -a/-o to && and ||

[Johannes Sixt]

Am 5/20/2014 15:50, schrieb Elia Pinto:
>  			# 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"

Should not be ||, but &&!

>  		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

As Matthieu noted, this is incorrect. It's not just a style violation,
it's a syntax error. Why did your test runs not hickup on that?

In this case you could even leave the original code structure without
changing the meaning:

			test $status = D || test $status = T && echo "$sm_path" && continue

But a better idiom is
			case "$status" in
			[DT])
				printf '%s\n' "$sm_path" &&
				continue
			esac

> @@ -1233,7 +1238,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

Wrong grouping. This could be more correct (I didn't test):

		if test -z "$url" ||
			{
				! test -d "$sm_path"/.git &&
				! test -f "$sm_path"/.git
			}

-- Hannes

[PATCH 11/19] t/t0025-crlf-auto.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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 f5f67a6..a13fc56 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

[PATCH 12/19] t/t0026-eol-config.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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 fe0164b..961ee32 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

[PATCH 13/19] t/t4102-apply-rename.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 14/19] t/t5000-tar-tree.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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 1cf0a4e..4b57cbd 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

[PATCH 15/19] t/t5403-post-checkout-hook.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 16/19] t/t5537-fetch-shallow.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index be951a4..2c75730 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,7 +173,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

[PATCH 17/19] t/t5538-push-shallow.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 18/19] t/t9814-git-p4-rename.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

[PATCH 19/19] t/test-lib-functions.sh: convert test -a/-o to && and ||

[Elia Pinto]

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
of -a and -o to save readers from the burden of thinking
about such things.

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

Re: [PATCH 00/19] convert test -a/-o to && and || patch series

[Matthieu Moy]

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

> Elia Pinto (19):

I went through the series (not very thoroughly) and it sounds good to
me.

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

Re: [PATCH 00/19] convert test -a/-o to && and || patch series

[Junio C Hamano]

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

> These patch series  convert test -a/-o to && and ||.
>
> This is the second version.

Perhaps slightly off-topic, but has the remainder of the previous $(...)
series been perfected and ready to apply?

Re: [PATCH 00/19] convert test -a/-o to && and || patch series

[Jonathan Nieder]

Hi,

Elia Pinto wrote:

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

Should this come with a new check in t/check-non-portable-shell.pl so
we don't regress in the future?

> Elia Pinto (19):
[...]
>  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                |   29 +++++++++++++++++------------
>  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/t5537-fetch-shallow.sh        |    2 +-
>  t/t5538-push-shallow.sh         |    2 +-
>  t/t9814-git-p4-rename.sh        |    4 ++--
>  t/test-lib-functions.sh         |    4 ++--
>  19 files changed, 49 insertions(+), 44 deletions(-)

I still think one patch per file is too many patches for this.  It would
be easier to read with, e.g., whichever ones were most complex as
separate patches and the rest (the "easy" ones) as a single patch.

Thanks,
Jonathan