| Message ID | xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | setup: tweak upgrade policy to grandfather worktreeconfig | expand |
On 7/13/2020 4:18 PM, Junio C Hamano wrote: > The "fail and warn" approach introduced in the previous step may be > with smaller impact to the codebase, but > > - it requires the end-user to read, understand and execute the > manual upgrade > > - it encourages to follow the same procedure blindly, making the > protection even less useful > > Let's instead keep failing hard without teaching how to bypass the > repository protection, but allow upgrading even when only the > worktreeconfig extension exists in an old repository, which is > likely to be set by a broke version of Git that did not update the > repository version when setting the extension. This is a more subtle way to handle the case. In fact, it silently makes extensions.worktreeConfig work as it did in 2.27, which means users will not have any troubles after upgrading. I also like that you are actually fixing the case where a user in the bad state _can_ get out using "git sparse-checkout init". This can be verified by adding this test: 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 sparse-checkout init && git -C repo config core.repositoryFormatVersion >actual && echo 1 >expect && test_cmp expect actual && git -C repo config core.repositoryFormatVersion 0 && git -C repo sparse-checkout init && git -C repo config core.repositoryFormatVersion >actual && test_cmp expect actual ' > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * This time with a log message to explain and justify the change, > with a matching tweak to the test script, designed to be applied > on top, but feel free to squash it in if you agree with me that > we do not need two separate commits for this. Since this commit removes all evidence of the previous one, I would recommend just squashing them together. Thanks for your fast feedback! -Stolee
Derrick Stolee <stolee@gmail.com> writes: > I also like that you are actually fixing the case where a user in > the bad state _can_ get out using "git sparse-checkout init". > > This can be verified by adding this test: > > 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 sparse-checkout init && > git -C repo config core.repositoryFormatVersion >actual && > echo 1 >expect && > test_cmp expect actual && > git -C repo config core.repositoryFormatVersion 0 && > git -C repo sparse-checkout init && > git -C repo config core.repositoryFormatVersion >actual && > test_cmp expect actual > ' > >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- >> >> * This time with a log message to explain and justify the change, >> with a matching tweak to the test script, designed to be applied >> on top, but feel free to squash it in if you agree with me that >> we do not need two separate commits for this. > > Since this commit removes all evidence of the previous one, I would > recommend just squashing them together. Alright, then care to do the honors ;-)? Let's make sure we have it in -rc1 to avoid nasty "regression" in the upcoming release. Thanks for raising the issue and exploring the solution space.
Hi, Derrick Stolee wrote: > On 7/13/2020 4:18 PM, Junio C Hamano wrote: >> The "fail and warn" approach introduced in the previous step may be >> with smaller impact to the codebase, but >> >> - it requires the end-user to read, understand and execute the >> manual upgrade >> >> - it encourages to follow the same procedure blindly, making the >> protection even less useful >> >> Let's instead keep failing hard without teaching how to bypass the >> repository protection, but allow upgrading even when only the >> worktreeconfig extension exists in an old repository, which is >> likely to be set by a broke version of Git that did not update the >> repository version when setting the extension. > > This is a more subtle way to handle the case. In fact, it > silently makes extensions.worktreeConfig work as it did in 2.27, > which means users will not have any troubles after upgrading. I'd like to propose a different endgame: Instead of looking at `extensions.*` settings one by one to see how various implementations handled them with repositoryFormatVersion=0, what if we treat repositoryFormatVersion=0 as a synonym of version=1? That is: 1. in new repositories, set repositoryFormatVersion = 1, since (1) Git versions in the wild should all already know how to handle it and (2) as we've learned, other Git implementations need to understand some of extensions.* anyway 2. whenever setting any extensions.*, automatically upgrade to repositoryFormatVersion = 1 3. when in an existing repository with extensions.* set and repositoryFormatVersion = 0, act as though repositoryFormatVersion = 1 4. document this new behavior with repositoryFormatVersion = 0 in Documentation/technical/repository-version.txt This way, the result is simpler than where we started. Unfortunately, we are not even that consistent about what to do with extensions.* settings when repositoryFormatVersion = 0 today. So we'll have to exercise some care and analyze them one by one to make sure this is safe (and if it isn't, at *that* point we'd come up with a more complex variant on (2) and (3) above). It's too late to go that far for 2.28. It would be tempting to try a simple revert of 14c7fa269e4 (check_repository_format_gently(): refuse extensions for old repositories, 2020-06-05) to get back to tried and true behavior but that does not do enough --- it still produces an error when trying to upgrade repository format when any extensions are set. So how about such a revert plus Junio's patch plus the analogous change to Junio's patch for extensions.preciousObjects extensions.partialClone ? Thanks, Jonathan
Jonathan Nieder <jrnieder@gmail.com> writes: > It's too late to go that far for 2.28. It would be tempting to try a > simple revert of 14c7fa269e4 (check_repository_format_gently(): refuse > extensions for old repositories, 2020-06-05) to get back to tried and > true behavior but that does not do enough --- it still produces an > error when trying to upgrade repository format when any extensions are > set. So how about such a revert plus Junio's patch plus the analogous > change to Junio's patch for > > extensions.preciousObjects > extensions.partialClone My illustration patch was done "assuming that worktreeconfig is the only thing we wrote by mistake without updating the format version", and if these two also share the same problem, I obviously is 100% fine with covering these other ones with the same approach. I like your "v0 and v1 are the same, but the repository is declared to be corrupt if the extentions that are not known by today's code is found in v0 repository", by the way. Assuming that the two you listed above plus worktreeconfig are the only ones known by today's code, that is. We seem to also know about "noop", so shouldn't it also be grandfathered in?
diff --git a/cache.h b/cache.h index e5885cc9ea..8ff46857f6 100644 --- a/cache.h +++ b/cache.h @@ -1042,8 +1042,8 @@ struct repository_format { int worktree_config; int is_bare; int hash_algo; - int has_extensions; char *work_tree; + int has_unallowed_extensions; struct string_list unknown_extensions; }; diff --git a/setup.c b/setup.c index 6ff6c54d39..65270440a9 100644 --- a/setup.c +++ b/setup.c @@ -455,12 +455,13 @@ 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)) { - data->has_extensions = 1; /* * record any known extensions here; otherwise, * we fall through to recording it as unknown, and * check_repository_format will complain */ + int is_unallowed_extension = 1; + if (!strcmp(ext, "noop")) ; else if (!strcmp(ext, "preciousobjects")) @@ -469,10 +470,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata) if (!value) return config_error_nonbool(var); data->partial_clone = xstrdup(value); - } else if (!strcmp(ext, "worktreeconfig")) + } else if (!strcmp(ext, "worktreeconfig")) { data->worktree_config = git_config_bool(var, value); - else + is_unallowed_extension = 0; + } else string_list_append(&data->unknown_extensions, ext); + + data->has_unallowed_extensions |= is_unallowed_extension; } return read_worktree_config(var, value, vdata); @@ -542,11 +546,6 @@ static int check_repository_format_gently(const char *gitdir, struct repository_ } } - if (candidate->version == 0 && candidate->has_extensions) { - warning(_("some extensions are enabled, but core.repositoryFormatVersion=0")); - warning(_("if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'")); - } - return 0; } @@ -565,7 +564,7 @@ int upgrade_repository_format(int target_version) return 0; if (verify_repository_format(&repo_fmt, &err) < 0 || - (!repo_fmt.version && repo_fmt.has_extensions)) { + (!repo_fmt.version && repo_fmt.has_unallowed_extensions)) { warning("unable to upgrade repository format from %d to %d: %s", repo_fmt.version, target_version, err.buf); strbuf_release(&err); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index d249428f44..88cdde255c 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -68,15 +68,6 @@ test_expect_success 'git sparse-checkout init' ' check_files repo a ' -test_expect_success 'warning about core.repositoryFormatVersion' ' - test_when_finished git -C repo config core.repositoryFormatVersion 1 && - git -C repo status 2>err && - test_must_be_empty err && - git -C repo config --local core.repositoryFormatVersion 0 && - git -C repo status 2>err && - test_i18ngrep "some extensions are enabled, but core.repositoryFormatVersion=0" err -' - test_expect_success 'git sparse-checkout list after init' ' git -C repo sparse-checkout list >actual && cat >expect <<-\EOF &&
The "fail and warn" approach introduced in the previous step may be with smaller impact to the codebase, but - it requires the end-user to read, understand and execute the manual upgrade - it encourages to follow the same procedure blindly, making the protection even less useful Let's instead keep failing hard without teaching how to bypass the repository protection, but allow upgrading even when only the worktreeconfig extension exists in an old repository, which is likely to be set by a broke version of Git that did not update the repository version when setting the extension. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This time with a log message to explain and justify the change, with a matching tweak to the test script, designed to be applied on top, but feel free to squash it in if you agree with me that we do not need two separate commits for this. Thanks. cache.h | 2 +- setup.c | 17 ++++++++--------- t/t1091-sparse-checkout-builtin.sh | 9 --------- 3 files changed, 9 insertions(+), 19 deletions(-)