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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matthieu Moy]

Elia Pinto  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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
---
 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 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 = 16 -a $mod_dst = 16
+   if test $mod_src = 16 && test $mod_dst = 16
then
range="$sha1_src...$sha1_dst"
elif test $mod_src = 16
@@ -1162,7 +1167,7 @@ cmd_summary() {
# i.e. deleted or changed to blob
test $mod_dst = 16 && echo "$errmsg"
else
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16 && test $mod_dst = 16
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