Johannes Sixt <j...@kdbg.org> 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
