Re: [PATCH v2 2/3] fsck: verify multi-pack-index when implictly enabled

From: Glen Choo <chooglen@google.com>
Date: Sep 29, 2021, 22:56 UTC
Message-ID: <kl6lr1d7ourp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <ebe894b4-af25-f79e-c43f-1839e075908d@sunshineco.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 20 quoted lines

>>   test_expect_success 'git-fsck incorrect offset' '
>>   	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
>>   		"incorrect object offset" \
>> -		"git -c core.multipackindex=true fsck"
>> +		"git -c core.multipackindex=true fsck" &&
>> +		test_must_fail git fsck &&
>> +		git -c core.multipackindex=false fsck
>>   '
>
> I guess the newly-added `test_must_fail git fsck` line is checking the 
> fallback case then `core.multipackindex` is not set. We could make this 
> check a bit more robust by _ensuring_ that it is unset rather than 
> relying upon whatever state the configuration is in by the time this 
> test is reached. Perhaps something like this:
>
>      ...
>      "git -c core.multipackindex=true fsck" &&
>      test_unconfig core.multipackindex &&
>      test_must_fail git fsck &&
>      git -c core.multipackindex=false fsck

I think this extra robustness is worth it. I sometimes find that the tests are a bit too interdependent to read on their own, so this is a good step forward.

> The indentation is a bit unusual. It aligns nicely and is, in some 
> sense, easy to read, but the two new lines are over-indented considering 
> that they are siblings of the corrupt_midx_and_verify() call.

I agree. This was a typo from me, not a conscious choice.