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

From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, chakrabortyabhradeep79@gmail.com
Subject: Re: [PATCH v2 1/2] t7700: check post-condition in kept-pack test
Date: Fri, 25 Mar 2022 11:22:53 -0700	[thread overview]
Message-ID: <xmqq7d8hc2o2.fsf@gitster.g> (raw)
In-Reply-To: <Yj39lEzd/9AwQJaP@nand.local> (Taylor Blau's message of "Fri, 25 Mar 2022 13:36:20 -0400")

Taylor Blau <me@ttaylorr.com> writes:

> 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.