From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 26 Jul 2016 06:12:39 GMT
Subject: Re: [PATCH 3/3] subtree: adjust style to match CodingGuidelines
Message-ID: <a12604f4-7dc9-7c35-0853-c3bffd734281@kdbg.org>
In-Reply-To: <20160726041416.9438-3-davvid@gmail.com>

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