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

[PATCHv2 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.
---
This is version 2 of the patch to git-submodule of the 
patch series "convert  test -a/-o to && and ||". 
It contains the fixes identified by Johannes and
Matthieu. 

 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

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

[Matthieu Moy]

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

> This is version 2 of the patch to git-submodule of the 
> patch series "convert  test -a/-o to && and ||". 
> It contains the fixes identified by Johannes and
> Matthieu. 

This version of the patch (not the whole series) is

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

Some comments for next time:

* I agree with Johannes that splitting the series with one patch per
  file is not the right way to split. As a reviewer, I want to

  - check that -a trivially translates to &&
  - check that -o trivially translates to ||
  - check non-trivial cases

  Interleaving these cases (especially the trivial and non-trivial
  cases) makes the review much harder. For this kind of series, the
  change is trivial, but the review is not (Johannes caught a || Vs &&
  inversion that I didn't find for example, which is quite serious), so
  the "optimize your patches for review" is even more important than
  usual.

* Again, to avoid mixing trivial and non-trivial changes, ...

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

turning a echo into a printf is good, but would be better done
separately.

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

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

[Elia Pinto]

2014-05-22 8:49 GMT+02:00 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> This is version 2 of the patch to git-submodule of the
>> patch series "convert  test -a/-o to && and ||".
>> It contains the fixes identified by Johannes and
>> Matthieu.
>
> This version of the patch (not the whole series) is
>
> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>
> Some comments for next time:
>
> * I agree with Johannes that splitting the series with one patch per
>   file is not the right way to split. As a reviewer, I want to
>
>   - check that -a trivially translates to &&
>   - check that -o trivially translates to ||
>   - check non-trivial cases
>
>   Interleaving these cases (especially the trivial and non-trivial
>   cases) makes the review much harder. For this kind of series, the
>   change is trivial, but the review is not (Johannes caught a || Vs &&
>   inversion that I didn't find for example, which is quite serious), so
>   the "optimize your patches for review" is even more important than
>   usual.
>
> * Again, to avoid mixing trivial and non-trivial changes, ...

First of all, thank you. I will take note of your valuable suggestions
for the future.

>
>> @@ -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
>
> turning a echo into a printf is good, but would be better done
> separately.

I had thought, but the change was in the fix of Johannes, and it did
not think was right to change this, especially that it was right
anyway. But I understand very well the observation.

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

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

[Johannes Sixt]

Am 5/22/2014 10:38, schrieb Elia Pinto:
> 2014-05-22 8:49 GMT+02:00 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
>> 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
>>
>> turning a echo into a printf is good, but would be better done
>> separately.
> 
> I had thought, but the change was in the fix of Johannes, and it did
> not think was right to change this, especially that it was right
> anyway. But I understand very well the observation.

My intent was to show the final state of this piece of code. I do agree
with Matthieu that the change from 'echo' to 'printf' is a different topic
(in particular, since this is not the only point in the script that would
need that change). Sorry for having sent you in circles.

-- Hannes