Re: [PATCH] remote: prefetch config
On Thu, Sep 5, 2024 at 7:38 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 52 quoted lines
>
> On 9/4/24 4:55 PM, Junio C Hamano wrote:
> > Derrick Stolee <stolee@gmail.com> 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.