Re: [PATCH] remote: prefetch config

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2024, 20:55 UTC
Message-ID: <xmqqfrqf87gb.fsf@gitster.g>
In-Reply-To: <e8232cdc-7a5d-485e-84f7-2c05e17f9907@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 10 quoted lines

> I agree that this would also be of interest, but more complicated. Thanks
> for starting with this simpler modification.
>
>> +	if (remote->prefetch == 0)
>> +		return 0;
>
> In the Git codebase, this would normally be written as
>
> 	if (!remote->prefetch)
> 		return 0;

;-)

Show 10 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.

So, the expected command line being too strit is *not* a fault of this patch, and with the style fix, I think this half of the solution is a good one.

Thanks.