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@xxxxxxxxxxxxx>
Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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
