On 9/4/24 4:55 PM, Junio C Hamano wrote:
Derrick Stolee <stolee@xxxxxxxxx> writes:
+ # Run maintenance prefetch task
+ GIT_TRACE2_EVENT="$(pwd)/prefetch.txt" git maintenance run --task=prefetch 2>/dev/null &&
+
+ # Check that remote1 was not fetched (prefetch=false)
+ test_subcommand ! git fetch remote1 --prefetch --prune --no-tags \
+ --no-write-fetch-head --recurse-submodules=no --quiet \
+ <prefetch.txt &&
I'm happy to see this use of test_subcommand to validate the behavior
of this patch!
I found it a bit disturbing that the pattern is overly specific.
The only thing we are interested in is that we are not fetching from
remote1, so it _should_ suffice if we could write
test_subcommand ! git fetch remote1 <prefetch.txt &&
to avoid being tied to how the current version of Git happens to
pass these command line option flags and the order it does so.
Looking at the implementation of test_subcommand, it seems that we
cannot quite do that (it assumes that the pattern it assembles out
of the parameters are to match the full argument list used in
invocation, enclosing them in a single [] pair and without giving
the caller an easy way to sneak wildcards like ".*" in), which is
sad.
I agree the ergonomics of the test_subcommand helper is a bit poor
(and not this patch author's fault). The trickiest part is the
negative case, as in this highlighted one. It's hard to read from
this if the subcommand wasn't found because the argument list is
too specific and doesn't match the exact arguments.
It helps that the same options are given for the other, positive
tests. But maybe that could be a hint as to how to make this test
a bit cleaner: make a variable describing the "uninteresting"
arguments. Something like...
args="--prefetch --prune --no-tags \
--no-write-fetch-head --recurse-submodules=no --quiet" &&
test_subcommand ! git fetch remote1 $args <prefetch.txt &&
test_subcommand git fetch remote2 $args <prefetch.txt &&
test_subcommand git fetch remote3 $args <prefetch.txt &&
Thanks,
-Stolee
