From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 04 Sep 2024 20:55:32 GMT
Subject: Re: [PATCH] remote: prefetch config
Message-ID: <xmqqfrqf87gb.fsf@gitster.g>
In-Reply-To: <e8232cdc-7a5d-485e-84f7-2c05e17f9907@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

> 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;

;-)

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