Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command
Jonathan Nieder jrnie...@gmail.com writes:
Perhaps something like the following would work?
tree-wide: convert test -a/-o to and ||
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 in git
of -a and -o to save readers from the burden of thinking
about such things.
Add a check-non-portable-shell.pl to avoid more instances of
test -a and -o arising in the future.
Yeah, the title is certainly better than -a or -b option I see
above ;-) and a single tree-wide fix may be OK while the tree is
quiescent.
I however do think better known is much less of an issue than that
-a/-o is more error prone e.g. 'test -n $x -a a = b' is buggy
because it does not consider that $x could be =.
[...]
-test $status = D -o $status = T echo $sm_path
continue
+ ( test $status = D || test $status = T ) echo
$sm_path continue
There's no need for a subshell for this. Better to use a block:
{
test $status = D ||
test $status = T
}
echo $sm_path
continue
Yes.
--
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/10] git-submodule.sh: don't use the -a or -b option with the test command
I am very sorry : the comment is wrong. I will repost shortly.
Best regards
2014-05-15 16:17 GMT+02:00 Elia Pinto gitter.spi...@gmail.com:
Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted as follow:
test $1 -a $2
should be written as:
test $1 test $2
Likewise
test $1 -o $2
should be written as:
test $1 test $2
But note that, in test, -a has higher precedence than -o while
and || have equal precedence in the shell.
The reason for this is that the precedence rules were never well
specified, and this made many sane-looking uses of test -a/-o problematic.
For example, if $x is =, these work according to POSIX (it's not
portable, but in practice it's okay):
$ test -z $x
$ test -z $x test a = b
but this doesn't
$ test -z $x -a a = b
bash: test: too many arguments
because it groups test -n = -a and is left with a = b.
Similarly, if $x is -f, these
$ test $x
$ test $x || test c = d
correctly adds an implicit -n, but this fails:
$ test $x -o c = d
bash: test: too many arguments
Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
Inspired from this discussion
http://permalink.gmane.org/gmane.comp.version-control.git/137056
git-submodule.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index b55d83a..d89e1d0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -864,7 +864,7 @@ Maybe you want to use 'update --init'?)
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
@@ -1059,13 +1059,13 @@ 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
+( 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 21
@@ -1125,7 +1125,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 +1162,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 +1233,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
say -$sha1 $displaypath
continue;
@@ -1402,7 +1402,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
--
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/10] git-submodule.sh: don't use the -a or -b option with the test command
Elia Pinto wrote:
Even though POSIX.1 lists -a/-o as options to test, they are
marked Obsolescent XSI. Scripts using these expressions
should be converted as follow:
[... many lines snipped ...]
This is a very long description, and it doesn't leave me excited by
the change.
Is there some potential bug this prevents, or is it just a style
fix? If the latter, do we have a way of checking for new examples
of the same thing to avoid having to repeat the same patch again in
the future?
Are there any platforms that were broken which this fixes? Even
posh seems to understand -a and -o.
Nowadays Documentation/CodingGuidelines says
- Fixing style violations while working on a real change as a
preparatory clean-up step is good, but otherwise avoid useless code
churn for the sake of conforming to the style.
Once it _is_ in the tree, it's not really worth the patch noise to
go and fix it up.
Cf. http://article.gmane.org/gmane.linux.kernel/943020
which I think goes too far (some patterns really are error prone
or distracting and it can be worth fixing them tree-wide), but it
makes a reasonable case that an idiom not being preferred in the
style guide is not *on its own* enough reason to change it.
Perhaps something like the following would work?
tree-wide: convert test -a/-o to and ||
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 in git
of -a and -o to save readers from the burden of thinking
about such things.
Add a check-non-portable-shell.pl to avoid more instances of
test -a and -o arising in the future.
[...]
- test $status = D -o $status = T echo $sm_path
continue
+ ( test $status = D || test $status = T ) echo
$sm_path continue
There's no need for a subshell for this. Better to use a block:
{
test $status = D ||
test $status = T
}
echo $sm_path
continue
or an if statement:
if test $status = D || test $status = T
then
echo $sm_path
continue
fi
or case:
case $status in
D|T)
echo $sm_path
continue
;;
esac
Hope that helps,
Jonathan
--
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
