Junio C Hamano <gitster@xxxxxxxxx> writes:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>
>>> If you don't mind, I was already going to squash Junio's commit into
>>> mine (almost completely replacing mine) but I could add a small
>>> commit on top that provides the following improvement to the error
>>> message:
>>
>> I don't mind at all. I'd just like to know that v2.28.0 avoids confusing
>> users in the same was as v2.28.0-rc0 confused me.
>
> In a nearby thread, Jonathan Nieder raised an interesting approach
> to avoid confusing users, which I think (if I am reading him
> correctly) makes sense (cf. <20200714040616.GA2208896@xxxxxxxxxx>)
>
> What if we accept the extensions the code before the topic in
> question that was merged in -rc0 introduced the "confusion" accepts
> even in v0? If we see extensions other than those handpicked and
> grandfathered ones (which are presumably the ones we add later and
> support in v1 and later repository versions) in a v0 repository, we
> keep ignoring. Also we'd loosen the overly strict code that
> prevents upgrading from v0 to v1 in the presence of any extensions
> in -rc0, so that the grandfathered ones will not prevent the
> upgrading.
>
> The original reasoning behind the strict check was because the users
> could have used extensions.frotz for their own use with their own
> meaning, trusting that Git would simply ignore it, and an upgrade to
> later version in which Git uses extensions.frotz for a purpose that
> is unrelated to the reason why these users used would just break the
> repository.
>
> But the ones that were (accidentally) honored in v0 couldn't have
> been used by the users for the purposes other than how Git would use
> them anyway, so there is no point to make them prevent the upgrade
> of the repository version from v0 to v1.
>
> At least, that is how I understood the world would look like in
> Jonathan's "different endgame".
>
> What do you three (Dscho, Derrick and Jonathan) think?
It seems that there is no quick concensus to go with your "different
endgame" and worse yet it seems nobody is interested in helping
much.
The current one on the table is NOT
<20200714040616.GA2208896@xxxxxxxxxx> but the two patches
<1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@xxxxxxxxx>
<e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@xxxxxxxxx>
which we may regret---it is far from a robust and complete solution,
but probably specific to users at Microsoft or something like that.
For example it special cases only the worktreeconfig and nothing
else, even though I suspect that other configuration variables were
also honored by mistake.
So...
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. It still has two known "breakages" and can use help from
extra eyeballs and real work.
I suspect that an expected test_must_fail not triggering t2404 may
even be a good thing if it is a sign of silent upgrading of the
repository version due to having grandfathered extensions in a v0
repository, but I didn't have time to dig further.
I'll shift my attention to other topics that should be in the
release for the rest of the day, but am pessimistic that I can tag
the -rc1 today, which won't happen until we at least have a
concensus on what to do with the (apparent) regression due to the
"upgrade repository version" topic.
Thanks.
setup.c | 52 +++++++++++++++++++++++++++-------------------
t/t0410-partial-clone.sh | 14 ++++++++++++-
t/t2404-worktree-config.sh | 2 +-
3 files changed, 45 insertions(+), 23 deletions(-)
diff --git a/setup.c b/setup.c
index 65270440a9..fe4e1ec066 100644
--- a/setup.c
+++ b/setup.c
@@ -455,28 +455,37 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
if (strcmp(var, "core.repositoryformatversion") == 0)
data->version = git_config_int(var, value);
else if (skip_prefix(var, "extensions.", &ext)) {
+ int unallowed_in_v0 = 1;
+
/*
- * record any known extensions here; otherwise,
- * we fall through to recording it as unknown, and
- * check_repository_format will complain
+ * The early ones are grandfathered---they existed in
+ * 2.27 which mistakenly honored even in repositories
+ * whose version is before v1 (where extensions are
+ * officially introduced).
*/
- int is_unallowed_extension = 1;
-
- if (!strcmp(ext, "noop"))
- ;
- else if (!strcmp(ext, "preciousobjects"))
+ if (!strcmp(ext, "noop")) {
+ unallowed_in_v0 = 0;
+ } else if (!strcmp(ext, "preciousobjects")) {
data->precious_objects = git_config_bool(var, value);
- else if (!strcmp(ext, "partialclone")) {
+ unallowed_in_v0 = 0;
+ } else if (!strcmp(ext, "partialclone")) {
if (!value)
return config_error_nonbool(var);
data->partial_clone = xstrdup(value);
+ unallowed_in_v0 = 0;
} else if (!strcmp(ext, "worktreeconfig")) {
data->worktree_config = git_config_bool(var, value);
- is_unallowed_extension = 0;
- } else
+ unallowed_in_v0 = 0;
+ /*
+ * Extensions are added by more "} else if (...) {"
+ * lines here, but do NOT mark them as allowed in v0
+ * by copy-pasting without thinking.
+ */
+ } else {
string_list_append(&data->unknown_extensions, ext);
+ }
- data->has_unallowed_extensions |= is_unallowed_extension;
+ data->has_unallowed_extensions |= unallowed_in_v0;
}
return read_worktree_config(var, value, vdata);
@@ -511,15 +520,16 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
die("%s", err.buf);
}
- if (candidate->version >= 1) {
- repository_format_precious_objects = candidate->precious_objects;
- set_repository_format_partial_clone(candidate->partial_clone);
- repository_format_worktree_config = candidate->worktree_config;
- } else {
- repository_format_precious_objects = 0;
- set_repository_format_partial_clone(NULL);
- repository_format_worktree_config = 0;
- }
+ /*
+ * Now we know the extensions in "candidate" repository are
+ * OK, let's copy them to the final place. Note that this is
+ * done even in v0 repositories, as long as the extensions are
+ * the grandfathered ones that used to be honored by mistake.
+ */
+ repository_format_precious_objects = candidate->precious_objects;
+ set_repository_format_partial_clone(candidate->partial_clone);
+ repository_format_worktree_config = candidate->worktree_config;
+
string_list_clear(&candidate->unknown_extensions, 0);
if (repository_format_worktree_config) {
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 463dc3a8be..2fc2d0bbfc 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '
test_cmp_config -C client 1 core.repositoryformatversion
'
-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '
+test_expect_success 'convert shallow clone to partial clone succeeds with grandfathered extension' '
rm -fr server client &&
test_create_repo server &&
test_commit -C server my_commit 1 &&
@@ -50,6 +50,18 @@ test_expect_success 'convert shallow clone to partial clone must fail with any e
git clone --depth=1 "file://$(pwd)/server" client &&
test_cmp_config -C client 0 core.repositoryformatversion &&
git -C client config extensions.partialclone origin &&
+ git -C client fetch --unshallow --filter="blob:none"
+'
+
+test_expect_failure 'convert shallow clone to partial clone must fail with unknown extension' '
+ rm -fr server client &&
+ test_create_repo server &&
+ test_commit -C server my_commit 1 &&
+ test_commit -C server my_commit2 1 &&
+ git clone --depth=1 "file://$(pwd)/server" client &&
+ test_cmp_config -C client 0 core.repositoryformatversion &&
+ git -C client config extensions.unknownExtension true &&
+ git -C client config extensions.partialclone origin &&
test_must_fail git -C client fetch --unshallow --filter="blob:none"
'
diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh
index 303a2644bd..b8c12df534 100755
--- a/t/t2404-worktree-config.sh
+++ b/t/t2404-worktree-config.sh
@@ -77,7 +77,7 @@ test_expect_success 'config.worktree no longer read without extension' '
test_cmp_config -C wt1 shared this.is &&
test_cmp_config -C wt2 shared this.is
'
-test_expect_success 'show advice when extensions.* are not enabled' '
+test_expect_failure 'show advice when extensions.* are not enabled' '
test_config core.repositoryformatversion 1 &&
test_config extensions.worktreeConfig true &&
git config --worktree test.one true &&
