Re: [PATCH] remote: prefetch config

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 5, 2024, 02:08 UTC
Message-ID: <52f6da10-e39a-4a2d-bd9a-5739a49235c2@gmail.com>
In-Reply-To: <xmqqfrqf87gb.fsf@gitster.g>

On 9/4/24 4:55 PM, Junio C Hamano wrote:

> Derrick Stolee <stolee@gmail.com> writes:

Show 27 quoted lines

>>> +		# 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