Subject: Re: [PATCH 2/4] fsck: check rev-index checksums

From: Derrick Stolee <[email-masked]>
Date: Tue, 18 Apr 2023 10:27:57 -0400

On 4/17/2023 6:24 PM, Taylor Blau wrote:
> On Mon, Apr 17, 2023 at 04:21:39PM +0000, Derrick Stolee via GitGitGadget wrote:

>> +test_expect_success 'set up rev-index corruption tests' '
> 
> s/set up/setup for easy `--run`-ing (e.g., ./t5325-*.sh --run=setup,fsck).

Makes sense.

>> +test_expect_success 'fsck catches invalid checksum' '
>> +	revfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&
> 
> Would this test be tighter if we introduced a sub-shell and cd'd into
> "corrupt" here?

corrupt_rev_and_verify does the subshell thing. Why should we do that
here in the test?
 
>> +	orig_size=$(wc -c <$revfile) &&
> 
> I'm nitpicking, but we may want to use `test_file_size` here instead of
> `wc -c`. The latter outnumbers the former in terms of number of uses,
> but I think we consider `test_file_size` to be canonical these days.

Makes sense.

Thanks,
-Stolee