[PATCH 00/20] avoid "test <cond> -a/-o <cond>"

[PATCH 00/20] avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

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

[Elia Pinto]

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

[PATCH 02/20] contrib/examples/git-clone.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 03/20] contrib/examples/git-commit.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 04/20] contrib/examples/git-merge.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 05/20] contrib/examples/git-repack.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 06/20] contrib/examples/git-resolve.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 07/20] git-bisect.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 08/20] git-mergetool.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 09/20] git-rebase--interactive.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 11/20] t/lib-httpd.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 12/20] t/t0025-crlf-auto.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 13/20] t/t0026-eol-config.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 14/20] t/t4102-apply-rename.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 16/20] t/t5403-post-checkout-hook.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 17/20] t/t5538-push-shallow.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 18/20] t/t9814-git-p4-rename.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

[PATCH 19/20] t/test-lib-functions.sh: avoid "test <cond> -a/-o <cond>"

[Elia Pinto]

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

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

[Elia Pinto]

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

Re: [PATCH 20/20] CodingGuidelines: avoid "test <cond> -a/-o <cond>"

[Torsten Bögershausen]

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)

Re: [PATCH 00/20] avoid "test <cond> -a/-o <cond>"

[Matthieu Moy]

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/

Re: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"

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

Re: [PATCH 02/20] contrib/examples/git-clone.sh: avoid "test <cond> -a/-o <cond>"

[Junio C Hamano]

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.

Re: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"

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

-- Hannes

Re: [PATCH 08/20] git-mergetool.sh: avoid "test <cond> -a/-o <cond>"

[David Aguilar]

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

Re: [PATCH 10/20] git-submodule.sh: avoid "test <cond> -a/-o <cond>"

[Johannes Sixt]

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)

Re: [PATCH 03/20] contrib/examples/git-commit.sh: avoid "test <cond> -a/-o <cond>"

[David Aguilar]

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

Re: [PATCH 05/20] contrib/examples/git-repack.sh: avoid "test <cond> -a/-o <cond>"

[David Aguilar]

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

Re: [PATCH 09/20] git-rebase--interactive.sh: avoid "test <cond> -a/-o <cond>"

[David Aguilar]

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

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"

[David Aguilar]

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

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"

[David Aguilar]

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

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"

[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"?

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

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test <cond> -a/-o <cond>"

[René Scharfe]

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"