Torsten Bögershausen <tbo...@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
--
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

Reply via email to