Lessley Dennington [off-list ref] writes:
On 11/23/21 3:42 PM, Junio C Hamano wrote:
quoted
"Lessley Dennington via GitGitGadget" [off-list ref]
writes:
quoted
From: Lessley Dennington <redacted>
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.
quoted
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.
On 11/24/21 10:36 AM, Junio C Hamano wrote:
Lessley Dennington [off-list ref] writes:
quoted
On 11/23/21 3:42 PM, Junio C Hamano wrote:
quoted
"Lessley Dennington via GitGitGadget" [off-list ref]
writes:
quoted
From: Lessley Dennington <redacted>
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.
quoted
quoted
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