Re: [PATCH v4 1/8] fetch: fix `--no-recurse-submodules` with multi-remote fetches

From: Glen Choo <chooglen@google.com>
Date: May 9, 2023, 18:27 UTC
Message-ID: <kl6l5y911ht4.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq1qjpz96s.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 5 quoted lines

>> +		git config fetch.parallel 0 &&
>
> Is this necessary for the purpose of the test, though?  It should
> not hurt, but we do not require the end-users to set it in real life
> for the parallel fetching to work, either, right?

IIUC it would make the test output deterministic if we fetched from both remotes. That doesn't happen here though, so I guess it's not doing anything right now.

Show 15 quoted lines

>> +		git fetch --all --no-recurse-submodules 2>../actual
>> +	) &&
>> +
>> +	cat >expect <<-EOF &&
>> +	From ../src
>> +	 * [new branch]      master     -> secondary/master
>> +	EOF
>> +	test_cmp expect actual
>> +'
>
> In the context of a series that attempts to introduce a new stable
> output format for machine consumption, which implies the traditional
> output can change to match human users' preference, this test feels
> a bit brittle, but let's wait until the end of the series to judge
> that.

I also find it a bit brittle to assert on the whole output when this test is about checking that we do not fetch the superproject.

Is there a reason you didn't go with the "grep for submodule lines" approach in the previous tests? If it's about catching regressions, IMO your PATCH 2/8 does a good enough job of doing that.

Wondering out loud, I wonder if it makes sense for us to make a bigger distinction between "tests whose purpose is to guard against unexpected changes in output" (i.e. snapshot tests) vs "tests that happen to use output as a way to assert behavior" (i.e. 'regular' behavioral tests). Many GUI app codebases have such a distinction and have different best practices around them.