| Message ID | pull.675.git.1594677321039.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | setup: warn about un-enabled extensions | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > I did actually run into this today. One of my pipelines is configured to > clone a bare repository, then set up a sparse secondary worktree. This > used to work, but all of a sudden, the git config --worktree > core.sparseCheckout true call failed because I'm now using v2.28.0-rc0. I guess a few people were independently hit and then approached the same issue from different angles? If so, can you two compare notes to help us all come up with a single good solution, preferrably by -rc1? Thanks.
On 7/13/2020 5:55 PM, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > When any `extensions.*` setting is configured, we newly ignore it unless > `core.repositoryFormatVersion` is set to a positive value. > > This might be quite surprising, e.g. when calling `git config --worktree > [...]` elicits a warning that it requires > `extensions.worktreeConfig = true` when that setting _is_ configured > (but ignored because `core.repositoryFormatVersion` is unset). > > Let's warn about this situation specifically, especially because there > might be already setups out there that configured a sparse worktree > using Git v2.27.0 (which does set `extensions.worktreeConfig` but not > `core.repositoryFormatVersion`) and users might want to work in those > setups with Git v2.28.0, too. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > Warn when extensions.* is ignored > > I did actually run into this today. One of my pipelines is configured to > clone a bare repository, then set up a sparse secondary worktree. This > used to work, but all of a sudden, the git config --worktree > core.sparseCheckout true call failed because I'm now using v2.28.0-rc0. I tried your situation with Junio's patch from earlier [1] [2]. [1] https://lore.kernel.org/git/pull.674.git.1594668051847.gitgitgadget@gmail.com/ [2] https://lore.kernel.org/git/xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com/ The issue here is that Junio's silent fix for sparse-checkout doesn't work here for "git config --worktree". However, I think that Johannes is making the same over-compensating warning message pattern as I was. That is, this warning happens for all extensions that are enabled when core.repositoryFormatVersion is less than 1. To attempt to summarize Junio's opinion, we should keep our situation isolated to this worktree config extension. Your patch does agree with the others in that we don't revert the behavior of failing to set the config, but I think in this instance we can specify the warning more carefully. 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: diff --git a/builtin/config.c b/builtin/config.c index 5e39f618854..b5de7982a93 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) else if (worktrees[0] && worktrees[1]) die(_("--worktree cannot be used with multiple " "working trees unless the config\n" - "extension worktreeConfig is enabled. " - "Please read \"CONFIGURATION FILE\"\n" + "extension worktreeConfig is enabled " + "and core.repositoryFormatVersion is at least\n" + "1. Please read \"CONFIGURATION FILE\"" "section in \"git help worktree\" for details")); else given_config_source.file = git_pathdup("config"); Thanks, -Stolee
Hi Stolee, On Mon, 13 Jul 2020, Derrick Stolee wrote: > On 7/13/2020 5:55 PM, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > When any `extensions.*` setting is configured, we newly ignore it unless > > `core.repositoryFormatVersion` is set to a positive value. > > > > This might be quite surprising, e.g. when calling `git config --worktree > > [...]` elicits a warning that it requires > > `extensions.worktreeConfig = true` when that setting _is_ configured > > (but ignored because `core.repositoryFormatVersion` is unset). > > > > Let's warn about this situation specifically, especially because there > > might be already setups out there that configured a sparse worktree > > using Git v2.27.0 (which does set `extensions.worktreeConfig` but not > > `core.repositoryFormatVersion`) and users might want to work in those > > setups with Git v2.28.0, too. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > Warn when extensions.* is ignored > > > > I did actually run into this today. One of my pipelines is configured to > > clone a bare repository, then set up a sparse secondary worktree. This > > used to work, but all of a sudden, the git config --worktree > > core.sparseCheckout true call failed because I'm now using v2.28.0-rc0. > > I tried your situation with Junio's patch from earlier [1] [2]. > > [1] https://lore.kernel.org/git/pull.674.git.1594668051847.gitgitgadget@gmail.com/ > [2] https://lore.kernel.org/git/xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com/ > > The issue here is that Junio's silent fix for sparse-checkout doesn't > work here for "git config --worktree". However, I think that Johannes > is making the same over-compensating warning message pattern as I was. > That is, this warning happens for all extensions that are enabled when > core.repositoryFormatVersion is less than 1. > > To attempt to summarize Junio's opinion, we should keep our situation > isolated to this worktree config extension. Your patch does agree with > the others in that we don't revert the behavior of failing to set the > config, but I think in this instance we can specify the warning more > carefully. Okay. > 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. Thanks, Dscho > > diff --git a/builtin/config.c b/builtin/config.c > index 5e39f618854..b5de7982a93 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) > else if (worktrees[0] && worktrees[1]) > die(_("--worktree cannot be used with multiple " > "working trees unless the config\n" > - "extension worktreeConfig is enabled. " > - "Please read \"CONFIGURATION FILE\"\n" > + "extension worktreeConfig is enabled " > + "and core.repositoryFormatVersion is at least\n" > + "1. Please read \"CONFIGURATION FILE\"" > "section in \"git help worktree\" for details")); > else > given_config_source.file = git_pathdup("config"); > > Thanks, > -Stolee > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@google.com>) 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? > Thanks, > Dscho > >> >> diff --git a/builtin/config.c b/builtin/config.c >> index 5e39f618854..b5de7982a93 100644 >> --- a/builtin/config.c >> +++ b/builtin/config.c >> @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) >> else if (worktrees[0] && worktrees[1]) >> die(_("--worktree cannot be used with multiple " >> "working trees unless the config\n" >> - "extension worktreeConfig is enabled. " >> - "Please read \"CONFIGURATION FILE\"\n" >> + "extension worktreeConfig is enabled " >> + "and core.repositoryFormatVersion is at least\n" >> + "1. Please read \"CONFIGURATION FILE\"" >> "section in \"git help worktree\" for details")); >> else >> given_config_source.file = git_pathdup("config"); >> >> Thanks, >> -Stolee >> >>
On 7/14/2020 11:27 AM, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@google.com>) > > 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? If "v0" includes "core.repositoryFormatVersion is unset" then I would consider this to be a way to avoid all user pain, which is positive. I'd be happy to test and review a patch that accomplishes this goal. CC'ing Ed Thomson because this extension stuff affects other tools, like libgit2. Thanks, -Stolee
Hi, On Tue, 14 Jul 2020, Derrick Stolee wrote: > On 7/14/2020 11:27 AM, Junio C Hamano wrote: > > Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@google.com>) > > > > 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? > > If "v0" includes "core.repositoryFormatVersion is unset" then I > would consider this to be a way to avoid all user pain, which is > positive. I concur. > I'd be happy to test and review a patch that accomplishes this > goal. Wouldn't that just be a matter of extending your patch to re-set `has_unhandled_extensions` also for `preciousObjects` and `partialClone`? Ciao, Dscho > > CC'ing Ed Thomson because this extension stuff affects other tools, > like libgit2. > > Thanks, > -Stolee > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Tue, 14 Jul 2020, Derrick Stolee wrote: > >> If "v0" includes "core.repositoryFormatVersion is unset" then I >> would consider this to be a way to avoid all user pain, which is >> positive. > > I concur. > >> I'd be happy to test and review a patch that accomplishes this >> goal. > > Wouldn't that just be a matter of extending your patch to re-set > `has_unhandled_extensions` also for `preciousObjects` and `partialClone`? It probably needs a bit more than that. For example there is this bit in check_repository_format_gently() that clears the unwanted extensions that we used to honor by mistake in v0 repository 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; } and the "different endgame" advocates to keep honoring these (and only these), the else clause probably needs to go. There may be some other tweaks necessary.
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> 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@google.com>) > > 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@google.com> but the two patches <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com> <e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com> 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 &&
Junio C Hamano <gitster@pobox.com> writes: > The current one on the table is NOT > <20200714040616.GA2208896@google.com> but the two patches > > <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com> > <e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com> > > For example it special cases only the worktreeconfig and nothing > else, even though I suspect that other configuration variables were > also honored by mistake. The attached may be a less ambitious and less risky update for the upcoming release. It is to be applied on top of the two-patch series from Derrick, and just marks the other "known and honored back then by mistake" extensions as OK to be there for upgrading. Thoughts? If people are happy with that, then we could apply and cut an -rc1 with it. Or if we are OK with the "just special case worktreeconfig; other extensions may have the same issue but we haven't heard actual complaints so we will leave them untouched", then -rc1 can be done with just those two patches. Now I do need to shift my attention to other topics in flight. Thanks. setup.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/setup.c b/setup.c index 65270440a9..a072c76d05 100644 --- a/setup.c +++ b/setup.c @@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata) data->version = git_config_int(var, value); else if (skip_prefix(var, "extensions.", &ext)) { /* - * record any known extensions here; otherwise, - * we fall through to recording it as unknown, and - * check_repository_format will complain + * Grandfather extensions that were known in 2.27 and + * were honored by mistake even in v0 repositories; it + * shoudn't be an error to upgrade v0 to v1 with them + * in the repository, as they couldn't have been used + * for incompatible purposes by the end user. */ - int is_unallowed_extension = 1; + int unallowed_in_v0 = 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; + } 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);
On 7/15/2020 1:01 PM, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> The current one on the table is NOT >> <20200714040616.GA2208896@google.com> but the two patches >> >> <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com> >> <e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com> >> >> For example it special cases only the worktreeconfig and nothing >> else, even though I suspect that other configuration variables were >> also honored by mistake. Sorry for the delay. I had your previous diff applied and was playing around with the two "known breakages". The diff below _does_ fail on t0410-partial-clone.sh, but it's because you do change the behavior. Here is my diff hunk for that test: diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 463dc3a8be..fc8da56528 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,7 +50,20 @@ 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 && - test_must_fail git -C client fetch --unshallow --filter="blob:none" + git -C client fetch --unshallow --filter="blob:none" +' + +test_expect_success '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" 2>err && + test_i18ngrep "unable to upgrade repository format from 0 to 1" err ' test_expect_success 'missing reflog object, but promised by a commit, passes fsck' ' > The attached may be a less ambitious and less risky update for the > upcoming release. It is to be applied on top of the two-patch > series from Derrick, and just marks the other "known and honored > back then by mistake" extensions as OK to be there for upgrading. > > Thoughts? If people are happy with that, then we could apply and > cut an -rc1 with it. Or if we are OK with the "just special case > worktreeconfig; other extensions may have the same issue but we > haven't heard actual complaints so we will leave them untouched", > then -rc1 can be done with just those two patches. The "special case wortreeConfig" of my submission was maybe based on an incorrect assumption that this is the only place where Git itself set an extension.* config without _also_ updating the core.repositoryFormatVersion config. The error I made in my v1 is to warn on ALL extensions, not just the ones that Git knows about. This new approach is a good middle ground between my v1 and v2. Of course, the _other_ option is to revert xl/upgrade-repo-format from v2.28.0 and take our time resolving this issue during the 2.29 cycle. I'm not sure how disruptive that action would be. > Now I do need to shift my attention to other topics in flight. I appreciate you spending so much time on this! It's a tough time to be noticing such a complicated situation that is not easily testable from a single Git version, but across versions. > > setup.c | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/setup.c b/setup.c > index 65270440a9..a072c76d05 100644 > --- a/setup.c > +++ b/setup.c > @@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata) > data->version = git_config_int(var, value); > else if (skip_prefix(var, "extensions.", &ext)) { > /* > - * record any known extensions here; otherwise, > - * we fall through to recording it as unknown, and > - * check_repository_format will complain > + * Grandfather extensions that were known in 2.27 and > + * were honored by mistake even in v0 repositories; it > + * shoudn't be an error to upgrade v0 to v1 with them > + * in the repository, as they couldn't have been used > + * for incompatible purposes by the end user. > */ > - int is_unallowed_extension = 1; > + int unallowed_in_v0 = 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; Your previous diff had this comment, which I thought to be helpful: + /* + * Extensions are added by more "} else if (...) {" + * lines here, but do NOT mark them as allowed in v0 + * by copy-pasting without thinking. + */ > - } else > + unallowed_in_v0 = 0; > + } 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); Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > Your previous diff had this comment, which I thought to be > helpful: > > + /* > + * Extensions are added by more "} else if (...) {" > + * lines here, but do NOT mark them as allowed in v0 > + * by copy-pasting without thinking. > + */ Yeah, but it felt somewhat strange to have it at the end of one entry, like this: + unallowed_in_v0 = 0; } else if (!strcmp(ext, "worktreeconfig")) { data->worktree_config = git_config_bool(var, value); + 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); In any case, I updated the comment in front of the if/else if/ cascade to essentially say the same thing, and with test updates this time. Thanks. -- >8 -- Subject: [PATCH] setup: grandfather other extensions that used to be honored by mistake We special cased worktreeconfig extension to be OK to exist when the repository gets converted from v0 to v1 in an earlier commit, but other extenions were also honored by mistake in v0. Mark them the same way so that they won't interfere with the upgrading from v0 to v1. A test in t0410 used to expect that presence of the extension.partialclone configuration variable to interfere, but that no longer is true. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- setup.c | 30 +++++++++++++++++++----------- t/t0410-partial-clone.sh | 18 ++++++++++++++++-- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/setup.c b/setup.c index 65270440a9..97292479d6 100644 --- a/setup.c +++ b/setup.c @@ -456,27 +456,35 @@ static int check_repo_format(const char *var, const char *value, void *vdata) data->version = git_config_int(var, value); else if (skip_prefix(var, "extensions.", &ext)) { /* - * record any known extensions here; otherwise, - * we fall through to recording it as unknown, and - * check_repository_format will complain + * Grandfather extensions that were known in 2.27 and + * were honored by mistake even in v0 repositories; it + * shoudn't be an error to upgrade v0 to v1 with them + * in the repository, as they couldn't have been used + * for incompatible purposes by the end user. + * + * When adding new extensions support in this if/elseif/... + * cascade, do not mark them as allowed in v0! */ - int is_unallowed_extension = 1; + int unallowed_in_v0 = 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; + } 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); diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 463dc3a8be..e9674fc257 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 is OK with a grandfathered extension' ' rm -fr server client && test_create_repo server && test_commit -C server my_commit 1 && @@ -50,7 +50,21 @@ 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 && - test_must_fail git -C client fetch --unshallow --filter="blob:none" + git -C client fetch --unshallow --filter="blob:none" && + test_cmp_config -C client 1 core.repositoryformatversion +' + +test_expect_success 'convert shallow clone to partial clone must fail with an 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.partialclone origin && + git -C client config extensions.unknown true && + test_must_fail git -C client fetch --unshallow --filter="blob:none" && + test_cmp_config -C client 0 core.repositoryformatversion ' test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '
Derrick Stolee <stolee@gmail.com> writes: > Of course, the _other_ option is to revert xl/upgrade-repo-format > from v2.28.0 and take our time resolving this issue during the > 2.29 cycle. I'm not sure how disruptive that action would be. Yes, that is becoming very much tempting at this point, isn't it? In any case, I've pushed out 'seen' with the "these extensions that used to be honored in v0 won't interfere with repository upgrade" patch I sent earlier, and I am hoping that it would be a reasonable middle ground that won't regress things for users while making sure we do not honor random future extensions by mistake.
Hi, Junio C Hamano wrote: > 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. Sorry for the delay. I do want to look at this this afternoon. [...] > 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. Ah, I hadn't realized an -rc1 tomorrow instead of today was on the table. ;-) I'll do what I can (in other words, expect a patch from me; but also, I am very interested in analysis, proposed patches, etc from others so that we can end up wit ha good fix with the solution space well explored). Thanks, Jonathan
On 7/15/2020 2:09 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> Your previous diff had this comment, which I thought to be >> helpful: >> >> + /* >> + * Extensions are added by more "} else if (...) {" >> + * lines here, but do NOT mark them as allowed in v0 >> + * by copy-pasting without thinking. >> + */ > > Yeah, but it felt somewhat strange to have it at the end of one > entry, like this: > > + unallowed_in_v0 = 0; > } else if (!strcmp(ext, "worktreeconfig")) { > data->worktree_config = git_config_bool(var, value); > + 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); > > > In any case, I updated the comment in front of the if/else if/ > cascade to essentially say the same thing, and with test updates > this time. Thanks. I applied and tested this version. LGTM! I'll also review Jonathan Nieder's patches when they arrive. Thanks, -Stolee
Hi, On Wed, 15 Jul 2020, Derrick Stolee wrote: > On 7/15/2020 2:09 PM, Junio C Hamano wrote: > > Derrick Stolee <stolee@gmail.com> writes: > > > >> Your previous diff had this comment, which I thought to be > >> helpful: > >> > >> + /* > >> + * Extensions are added by more "} else if (...) {" > >> + * lines here, but do NOT mark them as allowed in v0 > >> + * by copy-pasting without thinking. > >> + */ > > > > Yeah, but it felt somewhat strange to have it at the end of one > > entry, like this: > > > > + unallowed_in_v0 = 0; > > } else if (!strcmp(ext, "worktreeconfig")) { > > data->worktree_config = git_config_bool(var, value); > > + 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); > > > > > > In any case, I updated the comment in front of the if/else if/ > > cascade to essentially say the same thing, and with test updates > > this time. > > Thanks. I applied and tested this version. LGTM! Sorry for being slow at the party; I'd much rather deal with Git issues than with the paperwork I am fighting with. Thank you for working on this, I am pretty happy with the state you whipped it into, but take that with a grain of salt, as my brain is moosh. Ciao, Dscho
Hi Junio, On Wed, 15 Jul 2020, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > > > Of course, the _other_ option is to revert xl/upgrade-repo-format > > from v2.28.0 and take our time resolving this issue during the > > 2.29 cycle. I'm not sure how disruptive that action would be. > > Yes, that is becoming very much tempting at this point, isn't it? It did cross my mind, too. > In any case, I've pushed out 'seen' with the "these extensions that > used to be honored in v0 won't interfere with repository upgrade" > patch I sent earlier, and I am hoping that it would be a reasonable > middle ground that won't regress things for users while making sure > we do not honor random future extensions by mistake. Given that we still have -rc1 and -rc2 to make sure that things work, and given that your patch on top of Stolee's two patches looks like it is Doing The Best We Can Do, I am optimistic that your reasonable middle ground is the best way forward. Thanks, Dscho
Jonathan Nieder <jrnieder@gmail.com> writes: >> 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. > > Ah, I hadn't realized an -rc1 tomorrow instead of today was on the > table. ;-) It could be delayed even more if we do not have a good solution agreed upon. > I'll do what I can (in other words, expect a patch from me; but also, > I am very interested in analysis, proposed patches, etc from others so > that we can end up wit ha good fix with the solution space well > explored). Yup, that's the spirit. Thanks!
diff --git a/cache.h b/cache.h index 126ec56c7f..da2c71f366 100644 --- a/cache.h +++ b/cache.h @@ -1042,7 +1042,7 @@ struct repository_format { int worktree_config; int is_bare; int hash_algo; - int has_extensions; + int has_extensions, saw_extensions; char *work_tree; struct string_list unknown_extensions; }; diff --git a/setup.c b/setup.c index dbac2eabe8..0f45e2e174 100644 --- a/setup.c +++ b/setup.c @@ -489,6 +489,15 @@ static int check_repository_format_gently(const char *gitdir, struct repository_ read_repository_format(candidate, sb.buf); strbuf_release(&sb); + if (candidate->version < 1 && + (candidate->saw_extensions || candidate->has_extensions)) + advise(_("extensions.* settings require a positive repository " + "format version greater than zero.\n" + "\n" + "Please use the following call to enable extensions.* " + "config settings:\n" + "\"git config core.repositoryFormatVersion 1\"")); + /* * For historical use of check_repository_format() in git-init, * we treat a missing config as a silent "ok", even when nongit_ok @@ -584,8 +593,13 @@ int read_repository_format(struct repository_format *format, const char *path) { clear_repository_format(format); git_config_from_file(check_repo_format, path, format); - if (format->version == -1) + if (format->version == -1) { + int saw_extensions = format->has_extensions; + clear_repository_format(format); + + format->saw_extensions = saw_extensions; + } return format->version; } diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh index 9536d10919..1c08a45177 100755 --- a/t/t2404-worktree-config.sh +++ b/t/t2404-worktree-config.sh @@ -78,4 +78,19 @@ test_expect_success 'config.worktree no longer read without extension' ' test_cmp_config -C wt2 shared this.is ' +test_expect_success 'show advice when extensions.* are not enabled' ' + test_config core.repositoryformatversion 1 && + test_config extensions.worktreeConfig true && + git status 2>err && + test_i18ngrep ! "git config core.repositoryFormatVersion 1" err && + + test_config core.repositoryformatversion 0 && + git status 2>err && + test_i18ngrep "git config core.repositoryFormatVersion 1" err && + + git config --unset core.repositoryformatversion && + git status 2>err && + test_i18ngrep "git config core.repositoryFormatVersion 1" err +' + test_done