From: Jeff Hostetler <git@jeffhostetler.com>
Date: Wed, 23 Mar 2022 16:22:31 GMT
Subject: Re: [PATCH v7 21/29] t7527: create test for fsmonitor--daemon
Message-ID: <56a4a545-37b0-0cee-a8c8-1a019b0201c2@jeffhostetler.com>
In-Reply-To: <220322.86a6dhq1a6.gmgdl@evledraar.gmail.com>



On 3/22/22 2:35 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 22 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   t/t7527-builtin-fsmonitor.sh | 501 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 501 insertions(+)
>>   create mode 100755 t/t7527-builtin-fsmonitor.sh
>>
>> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
>> new file mode 100755
>> index 00000000000..d79635e7596
[...]
> 
>> +	while test "$#" -ne 0
>> +	do
>> +		case "$1" in
>> +		-C)
>> +			shift;
>> +			test "$#" -ne 0 || BUG "error: -C requires arg"
>> +			r="-C $1"
>> +			shift
>> +			;;
>> +		-tf)
>> +			shift;
>> +			test "$#" -ne 0 || BUG "error: -tf requires arg"
>> +			tf="$1"
>> +			shift
>> +			;;
>> +		-t2)
>> +			shift;
>> +			test "$#" -ne 0 || BUG "error: -t2 requires arg"
>> +			t2="$1"
>> +			shift
>> +			;;
>> +		-tk)
>> +			shift;
>> +			test "$#" -ne 0 || BUG "error: -tk requires arg"
>> +			tk="$1"
>> +			shift
>> +			;;
> 
> But (and IIRC I noted this in a previous iteration) if you &&-chain the
> "shift" here you can lose the more verbose BUG

Yeah, I looked at the test_commit() example that you referenced.
I thought it was too subtle and misleading.

I mean, the "shift" after the "case...esac" will either clobber
the "--key" or the "value" depending on whether the particular
case-arm shifted.  The shift error would only be thrown on a
missing value, since the while loop already tested $# for non-zero,
but at the point of the error, we'll just have a generic error message
and not know which key should have had a value -- without reading
the script in detail.

Also, in the k/v case-arms, it references $2 without confirming
that it exists.  In test_commit(), it just loads up local variables
(in advance of the soon-to-be-thrown shift error, so no big deal)
but if other people copy this as a model, they may do more in their
case-arms that may be more serious.


My version on the other hand, shifts away the key immediately,
tests whether the required value is present and errors with a
detailed message, and then references the value and shifts away
the value.

My way (IMHO) feels more straight-forward and easier for casual
readers to follow.  Yes, it is a bit more wordy, but I think it
is worth it.


FWIW, as I was writing this note I noticed that both test_commit()
and my start_daemon() examples have a bug where they won't detect
a missing value.  For example, if someone changes
    "test_commit -C repo"
to "test_commit -C --no-tag repo"
then $indir will be "--no-tag" and "repo" will be unclaimed and
an error will follow at some point later (when $indir is used
in a Git command).


I think I'll fix my function to handle that error case, but keep
the basic design that I have.


> 
>> +	start_daemon -tf "$PWD/.git/trace" &&
> 
> FWIW having an option parser take -tf to mean --tf is quite unlike our
> common conventions, usually it means both -t and -f.
> 
> In this case every single caller added here does provide -tf
> argument. Perhaps better as as unconditional $1 then?
> 

yes, very old-school of me to use single dashes here.  I'll change
it/them to use double-dashes (or relabel the keys to be single chars).
There are callers that do not pass the -tf key, so I'd rather keep it
as a key/value than assume a fixed $1.

Thanks
Jeff