On Thu, Sep 5, 2024 at 7:38 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: > > 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 > Agree with both the suggestions here. Updated my patch.
