From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 24 Mar 2022 18:59:34 GMT
Subject: Re: [PATCH v8 21/30] t7527: create test for fsmonitor--daemon
Message-ID: <xmqqils3gort.fsf@gitster.g>
In-Reply-To: <c8709da9457eb303132b5cad6a204a1de27aabc0.1648140586.git.gitgitgadget@gmail.com>

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I hadn't signed off on this one yet ;-)

> +is_value () {
> +	test -n "$1" && test "${1::1}" != "-"
> +}

${var:ofs:len} is a bash-ism.  If you run this test under /bin/dash
instead of /bin/dash, you'll likely see it fail.

If it were a good idea to see if $1 begins with a dash, a more
natural (to shell programmers) way to do so is

	case "$1" in -*) false ;; ?*) true ;; *) false ;; esac

but given how this is used below, we do not want to special case
dash.  

There isn't anything wrong in "mkdir ./-foo && start_daemon -C -foo"
in other words.

> +start_daemon () {
> +	r= &&
> +	tf= &&
> +	t2= &&
> +	tk= &&

FYI, you can write these on a single line, i.e.

	r= tf= t2= tk= &&

Spending lines and spaces for the meat of the script would enhance
readability but for things like a boilerplate "we clear variables
before using them", being concise may be less distracting.

> +	while test "$#" -ne 0
> +	do
> +		case "$1" in
> +		-C)
> +			shift;
> +			is_value $1 || BUG "error: -C requires value"
> +			r="-C $1"
> +			shift
> +			;;
> +	...
> +		esac
> +	done &&

A more natural way to write these loops is

	while ...
	do
		case "$1" in
		-C)
			r="-C ${2?}"
			shift
			;;
		... all other options you handle ...
		-*)
			echo >&2 "unknown option $1"
			exit 1
			;;
		*)
			break
			;;
		esac
		shift
	done

i.e. shifting out what we just saw is the default and happens
immediately after the case/esac, and extra shift after consuming
an option parameter happens in each case arm.

An acceptable slight variation is

		-C)
			shift
			r="-C ${1?}"
			;;

but the first form is more logical and clear, i.e. "when we see '-C',
we want two on the command line, -C itself and the parameter it takes"
is conveyed more strongly with "${2?}" there.

For an additional bonus, we could also accept the stuck form, i.e.

		case "$1" in
		-C)
			r="-C ${2?}"
			shift
			;;
		-C*)
			r="-C {$1#-C}"
			;;
		...