Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2022, 18:22 UTC
Message-ID: <xmqq7d8hc2o2.fsf@gitster.g>
In-Reply-To: <Yj39lEzd/9AwQJaP@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 19 quoted lines

> On Fri, Mar 25, 2022 at 01:23:27PM -0400, Derrick Stolee wrote:
>> > Since we've made sure "before" is a one-liner earlier, we could just
>> > say
>>
>> > 		test_cmp before kept &&
>> >
>> > instead, no?
>>
>> 'before' contains a .idx name and 'kept' contains a .keep name,
>> so this direct comparison does not work. We could do that
>> additional check like this:
>>
>> 	kept_name=$(cat kept) &&
>> 	echo ${kept_name%.keep}.idx >kept-idx &&
>> 	test_cmp before kept-idx &&
>
> I think keeping this kind of post-condition check pretty minimal is
> favorable, since this is functionality of `git repack` (i.e., that `-a`
> leaves you with one kept) that is already tested thoroughly elsewhere.

It is nice in principle, but for this particular script, whose helper function's implementation relies on these "assumptions" to work correctly (e.g. imagine what happens when 'before' file had 0 or 2 lines in it and you called the get_sorted_objects_from_packs helper), we should be more defensive, and that is where all my suggestions in the thread come from.