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

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 25, 2022, 13:55 UTC
Message-ID: <7ea0f7e4-180d-307d-2e2e-d33c3343317c@github.com>
In-Reply-To: <Yjy/UIydKw7v+4+7@nand.local>

On 3/24/2022 2:58 PM, Taylor Blau wrote:

> On Thu, Mar 24, 2022 at 06:34:56PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>

Show 14 quoted lines

>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
>> index 770d1432046..73452e23896 100755
>> --- a/t/t7700-repack.sh
>> +++ b/t/t7700-repack.sh
>> @@ -369,10 +369,56 @@ test_expect_success '--write-midx with preferred bitmap tips' '
>>  	)
>>  '
>>
>> +get_sorted_objects_from_packs () {
>> +	git show-index <$(cat) >raw &&
> 
> It seems a little odd to me to pass the name of a single file as input
> to get_sorted_objects_from_packs over stdin. I probably would have
> expected something like `git show-index <"$1" >raw && ...` instead.

Based on the way we are creating a file whose contents is the name of the .idx file, we would at least use '$(cat "$1")'. I kind of like the symmetry of the input/output redirection when using the helper, but I can easily change this.

> We may also want to s/packs/pack, since this function only will handle
> one index at a time.

Yes.

Show 5 quoted lines

>> +	cut -d" " -f2 raw | sort
> 
> Having the sort in there is my fault, but after reading this more
> carefully it's definitely unnecessary, since show-index will give us
> the results in lexical order by object name already.

Cool. Will drop.

Show 26 quoted lines

>> +}
>> +
>>  test_expect_success '--write-midx -b packs non-kept objects' '
>> -	GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
>> -		git repack --write-midx -a -b &&
>> -	test_subcommand_inexact git pack-objects --honor-pack-keep <trace.txt
>> +	git init repo &&
>> +	test_when_finished "rm -fr repo" &&
>> +	(
>> +		cd repo &&
>> +
>> +		# Create a kept pack-file
>> +		test_commit base &&
>> +		git repack -ad &&
>> +		find $objdir/pack -name "*.idx" >before &&
> 
> I thought that here it might be easier to say:
> 
>     before="$(find $objdir/pack -name "*.idx")"
> 
>> +		>$objdir/pack/$(basename $(cat before) .idx).keep &&
> 
> ...and then replace "$(cat before)" with "$before", along with the
> other uses of the before file below. But it gets a little funny when
> you want to discover which is the new pack, where it is more natural to
> dump the output of comm into a file.

For this reason, I'll continue to store the .idx names in files.

Show 13 quoted lines

>> +		# Get object list from the one non-kept pack-file
>> +		comm -13 before after >new-pack &&
> 
> You could write "new_pack=$(comm -13 before after)", but debugging this
> test would be difficult if the output of comm there contained more than
> one line.
>
>> +		get_sorted_objects_from_packs \
>> +			<new-pack \
> 
> Though we probably want to check that we only get one line anyway here,
> since get_sorted_objects_from_packs will barf if we had more than one
> line in file new-pack here, too.

Thanks. Easy to add a test_line_count before this check.

-Stolee