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

2014-06-10 Thread Junio C Hamano
Elia Pinto gitter.spi...@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.spi...@gmail.com
 ---

 This is the fourth revision of this patch.

H.

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?
--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread 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)


--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Junio C Hamano
Elia Pinto gitter.spi...@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?
--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Johannes Sixt
Am 6/10/2014 16:55, schrieb Junio C Hamano:
 Elia Pinto gitter.spi...@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
--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Junio C Hamano
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


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

2014-06-10 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

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


--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Junio C Hamano
Johannes Sixt j.s...@viscovery.net writes:

 Am 6/10/2014 16:55, schrieb Junio C Hamano:
 Elia Pinto gitter.spi...@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 ;-)
--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Junio C Hamano
Elia Pinto gitter.spi...@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

H.  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
--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Junio C Hamano
Elia Pinto gitter.spi...@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

H.  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
--
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] git-submodule.sh: avoid test cond -a/-o cond

2014-06-10 Thread Eric Sunshine
On Tue, Jun 10, 2014 at 12:43 PM, Elia Pinto gitter.spi...@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.spi...@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 21 
--
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