On Tue, May 09, 2023 at 11:27:35AM -0700, Glen Choo wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > >> + 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. Right, will drop. > >> + 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. Fair enough, will switch to `! grep "Fetching submodule"`. Patrick
Attachment:
signature.asc
Description: PGP signature
