Re: [PATCH v2 01/10] read-tree + fetch tests: test failing "--super-prefix" interaction

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 14, 2022, 19:14 UTC
Message-ID: <221114.86cz9pl4vn.gmgdl@evledraar.gmail.com>
In-Reply-To: <kl6lo7t91hvd.fsf@chooglen-macbookpro.roam.corp.google.com>

On Mon, Nov 14 2022, Glen Choo wrote:

Show 66 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> From: Glen Choo <chooglen@google.com>
>>
>> Ever since "git fetch --refetch" was introduced in 0f5e8851737 (Merge
>> branch 'rc/fetch-refetch', 2022-04-04) the test being added here would
>> fail. This is because "restore" will "read-tree .. --reset <hash>",
>> which will in turn invoke "fetch". The "fetch" will then die with:
>>
>> 	fatal: fetch doesn't support --super-prefix
>>
>> This edge case and other "--super-prefix" bugs will be fixed in
>> subsequent commits, but let's first add a "test_expect_failure" test
>> for it. It passes until the very last command in the test.
>>
>> Signed-off-by: Glen Choo <chooglen@google.com>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t5616-partial-clone.sh | 43 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
>> index 037941b95d2..e56466580cf 100755
>> --- a/t/t5616-partial-clone.sh
>> +++ b/t/t5616-partial-clone.sh
>> @@ -644,6 +644,49 @@ test_expect_success 'repack does not loosen promisor objects' '
>>  	grep "loosen_unused_packed_objects/loosened:0" trace
>>  '
>>  
>> +test_expect_failure 'lazy-fetch in submodule succeeds' '
>> +	# setup
>> +	test_config_global protocol.file.allow always &&
>> +
>> +	git init src-sub &&
>> +	git -C src-sub config uploadpack.allowfilter 1 &&
>> +	git -C src-sub config uploadpack.allowanysha1inwant 1 &&
>> +
>> +	# This blob must be missing in the subsequent commit.
>> +	echo foo >src-sub/file &&
>> +	git -C src-sub add file &&
>> +	git -C src-sub commit -m "submodule one" &&
>> +	SUB_ONE=$(git -C src-sub rev-parse HEAD) &&
>> +
>> +	echo bar >src-sub/file &&
>> +	git -C src-sub add file &&
>> +	git -C src-sub commit -m "submodule two" &&
>> +	SUB_TWO=$(git -C src-sub rev-parse HEAD) &&
>> +
>> +	git init src-super &&
>> +	git -C src-super config uploadpack.allowfilter 1 &&
>> +	git -C src-super config uploadpack.allowanysha1inwant 1 &&
>> +	git -C src-super submodule add ../src-sub src-sub &&
>> +
>> +	git -C src-super/src-sub checkout $SUB_ONE &&
>> +	git -C src-super add src-sub &&
>> +	git -C src-super commit -m "superproject one" &&
>> +
>> +	git -C src-super/src-sub checkout $SUB_TWO &&
>> +	git -C src-super add src-sub &&
>> +	git -C src-super commit -m "superproject two" &&
>> +
>> +	# the fetch
>> +	test_when_finished "rm -rf src-super src-sub client" &&
>
> (Genuinely curious) are we okay with test_when_finished in the middle of
> the test body instead of at the top?

Yeah, and it's not just supported, but preferred, usually we do:

	test_when_finished "rm -rf repo" &&
	git init repo &&

So at the top makes sense, but if there's a bunch of stuff we might fail on first it makes sense not to attempt the cleanup.

But I see in this case the "src-super" part of it should be earlier, and "src-sub", but the "client" should be just before the clone below, don't know how I missed that. Will fix.

Show 5 quoted lines

>> +
>> +	test_config_global protocol.file.allow always &&
>
> We have this exact same test_config_global line at the top of this test,
> so we can drop this one.

Ditto, thanks, I'll fix that. A mistake when combining the tests.

You had these as two tests, but one was mandatory setup for the other, and when making it test_expect_failure I wanted to have it atomic..