[0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)

Message

Jonathan Nieder July 16, 2020, 6:20 a.m. UTC

Junio C Hamano wrote:

> Here is my quick attempt to see how far we can go with the
> "different endgame" approach, to be applied on top of those two
> patches.

Apologies again for the delay.

Here are patches implementing the minimal fix that I'd recommend.
These apply against "master" without requiring any other patches
as prerequisites.  Thoughts?

Jonathan Nieder (2):
  Revert "check_repository_format_gently(): refuse extensions for old
    repositories"
  repository: allow repository format upgrade with extensions

 cache.h                  |  1 -
 setup.c                  | 24 ++++++++++--------------
 t/t0410-partial-clone.sh | 15 +++++++++++++--
 3 files changed, 23 insertions(+), 17 deletions(-)

Comments

Johannes Schindelin July 16, 2020, 8:13 a.m. UTC | #1

Hi Jonathan,

On Wed, 15 Jul 2020, Jonathan Nieder wrote:

> Junio C Hamano wrote:
>
> > Here is my quick attempt to see how far we can go with the
> > "different endgame" approach, to be applied on top of those two
> > patches.
>
> Here are patches implementing the minimal fix that I'd recommend.
> These apply against "master" without requiring any other patches
> as prerequisites.  Thoughts?

IIUC all of the existing `extensions.*` predate the reverted strict check,
right? And the idea is that future `extensions.*` will only work when
`repositoryFormatVersion` is larger than 1, right?

I would have been fine with Junio's patch on top of Stolee's, and I am
equally fine with this patch series. My main aim is not so much
future-proofing, though, as it is to avoid regressions in existing setups.

Thanks,
Dscho

Derrick Stolee July 16, 2020, 12:17 p.m. UTC | #2

On 7/16/2020 4:13 AM, Johannes Schindelin wrote:
> Hi Jonathan,
> 
> On Wed, 15 Jul 2020, Jonathan Nieder wrote:
> 
>> Junio C Hamano wrote:
>>
>>> Here is my quick attempt to see how far we can go with the
>>> "different endgame" approach, to be applied on top of those two
>>> patches.
>>
>> Here are patches implementing the minimal fix that I'd recommend.
>> These apply against "master" without requiring any other patches
>> as prerequisites.  Thoughts?
> 
> IIUC all of the existing `extensions.*` predate the reverted strict check,
> right? And the idea is that future `extensions.*` will only work when
> `repositoryFormatVersion` is larger than 1, right?
> 
> I would have been fine with Junio's patch on top of Stolee's, and I am
> equally fine with this patch series. My main aim is not so much
> future-proofing, though, as it is to avoid regressions in existing setups.

I'm fine either way.  I think that Jonathan's patch comes from a
more informed place than my patches, so his are probably safer.

The situation that caught my interest is covered by this test
that was part of my patches:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 7cd45fc1394..6c0b82c3930 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -68,6 +68,18 @@ test_expect_success 'git sparse-checkout init' '
        check_files repo a
 '
 
+test_expect_success 'git sparse-checkout works if repository format is wrong' '
+       test_when_finished git -C repo config core.repositoryFormatVersion 1 &&
+       git -C repo config --unset core.repositoryFormatVersion &&
+       git -C repo sparse-checkout init &&
+       git -C repo config core.repositoryFormatVersion >actual &&
+       echo 1 >expect &&
+       git -C repo config core.repositoryFormatVersion 0 &&
+       git -C repo sparse-checkout init &&
+       git -C repo config core.repositoryFormatVersion >actual &&
+       test_cmp expect actual
+'
+
 test_expect_success 'git sparse-checkout list after init' '
        git -C repo sparse-checkout list >actual &&
        cat >expect <<-\EOF &&

and this test passes with Jonathan's series. I think this kind
of behavior is covered by his change to the 'converting to partial
clone fails with noop extension' test in t0410-partial-clone.sh,
so a duplicate test in t1091-sparse-checkout-builtin.sh may be
overkill.

Thanks, all.

-Stolee