Subject: RE: [PATCH v8 21/30] t7527: create test for fsmonitor--daemon

From: <[email-masked]>
Date: Thu, 24 Mar 2022 15:05:00 -0400

On March 24, 2022 3:00 PM, Junio C Hamano wrote:
>Subject: Re: [PATCH v8 21/30] t7527: create test for fsmonitor--daemon
>
>"Jeff Hostetler via GitGitGadget" <[email-masked]> writes:
>
>> From: Jeff Hostetler <[email-masked]>
>>
>> Signed-off-by: Jeff Hostetler <[email-masked]>
>> Signed-off-by: Junio C Hamano <[email-masked]>
>
>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}"
>			;;
>		...

May I request a bit of extra time on the -rc0 to -rc1 cycle for this? I have a feeling that while testing this is probably going to go well, I would like to have a bit of extra time for anything that might come up. There are a lot of moving parts to this series. Not being critical, but debugging scripts on my platforms can be a bit rough at times.

Thanks,
Randall