* [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
@ 2014-06-10 12:28 Elia Pinto
2014-06-10 14:42 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Elia Pinto @ 2014-06-10 12:28 UTC (permalink / raw)
To: git; +Cc: jrnieder, gitster, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 12:28 [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>" Elia Pinto
@ 2014-06-10 14:42 ` Junio C Hamano
2014-06-10 14:52 ` Torsten Bögershausen
2014-06-10 14:55 ` Junio C Hamano
2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 14:42 UTC (permalink / raw)
To: Elia Pinto; +Cc: git, jrnieder
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 12:28 [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>" Elia Pinto
2014-06-10 14:42 ` Junio C Hamano
@ 2014-06-10 14:52 ` Torsten Bögershausen
2014-06-10 15:23 ` Junio C Hamano
2014-06-10 14:55 ` Junio C Hamano
2 siblings, 1 reply; 12+ messages in thread
From: Torsten Bögershausen @ 2014-06-10 14:52 UTC (permalink / raw)
To: Elia Pinto, git; +Cc: jrnieder, gitster
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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 12:28 [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>" Elia Pinto
2014-06-10 14:42 ` Junio C Hamano
2014-06-10 14:52 ` Torsten Bögershausen
@ 2014-06-10 14:55 ` Junio C Hamano
2014-06-10 15:20 ` Johannes Sixt
2 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 14:55 UTC (permalink / raw)
To: Elia Pinto; +Cc: git, jrnieder
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 14:55 ` Junio C Hamano
@ 2014-06-10 15:20 ` Johannes Sixt
2014-06-10 15:36 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: Johannes Sixt @ 2014-06-10 15:20 UTC (permalink / raw)
To: Junio C Hamano, Elia Pinto; +Cc: git, jrnieder
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 14:52 ` Torsten Bögershausen
@ 2014-06-10 15:23 ` Junio C Hamano
2014-06-10 15:32 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 15:23 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Elia Pinto, git, jrnieder
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 15:23 ` Junio C Hamano
@ 2014-06-10 15:32 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 15:32 UTC (permalink / raw)
To: Elia Pinto; +Cc: Torsten Bögershausen, git, jrnieder
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 15:20 ` Johannes Sixt
@ 2014-06-10 15:36 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 15:36 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Elia Pinto, git, jrnieder
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 ;-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 16:43 Elia Pinto
2014-06-10 17:02 ` Junio C Hamano
2014-06-10 17:03 ` Junio C Hamano
@ 2014-06-10 19:47 ` Eric Sunshine
2 siblings, 0 replies; 12+ messages in thread
From: Eric Sunshine @ 2014-06-10 19:47 UTC (permalink / raw)
To: Elia Pinto; +Cc: Git List, Junio C Hamano, Jonathan Nieder
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 &&
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 16:43 Elia Pinto
2014-06-10 17:02 ` Junio C Hamano
@ 2014-06-10 17:03 ` Junio C Hamano
2014-06-10 19:47 ` Eric Sunshine
2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 17:03 UTC (permalink / raw)
To: Elia Pinto; +Cc: git, jrnieder
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"
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
2014-06-10 16:43 Elia Pinto
@ 2014-06-10 17:02 ` Junio C Hamano
2014-06-10 17:03 ` Junio C Hamano
2014-06-10 19:47 ` Eric Sunshine
2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2014-06-10 17:02 UTC (permalink / raw)
To: Elia Pinto; +Cc: git, jrnieder
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"
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>"
@ 2014-06-10 16:43 Elia Pinto
2014-06-10 17:02 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Elia Pinto @ 2014-06-10 16:43 UTC (permalink / raw)
To: git; +Cc: gitster, jrnieder, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-06-10 20:35 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-10 12:28 [PATCH] git-submodule.sh: avoid "test <cond> -a/-o <cond>" Elia Pinto
2014-06-10 14:42 ` Junio C Hamano
2014-06-10 14:52 ` Torsten Bögershausen
2014-06-10 15:23 ` Junio C Hamano
2014-06-10 15:32 ` Junio C Hamano
2014-06-10 14:55 ` Junio C Hamano
2014-06-10 15:20 ` Johannes Sixt
2014-06-10 15:36 ` Junio C Hamano
2014-06-10 16:43 Elia Pinto
2014-06-10 17:02 ` Junio C Hamano
2014-06-10 17:03 ` Junio C Hamano
2014-06-10 19:47 ` Eric Sunshine
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.