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

[PATCH] 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>
---

This is the fourth revision of this patch.

Change based on Junio suggestions http://www.spinics.net/lists/git/msg233569.html

 git-submodule.sh |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..d6a1dea 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -393,7 +393,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
 
@@ -450,7 +450,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
@@ -832,7 +832,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"
@@ -857,11 +857,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
@@ -1031,7 +1031,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)
@@ -1056,13 +1056,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 &&
@@ -1122,7 +1126,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
@@ -1159,7 +1163,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"
@@ -1230,7 +1234,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;
@@ -1399,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] git-submodule.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>
> ---
>
> This is the fourth revision of this patch.

Hmmmm.

When applied on top of 'master', this seems to break 7406; fails the
same way with either bash 4.2-2ubuntu2.1 which identifes itself as
4.2.25 or dash 0.5.7-2ubuntu2.

-- >8 --

t7406-submodule-update.sh .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 14/43 subtests

Test Summary Report
-------------------
t7406-submodule-update.sh (Wstat: 256 Tests: 43 Failed: 14)
  Failed tests:  4-6, 10-15, 18, 30-33

-- 8< --

Which shell did you test this patch with?

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

[Torsten Bögershausen]

On 2014-06-10 14.28, Elia Pinto wrote:
[]
>  		# before the first commit: compare with an empty tree
>  		head=$(git hash-object -w -t tree --stdin </dev/null)
> @@ -1056,13 +1056,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])
Does this look strange? ^
Should it be
case "$status" in
D|T)

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2014-06-10 14.28, Elia Pinto wrote:
> []
>>  		# before the first commit: compare with an empty tree
>>  		head=$(git hash-object -w -t tree --stdin </dev/null)
>> @@ -1056,13 +1056,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])
> Does this look strange? ^
> Should it be
> case "$status" in
> D|T)

Actually POSIX allows matching parentheses for case arm labels
(surprise!).

And some shells misparse

	var=$( ... case arm) action ;; esac ... )

as if the ')' after the arm label closes the whole command
substitution.

Having said that, I'd prefer to see the following squashed into that
patch.

The first hunk is purely a bugfix.  It used to be 

	if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git

that is: unless "$sm_path/.git" is directory or file, do this.
And the rewrite broke that logic.

The second hunk is to avoid "case" that confuses without helping
readability that much.

I would also have preferred to see the echo to printf substitution
left out of this patch.  There are other places where $sm_path is
echoed and fixing only one of them in an otherwise unrelated patch
feels wrong---it should be a separate follow-up patch, I would
think.

 git-submodule.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d6a1dea..27ca7d5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?")"
 			continue
 		fi
 
-		if ! test -d "$sm_path"/.git || test -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"
@@ -1056,11 +1056,11 @@ 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)
-			case "$status" in
-			([DT])
-				printf '%s\n' "$sm_path" &&
+			if test "$status" = D || test "$status" = T
+			then
+				printf '%s\n' "$sm_path"
 				continue
-			esac
+			fi
 			# Respect the ignore setting for --for-status.
 			if test -n "$for_status"
 			then

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

[Junio C Hamano]

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

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> On 2014-06-10 14.28, Elia Pinto wrote:
>> []
>>>  		# before the first commit: compare with an empty tree
>>>  		head=$(git hash-object -w -t tree --stdin </dev/null)
>>> @@ -1056,13 +1056,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])
>> Does this look strange? ^
>> Should it be
>> case "$status" in
>> D|T)
>
> Actually POSIX allows matching parentheses for case arm labels
> (surprise!).
>
> And some shells misparse
>
> 	var=$( ... case arm) action ;; esac ... )
>
> as if the ')' after the arm label closes the whole command
> substitution.
>
> Having said that, I'd prefer to see the following squashed into that
> patch.
> ...
> I would also have preferred to see the echo to printf substitution
> left out of this patch.  There are other places where $sm_path is
> echoed and fixing only one of them in an otherwise unrelated patch
> feels wrong---it should be a separate follow-up patch, I would
> think.

... which may look like this (after removing s/echo/printf/ in that
hunk from this "test -a/-o" patch).

 git-submodule.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d0d9b58..9245abf 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -235,7 +235,7 @@ module_name()
 		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
 	test -z "$name" &&
 	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
-	echo "$name"
+	printf '%s\n' "$name"
 }
 
 #
@@ -305,10 +305,10 @@ module_clone()
 	b=${b%/}
 
 	# Turn each leading "*/" component into "../"
-	rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
-	echo "gitdir: $rel/$a" >"$sm_path/.git"
+	rel=$(printf '%s\n' "$b" | sed -e 's|[^/][^/]*|..|g')
+	printf '%s\n' "gitdir: $rel/$a" >"$sm_path/.git"
 
-	rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
+	rel=$(printf '%s\n' "$a" | sed -e 's|[^/][^/]*|..|g')
 	(clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
 }
 
@@ -389,7 +389,7 @@ cmd_add()
 	sm_path=$2
 
 	if test -z "$sm_path"; then
-		sm_path=$(echo "$repo" |
+		sm_path=$(printf '%s\n' "$repo" |
 			sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
 	fi
 
@@ -1058,7 +1058,7 @@ cmd_summary() {
 			# Always show modules deleted or type-changed (blob<->module)
 			if test "$status" = D || test "$status" = T
 			then
-				echo "$sm_path"
+				printf '%s\n' "$sm_path"
 				continue
 			fi
 			# Respect the ignore setting for --for-status.
@@ -1070,7 +1070,7 @@ cmd_summary() {
 			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 &&
-			echo "$sm_path"
+			printf '%s\n' "$sm_path"
 		done
 	)
 
@@ -1311,7 +1311,7 @@ cmd_sync()
 		./*|../*)
 			# rewrite foo/bar as ../.. to find path from
 			# submodule work tree to superproject work tree
-			up_path="$(echo "$sm_path" | sed "s/[^/][^/]*/../g")" &&
+			up_path="$(printf '%s\n' "$sm_path" | sed "s/[^/][^/]*/../g")" &&
 			# guarantee a trailing /
 			up_path=${up_path%/}/ &&
 			# path from submodule work tree to submodule origin repo

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

[Junio C Hamano]

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

> @@ -832,7 +832,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

Which part of test conditions does that "!" apply in the original,
and in the updated? 

I think the new test after || also needs negation, no?

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

[Johannes Sixt]

Am 6/10/2014 16:55, schrieb Junio C Hamano:
> Elia Pinto <gitter.spiros@gmail.com> writes:
> 
>> @@ -832,7 +832,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
> 
> Which part of test conditions does that "!" apply in the original,
> and in the updated? 
> 
> I think the new test after || also needs negation, no?

Not just that; the || must be turned into && as well.

I noticed a similar construct later in the patch in a review of an earlier
iteration, but missed this one.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 6/10/2014 16:55, schrieb Junio C Hamano:
>> Elia Pinto <gitter.spiros@gmail.com> writes:
>> 
>>> @@ -832,7 +832,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
>> 
>> Which part of test conditions does that "!" apply in the original,
>> and in the updated? 
>> 
>> I think the new test after || also needs negation, no?
>
> Not just that; the || must be turned into && as well.

I think our mails crossed ;-)

[PATCH] 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>
---
This is the fifth revision.

Change based on Junio bugfix and better rewrite of the case condition
http://permalink.gmane.org/gmane.comp.version-control.git/251198

I dropped also the echo -> printf replacement for doing
it in another patch.

Pass all the t/*submodule* tests. Finally ! :=)

Thank you all very much and sorry for the mess.

 git-submodule.sh |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..e128a4a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -393,7 +393,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
 
@@ -450,7 +450,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
@@ -832,7 +832,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"
@@ -857,11 +857,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
@@ -1031,7 +1031,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)
@@ -1056,13 +1056,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
+			if test "$status" = D || test "$status" = T
+                        then
+				echo "$sm_path" &&
+				continue
+			fi
 			# 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 &&
@@ -1122,7 +1126,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
@@ -1159,7 +1163,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"
@@ -1230,7 +1234,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;
@@ -1399,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] git-submodule.sh: avoid "test <cond> -a/-o <cond>"

[Junio C Hamano]

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

> @@ -832,7 +832,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

Hmmmm.  Is the above correct?

    $ if ! false && true; then echo true; else echo false; fi
    true

In other words, "! cmd1 && cmd2" parses not as "! (cmd1 && cmd2)"
but as "(! cmd1) && cmd2".

It almost makes me wonder if the code may become simpler if we did
it the way in the attached.  That is, "if $sm_path/.git is there
(whether as an embedded repository, or a file pointing to a
repository elsewhere), then grab its HEAD, otherwise $sm_path is a
submodule that hasn't been run 'submodule init' on, so run the whole
nine yards starting from module_clone".

 git-submodule.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..018f1bb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -832,15 +832,15 @@ Maybe you want to use 'update --init'?")"
 			continue
 		fi
 
-		if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
+		if test -e "$sm_path/.git"
 		then
-			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
-			cloned_modules="$cloned_modules;$name"
-			subsha1=
-		else
 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
 			die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
+		else
+			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
+			cloned_modules="$cloned_modules;$name"
+			subsha1=
 		fi
 
 		if test -n "$remote"

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

[Junio C Hamano]

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

> @@ -832,7 +832,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

Hmmmm.  Is the above correct?

    $ if ! false && true; then echo true; else echo false; fi
    true

In other words, "! cmd1 && cmd2" parses not as "! (cmd1 && cmd2)"
but as "(! cmd1) && cmd2".

It almost makes me wonder if the code may become simpler if we did
it the way in the attached.  That is, "if $sm_path/.git is there
(whether as an embedded repository, or a file pointing to a
repository elsewhere), then grab its HEAD, otherwise $sm_path is a
submodule that hasn't been run 'submodule init' on, so run the whole
nine yards starting from module_clone".

Such a rewrite is not within the scope of this series, so 

	if ! test -d "$sm_path/.git" && ! test -f "$sm_path/.git"

may be the closest to the original intent, I would think.

 git-submodule.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..018f1bb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -832,15 +832,15 @@ Maybe you want to use 'update --init'?")"
 			continue
 		fi
 
-		if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
+		if test -e "$sm_path/.git"
 		then
-			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
-			cloned_modules="$cloned_modules;$name"
-			subsha1=
-		else
 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
 			die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
+		else
+			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
+			cloned_modules="$cloned_modules;$name"
+			subsha1=
 		fi
 
 		if test -n "$remote"

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

[Eric Sunshine]

On Tue, Jun 10, 2014 at 12:43 PM, 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 is the fifth revision.
>
> Change based on Junio bugfix and better rewrite of the case condition
> http://permalink.gmane.org/gmane.comp.version-control.git/251198
>
> I dropped also the echo -> printf replacement for doing
> it in another patch.
>
> Pass all the t/*submodule* tests. Finally ! :=)
>
> Thank you all very much and sorry for the mess.
>
>  git-submodule.sh |   32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e146b83..e128a4a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
>                 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
> +                       if test "$status" = D || test "$status" = T
> +                        then
> +                               echo "$sm_path" &&

Unnecessary &&.

> +                               continue
> +                       fi
>                         # 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 &&