Re: [PATCH 08/16] t7527: add parameters to start_daemon to handle args and subshell

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 11 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> fixup! t7527: create test for fsmonitor--daemon
>
> Add parameters to start_daemon() and handle subshell and logging args.
>
> Remove /dev/null from commands.
>
> Fix &&-chaining of functions.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/t7527-builtin-fsmonitor.sh | 217 ++++++++++++++++++-----------------
>  1 file changed, 111 insertions(+), 106 deletions(-)
>
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 0ccbfb9616f..026382a0d86 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -11,30 +11,85 @@ then
>  fi
>  
>  stop_daemon_delete_repo () {
> -	r=$1
> -	git -C $r fsmonitor--daemon stop >/dev/null 2>/dev/null
> +	r=$1 &&
> +	test_might_fail git -C $r fsmonitor--daemon stop &&
>  	rm -rf $1
> -	return 0
>  }
>  
>  start_daemon () {
> -	case "$#" in
> -		1) r="-C $1";;
> -		*) r="";
> -	esac
> +	r="" &&
> +	tf="" &&
> +	t2="" &&
> +	tk="" &&

We usually just do : 

	x= &&

Not:

	x="" &&

I.e. no need for the "".

>  
> -	git $r fsmonitor--daemon start || return $?
> -	git $r fsmonitor--daemon status || return $?
> +	while test "$#" -ne 0
> +	do
> +		case "$1" in
> +		-C)
> +			shift;
> +			test "$#" -ne 0 ||
> +				{ echo >&2 "error: -C requires arg"; exit 1; }

IMO this exhaustive checking is better just skipped, we e.g.don't do
this in "test_commit". Can't you just...

> +			r="-C $1"
> +			shift

...properly &&-chain these whole things and have the "shift too many"
and exit code from "shift" catch this?

> +			;;
> +		-tf)
> +			shift;
> +			test "$#" -ne 0 ||
> +				{ echo >&2 "error: -tf requires arg"; exit 1; }
> +			tf="$1"
> +			shift
> +			;;
> +		-t2)
> +			shift;
> +			test "$#" -ne 0 ||
> +				{ echo >&2 "error: -t2 requires arg"; exit 1; }
> +			t2="$1"
> +			shift
> +			;;
> +		-tk)
> +			shift;
> +			test "$#" -ne 0 ||
> +				{ echo >&2 "error: -tk requires arg"; exit 1; }
> +			tk="$1"
> +			shift
> +			;;
> +		*)

ditto.

> +			echo >&2 "error: unknown option: '$1'"
> +			exit 1

But this sort of case is needed, but should use BUG "..."

> +			;;
> +		esac
> +	done &&
> +
> +	(
> +		if test ! -z "$tf"

Instead of "! -z x" use "-n x" ?

> +		then
> +			GIT_TRACE_FSMONITOR="$tf"
> +			export GIT_TRACE_FSMONITOR
> +		fi &&
> +
> +		if test ! -z "$t2"
> +		then
> +			GIT_TRACE2_PERF="$t2"
> +			export GIT_TRACE2_PERF
> +		fi &&
> +
> +		if test ! -z "$tk"

ditto.

> +		then
> +			GIT_TEST_FSMONITOR_TOKEN="$tk"
> +			export GIT_TEST_FSMONITOR_TOKEN

> +		fi &&
> -	touch test_flush/file_1 &&
> -	touch test_flush/file_2 &&
> +	> test_flush/file_1 &&
> +	> test_flush/file_2 &&

style: ">x" not "> x" (ditto below).