[PATCH] test: aggregate-results.sh: consistent style. zero forks.

[PATCH] test: aggregate-results.sh: consistent style. zero forks.

[Tomi Ollila]

- all variables in $((...)) without leading $
- all comparisons use -gt, -eq or -ne
- no -a nor -o inside [ ... ] expressions
- all indentation levels using one tab

Dropped unnecessary empty string check when reading results files.

Replaced pluralize() which was executed in subshell with
pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
$1. Calls to pluralize_s are done in context of current shell, so
no forks to subshells executed.
---

I have some other changes planned for this -- before that I wanted
to clead some accumulated inconsistencies...

2 changes I wanted but did not do:
   1) #!/bin/sh
   2) set -euf

 test/aggregate-results.sh | 92 +++++++++++++++------------------------
 1 file changed, 36 insertions(+), 56 deletions(-)

diff --git a/test/aggregate-results.sh b/test/aggregate-results.sh
index 63228546..0f94948b 100755
--- a/test/aggregate-results.sh
+++ b/test/aggregate-results.sh
@@ -13,81 +13,61 @@ do
 	while read type value
 	do
 		case $type in
-		'')
-			continue ;;
 		fixed)
-			fixed=$(($fixed + $value)) ;;
+			fixed=$((fixed + value)) ;;
 		success)
-			success=$(($success + $value)) ;;
+			success=$((success + value)) ;;
 		failed)
-			failed=$(($failed + $value)) ;;
+			failed=$((failed + value)) ;;
 		broken)
-			broken=$(($broken + $value)) ;;
+			broken=$((broken + value)) ;;
 		total)
-			total=$(($total + $value)) ;;
+			total=$((total + value)) ;;
 		esac
 	done <"$file"
 done
 
-pluralize () {
-    case $2 in
-	1)
-	    case $1 in
-		test)
-		    echo test ;;
-		failure)
-		    echo failure ;;
-	    esac
-	    ;;
-	*)
-	    case $1 in
-		test)
-		    echo tests ;;
-		failure)
-		    echo failures ;;
-	    esac
-	    ;;
-    esac
-}
+pluralize_s () { [ $1 -eq 1 ] && s='' || s='s'; }
 
 echo "Notmuch test suite complete."
-if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
-    tests=$(pluralize "test" $total)
-    printf "All $total $tests "
-    if [ "$broken" = "0" ]; then
-	echo "passed."
-    else
-	failures=$(pluralize "failure" $broken)
-	echo "behaved as expected ($broken expected $failures)."
-    fi;
+
+if [ $fixed -eq 0 ] && [ $failed -eq 0 ]; then
+	pluralize_s $total
+	printf "All $total test$s "
+	if [ $broken -eq 0 ]; then
+		echo "passed."
+	else
+		pluralize_s $broken
+		echo "behaved as expected ($broken expected failure$s)."
+	fi
 else
-    echo "$success/$total tests passed."
-    if [ "$broken" != "0" ]; then
-	tests=$(pluralize "test" $broken)
-	echo "$broken broken $tests failed as expected."
-    fi
-    if [ "$fixed" != "0" ]; then
-	tests=$(pluralize "test" $fixed)
-	echo "$fixed broken $tests now fixed."
-    fi
-    if [ "$failed" != "0" ]; then
-	tests=$(pluralize "test" $failed)
-	echo "$failed $tests failed."
-    fi
+	echo "$success/$total tests passed."
+	if [ $broken -ne 0 ]; then
+		pluralize_s $broken
+		echo "$broken broken test$s failed as expected."
+	fi
+	if [ $fixed -ne 0 ]; then
+		pluralize_s $fixed
+		echo "$fixed broken test$s now fixed."
+	fi
+	if [ $failed -ne 0 ]; then
+		pluralize_s $failed
+		echo "$failed test$s failed."
+	fi
 fi
 
-skipped=$(($total - $fixed - $success - $failed - $broken))
-if [ "$skipped" != "0" ]; then
-    tests=$(pluralize "test" $skipped)
-    echo "$skipped $tests skipped."
+skipped=$((total - fixed - success - failed - broken))
+if [ $skipped -ne 0 ]; then
+	pluralize_s $skipped
+	echo "$skipped test$s skipped."
 fi
 
 # Note that we currently do not consider skipped tests as failing the
 # build.
 
-if [ $success -gt 0 -a $fixed -eq 0 -a $failed -eq 0 ]
+if [ $success -gt 0 ] && [ $fixed -eq 0 ] && [ $failed -eq 0 ]
 then
-    exit 0
+	exit 0
 else
-    exit 1
+	exit 1
 fi
-- 
2.21.0

Re: [PATCH] test: aggregate-results.sh: consistent style. zero forks.

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

On Tue 2019-06-04 22:46:24 +0300, Tomi Ollila wrote:
> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>
> Dropped unnecessary empty string check when reading results files.
>
> Replaced pluralize() which was executed in subshell with
> pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
> $1. Calls to pluralize_s are done in context of current shell, so
> no forks to subshells executed.

I'm fine with all these changes, but:

> -if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
> +if [ $fixed -eq 0 ] && [ $failed -eq 0 ]; then

I think we've set $fixed and $failed above to a non-empty string value,
so this is technically correct.  But the shell programmer nit-picker in
me gets nervous seeing any variable used inside a test without proper
wrapping, and i'm going to have trouble adopting $foo instead of "$foo"
in other shell scripts.  it seems to require a lot of global reasoning
about the state of a given variable to use it without quotes safely, and
it introduces some subtle requirements (like, no unsetting these
variables and no setting them to the empty string).

So anyway, i don't see the harm in using "$foo" instead in this case,
and it seems like it actually reduces the cognitive burden of
maintiaining the code.  i'm happy to listen to any compelling story
yo've got for why this is an important change, but i'd prefer to avoid
it.  (i don't at all mind unwrapping "0" to 0 though)

thanks for doing this cleanup!

     --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[PATCH V2] test: aggregate-results.sh: consistent style. zero forks.

[Tomi Ollila]

- all variables in $((...)) without leading $
- all comparisons use -gt, -eq or -ne
- no -a nor -o inside [ ... ] expressions
- all indentation levels using one tab

Dropped unnecessary empty string check when reading results files.

Replaced pluralize() which was executed in subshell with
pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
$1. Calls to pluralize_s are done in context of current shell, so
no forks to subshells executed.
---

V2: added quotes all "$variable" references where empty values or
    IFS characters could make a difference. Not in this script, but
    servers better example as a usage style elsewhere (where it could
    matter).

 test/aggregate-results.sh | 92 +++++++++++++++------------------------
 1 file changed, 36 insertions(+), 56 deletions(-)

diff --git a/test/aggregate-results.sh b/test/aggregate-results.sh
index 63228546..05fb0a92 100755
--- a/test/aggregate-results.sh
+++ b/test/aggregate-results.sh
@@ -13,81 +13,61 @@ do
 	while read type value
 	do
 		case $type in
-		'')
-			continue ;;
 		fixed)
-			fixed=$(($fixed + $value)) ;;
+			fixed=$((fixed + value)) ;;
 		success)
-			success=$(($success + $value)) ;;
+			success=$((success + value)) ;;
 		failed)
-			failed=$(($failed + $value)) ;;
+			failed=$((failed + value)) ;;
 		broken)
-			broken=$(($broken + $value)) ;;
+			broken=$((broken + value)) ;;
 		total)
-			total=$(($total + $value)) ;;
+			total=$((total + value)) ;;
 		esac
 	done <"$file"
 done
 
-pluralize () {
-    case $2 in
-	1)
-	    case $1 in
-		test)
-		    echo test ;;
-		failure)
-		    echo failure ;;
-	    esac
-	    ;;
-	*)
-	    case $1 in
-		test)
-		    echo tests ;;
-		failure)
-		    echo failures ;;
-	    esac
-	    ;;
-    esac
-}
+pluralize_s () { [ "$1" -eq 1 ] && s='' || s='s'; }
 
 echo "Notmuch test suite complete."
-if [ "$fixed" = "0" ] && [ "$failed" = "0" ]; then
-    tests=$(pluralize "test" $total)
-    printf "All $total $tests "
-    if [ "$broken" = "0" ]; then
-	echo "passed."
-    else
-	failures=$(pluralize "failure" $broken)
-	echo "behaved as expected ($broken expected $failures)."
-    fi;
+
+if [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ]; then
+	pluralize_s "$total"
+	printf "All $total test$s "
+	if [ "$broken" -eq 0 ]; then
+		echo "passed."
+	else
+		pluralize_s "$broken"
+		echo "behaved as expected ($broken expected failure$s)."
+	fi
 else
-    echo "$success/$total tests passed."
-    if [ "$broken" != "0" ]; then
-	tests=$(pluralize "test" $broken)
-	echo "$broken broken $tests failed as expected."
-    fi
-    if [ "$fixed" != "0" ]; then
-	tests=$(pluralize "test" $fixed)
-	echo "$fixed broken $tests now fixed."
-    fi
-    if [ "$failed" != "0" ]; then
-	tests=$(pluralize "test" $failed)
-	echo "$failed $tests failed."
-    fi
+	echo "$success/$total tests passed."
+	if [ "$broken" -ne 0 ]; then
+		pluralize_s "$broken"
+		echo "$broken broken test$s failed as expected."
+	fi
+	if [ "$fixed" -ne 0 ]; then
+		pluralize_s "$fixed"
+		echo "$fixed broken test$s now fixed."
+	fi
+	if [ "$failed" -ne 0 ]; then
+		pluralize_s "$failed"
+		echo "$failed test$s failed."
+	fi
 fi
 
-skipped=$(($total - $fixed - $success - $failed - $broken))
-if [ "$skipped" != "0" ]; then
-    tests=$(pluralize "test" $skipped)
-    echo "$skipped $tests skipped."
+skipped=$((total - fixed - success - failed - broken))
+if [ "$skipped" -ne 0 ]; then
+	pluralize_s "$skipped"
+	echo "$skipped test$s skipped."
 fi
 
 # Note that we currently do not consider skipped tests as failing the
 # build.
 
-if [ $success -gt 0 -a $fixed -eq 0 -a $failed -eq 0 ]
+if [ "$success" -gt 0 ] && [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ]
 then
-    exit 0
+	exit 0
 else
-    exit 1
+	exit 1
 fi
-- 
2.21.0

Re: [PATCH V2] test: aggregate-results.sh: consistent style. zero forks.

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

On Mon 2019-06-10 21:39:23 +0300, Tomi Ollila wrote:
> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>
> Dropped unnecessary empty string check when reading results files.
>
> Replaced pluralize() which was executed in subshell with
> pluralize_s(). pluralize_s sets $s to 's' or '' based on value of
> $1. Calls to pluralize_s are done in context of current shell, so
> no forks to subshells executed.
> ---
>
> V2: added quotes all "$variable" references where empty values or
>     IFS characters could make a difference. Not in this script, but
>     servers better example as a usage style elsewhere (where it could
>     matter).

LGTM.  Thanks, Tomi!

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH V2] test: aggregate-results.sh: consistent style. zero forks.

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> - all variables in $((...)) without leading $
> - all comparisons use -gt, -eq or -ne
> - no -a nor -o inside [ ... ] expressions
> - all indentation levels using one tab
>

pushed to master

d