Re: [PATCH 3/3] subtree: adjust style to match CodingGuidelines

Johannes Sixt [off-list ref] 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:

quoted

@@ -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.

quoted

 	fi
 }

 say()
 {
-	if [ -z "$quiet" ]; then
-		printf "%s\n" "$*" >&2
+	if test -z "$quiet"
+	then
+		printf "%s\n" "$@" >&2

Same here.

quoted

 	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.

quoted

 	fi
 }

...

quoted

@@ -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 "$*".

quoted

 debug

...

quoted

 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.)

quoted

+		if test -r "$cachedir/$oldrev"
+		then
 			read newrev <"$cachedir/$oldrev"
 			echo $newrev
 		fi

...

quoted

@@ -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.

quoted

+		then
 			set_notree $rev
-			if [ -n "$newparents" ]; then
+			if test -n "$newparents"
+			then
 				cache_set $rev $rev
 			fi
 			continue

-- Hannes