Subject: Re: [PATCH v4 2/4] test-read-cache: set up repo after git directory

From: Lessley Dennington <[email-masked]>
Date: Mon, 29 Nov 2021 15:01:55 -0800

On 11/24/21 10:36 AM, Junio C Hamano wrote:
> Lessley Dennington <[email-masked]> writes:
> 
>> On 11/23/21 3:42 PM, Junio C Hamano wrote:
>>> "Lessley Dennington via GitGitGadget" <[email-masked]>
>>> writes:
>>>
>>>> From: Lessley Dennington <[email-masked]>
>>>>
>>>> Move repo setup to occur after git directory is set up. This will ensure
>>>> enabling the sparse index for `diff` (and guarding against the nongit
>>>> scenario) will not cause tests to start failing, since that change will include
>>>> adding a check to prepare_repo_settings() with the new BUG.
> 
>>> This looks obviously the right thing to do.  Would anything break
>>> because of the "wrong" ordering of events in the original code?
>>> IOW, can this "bugfix" be protected with a new test against
>>> regression?
>>>
>> Yep! Tests 2, 3, 28, and 34 in t1092-sparse-checkout-compatibility.sh
>> will fail without this change.
> 
> I do not understand.  When 1/4 and 2/4 are applied, no tests in
> t1092 fail for me.
> 
> I think the presentation order of this series is not reviewer
> friendly; "the new BUG" is introduced in a separate step and
> obscures the reason why this step is needed.  It is better than
> adding "the new BUG" first and let some tests fail and then fix the
> breakage the series caused in later steps, though.
> 
> 
> 
> 
What I meant was that those tests fail with the first patch in the
series. Once this patch is applied they no longer fail.

That being said, I agree with this feedback - I have reorganized the
changes for v5 in a way in which I hope will make more sense and will
improve the reviewer experience.

Lessley