From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 05 Sep 2024 15:00:00 GMT
Subject: Re: [PATCH] remote: prefetch config
Message-ID: <xmqqtteu5eof.fsf@gitster.g>
In-Reply-To: <52f6da10-e39a-4a2d-bd9a-5739a49235c2@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

> 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).

I suspect that we could do

    test_subcommand ! git fetch remote1 --prefetch '.*' <prefetch.txt

which would be rewritten to this pattern

	\["git", "fetch", "remote1", "--prefetch", ".*"\]

if I am reading how the expr given to grep is built by the
test_subcommand implementation.  As long a there is at least one
actual argument after the "--prefetch" one, .* would slurp
everything.

But it is ugly.  In any case, this is a tangent unrelated to the
topic of the patch on this thread.