Re: [PATCH 3/3] subtree: adjust style to match CodingGuidelines
Johannes Sixt writes:
> These caught my eye browsing through my inbox. I'm not a subtree user.
All good comments.
Let's queue 1/3 and 2/3 and fast-track them down to 'master'. Style
fixes can come independently later.
Thanks.
> Am 26.07.2016 um 06:14 schrieb David Aguilar:
>> @@ -50,87 +51,145 @@ prefix=
>>
>> debug()
>> {
>> -if [ -n "$debug" ]; then
>> -printf "%s\n" "$*" >&2
>> +if test -n "$debug"
>> +then
>> +printf "%s\n" "$@" >&2
>
> Are you sure you want this? It prints each argument of the 'debug'
> invocation on its own line.
>
>> fi
>> }
>>
>> say()
>> {
>> -if [ -z "$quiet" ]; then
>> -printf "%s\n" "$*" >&2
>> +if test -z "$quiet"
>> +then
>> +printf "%s\n" "$@" >&2
>
> Same here.
>
>> fi
>> }
>>
>> progress()
>> {
>> -if [ -z "$quiet" ]; then
>> -printf "%s\r" "$*" >&2
>> +if test -z "$quiet"
>> +then
>> +printf "%s\r" "$@" >&2
>
> But here I'm pretty sure that this is not wanted; the original is
> clearly correct.
>
>> fi
>> }
> ...
>> @@ -139,22 +198,27 @@ debug "command: {$command}"
>> debug "quiet: {$quiet}"
>> debug "revs: {$revs}"
>> debug "dir: {$dir}"
>> -debug "opts: {$*}"
>> +debug "opts: {$@}"
>
> When the arguments of a script or function are to be printed for the
> user's entertainment/education, then it is safer (and, therefore,
> idiomatic) to use "$*".
>
>> debug
> ...
>> cache_get()
>> {
>> -for oldrev in $*; do
>> -if [ -r "$cachedir/$oldrev" ]; then
>> +for oldrev in "$@"
>> +do
>
> It is idiomatic to write this as
>
> for oldrev
> do
>
> (But your move from bare $* to quoted "$@" fits better under the "fix
> quoting" topic of this patch.)
>
>> +if test -r "$cachedir/$oldrev"
>> +then
>> read newrev <"$cachedir/$oldrev"
>> echo $newrev
>> fi
> ...
>> @@ -631,17 +749,19 @@ cmd_split()
>> debug " parents: $parents"
>> newparents=$(cache_get $parents)
>> debug " newparents: $newparents"
>> -
>> +
>> tree=$(subtree_for_commit $rev "$dir")
>> debug " tree is: $tree"
>>
>> check_parents $parents
>> -
>> +
>> # ugly. is there no better way to tell if this is a subtree
>> # vs. a mainline commit? Does it matter?
>> -if [ -z $tree ]; then
>> +if test -z $tree
>
> This works by accident. When $tree is empty, this reduces to 'test
> -z', which happens to evaluate to true, just what we want. But it be
> appropriate to put $tree in double-quotes nevertheless.
>
>> +then
>> set_notree $rev
>> -if [ -n "$newparents" ]; then
>> +if test -n "$newparents"
>> +then
>> cache_set $rev $rev
>> fi
>> continue
>
> -- 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 3/3] subtree: adjust style to match CodingGuidelines
These caught my eye browsing through my inbox. I'm not a subtree user.
Am 26.07.2016 um 06:14 schrieb David Aguilar:
@@ -50,87 +51,145 @@ prefix=
debug()
{
- if [ -n "$debug" ]; then
- printf "%s\n" "$*" >&2
+ if test -n "$debug"
+ then
+ printf "%s\n" "$@" >&2
Are you sure you want this? It prints each argument of the 'debug'
invocation on its own line.
fi
}
say()
{
- if [ -z "$quiet" ]; then
- printf "%s\n" "$*" >&2
+ if test -z "$quiet"
+ then
+ printf "%s\n" "$@" >&2
Same here.
fi
}
progress()
{
- if [ -z "$quiet" ]; then
- printf "%s\r" "$*" >&2
+ if test -z "$quiet"
+ then
+ printf "%s\r" "$@" >&2
But here I'm pretty sure that this is not wanted; the original is
clearly correct.
fi
}
...
@@ -139,22 +198,27 @@ debug "command: {$command}"
debug "quiet: {$quiet}"
debug "revs: {$revs}"
debug "dir: {$dir}"
-debug "opts: {$*}"
+debug "opts: {$@}"
When the arguments of a script or function are to be printed for the
user's entertainment/education, then it is safer (and, therefore,
idiomatic) to use "$*".
debug
...
cache_get()
{
- for oldrev in $*; do
- if [ -r "$cachedir/$oldrev" ]; then
+ for oldrev in "$@"
+ do
It is idiomatic to write this as
for oldrev
do
(But your move from bare $* to quoted "$@" fits better under the "fix
quoting" topic of this patch.)
+ if test -r "$cachedir/$oldrev"
+ then
read newrev <"$cachedir/$oldrev"
echo $newrev
fi
...
@@ -631,17 +749,19 @@ cmd_split()
debug " parents: $parents"
newparents=$(cache_get $parents)
debug " newparents: $newparents"
-
+
tree=$(subtree_for_commit $rev "$dir")
debug " tree is: $tree"
check_parents $parents
-
+
# ugly. is there no better way to tell if this is a subtree
# vs. a mainline commit? Does it matter?
- if [ -z $tree ]; then
+ if test -z $tree
This works by accident. When $tree is empty, this reduces to 'test -z',
which happens to evaluate to true, just what we want. But it be
appropriate to put $tree in double-quotes nevertheless.
+ then
set_notree $rev
- if [ -n "$newparents" ]; then
+ if test -n "$newparents"
+ then
cache_set $rev $rev
fi
continue
-- 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
[PATCH 3/3] subtree: adjust style to match CodingGuidelines
Prefer "test" over "[ ... ]", use double-quotes around variables, break
long lines, and properly indent "case" statements.
Signed-off-by: David Aguilar
---
contrib/subtree/git-subtree.sh | 544 ++---
1 file changed, 341 insertions(+), 203 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b567eae..084c884 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,8 +4,9 @@
#
# Copyright (C) 2009 Avery Pennarun
#
-if [ $# -eq 0 ]; then
-set -- -h
+if test $# -eq 0
+then
+ set -- -h
fi
OPTS_SPEC="\
git subtree add --prefix=
@@ -50,87 +51,145 @@ prefix=
debug()
{
- if [ -n "$debug" ]; then
- printf "%s\n" "$*" >&2
+ if test -n "$debug"
+ then
+ printf "%s\n" "$@" >&2
fi
}
say()
{
- if [ -z "$quiet" ]; then
- printf "%s\n" "$*" >&2
+ if test -z "$quiet"
+ then
+ printf "%s\n" "$@" >&2
fi
}
progress()
{
- if [ -z "$quiet" ]; then
- printf "%s\r" "$*" >&2
+ if test -z "$quiet"
+ then
+ printf "%s\r" "$@" >&2
fi
}
assert()
{
- if "$@"; then
- :
- else
+ if ! "$@"
+ then
die "assertion failed: " "$@"
fi
}
-#echo "Options: $*"
-
-while [ $# -gt 0 ]; do
+while test $# -gt 0
+do
opt="$1"
shift
+
case "$opt" in
- -q) quiet=1 ;;
- -d) debug=1 ;;
- --annotate) annotate="$1"; shift ;;
- --no-annotate) annotate= ;;
- -b) branch="$1"; shift ;;
- -P) prefix="${1%/}"; shift ;;
- -m) message="$1"; shift ;;
- --no-prefix) prefix= ;;
- --onto) onto="$1"; shift ;;
- --no-onto) onto= ;;
- --rejoin) rejoin=1 ;;
- --no-rejoin) rejoin= ;;
- --ignore-joins) ignore_joins=1 ;;
- --no-ignore-joins) ignore_joins= ;;
- --squash) squash=1 ;;
- --no-squash) squash= ;;
- --) break ;;
- *) die "Unexpected option: $opt" ;;
+ -q)
+ quiet=1
+ ;;
+ -d)
+ debug=1
+ ;;
+ --annotate)
+ annotate="$1"
+ shift
+ ;;
+ --no-annotate)
+ annotate=
+ ;;
+ -b)
+ branch="$1"
+ shift
+ ;;
+ -P)
+ prefix="${1%/}"
+ shift
+ ;;
+ -m)
+ message="$1"
+ shift
+ ;;
+ --no-prefix)
+ prefix=
+ ;;
+ --onto)
+ onto="$1"
+ shift
+ ;;
+ --no-onto)
+ onto=
+ ;;
+ --rejoin)
+ rejoin=1
+ ;;
+ --no-rejoin)
+ rejoin=
+ ;;
+ --ignore-joins)
+ ignore_joins=1
+ ;;
+ --no-ignore-joins)
+ ignore_joins=
+ ;;
+ --squash)
+ squash=1
+ ;;
+ --no-squash)
+ squash=
+ ;;
+ --)
+ break
+ ;;
+ *)
+ die "Unexpected option: $opt"
+ ;;
esac
done
command="$1"
shift
+
case "$command" in
- add|merge|pull) default= ;;
- split|push) default="--default HEAD" ;;
- *) die "Unknown command '$command'" ;;
+add|merge|pull)
+ default=
+ ;;
+split|push)
+ default="--default HEAD"
+ ;;
+*)
+ die "Unknown command '$command'"
+ ;;
esac
-if [ -z "$prefix" ]; then
+if test -z "$prefix"
+then
die "You must provide the --prefix option."
fi
case "$command" in
- add) [ -e "$prefix" ] &&
- die "prefix '$prefix' already exists." ;;
- *) [ -e "$prefix" ] ||
- die "'$prefix' does not exist; use 'git subtree add'" ;;
+add)
+ test -e "$prefix" && die "prefix '$prefix' already exists."
+ ;;
+*)
+ test -e "$prefix" ||
+ die "'$prefix' does not exist; use 'git subtree add'"
+ ;;
esac
dir="$(dirname "$prefix/.")"
-if [ "$command" != "pull" -a "$command" != "add" -a "$command" != "push" ];
then
+if test "$command" != "pull" &&
+ test "$command" != "add" &&
+ test "$command" != "push"
+then
revs=$(git rev-parse $default --revs-only "$@") || exit $?
dirs="$(git rev-parse --no-revs --no-flags "$@")" || exit $?
- if [ -n "$dirs" ]; then
+ if test -n "$dirs"
+ then
die "Error: Use --prefix instead of bare filenames."
fi
fi
@@ -139,22 +198,27 @@ debug "command: {$command}"
