[PATCH] git-submodule.sh: avoid test cond -a/-o cond
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.
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 21
@@ -1122,7 +1126,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
@@ -1159,7 +1163,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
@@ -1230,7 +1234,11 @@ cmd_status()
say U$sha1 $displaypath
continue
fi
- if test -z $url || !
Re: [PATCH] git-submodule.sh: avoid test cond -a/-o cond
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
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
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
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
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
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
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
[PATCH] git-submodule.sh: avoid test cond -a/-o cond
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
@@ -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 21
@@ -1122,7 +1126,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
@@ -1159,7 +1163,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=
Re: [PATCH] git-submodule.sh: avoid test cond -a/-o cond
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
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
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
