Re: [PATCH v3 07/10] t: prepare for GIT_TEST_WRITE_REV_INDEX

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, dstolee@microsoft.com, jrnieder@gmail.com
Subject: Re: [PATCH v3 07/10] t: prepare for GIT_TEST_WRITE_REV_INDEX
Date: Thu, 28 Jan 2021 18:42:46 -0800	[thread overview]
Message-ID: <xmqqsg6ktqe1.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <YBNasXXZc1BEkMxH@coredump.intra.peff.net> (Jeff King's message of "Thu, 28 Jan 2021 19:45:37 -0500")

Jeff King <peff@peff.net> writes:

>>  	# Ensure that there are exactly 6 files (3 .pack and 3 .idx).
>> -	ls http_child/.git/objects/pack/* >filelist &&
>> -	test_line_count = 6 filelist
>> +	ls http_child/.git/objects/pack/*.pack >packlist &&
>> +	ls http_child/.git/objects/pack/*.idx >idxlist &&
>> +	test_line_count = 3 idxlist &&
>> +	test_line_count = 3 packlist
>>  '
>
> Hmm. Too bad we can't rely on shell brace expansion, as:
>
>   ls http_child/.git/objects/pack/*.{pack,idx}
>
> would be more readable. You could still do it in a single "ls" by
> writing out both arguments manually, but it's probably not that
> important.

This part looks a bit familiar as I had to fix the interaction with
Jonathan's topic, IIRC.  We need to update the comment.  It is not
ensuring "exact 6"---it merely is interested in having three
pack/idx pair, and carefully expressing that by preparing for the
presence of other cruft in objects/pack/ directory other people may
create (like ".rev", but we may gain more).

I wonder if we even _care_ about .idx.  Why not just count .pack, to
prepare for a possible distant future where we do not even write .idx
but append to existing multi-pack-index as we download a new pack
stream and store it in a .pack, or something?

>> -	ls .git/objects/pack | sort >existing_packs &&
>> +	ls .git/objects/pack/pack-*.pack | sort >existing_packs &&
>>  	test_commit "$(test_oid obj3)" &&
>>  	test_commit "$(test_oid obj4)" &&
>>  
>>  	git gc --auto 2>err &&
>>  	test_i18ngrep ! "^warning:" err &&
>> -	ls .git/objects/pack/ | sort >post_packs &&
>> +	ls .git/objects/pack/pack-*.pack | sort >post_packs &&
>>  	comm -1 -3 existing_packs post_packs >new &&
>>  	comm -2 -3 existing_packs post_packs >del &&
>>  	test_line_count = 0 del && # No packs are deleted
>> -	test_line_count = 2 new # There is one new pack and its .idx
>> +	test_line_count = 1 new # There is one new pack
>>  '
>
> This one is making the test a bit looser (it would miss a case where we
> somehow failed to generate the .idx). That seems like an unlikely bug,
> but I wonder if we can keep the original behavior. I guess:
>
>   ls .git/objects/pack/*.pack \
>      .git/objects/pack/*.idx |
>      sort >post_packs
>
> would work?

I guess we are looking at the same issue from opposite angle.  I
suspect that it might even be a good thing to only care about .pack
and ignore everything else in the longer run.