{"thread":{"id":"53852","subject":"[PATCH] setup: warn about un-enabled extensions","startedAt":"2020-07-13T21:55:25Z","lastAt":"2020-07-17T17:08:03Z","messageCount":39,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Derrick Stolee","Johannes Schindelin","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401519","messageId":"pull.675.git.1594677321039.gitgitgadget@gmail.com","threadId":"53852","inReplyTo":null,"subject":"[PATCH] setup: warn about un-enabled extensions","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-13T21:55:20Z","receivedAt":"2020-07-13T21:55:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen any `extensions.*` setting is configured, we newly ignore it unless\n`core.repositoryFormatVersion` is set to a positive value.\n\nThis might be quite surprising, e.g. when calling `git config --worktree\n[...]` elicits a warning that it requires\n`extensions.worktreeConfig = true` when that setting _is_ configured\n(but ignored because `core.repositoryFormatVersion` is unset).\n\nLet's warn about this situation specifically, especially because there\nmight be already setups out there that configured a sparse worktree\nusing Git v2.27.0 (which does set `extensions.worktreeConfig` but not\n`core.repositoryFormatVersion`) and users might want to work in those\nsetups with Git v2.28.0, too.\n\nSigned-off-by: Johannes Schindelin \n---\n Warn when extensions.* is ignored\n \n I did actually run into this today. One of my pipelines is configured to\n clone a bare repository, then set up a sparse secondary worktree. This\n used to work, but all of a sudden, the git config --worktree\n core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-675%2Fdscho%2Frepo-format-version-advice-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-675/dscho/repo-format-version-advice-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/675\n\n cache.h | 2 +-\n setup.c | 16 +++++++++++++++-\n t/t2404-worktree-config.sh | 15 +++++++++++++++\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 126ec56c7f..da2c71f366 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,7 +1042,7 @@ struct repository_format {\n \tint worktree_config;\n \tint is_bare;\n \tint hash_algo;\n-\tint has_extensions;\n+\tint has_extensions, saw_extensions;\n \tchar *work_tree;\n \tstruct string_list unknown_extensions;\n };\ndiff --git a/setup.c b/setup.c\nindex dbac2eabe8..0f45e2e174 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -489,6 +489,15 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \tread_repository_format(candidate, sb.buf);\n \tstrbuf_release(&sb);\n \n+\tif (candidate->version < 1 &&\n+\t (candidate->saw_extensions || candidate->has_extensions))\n+\t\tadvise(_(\"extensions.* settings require a positive repository \"\n+\t\t\t \"format version greater than zero.\\n\"\n+\t\t\t \"\\n\"\n+\t\t\t \"Please use the following call to enable extensions.* \"\n+\t\t\t \"config settings:\\n\"\n+\t\t\t \"\\\"git config core.repositoryFormatVersion 1\\\"\"));\n+\n \t/*\n \t * For historical use of check_repository_format() in git-init,\n \t * we treat a missing config as a silent \"ok\", even when nongit_ok\n@@ -584,8 +593,13 @@ int read_repository_format(struct repository_format *format, const char *path)\n {\n \tclear_repository_format(format);\n \tgit_config_from_file(check_repo_format, path, format);\n-\tif (format->version == -1)\n+\tif (format->version == -1) {\n+\t\tint saw_extensions = format->has_extensions;\n+\n \t\tclear_repository_format(format);\n+\n+\t\tformat->saw_extensions = saw_extensions;\n+\t}\n \treturn format->version;\n }\n \ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 9536d10919..1c08a45177 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -78,4 +78,19 @@ test_expect_success 'config.worktree no longer read without extension' '\n \ttest_cmp_config -C wt2 shared this.is\n '\n \n+test_expect_success 'show advice when extensions.* are not enabled' '\n+\ttest_config core.repositoryformatversion 1 &&\n+\ttest_config extensions.worktreeConfig true &&\n+\tgit status 2>err &&\n+\ttest_i18ngrep ! \"git config core.repositoryFormatVersion 1\" err &&\n+\n+\ttest_config core.repositoryformatversion 0 &&\n+\tgit status 2>err &&\n+\ttest_i18ngrep \"git config core.repositoryFormatVersion 1\" err &&\n+\n+\tgit config --unset core.repositoryformatversion &&\n+\tgit status 2>err &&\n+\ttest_i18ngrep \"git config core.repositoryFormatVersion 1\" err\n+'\n+\n test_done\n\nbase-commit: bd42bbe1a46c0fe486fc33e82969275e27e4dc19\n-- \ngitgitgadget\n"},{"id":"401521","messageId":"xmqqh7ubm3ol.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"pull.675.git.1594677321039.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-13T22:48:58Z","receivedAt":"2020-07-13T22:49:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> I did actually run into this today. One of my pipelines is configured to\n> clone a bare repository, then set up a sparse secondary worktree. This\n> used to work, but all of a sudden, the git config --worktree\n> core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.\n\nI guess a few people were independently hit and then approached the\nsame issue from different angles? If so, can you two compare notes\nto help us all come up with a single good solution, preferrably by\n-rc1?\n\nThanks.\n\n"},{"id":"401523","messageId":"0bede821-139a-d805-934a-142004abaa4c@gmail.com","threadId":"53852","inReplyTo":"pull.675.git.1594677321039.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-14T00:24:53Z","receivedAt":"2020-07-14T00:24:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/13/2020 5:55 PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When any `extensions.*` setting is configured, we newly ignore it unless\n> `core.repositoryFormatVersion` is set to a positive value.\n> \n> This might be quite surprising, e.g. when calling `git config --worktree\n> [...]` elicits a warning that it requires\n> `extensions.worktreeConfig = true` when that setting _is_ configured\n> (but ignored because `core.repositoryFormatVersion` is unset).\n> \n> Let's warn about this situation specifically, especially because there\n> might be already setups out there that configured a sparse worktree\n> using Git v2.27.0 (which does set `extensions.worktreeConfig` but not\n> `core.repositoryFormatVersion`) and users might want to work in those\n> setups with Git v2.28.0, too.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> Warn when extensions.* is ignored\n> \n> I did actually run into this today. One of my pipelines is configured to\n> clone a bare repository, then set up a sparse secondary worktree. This\n> used to work, but all of a sudden, the git config --worktree\n> core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.\n\nI tried your situation with Junio's patch from earlier [1] [2].\n\n[1] https://lore.kernel.org/git/pull.674.git.1594668051847.gitgitgadget@gmail.com/\n[2] https://lore.kernel.org/git/xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com/\t\n\nThe issue here is that Junio's silent fix for sparse-checkout doesn't\nwork here for \"git config --worktree\". However, I think that Johannes\nis making the same over-compensating warning message pattern as I was.\nThat is, this warning happens for all extensions that are enabled when\ncore.repositoryFormatVersion is less than 1.\n\nTo attempt to summarize Junio's opinion, we should keep our situation\nisolated to this worktree config extension. Your patch does agree with\nthe others in that we don't revert the behavior of failing to set the\nconfig, but I think in this instance we can specify the warning more\ncarefully.\n\nIf you don't mind, I was already going to squash Junio's commit into\nmine (almost completely replacing mine) but I could add a small\ncommit on top that provides the following improvement to the error\nmessage:\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 5e39f618854..b5de7982a93 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n else if (worktrees[0] && worktrees[1])\n die(_(\"--worktree cannot be used with multiple \"\n \"working trees unless the config\\n\"\n- \"extension worktreeConfig is enabled. \"\n- \"Please read \\\"CONFIGURATION FILE\\\"\\n\"\n+ \"extension worktreeConfig is enabled \"\n+ \"and core.repositoryFormatVersion is at least\\n\"\n+ \"1. Please read \\\"CONFIGURATION FILE\\\"\"\n \"section in \\\"git help worktree\\\" for details\"));\n else\n given_config_source.file = git_pathdup(\"config\");\n\nThanks,\n-Stolee\n"},{"id":"401544","messageId":"nycvar.QRO.7.76.6.2007141420300.52@tvgsbejvaqbjf.bet","threadId":"53852","inReplyTo":"0bede821-139a-d805-934a-142004abaa4c@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-14T12:21:30Z","receivedAt":"2020-07-14T12:21:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Mon, 13 Jul 2020, Derrick Stolee wrote:\n\n> On 7/13/2020 5:55 PM, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > When any `extensions.*` setting is configured, we newly ignore it unless\n> > `core.repositoryFormatVersion` is set to a positive value.\n> >\n> > This might be quite surprising, e.g. when calling `git config --worktree\n> > [...]` elicits a warning that it requires\n> > `extensions.worktreeConfig = true` when that setting _is_ configured\n> > (but ignored because `core.repositoryFormatVersion` is unset).\n> >\n> > Let's warn about this situation specifically, especially because there\n> > might be already setups out there that configured a sparse worktree\n> > using Git v2.27.0 (which does set `extensions.worktreeConfig` but not\n> > `core.repositoryFormatVersion`) and users might want to work in those\n> > setups with Git v2.28.0, too.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > Warn when extensions.* is ignored\n> >\n> > I did actually run into this today. One of my pipelines is configured to\n> > clone a bare repository, then set up a sparse secondary worktree. This\n> > used to work, but all of a sudden, the git config --worktree\n> > core.sparseCheckout true call failed because I'm now using v2.28.0-rc0.\n>\n> I tried your situation with Junio's patch from earlier [1] [2].\n>\n> [1] https://lore.kernel.org/git/pull.674.git.1594668051847.gitgitgadget@gmail.com/\n> [2] https://lore.kernel.org/git/xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com/\n>\n> The issue here is that Junio's silent fix for sparse-checkout doesn't\n> work here for \"git config --worktree\". However, I think that Johannes\n> is making the same over-compensating warning message pattern as I was.\n> That is, this warning happens for all extensions that are enabled when\n> core.repositoryFormatVersion is less than 1.\n>\n> To attempt to summarize Junio's opinion, we should keep our situation\n> isolated to this worktree config extension. Your patch does agree with\n> the others in that we don't revert the behavior of failing to set the\n> config, but I think in this instance we can specify the warning more\n> carefully.\n\nOkay.\n\n> If you don't mind, I was already going to squash Junio's commit into\n> mine (almost completely replacing mine) but I could add a small\n> commit on top that provides the following improvement to the error\n> message:\n\nI don't mind at all. I'd just like to know that v2.28.0 avoids confusing\nusers in the same was as v2.28.0-rc0 confused me.\n\nThanks,\nDscho\n\n>\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 5e39f618854..b5de7982a93 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> else if (worktrees[0] && worktrees[1])\n> die(_(\"--worktree cannot be used with multiple \"\n> \"working trees unless the config\\n\"\n> - \"extension worktreeConfig is enabled. \"\n> - \"Please read \\\"CONFIGURATION FILE\\\"\\n\"\n> + \"extension worktreeConfig is enabled \"\n> + \"and core.repositoryFormatVersion is at least\\n\"\n> + \"1. Please read \\\"CONFIGURATION FILE\\\"\"\n> \"section in \\\"git help worktree\\\" for details\"));\n> else\n> given_config_source.file = git_pathdup(\"config\");\n>\n> Thanks,\n> -Stolee\n>\n>\n"},{"id":"401551","messageId":"xmqqzh82ktgm.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"nycvar.QRO.7.76.6.2007141420300.52@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-14T15:27:21Z","receivedAt":"2020-07-14T15:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> If you don't mind, I was already going to squash Junio's commit into\n>> mine (almost completely replacing mine) but I could add a small\n>> commit on top that provides the following improvement to the error\n>> message:\n>\n> I don't mind at all. I'd just like to know that v2.28.0 avoids confusing\n> users in the same was as v2.28.0-rc0 confused me.\n\nIn a nearby thread, Jonathan Nieder raised an interesting approach\nto avoid confusing users, which I think (if I am reading him\ncorrectly) makes sense (cf. <20200714040616.GA2208896@google.com>)\n\nWhat if we accept the extensions the code before the topic in\nquestion that was merged in -rc0 introduced the \"confusion\" accepts\neven in v0? If we see extensions other than those handpicked and\ngrandfathered ones (which are presumably the ones we add later and\nsupport in v1 and later repository versions) in a v0 repository, we\nkeep ignoring. Also we'd loosen the overly strict code that\nprevents upgrading from v0 to v1 in the presence of any extensions\nin -rc0, so that the grandfathered ones will not prevent the\nupgrading.\n\nThe original reasoning behind the strict check was because the users\ncould have used extensions.frotz for their own use with their own\nmeaning, trusting that Git would simply ignore it, and an upgrade to\nlater version in which Git uses extensions.frotz for a purpose that\nis unrelated to the reason why these users used would just break the\nrepository. \n\nBut the ones that were (accidentally) honored in v0 couldn't have\nbeen used by the users for the purposes other than how Git would use\nthem anyway, so there is no point to make them prevent the upgrade\nof the repository version from v0 to v1.\n\nAt least, that is how I understood the world would look like in\nJonathan's \"different endgame\".\n\nWhat do you three (Dscho, Derrick and Jonathan) think? \n\n\n\n> Thanks,\n> Dscho\n>\n>>\n>> diff --git a/builtin/config.c b/builtin/config.c\n>> index 5e39f618854..b5de7982a93 100644\n>> --- a/builtin/config.c\n>> +++ b/builtin/config.c\n>> @@ -678,8 +678,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>> else if (worktrees[0] && worktrees[1])\n>> die(_(\"--worktree cannot be used with multiple \"\n>> \"working trees unless the config\\n\"\n>> - \"extension worktreeConfig is enabled. \"\n>> - \"Please read \\\"CONFIGURATION FILE\\\"\\n\"\n>> + \"extension worktreeConfig is enabled \"\n>> + \"and core.repositoryFormatVersion is at least\\n\"\n>> + \"1. Please read \\\"CONFIGURATION FILE\\\"\"\n>> \"section in \\\"git help worktree\\\" for details\"));\n>> else\n>> given_config_source.file = git_pathdup(\"config\");\n>>\n>> Thanks,\n>> -Stolee\n>>\n>>\n"},{"id":"401553","messageId":"18c65b85-2f2a-ff96-1ea7-e16befa6928f@gmail.com","threadId":"53852","inReplyTo":"xmqqzh82ktgm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-14T15:40:16Z","receivedAt":"2020-07-14T15:40:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/14/2020 11:27 AM, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n>>> If you don't mind, I was already going to squash Junio's commit into\n>>> mine (almost completely replacing mine) but I could add a small\n>>> commit on top that provides the following improvement to the error\n>>> message:\n>>\n>> I don't mind at all. I'd just like to know that v2.28.0 avoids confusing\n>> users in the same was as v2.28.0-rc0 confused me.\n> \n> In a nearby thread, Jonathan Nieder raised an interesting approach\n> to avoid confusing users, which I think (if I am reading him\n> correctly) makes sense (cf. <20200714040616.GA2208896@google.com>)\n> \n> What if we accept the extensions the code before the topic in\n> question that was merged in -rc0 introduced the \"confusion\" accepts\n> even in v0? If we see extensions other than those handpicked and\n> grandfathered ones (which are presumably the ones we add later and\n> support in v1 and later repository versions) in a v0 repository, we\n> keep ignoring. Also we'd loosen the overly strict code that\n> prevents upgrading from v0 to v1 in the presence of any extensions\n> in -rc0, so that the grandfathered ones will not prevent the\n> upgrading.\n> \n> The original reasoning behind the strict check was because the users\n> could have used extensions.frotz for their own use with their own\n> meaning, trusting that Git would simply ignore it, and an upgrade to\n> later version in which Git uses extensions.frotz for a purpose that\n> is unrelated to the reason why these users used would just break the\n> repository. \n> \n> But the ones that were (accidentally) honored in v0 couldn't have\n> been used by the users for the purposes other than how Git would use\n> them anyway, so there is no point to make them prevent the upgrade\n> of the repository version from v0 to v1.\n> \n> At least, that is how I understood the world would look like in\n> Jonathan's \"different endgame\".\n> \n> What do you three (Dscho, Derrick and Jonathan) think? \n\nIf \"v0\" includes \"core.repositoryFormatVersion is unset\" then I\nwould consider this to be a way to avoid all user pain, which is\npositive.\n\nI'd be happy to test and review a patch that accomplishes this\ngoal.\n\nCC'ing Ed Thomson because this extension stuff affects other tools,\nlike libgit2.\n\nThanks,\n-Stolee\n\n"},{"id":"401561","messageId":"nycvar.QRO.7.76.6.2007142227280.52@tvgsbejvaqbjf.bet","threadId":"53852","inReplyTo":"18c65b85-2f2a-ff96-1ea7-e16befa6928f@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-14T20:30:12Z","receivedAt":"2020-07-14T20:30:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 14 Jul 2020, Derrick Stolee wrote:\n\n> On 7/14/2020 11:27 AM, Junio C Hamano wrote:\n> > Johannes Schindelin writes:\n> >\n> >>> If you don't mind, I was already going to squash Junio's commit into\n> >>> mine (almost completely replacing mine) but I could add a small\n> >>> commit on top that provides the following improvement to the error\n> >>> message:\n> >>\n> >> I don't mind at all. I'd just like to know that v2.28.0 avoids confusing\n> >> users in the same was as v2.28.0-rc0 confused me.\n> >\n> > In a nearby thread, Jonathan Nieder raised an interesting approach\n> > to avoid confusing users, which I think (if I am reading him\n> > correctly) makes sense (cf. <20200714040616.GA2208896@google.com>)\n> >\n> > What if we accept the extensions the code before the topic in\n> > question that was merged in -rc0 introduced the \"confusion\" accepts\n> > even in v0? If we see extensions other than those handpicked and\n> > grandfathered ones (which are presumably the ones we add later and\n> > support in v1 and later repository versions) in a v0 repository, we\n> > keep ignoring. Also we'd loosen the overly strict code that\n> > prevents upgrading from v0 to v1 in the presence of any extensions\n> > in -rc0, so that the grandfathered ones will not prevent the\n> > upgrading.\n> >\n> > The original reasoning behind the strict check was because the users\n> > could have used extensions.frotz for their own use with their own\n> > meaning, trusting that Git would simply ignore it, and an upgrade to\n> > later version in which Git uses extensions.frotz for a purpose that\n> > is unrelated to the reason why these users used would just break the\n> > repository.\n> >\n> > But the ones that were (accidentally) honored in v0 couldn't have\n> > been used by the users for the purposes other than how Git would use\n> > them anyway, so there is no point to make them prevent the upgrade\n> > of the repository version from v0 to v1.\n> >\n> > At least, that is how I understood the world would look like in\n> > Jonathan's \"different endgame\".\n> >\n> > What do you three (Dscho, Derrick and Jonathan) think?\n>\n> If \"v0\" includes \"core.repositoryFormatVersion is unset\" then I\n> would consider this to be a way to avoid all user pain, which is\n> positive.\n\nI concur.\n\n> I'd be happy to test and review a patch that accomplishes this\n> goal.\n\nWouldn't that just be a matter of extending your patch to re-set\n`has_unhandled_extensions` also for `preciousObjects` and `partialClone`?\n\nCiao,\nDscho\n\n>\n> CC'ing Ed Thomson because this extension stuff affects other tools,\n> like libgit2.\n>\n> Thanks,\n> -Stolee\n>\n>\n"},{"id":"401564","messageId":"xmqqimeplt7m.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"nycvar.QRO.7.76.6.2007142227280.52@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-14T20:47:25Z","receivedAt":"2020-07-14T20:47:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Tue, 14 Jul 2020, Derrick Stolee wrote:\n>\n>> If \"v0\" includes \"core.repositoryFormatVersion is unset\" then I\n>> would consider this to be a way to avoid all user pain, which is\n>> positive.\n>\n> I concur.\n>\n>> I'd be happy to test and review a patch that accomplishes this\n>> goal.\n>\n> Wouldn't that just be a matter of extending your patch to re-set\n> `has_unhandled_extensions` also for `preciousObjects` and `partialClone`?\n\nIt probably needs a bit more than that. For example there is this\nbit in check_repository_format_gently() that clears the unwanted\nextensions that we used to honor by mistake in v0 repository\n\n\tif (candidate->version >= 1) {\n\t\trepository_format_precious_objects = candidate->precious_objects;\n\t\tset_repository_format_partial_clone(candidate->partial_clone);\n\t\trepository_format_worktree_config = candidate->worktree_config;\n\t} else {\n\t\trepository_format_precious_objects = 0;\n\t\tset_repository_format_partial_clone(NULL);\n\t\trepository_format_worktree_config = 0;\n\t}\n\nand the \"different endgame\" advocates to keep honoring these (and\nonly these), the else clause probably needs to go. There may be\nsome other tweaks necessary.\n"},{"id":"401593","messageId":"xmqqpn8wkben.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"xmqqzh82ktgm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T16:09:36Z","receivedAt":"2020-07-15T16:09:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>>> If you don't mind, I was already going to squash Junio's commit into\n>>> mine (almost completely replacing mine) but I could add a small\n>>> commit on top that provides the following improvement to the error\n>>> message:\n>>\n>> I don't mind at all. I'd just like to know that v2.28.0 avoids confusing\n>> users in the same was as v2.28.0-rc0 confused me.\n>\n> In a nearby thread, Jonathan Nieder raised an interesting approach\n> to avoid confusing users, which I think (if I am reading him\n> correctly) makes sense (cf. <20200714040616.GA2208896@google.com>)\n>\n> What if we accept the extensions the code before the topic in\n> question that was merged in -rc0 introduced the \"confusion\" accepts\n> even in v0? If we see extensions other than those handpicked and\n> grandfathered ones (which are presumably the ones we add later and\n> support in v1 and later repository versions) in a v0 repository, we\n> keep ignoring. Also we'd loosen the overly strict code that\n> prevents upgrading from v0 to v1 in the presence of any extensions\n> in -rc0, so that the grandfathered ones will not prevent the\n> upgrading.\n>\n> The original reasoning behind the strict check was because the users\n> could have used extensions.frotz for their own use with their own\n> meaning, trusting that Git would simply ignore it, and an upgrade to\n> later version in which Git uses extensions.frotz for a purpose that\n> is unrelated to the reason why these users used would just break the\n> repository. \n>\n> But the ones that were (accidentally) honored in v0 couldn't have\n> been used by the users for the purposes other than how Git would use\n> them anyway, so there is no point to make them prevent the upgrade\n> of the repository version from v0 to v1.\n>\n> At least, that is how I understood the world would look like in\n> Jonathan's \"different endgame\".\n>\n> What do you three (Dscho, Derrick and Jonathan) think? \n\nIt seems that there is no quick concensus to go with your \"different\nendgame\" and worse yet it seems nobody is interested in helping\nmuch.\n\nThe current one on the table is NOT\n<20200714040616.GA2208896@google.com> but the two patches\n\n<1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>\n\n\nwhich we may regret---it is far from a robust and complete solution,\nbut probably specific to users at Microsoft or something like that.\nFor example it special cases only the worktreeconfig and nothing\nelse, even though I suspect that other configuration variables were\nalso honored by mistake.\n\nSo...\n\nHere is my quick attempt to see how far we can go with the\n\"different endgame\" approach, to be applied on top of those two\npatches. It still has two known \"breakages\" and can use help from\nextra eyeballs and real work.\n\nI suspect that an expected test_must_fail not triggering t2404 may\neven be a good thing if it is a sign of silent upgrading of the\nrepository version due to having grandfathered extensions in a v0\nrepository, but I didn't have time to dig further.\n\nI'll shift my attention to other topics that should be in the\nrelease for the rest of the day, but am pessimistic that I can tag\nthe -rc1 today, which won't happen until we at least have a\nconcensus on what to do with the (apparent) regression due to the\n\"upgrade repository version\" topic.\n\nThanks.\n\n setup.c | 52 +++++++++++++++++++++++++++-------------------\n t/t0410-partial-clone.sh | 14 ++++++++++++-\n t/t2404-worktree-config.sh | 2 +-\n 3 files changed, 45 insertions(+), 23 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 65270440a9..fe4e1ec066 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,28 +455,37 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n \tif (strcmp(var, \"core.repositoryformatversion\") == 0)\n \t\tdata->version = git_config_int(var, value);\n \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n+\t\tint unallowed_in_v0 = 1;\n+\n \t\t/*\n-\t\t * record any known extensions here; otherwise,\n-\t\t * we fall through to recording it as unknown, and\n-\t\t * check_repository_format will complain\n+\t\t * The early ones are grandfathered---they existed in\n+\t\t * 2.27 which mistakenly honored even in repositories\n+\t\t * whose version is before v1 (where extensions are\n+\t\t * officially introduced).\n \t\t */\n-\t\tint is_unallowed_extension = 1;\n-\n-\t\tif (!strcmp(ext, \"noop\"))\n-\t\t\t;\n-\t\telse if (!strcmp(ext, \"preciousobjects\"))\n+\t\tif (!strcmp(ext, \"noop\")) {\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else if (!strcmp(ext, \"preciousobjects\")) {\n \t\t\tdata->precious_objects = git_config_bool(var, value);\n-\t\telse if (!strcmp(ext, \"partialclone\")) {\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else if (!strcmp(ext, \"partialclone\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(var);\n \t\t\tdata->partial_clone = xstrdup(value);\n+\t\t\tunallowed_in_v0 = 0;\n \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\t\tis_unallowed_extension = 0;\n-\t\t} else\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t/*\n+\t\t * Extensions are added by more \"} else if (...) {\"\n+\t\t * lines here, but do NOT mark them as allowed in v0\n+\t\t * by copy-pasting without thinking.\n+\t\t */\n+\t\t} else {\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\t\t}\n \n-\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n+\t\tdata->has_unallowed_extensions |= unallowed_in_v0;\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\n@@ -511,15 +520,16 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\tdie(\"%s\", err.buf);\n \t}\n \n-\tif (candidate->version >= 1) {\n-\t\trepository_format_precious_objects = candidate->precious_objects;\n-\t\tset_repository_format_partial_clone(candidate->partial_clone);\n-\t\trepository_format_worktree_config = candidate->worktree_config;\n-\t} else {\n-\t\trepository_format_precious_objects = 0;\n-\t\tset_repository_format_partial_clone(NULL);\n-\t\trepository_format_worktree_config = 0;\n-\t}\n+\t/*\n+\t * Now we know the extensions in \"candidate\" repository are\n+\t * OK, let's copy them to the final place. Note that this is\n+\t * done even in v0 repositories, as long as the extensions are\n+\t * the grandfathered ones that used to be honored by mistake.\n+\t */\n+\trepository_format_precious_objects = candidate->precious_objects;\n+\tset_repository_format_partial_clone(candidate->partial_clone);\n+\trepository_format_worktree_config = candidate->worktree_config;\n+\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n \tif (repository_format_worktree_config) {\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be..2fc2d0bbfc 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '\n \ttest_cmp_config -C client 1 core.repositoryformatversion\n '\n \n-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '\n+test_expect_success 'convert shallow clone to partial clone succeeds with grandfathered extension' '\n \trm -fr server client &&\n \ttest_create_repo server &&\n \ttest_commit -C server my_commit 1 &&\n@@ -50,6 +50,18 @@ test_expect_success 'convert shallow clone to partial clone must fail with any e\n \tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n \ttest_cmp_config -C client 0 core.repositoryformatversion &&\n \tgit -C client config extensions.partialclone origin &&\n+\tgit -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n+test_expect_failure 'convert shallow clone to partial clone must fail with unknown extension' '\n+\trm -fr server client &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server my_commit 1 &&\n+\ttest_commit -C server my_commit2 1 &&\n+\tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n+\ttest_cmp_config -C client 0 core.repositoryformatversion &&\n+\tgit -C client config extensions.unknownExtension true &&\n+\tgit -C client config extensions.partialclone origin &&\n \ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n '\n \ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 303a2644bd..b8c12df534 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -77,7 +77,7 @@ test_expect_success 'config.worktree no longer read without extension' '\n \ttest_cmp_config -C wt1 shared this.is &&\n \ttest_cmp_config -C wt2 shared this.is\n '\n-test_expect_success 'show advice when extensions.* are not enabled' '\n+test_expect_failure 'show advice when extensions.* are not enabled' '\n \ttest_config core.repositoryformatversion 1 &&\n \ttest_config extensions.worktreeConfig true &&\n \tgit config --worktree test.one true &&\n"},{"id":"401594","messageId":"xmqqlfjkk8zv.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"xmqqpn8wkben.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T17:01:40Z","receivedAt":"2020-07-15T17:01:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The current one on the table is NOT\n> <20200714040616.GA2208896@google.com> but the two patches\n>\n> <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>\n> \n>\n> For example it special cases only the worktreeconfig and nothing\n> else, even though I suspect that other configuration variables were\n> also honored by mistake.\n\nThe attached may be a less ambitious and less risky update for the\nupcoming release. It is to be applied on top of the two-patch\nseries from Derrick, and just marks the other \"known and honored\nback then by mistake\" extensions as OK to be there for upgrading.\n\nThoughts? If people are happy with that, then we could apply and\ncut an -rc1 with it. Or if we are OK with the \"just special case\nworktreeconfig; other extensions may have the same issue but we\nhaven't heard actual complaints so we will leave them untouched\",\nthen -rc1 can be done with just those two patches.\n\nNow I do need to shift my attention to other topics in flight.\n\nThanks.\n\n\n setup.c | 27 ++++++++++++++++-----------\n 1 file changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 65270440a9..a072c76d05 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n \t\tdata->version = git_config_int(var, value);\n \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n \t\t/*\n-\t\t * record any known extensions here; otherwise,\n-\t\t * we fall through to recording it as unknown, and\n-\t\t * check_repository_format will complain\n+\t\t * Grandfather extensions that were known in 2.27 and\n+\t\t * were honored by mistake even in v0 repositories; it\n+\t\t * shoudn't be an error to upgrade v0 to v1 with them\n+\t\t * in the repository, as they couldn't have been used\n+\t\t * for incompatible purposes by the end user.\n \t\t */\n-\t\tint is_unallowed_extension = 1;\n+\t\tint unallowed_in_v0 = 1;\n \n-\t\tif (!strcmp(ext, \"noop\"))\n-\t\t\t;\n-\t\telse if (!strcmp(ext, \"preciousobjects\"))\n+\t\tif (!strcmp(ext, \"noop\")) {\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else if (!strcmp(ext, \"preciousobjects\")) {\n \t\t\tdata->precious_objects = git_config_bool(var, value);\n-\t\telse if (!strcmp(ext, \"partialclone\")) {\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else if (!strcmp(ext, \"partialclone\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(var);\n \t\t\tdata->partial_clone = xstrdup(value);\n+\t\t\tunallowed_in_v0 = 0;\n \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\t\tis_unallowed_extension = 0;\n-\t\t} else\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else {\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\t\t}\n \n-\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n+\t\tdata->has_unallowed_extensions |= unallowed_in_v0;\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\n"},{"id":"401595","messageId":"31f52913-8745-18b4-63fc-37d2a9aea8d0@gmail.com","threadId":"53852","inReplyTo":"xmqqlfjkk8zv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-15T18:00:42Z","receivedAt":"2020-07-15T18:00:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/15/2020 1:01 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> The current one on the table is NOT\n>> <20200714040616.GA2208896@google.com> but the two patches\n>>\n>> <1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com>\n>> \n>>\n>> For example it special cases only the worktreeconfig and nothing\n>> else, even though I suspect that other configuration variables were\n>> also honored by mistake.\n\nSorry for the delay. I had your previous diff applied and was\nplaying around with the two \"known breakages\".\n\nThe diff below _does_ fail on t0410-partial-clone.sh, but it's\nbecause you do change the behavior. Here is my diff hunk for that\ntest:\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be..fc8da56528 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '\n test_cmp_config -C client 1 core.repositoryformatversion\n '\n \n-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '\n+test_expect_success 'convert shallow clone to partial clone succeeds with grandfathered extension' '\n rm -fr server client &&\n test_create_repo server &&\n test_commit -C server my_commit 1 &&\n@@ -50,7 +50,20 @@ test_expect_success 'convert shallow clone to partial clone must fail with any e\n git clone --depth=1 \"file://$(pwd)/server\" client &&\n test_cmp_config -C client 0 core.repositoryformatversion &&\n git -C client config extensions.partialclone origin &&\n- test_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+ git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n+test_expect_success 'convert shallow clone to partial clone must fail with unknown extension' '\n+ rm -fr server client &&\n+ test_create_repo server &&\n+ test_commit -C server my_commit 1 &&\n+ test_commit -C server my_commit2 1 &&\n+ git clone --depth=1 \"file://$(pwd)/server\" client &&\n+ test_cmp_config -C client 0 core.repositoryformatversion &&\n+ git -C client config extensions.unknownExtension true &&\n+ git -C client config extensions.partialclone origin &&\n+ test_must_fail git -C client fetch --unshallow --filter=\"blob:none\" 2>err &&\n+ test_i18ngrep \"unable to upgrade repository format from 0 to 1\" err\n '\n \n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n\n\n> The attached may be a less ambitious and less risky update for the\n> upcoming release. It is to be applied on top of the two-patch\n> series from Derrick, and just marks the other \"known and honored\n> back then by mistake\" extensions as OK to be there for upgrading.\n> \n> Thoughts? If people are happy with that, then we could apply and\n> cut an -rc1 with it. Or if we are OK with the \"just special case\n> worktreeconfig; other extensions may have the same issue but we\n> haven't heard actual complaints so we will leave them untouched\",\n> then -rc1 can be done with just those two patches.\n\nThe \"special case wortreeConfig\" of my submission was maybe based\non an incorrect assumption that this is the only place where Git\nitself set an extension.* config without _also_ updating the\ncore.repositoryFormatVersion config.\n\nThe error I made in my v1 is to warn on ALL extensions, not just\nthe ones that Git knows about. This new approach is a good middle\nground between my v1 and v2.\n\nOf course, the _other_ option is to revert xl/upgrade-repo-format\nfrom v2.28.0 and take our time resolving this issue during the\n2.29 cycle. I'm not sure how disruptive that action would be.\n\n> Now I do need to shift my attention to other topics in flight.\n\nI appreciate you spending so much time on this! It's a tough\ntime to be noticing such a complicated situation that is not\neasily testable from a single Git version, but across versions.\n\n> \n> setup.c | 27 ++++++++++++++++-----------\n> 1 file changed, 16 insertions(+), 11 deletions(-)\n> \n> diff --git a/setup.c b/setup.c\n> index 65270440a9..a072c76d05 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -456,27 +456,32 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n> \t\tdata->version = git_config_int(var, value);\n> \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n> \t\t/*\n> -\t\t * record any known extensions here; otherwise,\n> -\t\t * we fall through to recording it as unknown, and\n> -\t\t * check_repository_format will complain\n> +\t\t * Grandfather extensions that were known in 2.27 and\n> +\t\t * were honored by mistake even in v0 repositories; it\n> +\t\t * shoudn't be an error to upgrade v0 to v1 with them\n> +\t\t * in the repository, as they couldn't have been used\n> +\t\t * for incompatible purposes by the end user.\n> \t\t */\n> -\t\tint is_unallowed_extension = 1;\n> +\t\tint unallowed_in_v0 = 1;\n> \n> -\t\tif (!strcmp(ext, \"noop\"))\n> -\t\t\t;\n> -\t\telse if (!strcmp(ext, \"preciousobjects\"))\n> +\t\tif (!strcmp(ext, \"noop\")) {\n> +\t\t\tunallowed_in_v0 = 0;\n> +\t\t} else if (!strcmp(ext, \"preciousobjects\")) {\n> \t\t\tdata->precious_objects = git_config_bool(var, value);\n> -\t\telse if (!strcmp(ext, \"partialclone\")) {\n> +\t\t\tunallowed_in_v0 = 0;\n> +\t\t} else if (!strcmp(ext, \"partialclone\")) {\n> \t\t\tif (!value)\n> \t\t\t\treturn config_error_nonbool(var);\n> \t\t\tdata->partial_clone = xstrdup(value);\n> +\t\t\tunallowed_in_v0 = 0;\n> \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n> \t\t\tdata->worktree_config = git_config_bool(var, value);\n> -\t\t\tis_unallowed_extension = 0;\n\nYour previous diff had this comment, which I thought to be\nhelpful: \n\n+\t\t/*\n+\t\t * Extensions are added by more \"} else if (...) {\"\n+\t\t * lines here, but do NOT mark them as allowed in v0\n+\t\t * by copy-pasting without thinking.\n+\t\t */\n\n> -\t\t} else\n> +\t\t\tunallowed_in_v0 = 0;\n> +\t\t} else {\n> \t\t\tstring_list_append(&data->unknown_extensions, ext);\n> +\t\t}\n> \n> -\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n> +\t\tdata->has_unallowed_extensions |= unallowed_in_v0;\n> \t}\n> \n> \treturn read_worktree_config(var, value, vdata);\n\nThanks,\n-Stolee\n\n\n"},{"id":"401596","messageId":"xmqqh7u8k5va.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"31f52913-8745-18b4-63fc-37d2a9aea8d0@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T18:09:13Z","receivedAt":"2020-07-15T18:09:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Your previous diff had this comment, which I thought to be\n> helpful: \n>\n> +\t\t/*\n> +\t\t * Extensions are added by more \"} else if (...) {\"\n> +\t\t * lines here, but do NOT mark them as allowed in v0\n> +\t\t * by copy-pasting without thinking.\n> +\t\t */\n\nYeah, but it felt somewhat strange to have it at the end of one\nentry, like this:\n\n+\t\t\tunallowed_in_v0 = 0;\n \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t/*\n+\t\t * Extensions are added by more \"} else if (...) {\"\n+\t\t * lines here, but do NOT mark them as allowed in v0\n+\t\t * by copy-pasting without thinking.\n+\t\t */\n+\t\t} else {\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n\n\nIn any case, I updated the comment in front of the if/else if/\ncascade to essentially say the same thing, and with test updates\nthis time.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] setup: grandfather other extensions that used to be honored by mistake\n\nWe special cased worktreeconfig extension to be OK to exist when the\nrepository gets converted from v0 to v1 in an earlier commit, but\nother extenions were also honored by mistake in v0. Mark them the\nsame way so that they won't interfere with the upgrading from v0 to\nv1.\n\nA test in t0410 used to expect that presence of the\nextension.partialclone configuration variable to interfere, but that\nno longer is true.\n\nSigned-off-by: Junio C Hamano \n---\n setup.c | 30 +++++++++++++++++++-----------\n t/t0410-partial-clone.sh | 18 ++++++++++++++++--\n 2 files changed, 35 insertions(+), 13 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 65270440a9..97292479d6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -456,27 +456,35 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n \t\tdata->version = git_config_int(var, value);\n \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n \t\t/*\n-\t\t * record any known extensions here; otherwise,\n-\t\t * we fall through to recording it as unknown, and\n-\t\t * check_repository_format will complain\n+\t\t * Grandfather extensions that were known in 2.27 and\n+\t\t * were honored by mistake even in v0 repositories; it\n+\t\t * shoudn't be an error to upgrade v0 to v1 with them\n+\t\t * in the repository, as they couldn't have been used\n+\t\t * for incompatible purposes by the end user.\n+\t\t *\n+\t\t * When adding new extensions support in this if/elseif/...\n+\t\t * cascade, do not mark them as allowed in v0!\n \t\t */\n-\t\tint is_unallowed_extension = 1;\n+\t\tint unallowed_in_v0 = 1;\n \n-\t\tif (!strcmp(ext, \"noop\"))\n-\t\t\t;\n-\t\telse if (!strcmp(ext, \"preciousobjects\"))\n+\t\tif (!strcmp(ext, \"noop\")) {\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else if (!strcmp(ext, \"preciousobjects\")) {\n \t\t\tdata->precious_objects = git_config_bool(var, value);\n-\t\telse if (!strcmp(ext, \"partialclone\")) {\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else if (!strcmp(ext, \"partialclone\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(var);\n \t\t\tdata->partial_clone = xstrdup(value);\n+\t\t\tunallowed_in_v0 = 0;\n \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\t\tis_unallowed_extension = 0;\n-\t\t} else\n+\t\t\tunallowed_in_v0 = 0;\n+\t\t} else {\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\t\t}\n \n-\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n+\t\tdata->has_unallowed_extensions |= unallowed_in_v0;\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be..e9674fc257 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '\n \ttest_cmp_config -C client 1 core.repositoryformatversion\n '\n \n-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '\n+test_expect_success 'convert shallow clone to partial clone is OK with a grandfathered extension' '\n \trm -fr server client &&\n \ttest_create_repo server &&\n \ttest_commit -C server my_commit 1 &&\n@@ -50,7 +50,21 @@ test_expect_success 'convert shallow clone to partial clone must fail with any e\n \tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n \ttest_cmp_config -C client 0 core.repositoryformatversion &&\n \tgit -C client config extensions.partialclone origin &&\n-\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client 1 core.repositoryformatversion\n+'\n+\n+test_expect_success 'convert shallow clone to partial clone must fail with an unknown extension' '\n+\trm -fr server client &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server my_commit 1 &&\n+\ttest_commit -C server my_commit2 1 &&\n+\tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n+\ttest_cmp_config -C client 0 core.repositoryformatversion &&\n+\tgit -C client config extensions.partialclone origin &&\n+\tgit -C client config extensions.unknown true &&\n+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client 0 core.repositoryformatversion\n '\n \n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n-- \n2.28.0-rc0\n\n\n\n"},{"id":"401597","messageId":"xmqqd04wk5kr.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"31f52913-8745-18b4-63fc-37d2a9aea8d0@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T18:15:32Z","receivedAt":"2020-07-15T18:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Of course, the _other_ option is to revert xl/upgrade-repo-format\n> from v2.28.0 and take our time resolving this issue during the\n> 2.29 cycle. I'm not sure how disruptive that action would be.\n\nYes, that is becoming very much tempting at this point, isn't it?\n\nIn any case, I've pushed out 'seen' with the \"these extensions that\nused to be honored in v0 won't interfere with repository upgrade\"\npatch I sent earlier, and I am hoping that it would be a reasonable\nmiddle ground that won't regress things for users while making sure\nwe do not honor random future extensions by mistake.\n"},{"id":"401598","messageId":"20200715182011.GA2950865@google.com","threadId":"53852","inReplyTo":"xmqqpn8wkben.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-15T18:20:11Z","receivedAt":"2020-07-15T18:20:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n\n> It seems that there is no quick concensus to go with your \"different\n> endgame\" and worse yet it seems nobody is interested in helping\n> much.\n\nSorry for the delay. I do want to look at this this afternoon.\n\n[...]\n> I'll shift my attention to other topics that should be in the\n> release for the rest of the day, but am pessimistic that I can tag\n> the -rc1 today, which won't happen until we at least have a\n> concensus on what to do with the (apparent) regression due to the\n> \"upgrade repository version\" topic.\n\nAh, I hadn't realized an -rc1 tomorrow instead of today was on the\ntable. ;-)\n\nI'll do what I can (in other words, expect a patch from me; but also,\nI am very interested in analysis, proposed patches, etc from others so\nthat we can end up wit ha good fix with the solution space well\nexplored).\n\nThanks,\nJonathan\n"},{"id":"401602","messageId":"3544e544-0ed5-c2d7-ff25-ad6d9349905e@gmail.com","threadId":"53852","inReplyTo":"xmqqh7u8k5va.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-15T18:40:52Z","receivedAt":"2020-07-15T18:40:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/15/2020 2:09 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> Your previous diff had this comment, which I thought to be\n>> helpful: \n>>\n>> +\t\t/*\n>> +\t\t * Extensions are added by more \"} else if (...) {\"\n>> +\t\t * lines here, but do NOT mark them as allowed in v0\n>> +\t\t * by copy-pasting without thinking.\n>> +\t\t */\n> \n> Yeah, but it felt somewhat strange to have it at the end of one\n> entry, like this:\n> \n> +\t\t\tunallowed_in_v0 = 0;\n> \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n> \t\t\tdata->worktree_config = git_config_bool(var, value);\n> +\t\t\tunallowed_in_v0 = 0;\n> +\t\t/*\n> +\t\t * Extensions are added by more \"} else if (...) {\"\n> +\t\t * lines here, but do NOT mark them as allowed in v0\n> +\t\t * by copy-pasting without thinking.\n> +\t\t */\n> +\t\t} else {\n> \t\t\tstring_list_append(&data->unknown_extensions, ext);\n> \n> \n> In any case, I updated the comment in front of the if/else if/\n> cascade to essentially say the same thing, and with test updates\n> this time.\n\nThanks. I applied and tested this version. LGTM!\n\nI'll also review Jonathan Nieder's patches when they arrive.\n\nThanks,\n-Stolee\n\n"},{"id":"401604","messageId":"nycvar.QRO.7.76.6.2007152115021.52@tvgsbejvaqbjf.bet","threadId":"53852","inReplyTo":"3544e544-0ed5-c2d7-ff25-ad6d9349905e@gmail.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-15T19:16:53Z","receivedAt":"2020-07-15T19:17:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 15 Jul 2020, Derrick Stolee wrote:\n\n> On 7/15/2020 2:09 PM, Junio C Hamano wrote:\n> > Derrick Stolee writes:\n> >\n> >> Your previous diff had this comment, which I thought to be\n> >> helpful:\n> >>\n> >> +\t\t/*\n> >> +\t\t * Extensions are added by more \"} else if (...) {\"\n> >> +\t\t * lines here, but do NOT mark them as allowed in v0\n> >> +\t\t * by copy-pasting without thinking.\n> >> +\t\t */\n> >\n> > Yeah, but it felt somewhat strange to have it at the end of one\n> > entry, like this:\n> >\n> > +\t\t\tunallowed_in_v0 = 0;\n> > \t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n> > \t\t\tdata->worktree_config = git_config_bool(var, value);\n> > +\t\t\tunallowed_in_v0 = 0;\n> > +\t\t/*\n> > +\t\t * Extensions are added by more \"} else if (...) {\"\n> > +\t\t * lines here, but do NOT mark them as allowed in v0\n> > +\t\t * by copy-pasting without thinking.\n> > +\t\t */\n> > +\t\t} else {\n> > \t\t\tstring_list_append(&data->unknown_extensions, ext);\n> >\n> >\n> > In any case, I updated the comment in front of the if/else if/\n> > cascade to essentially say the same thing, and with test updates\n> > this time.\n>\n> Thanks. I applied and tested this version. LGTM!\n\nSorry for being slow at the party; I'd much rather deal with Git issues\nthan with the paperwork I am fighting with.\n\nThank you for working on this, I am pretty happy with the state you\nwhipped it into, but take that with a grain of salt, as my brain is moosh.\n\nCiao,\nDscho\n"},{"id":"401605","messageId":"nycvar.QRO.7.76.6.2007152117310.52@tvgsbejvaqbjf.bet","threadId":"53852","inReplyTo":"xmqqd04wk5kr.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-15T19:21:31Z","receivedAt":"2020-07-15T19:21:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 15 Jul 2020, Junio C Hamano wrote:\n\n> Derrick Stolee writes:\n>\n> > Of course, the _other_ option is to revert xl/upgrade-repo-format\n> > from v2.28.0 and take our time resolving this issue during the\n> > 2.29 cycle. I'm not sure how disruptive that action would be.\n>\n> Yes, that is becoming very much tempting at this point, isn't it?\n\nIt did cross my mind, too.\n\n> In any case, I've pushed out 'seen' with the \"these extensions that\n> used to be honored in v0 won't interfere with repository upgrade\"\n> patch I sent earlier, and I am hoping that it would be a reasonable\n> middle ground that won't regress things for users while making sure\n> we do not honor random future extensions by mistake.\n\nGiven that we still have -rc1 and -rc2 to make sure that things work, and\ngiven that your patch on top of Stolee's two patches looks like it is\nDoing The Best We Can Do, I am optimistic that your reasonable middle\nground is the best way forward.\n\nThanks,\nDscho\n"},{"id":"401613","messageId":"xmqqlfjki576.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200715182011.GA2950865@google.com","subject":"Re: [PATCH] setup: warn about un-enabled extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T02:06:37Z","receivedAt":"2020-07-16T02:06:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> I'll shift my attention to other topics that should be in the\n>> release for the rest of the day, but am pessimistic that I can tag\n>> the -rc1 today, which won't happen until we at least have a\n>> concensus on what to do with the (apparent) regression due to the\n>> \"upgrade repository version\" topic.\n>\n> Ah, I hadn't realized an -rc1 tomorrow instead of today was on the\n> table. ;-)\n\nIt could be delayed even more if we do not have a good solution\nagreed upon.\n\n> I'll do what I can (in other words, expect a patch from me; but also,\n> I am very interested in analysis, proposed patches, etc from others so\n> that we can end up wit ha good fix with the solution space well\n> explored).\n\nYup, that's the spirit. Thanks!\n\n"},{"id":"401614","messageId":"20200716062054.GA3242764@google.com","threadId":"53852","inReplyTo":"xmqqpn8wkben.fsf@gitster.c.googlers.com","subject":"[PATCH 0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-16T06:20:54Z","receivedAt":"2020-07-16T06:20:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Here is my quick attempt to see how far we can go with the\n> \"different endgame\" approach, to be applied on top of those two\n> patches.\n\nApologies again for the delay.\n\nHere are patches implementing the minimal fix that I'd recommend.\nThese apply against \"master\" without requiring any other patches\nas prerequisites. Thoughts?\n\nJonathan Nieder (2):\n Revert \"check_repository_format_gently(): refuse extensions for old\n repositories\"\n repository: allow repository format upgrade with extensions\n\n cache.h | 1 -\n setup.c | 24 ++++++++++--------------\n t/t0410-partial-clone.sh | 15 +++++++++++++--\n 3 files changed, 23 insertions(+), 17 deletions(-)\n"},{"id":"401615","messageId":"20200716062429.GB3242764@google.com","threadId":"53852","inReplyTo":"20200716062054.GA3242764@google.com","subject":"[PATCH 1/2] Revert \"check_repository_format_gently(): refuse extensions for old repositories\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-16T06:24:29Z","receivedAt":"2020-07-16T06:24:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This reverts commit 14c7fa269e42df4133edd9ae7763b678ed6594cd.\n\nThe core.repositoryFormatVersion field was introduced in ab9cb76f661\n(Repository format version check., 2005-11-25), providing a welcome\nbit of forward compatibility, thanks to some welcome analysis by\nMartin Atukunda. The semantics are simple: a repository with\ncore.repositoryFormatVersion set to 0 should be comprehensible by all\nGit implementations in active use; and Git implementations should\nerror out early instead of trying to act on Git repositories with\nhigher core.repositoryFormatVersion values representing new formats\nthat they do not understand.\n\nA new repository format did not need to be defined until 00a09d57eb8\n(introduce \"extensions\" form of core.repositoryformatversion,\n2015-06-23). This provided a finer-grained extension mechanism for\nGit repositories. In a repository with core.repositoryFormatVersion\nset to 1, Git implementations can act on \"extensions.*\" settings that\nmodify how a repository is interpreted. In repository format version\n1, unrecognized extensions settings cause Git to error out.\n\nWhat happens if a user sets an extension setting but forgets to\nincrease the repository format version to 1? The extension settings\nwere still recognized in that case; worse, unrecognized extensions\nsettings do *not* cause Git to error out. So combining repository\nformat version 0 with extensions settings produces in some sense the\nworst of both worlds.\n\nTo improve that situation, since 14c7fa269e4\n(check_repository_format_gently(): refuse extensions for old\nrepositories, 2020-06-05) Git instead ignores extensions in v0 mode.\nThis way, v0 repositories get the historical (pre-2015) behavior and\nmaintain compatibility with Git implementations that do not know about\nthe v1 format. Unfortunately, users had been using this sort of\nconfiguration and this behavior change came to many as a surprise:\n\n- users of \"git config --worktree\" that had followed its advice\n to enable extensions.worktreeConfig (without also increasing the\n repository format version) would find their worktree configuration\n no longer taking effect\n\n- tools such as copybara[*] that had set extensions.partialClone in\n existing repositories (without also increasing the repository format\n version) would find that setting no longer taking effect\n\nThe behavior introduced in 14c7fa269e4 might be a good behavior if we\nwere traveling back in time to 2015, but we're far too late. For some\nreason I thought that it was what had been originally implemented and\nthat it had regressed. Apologies for not doing my research when\n14c7fa269e4 was under development.\n\nLet's return to the behavior we've had since 2015: always act on\nextensions.* settings, regardless of repository format version. While\nwe're here, include some tests to describe the effect on the \"upgrade\nrepository version\" code path.\n\n[*] https://github.com/google/copybara/commit/ca76c0b1e13c4e36448d12c2aba4a5d9d98fb6e7\n\nReported-by: Johannes Schindelin \nSigned-off-by: Jonathan Nieder \n---\n setup.c | 12 +++---------\n t/t0410-partial-clone.sh | 15 +++++++++++++--\n 2 files changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex dbac2eabe8f..87bf0112cf3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -507,15 +507,9 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\tdie(\"%s\", err.buf);\n \t}\n \n-\tif (candidate->version >= 1) {\n-\t\trepository_format_precious_objects = candidate->precious_objects;\n-\t\tset_repository_format_partial_clone(candidate->partial_clone);\n-\t\trepository_format_worktree_config = candidate->worktree_config;\n-\t} else {\n-\t\trepository_format_precious_objects = 0;\n-\t\tset_repository_format_partial_clone(NULL);\n-\t\trepository_format_worktree_config = 0;\n-\t}\n+\trepository_format_precious_objects = candidate->precious_objects;\n+\tset_repository_format_partial_clone(candidate->partial_clone);\n+\trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n \tif (repository_format_worktree_config) {\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be0..51d1eba6050 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -42,14 +42,25 @@ test_expect_success 'convert shallow clone to partial clone' '\n \ttest_cmp_config -C client 1 core.repositoryformatversion\n '\n \n-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '\n+test_expect_success 'converting to partial clone fails with noop extension' '\n \trm -fr server client &&\n \ttest_create_repo server &&\n \ttest_commit -C server my_commit 1 &&\n \ttest_commit -C server my_commit2 1 &&\n \tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n \ttest_cmp_config -C client 0 core.repositoryformatversion &&\n-\tgit -C client config extensions.partialclone origin &&\n+\tgit -C client config extensions.noop true &&\n+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n+test_expect_success 'converting to partial clone fails with unrecognized extension' '\n+\trm -fr server client &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server my_commit 1 &&\n+\ttest_commit -C server my_commit2 1 &&\n+\tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n+\ttest_cmp_config -C client 0 core.repositoryformatversion &&\n+\tgit -C client config extensions.nonsense true &&\n \ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n '\n \n-- \n2.28.0.rc0.105.gf9edc3c819\n\n"},{"id":"401616","messageId":"20200716062818.GC3242764@google.com","threadId":"53852","inReplyTo":"20200716062054.GA3242764@google.com","subject":"[PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-16T06:28:18Z","receivedAt":"2020-07-16T06:28:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Now that we officially permit repository extensions in repository\nformat v0, permit upgrading a repository with extensions from v0 to v1\nas well.\n\nFor example, this means a repository where the user has set\n\"extensions.preciousObjects\" can use \"git fetch --filter=blob:none\norigin\" to upgrade the repository to use v1 and the partial clone\nextension.\n\nTo avoid mistakes, continue to forbid repository format upgrades in v0\nrepositories with an unrecognized extension. This way, a v0 user\nusing a misspelled extension field gets a chance to correct the\nmistake before updating to the less forgiving v1 format.\n\nWhile we're here, make the error message for failure to upgrade the\nrepository format a bit shorter, and present it as an error, not a\nwarning.\n\nReported-by: Huan Huan Chen \nSigned-off-by: Jonathan Nieder \n---\nApologies again for the trouble, and thanks for your patient help.\n\n cache.h | 1 -\n setup.c | 12 +++++++-----\n t/t0410-partial-clone.sh | 4 ++--\n 3 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 126ec56c7f3..654426460cc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,7 +1042,6 @@ struct repository_format {\n \tint worktree_config;\n \tint is_bare;\n \tint hash_algo;\n-\tint has_extensions;\n \tchar *work_tree;\n \tstruct string_list unknown_extensions;\n };\ndiff --git a/setup.c b/setup.c\nindex 87bf0112cf3..3a81307602e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,7 +455,6 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n \tif (strcmp(var, \"core.repositoryformatversion\") == 0)\n \t\tdata->version = git_config_int(var, value);\n \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n-\t\tdata->has_extensions = 1;\n \t\t/*\n \t\t * record any known extensions here; otherwise,\n \t\t * we fall through to recording it as unknown, and\n@@ -553,13 +552,16 @@ int upgrade_repository_format(int target_version)\n \tif (repo_fmt.version >= target_version)\n \t\treturn 0;\n \n-\tif (verify_repository_format(&repo_fmt, &err) < 0 ||\n-\t (!repo_fmt.version && repo_fmt.has_extensions)) {\n-\t\twarning(\"unable to upgrade repository format from %d to %d: %s\",\n-\t\t\trepo_fmt.version, target_version, err.buf);\n+\tif (verify_repository_format(&repo_fmt, &err) < 0) {\n+\t\terror(\"cannot upgrade repository format from %d to %d: %s\",\n+\t\t repo_fmt.version, target_version, err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn -1;\n \t}\n+\tif (!repo_fmt.version && repo_fmt.unknown_extensions.nr)\n+\t\treturn error(\"cannot upgrade repository format: \"\n+\t\t\t \"unknown extension %s\",\n+\t\t\t repo_fmt.unknown_extensions.items[0].string);\n \n \tstrbuf_addf(&repo_version, \"%d\", target_version);\n \tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 51d1eba6050..6aa0f313bdd 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '\n \ttest_cmp_config -C client 1 core.repositoryformatversion\n '\n \n-test_expect_success 'converting to partial clone fails with noop extension' '\n+test_expect_success 'convert to partial clone with noop extension' '\n \trm -fr server client &&\n \ttest_create_repo server &&\n \ttest_commit -C server my_commit 1 &&\n@@ -50,7 +50,7 @@ test_expect_success 'converting to partial clone fails with noop extension' '\n \tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n \ttest_cmp_config -C client 0 core.repositoryformatversion &&\n \tgit -C client config extensions.noop true &&\n-\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+\tgit -C client fetch --unshallow --filter=\"blob:none\"\n '\n \n test_expect_success 'converting to partial clone fails with unrecognized extension' '\n-- \n2.28.0.rc0.105.gf9edc3c819\n\n"},{"id":"401617","messageId":"xmqqh7u8hrka.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200716062818.GC3242764@google.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T07:01:09Z","receivedAt":"2020-07-16T07:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Now that we officially permit repository extensions in repository\n> format v0, permit upgrading a repository with extensions from v0 to v1\n> as well.\n>\n> For example, this means a repository where the user has set\n> \"extensions.preciousObjects\" can use \"git fetch --filter=blob:none\n> origin\" to upgrade the repository to use v1 and the partial clone\n> extension.\n>\n> To avoid mistakes, continue to forbid repository format upgrades in v0\n> repositories with an unrecognized extension. This way, a v0 user\n> using a misspelled extension field gets a chance to correct the\n> mistake before updating to the less forgiving v1 format.\n\nThis needs to be managed carefully. When the next extension is\nadded to the codebase, that extension may be \"known\" to Git, but I\ndo not think it is a good idea to honor it in v0 repository, or\nallow upgrading v0 repository to v1 with such an extension that\nweren't \"known\" to Git. For example, a topic in flight adds\nobjectformat extension and I do not think it should be honored in v0\nrepository.\n\nHaving said that, the approach is OK for now at the tip of tonight's\nmaster, but the point is \"known\" vs \"unknown\" must be fixed right\nwith some means. E.g. tell people to throw the \"new\" extensions to\nthe list of \"unknown extensions\" in check_repo_format() when they\nadd new ones, or something.\n\nThanks.\n\n> +\tif (!repo_fmt.version && repo_fmt.unknown_extensions.nr)\n> +\t\treturn error(\"cannot upgrade repository format: \"\n> +\t\t\t \"unknown extension %s\",\n> +\t\t\t repo_fmt.unknown_extensions.items[0].string);\n> \n> \tstrbuf_addf(&repo_version, \"%d\", target_version);\n> \tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 51d1eba6050..6aa0f313bdd 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -42,7 +42,7 @@ test_expect_success 'convert shallow clone to partial clone' '\n> \ttest_cmp_config -C client 1 core.repositoryformatversion\n> '\n> \n> -test_expect_success 'converting to partial clone fails with noop extension' '\n> +test_expect_success 'convert to partial clone with noop extension' '\n> \trm -fr server client &&\n> \ttest_create_repo server &&\n> \ttest_commit -C server my_commit 1 &&\n> @@ -50,7 +50,7 @@ test_expect_success 'converting to partial clone fails with noop extension' '\n> \tgit clone --depth=1 \"file://$(pwd)/server\" client &&\n> \ttest_cmp_config -C client 0 core.repositoryformatversion &&\n> \tgit -C client config extensions.noop true &&\n> -\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n> +\tgit -C client fetch --unshallow --filter=\"blob:none\"\n> '\n> \n> test_expect_success 'converting to partial clone fails with unrecognized extension' '\n"},{"id":"401619","messageId":"nycvar.QRO.7.76.6.2007161011270.54@tvgsbejvaqbjf.bet","threadId":"53852","inReplyTo":"20200716062054.GA3242764@google.com","subject":"Re: [PATCH 0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-16T08:13:27Z","receivedAt":"2020-07-16T09:40:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 15 Jul 2020, Jonathan Nieder wrote:\n\n> Junio C Hamano wrote:\n>\n> > Here is my quick attempt to see how far we can go with the\n> > \"different endgame\" approach, to be applied on top of those two\n> > patches.\n>\n> Here are patches implementing the minimal fix that I'd recommend.\n> These apply against \"master\" without requiring any other patches\n> as prerequisites. Thoughts?\n\nIIUC all of the existing `extensions.*` predate the reverted strict check,\nright? And the idea is that future `extensions.*` will only work when\n`repositoryFormatVersion` is larger than 1, right?\n\nI would have been fine with Junio's patch on top of Stolee's, and I am\nequally fine with this patch series. My main aim is not so much\nfuture-proofing, though, as it is to avoid regressions in existing setups.\n\nThanks,\nDscho\n"},{"id":"401622","messageId":"20200716105629.GC376357@coredump.intra.peff.net","threadId":"53852","inReplyTo":"20200716062429.GB3242764@google.com","subject":"Re: [PATCH 1/2] Revert \"check_repository_format_gently(): refuse extensions for old repositories\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-16T10:56:29Z","receivedAt":"2020-07-16T10:56:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 15, 2020 at 11:24:29PM -0700, Jonathan Nieder wrote:\n\n> The behavior introduced in 14c7fa269e4 might be a good behavior if we\n> were traveling back in time to 2015, but we're far too late. For some\n> reason I thought that it was what had been originally implemented and\n> that it had regressed. Apologies for not doing my research when\n> 14c7fa269e4 was under development.\n\nThanks for a good summary of the situation. I agree that the current\n(well, pre-14c7fa269e4) behavior is a bug (mine) from 2015, and we\nprobably have to accept that state of affairs to some degree in order to\navoid breaking existing cases.\n\nIt is unfortunate that this means that somebody with version=0 and\nextensions.preciousObjects is in danger of running a pre-2015 version of\nGit and having that extension totally ignored, which could be a\ndata-safety issue. But the farther we get from 2015 the less likely that\nis to be a problem (and the more likely somebody is to be depending on\nthe current behavior of v0+preciousObjects).\n\n> Let's return to the behavior we've had since 2015: always act on\n> extensions.* settings, regardless of repository format version. While\n> we're here, include some tests to describe the effect on the \"upgrade\n> repository version\" code path.\n\nSo this makes sense to me as a first step.\n\n-Peff\n"},{"id":"401623","messageId":"20200716110007.GD376357@coredump.intra.peff.net","threadId":"53852","inReplyTo":"xmqqh7u8hrka.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-16T11:00:07Z","receivedAt":"2020-07-16T11:00:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 12:01:09AM -0700, Junio C Hamano wrote:\n\n> > To avoid mistakes, continue to forbid repository format upgrades in v0\n> > repositories with an unrecognized extension. This way, a v0 user\n> > using a misspelled extension field gets a chance to correct the\n> > mistake before updating to the less forgiving v1 format.\n> \n> This needs to be managed carefully. When the next extension is\n> added to the codebase, that extension may be \"known\" to Git, but I\n> do not think it is a good idea to honor it in v0 repository, or\n> allow upgrading v0 repository to v1 with such an extension that\n> weren't \"known\" to Git. For example, a topic in flight adds\n> objectformat extension and I do not think it should be honored in v0\n> repository.\n> \n> Having said that, the approach is OK for now at the tip of tonight's\n> master, but the point is \"known\" vs \"unknown\" must be fixed right\n> with some means. E.g. tell people to throw the \"new\" extensions to\n> the list of \"unknown extensions\" in check_repo_format() when they\n> add new ones, or something.\n\nYeah, I agree with this line of reasoning. I'd prefer to see it\naddressed now, so that we don't have to remember to do anything later.\nI.e., for this patch to put the existing known extensions into the\n\"good\" list for v0, locking it into place forever, and leaving the\nobjectformat topic with nothing particular it needs to do.\n\nBut in the name of -rc1 expediency, I'm also OK moving forward with this\nfor now.\n\n-Peff\n"},{"id":"401625","messageId":"e32f27f4-6469-fe68-8263-bd10a101d380@gmail.com","threadId":"53852","inReplyTo":"nycvar.QRO.7.76.6.2007161011270.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/2] extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions)","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-16T12:17:03Z","receivedAt":"2020-07-16T12:17:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/16/2020 4:13 AM, Johannes Schindelin wrote:\n> Hi Jonathan,\n> \n> On Wed, 15 Jul 2020, Jonathan Nieder wrote:\n> \n>> Junio C Hamano wrote:\n>>\n>>> Here is my quick attempt to see how far we can go with the\n>>> \"different endgame\" approach, to be applied on top of those two\n>>> patches.\n>>\n>> Here are patches implementing the minimal fix that I'd recommend.\n>> These apply against \"master\" without requiring any other patches\n>> as prerequisites. Thoughts?\n> \n> IIUC all of the existing `extensions.*` predate the reverted strict check,\n> right? And the idea is that future `extensions.*` will only work when\n> `repositoryFormatVersion` is larger than 1, right?\n> \n> I would have been fine with Junio's patch on top of Stolee's, and I am\n> equally fine with this patch series. My main aim is not so much\n> future-proofing, though, as it is to avoid regressions in existing setups.\n\nI'm fine either way. I think that Jonathan's patch comes from a\nmore informed place than my patches, so his are probably safer.\n\nThe situation that caught my interest is covered by this test\nthat was part of my patches:\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 7cd45fc1394..6c0b82c3930 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -68,6 +68,18 @@ test_expect_success 'git sparse-checkout init' '\n check_files repo a\n '\n \n+test_expect_success 'git sparse-checkout works if repository format is wrong' '\n+ test_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n+ git -C repo config --unset core.repositoryFormatVersion &&\n+ git -C repo sparse-checkout init &&\n+ git -C repo config core.repositoryFormatVersion >actual &&\n+ echo 1 >expect &&\n+ git -C repo config core.repositoryFormatVersion 0 &&\n+ git -C repo sparse-checkout init &&\n+ git -C repo config core.repositoryFormatVersion >actual &&\n+ test_cmp expect actual\n+'\n+\n test_expect_success 'git sparse-checkout list after init' '\n git -C repo sparse-checkout list >actual &&\n cat >expect <<-\\EOF &&\n\nand this test passes with Jonathan's series. I think this kind\nof behavior is covered by his change to the 'converting to partial\nclone fails with noop extension' test in t0410-partial-clone.sh,\nso a duplicate test in t1091-sparse-checkout-builtin.sh may be\noverkill.\n\nThanks, all.\n\n-Stolee\n"},{"id":"401630","messageId":"20200716122513.GA1050962@coredump.intra.peff.net","threadId":"53852","inReplyTo":"20200716110007.GD376357@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-16T12:25:13Z","receivedAt":"2020-07-16T12:25:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 07:00:08AM -0400, Jeff King wrote:\n\n> > > To avoid mistakes, continue to forbid repository format upgrades in v0\n> > > repositories with an unrecognized extension. This way, a v0 user\n> > > using a misspelled extension field gets a chance to correct the\n> > > mistake before updating to the less forgiving v1 format.\n> > \n> > This needs to be managed carefully. When the next extension is\n> > added to the codebase, that extension may be \"known\" to Git, but I\n> > do not think it is a good idea to honor it in v0 repository, or\n> > allow upgrading v0 repository to v1 with such an extension that\n> > weren't \"known\" to Git. For example, a topic in flight adds\n> > objectformat extension and I do not think it should be honored in v0\n> > repository.\n> > \n> > Having said that, the approach is OK for now at the tip of tonight's\n> > master, but the point is \"known\" vs \"unknown\" must be fixed right\n> > with some means. E.g. tell people to throw the \"new\" extensions to\n> > the list of \"unknown extensions\" in check_repo_format() when they\n> > add new ones, or something.\n> \n> Yeah, I agree with this line of reasoning. I'd prefer to see it\n> addressed now, so that we don't have to remember to do anything later.\n> I.e., for this patch to put the existing known extensions into the\n> \"good\" list for v0, locking it into place forever, and leaving the\n> objectformat topic with nothing particular it needs to do.\n> \n> But in the name of -rc1 expediency, I'm also OK moving forward with this\n> for now.\n\nHmm, this is actually a bit trickier than I expected because of the way\nthe code is written. It's much easier to complain about extensions in a\nv0 repository than it is to ignore them. But I'm not sure if that isn't\nthe right way to go anyway.\n\nThe patch I came up with is below (and goes on top of Jonathan's). Even\nif we decide this is the right direction, it can definitely happen\npost-v2.28.\n\n-- >8 --\nSubject: verify_repository_format(): complain about new extensions in v0 repo\n\nWe made the mistake in the past of respecting extensions.* even when the\nrepository format version was set to 0. This is bad because forgetting\nto bump the repository version means that older versions of Git (which\ndo not know about our extensions) won't complain. I.e., it's not a\nproblem in itself, but it means your repository is in a state which does\nnot give you the protection you think you're getting from older\nversions.\n\nFor compatibility reasons, we are stuck with that decision for existing\nextensions. However, we'd prefer not to extend the damage further. We\ncan do that by catching any newly-added extensions and complaining about\nthe repository format.\n\nNote that this is a pretty heavy hammer: we'll refuse to work with the\nrepository at all. A lesser option would be to ignore (possibly with a\nwarning) any new extensions. But because of the way the extensions are\nhandled, that puts the burden on each new extension that is added to\nremember to \"undo\" itself (because they are handled before we know\nfor sure whether we are in a v1 repo or not, since we don't insist on a\nparticular ordering of config entries).\n\nSo one option would be to rewrite that handling to record any new\nextensions (and their values) during the config parse, and then only\nafter proceed to handle new ones only if we're in a v1 repository. But\nI'm not sure if it's worth the trouble:\n\n - ignoring extensions is likely to end up with broken results anyway\n (e.g., ignoring a proposed objectformat extension means parsing any\n object data is likely to encounter errors)\n\n - this is a sign that whatever tool wrote the extension field is\n broken. We may be better off notifying immediately and forcefully so\n that such tools don't even appear to work accidentally.\n\nThe only downside is that fixing the istuation is a little tricky,\nbecause programs like \"git config\" won't want to work with the\nrepository. But:\n\n git config --file=.git/config core.repositoryformatversion 1\n\nshould still suffice.\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +\n setup.c | 96 ++++++++++++++++++++++++++++++++++-------\n t/t1302-repo-version.sh | 3 ++\n 3 files changed, 85 insertions(+), 16 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 654426460c..0290849c19 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1044,6 +1044,7 @@ struct repository_format {\n \tint hash_algo;\n \tchar *work_tree;\n \tstruct string_list unknown_extensions;\n+\tstruct string_list v1_only_extensions;\n };\n \n /*\n@@ -1057,6 +1058,7 @@ struct repository_format {\n \t.is_bare = -1, \\\n \t.hash_algo = GIT_HASH_SHA1, \\\n \t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n+\t.v1_only_extensions = STRING_LIST_INIT_DUP, \\\n }\n \n /*\ndiff --git a/setup.c b/setup.c\nindex 3a81307602..c1480b2b60 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -447,6 +447,54 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n+enum extension_result {\n+\tEXTENSION_ERROR = -1, /* compatible with error(), etc */\n+\tEXTENSION_UNKNOWN = 0,\n+\tEXTENSION_OK = 1\n+};\n+\n+/*\n+ * Do not add new extensions to this function. It handles extensions which are\n+ * respected even in v0-format repositories for historical compatibility.\n+ */\n+enum extension_result handle_extension_v0(const char *var,\n+\t\t\t\t\t const char *value,\n+\t\t\t\t\t const char *ext,\n+\t\t\t\t\t struct repository_format *data)\n+{\n+\t\tif (!strcmp(ext, \"noop\")) {\n+\t\t\treturn EXTENSION_OK;\n+\t\t} else if (!strcmp(ext, \"preciousobjects\")) {\n+\t\t\tdata->precious_objects = git_config_bool(var, value);\n+\t\t\treturn EXTENSION_OK;\n+\t\t} else if (!strcmp(ext, \"partialclone\")) {\n+\t\t\tif (!value)\n+\t\t\t\treturn config_error_nonbool(var);\n+\t\t\tdata->partial_clone = xstrdup(value);\n+\t\t\treturn EXTENSION_OK;\n+\t\t} else if (!strcmp(ext, \"worktreeconfig\")) {\n+\t\t\tdata->worktree_config = git_config_bool(var, value);\n+\t\t\treturn EXTENSION_OK;\n+\t\t}\n+\n+\t\treturn EXTENSION_UNKNOWN;\n+}\n+\n+/*\n+ * Record any new extensions in this function.\n+ */\n+enum extension_result handle_extension(const char *var,\n+\t\t\t\t const char *value,\n+\t\t\t\t const char *ext,\n+\t\t\t\t struct repository_format *data)\n+{\n+\tif (!strcmp(ext, \"noop-v1\")) {\n+\t\treturn EXTENSION_OK;\n+\t}\n+\n+\treturn EXTENSION_UNKNOWN;\n+}\n+\n static int check_repo_format(const char *var, const char *value, void *vdata)\n {\n \tstruct repository_format *data = vdata;\n@@ -455,23 +503,25 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n \tif (strcmp(var, \"core.repositoryformatversion\") == 0)\n \t\tdata->version = git_config_int(var, value);\n \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n-\t\t/*\n-\t\t * record any known extensions here; otherwise,\n-\t\t * we fall through to recording it as unknown, and\n-\t\t * check_repository_format will complain\n-\t\t */\n-\t\tif (!strcmp(ext, \"noop\"))\n-\t\t\t;\n-\t\telse if (!strcmp(ext, \"preciousobjects\"))\n-\t\t\tdata->precious_objects = git_config_bool(var, value);\n-\t\telse if (!strcmp(ext, \"partialclone\")) {\n-\t\t\tif (!value)\n-\t\t\t\treturn config_error_nonbool(var);\n-\t\t\tdata->partial_clone = xstrdup(value);\n-\t\t} else if (!strcmp(ext, \"worktreeconfig\"))\n-\t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\telse\n+\t\tswitch (handle_extension_v0(var, value, ext, data)) {\n+\t\tcase EXTENSION_ERROR:\n+\t\t\treturn -1;\n+\t\tcase EXTENSION_OK:\n+\t\t\treturn 0;\n+\t\tcase EXTENSION_UNKNOWN:\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tswitch (handle_extension(var, value, ext, data)) {\n+\t\tcase EXTENSION_ERROR:\n+\t\t\treturn -1;\n+\t\tcase EXTENSION_OK:\n+\t\t\tstring_list_append(&data->v1_only_extensions, ext);\n+\t\t\treturn 0;\n+\t\tcase EXTENSION_UNKNOWN:\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\n@@ -510,6 +560,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \tset_repository_format_partial_clone(candidate->partial_clone);\n \trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n+\tstring_list_clear(&candidate->v1_only_extensions, 0);\n \n \tif (repository_format_worktree_config) {\n \t\t/*\n@@ -588,6 +639,7 @@ int read_repository_format(struct repository_format *format, const char *path)\n void clear_repository_format(struct repository_format *format)\n {\n \tstring_list_clear(&format->unknown_extensions, 0);\n+\tstring_list_clear(&format->v1_only_extensions, 0);\n \tfree(format->work_tree);\n \tfree(format->partial_clone);\n \tinit_repository_format(format);\n@@ -613,6 +665,18 @@ int verify_repository_format(const struct repository_format *format,\n \t\treturn -1;\n \t}\n \n+\tif (format->version == 0 && format->v1_only_extensions.nr) {\n+\t\tint i;\n+\n+\t\tstrbuf_addstr(err,\n+\t\t\t _(\"repo version is 0, but v1-only extensions found:\"));\n+\n+\t\tfor (i = 0; i < format->v1_only_extensions.nr; i++)\n+\t\t\tstrbuf_addf(err, \"\\n\\t%s\",\n+\t\t\t\t format->v1_only_extensions.items[i].string);\n+\t\treturn -1;\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex d60c042ce8..0acabb6d11 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -87,6 +87,9 @@ allow 1\n allow 1 noop\n abort 1 no-such-extension\n allow 0 no-such-extension\n+allow 0 noop\n+abort 0 noop-v1\n+allow 1 noop-v1\n EOF\n \n test_expect_success 'precious-objects allowed' '\n-- \n2.28.0.rc0.424.g7d08728e23\n\n"},{"id":"401632","messageId":"2d5bc430-f6b3-1b79-a46f-d3d6d6b9fa89@gmail.com","threadId":"53852","inReplyTo":"20200716122513.GA1050962@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-16T12:53:27Z","receivedAt":"2020-07-16T12:53:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/16/2020 8:25 AM, Jeff King wrote:\n> On Thu, Jul 16, 2020 at 07:00:08AM -0400, Jeff King wrote:\n> \n>>>> To avoid mistakes, continue to forbid repository format upgrades in v0\n>>>> repositories with an unrecognized extension. This way, a v0 user\n>>>> using a misspelled extension field gets a chance to correct the\n>>>> mistake before updating to the less forgiving v1 format.\n>>>\n>>> This needs to be managed carefully. When the next extension is\n>>> added to the codebase, that extension may be \"known\" to Git, but I\n>>> do not think it is a good idea to honor it in v0 repository, or\n>>> allow upgrading v0 repository to v1 with such an extension that\n>>> weren't \"known\" to Git. For example, a topic in flight adds\n>>> objectformat extension and I do not think it should be honored in v0\n>>> repository.\n>>>\n>>> Having said that, the approach is OK for now at the tip of tonight's\n>>> master, but the point is \"known\" vs \"unknown\" must be fixed right\n>>> with some means. E.g. tell people to throw the \"new\" extensions to\n>>> the list of \"unknown extensions\" in check_repo_format() when they\n>>> add new ones, or something.\n>>\n>> Yeah, I agree with this line of reasoning. I'd prefer to see it\n>> addressed now, so that we don't have to remember to do anything later.\n>> I.e., for this patch to put the existing known extensions into the\n>> \"good\" list for v0, locking it into place forever, and leaving the\n>> objectformat topic with nothing particular it needs to do.\n>>\n>> But in the name of -rc1 expediency, I'm also OK moving forward with this\n>> for now.\n> \n> Hmm, this is actually a bit trickier than I expected because of the way\n> the code is written. It's much easier to complain about extensions in a\n> v0 repository than it is to ignore them. But I'm not sure if that isn't\n> the right way to go anyway.\n> \n> The patch I came up with is below (and goes on top of Jonathan's). Even\n> if we decide this is the right direction, it can definitely happen\n> post-v2.28.\n> \n> -- >8 --\n> Subject: verify_repository_format(): complain about new extensions in v0 repo\n> \n> We made the mistake in the past of respecting extensions.* even when the\n> repository format version was set to 0. This is bad because forgetting\n> to bump the repository version means that older versions of Git (which\n> do not know about our extensions) won't complain. I.e., it's not a\n> problem in itself, but it means your repository is in a state which does\n> not give you the protection you think you're getting from older\n> versions.\n> \n> For compatibility reasons, we are stuck with that decision for existing\n> extensions. However, we'd prefer not to extend the damage further. We\n> can do that by catching any newly-added extensions and complaining about\n> the repository format.\n> \n> Note that this is a pretty heavy hammer: we'll refuse to work with the\n> repository at all. A lesser option would be to ignore (possibly with a\n> warning) any new extensions. But because of the way the extensions are\n> handled, that puts the burden on each new extension that is added to\n> remember to \"undo\" itself (because they are handled before we know\n> for sure whether we are in a v1 repo or not, since we don't insist on a\n> particular ordering of config entries).\n> \n> So one option would be to rewrite that handling to record any new\n> extensions (and their values) during the config parse, and then only\n> after proceed to handle new ones only if we're in a v1 repository. But\n> I'm not sure if it's worth the trouble:\n> \n> - ignoring extensions is likely to end up with broken results anyway\n> (e.g., ignoring a proposed objectformat extension means parsing any\n> object data is likely to encounter errors)\n> \n> - this is a sign that whatever tool wrote the extension field is\n> broken. We may be better off notifying immediately and forcefully so\n> that such tools don't even appear to work accidentally.\n> \n> The only downside is that fixing the istuation is a little tricky,\n\ns/istuation/situation\n\n> because programs like \"git config\" won't want to work with the\n> repository. But:\n> \n> git config --file=.git/config core.repositoryformatversion 1\n> \n> should still suffice.\n> \n> Signed-off-by: Jeff King \n> ---\n> cache.h | 2 +\n> setup.c | 96 ++++++++++++++++++++++++++++++++++-------\n> t/t1302-repo-version.sh | 3 ++\n> 3 files changed, 85 insertions(+), 16 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index 654426460c..0290849c19 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1044,6 +1044,7 @@ struct repository_format {\n> \tint hash_algo;\n> \tchar *work_tree;\n> \tstruct string_list unknown_extensions;\n> +\tstruct string_list v1_only_extensions;\n> };\n> \n> /*\n> @@ -1057,6 +1058,7 @@ struct repository_format {\n> \t.is_bare = -1, \\\n> \t.hash_algo = GIT_HASH_SHA1, \\\n> \t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n> +\t.v1_only_extensions = STRING_LIST_INIT_DUP, \\\n> }\n> \n> /*\n> diff --git a/setup.c b/setup.c\n> index 3a81307602..c1480b2b60 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -447,6 +447,54 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)\n> \treturn 0;\n> }\n> \n> +enum extension_result {\n> +\tEXTENSION_ERROR = -1, /* compatible with error(), etc */\n> +\tEXTENSION_UNKNOWN = 0,\n> +\tEXTENSION_OK = 1\n> +};\n> +\n> +/*\n> + * Do not add new extensions to this function. It handles extensions which are\n> + * respected even in v0-format repositories for historical compatibility.\n> + */\n> +enum extension_result handle_extension_v0(const char *var,\n> +\t\t\t\t\t const char *value,\n> +\t\t\t\t\t const char *ext,\n> +\t\t\t\t\t struct repository_format *data)\n...\n> +/*\n> + * Record any new extensions in this function.\n> + */\n> +enum extension_result handle_extension(const char *var,\n> +\t\t\t\t const char *value,\n> +\t\t\t\t const char *ext,\n> +\t\t\t\t struct repository_format *data)\n\nI like the split between these two methods to make it\nreally clear the difference between \"v0\" and \"v1\".\n\n> \tstruct repository_format *data = vdata;\n> @@ -455,23 +503,25 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n> \tif (strcmp(var, \"core.repositoryformatversion\") == 0)\n> \t\tdata->version = git_config_int(var, value);\n> \telse if (skip_prefix(var, \"extensions.\", &ext)) {\n...\n> +\t\tswitch (handle_extension_v0(var, value, ext, data)) {\n> +\t\tcase EXTENSION_ERROR:\n> +\t\t\treturn -1;\n> +\t\tcase EXTENSION_OK:\n> +\t\t\treturn 0;\n> +\t\tcase EXTENSION_UNKNOWN:\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tswitch (handle_extension(var, value, ext, data)) {\n> +\t\tcase EXTENSION_ERROR:\n> +\t\t\treturn -1;\n> +\t\tcase EXTENSION_OK:\n> +\t\t\tstring_list_append(&data->v1_only_extensions, ext);\n> +\t\t\treturn 0;\n> +\t\tcase EXTENSION_UNKNOWN:\n> \t\t\tstring_list_append(&data->unknown_extensions, ext);\n> +\t\t\treturn 0;\n> +\t\t}\n> \t}\n\nAnd it makes this loop much cleaner.\n> @@ -613,6 +665,18 @@ int verify_repository_format(const struct repository_format *format,\n> \t\treturn -1;\n> \t}\n> \n> +\tif (format->version == 0 && format->v1_only_extensions.nr) {\n> +\t\tint i;\n> +\n> +\t\tstrbuf_addstr(err,\n> +\t\t\t _(\"repo version is 0, but v1-only extensions found:\"));\n> +\n> +\t\tfor (i = 0; i < format->v1_only_extensions.nr; i++)\n> +\t\t\tstrbuf_addf(err, \"\\n\\t%s\",\n> +\t\t\t\t format->v1_only_extensions.items[i].string);\n> +\t\treturn -1;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\n> index d60c042ce8..0acabb6d11 100755\n> --- a/t/t1302-repo-version.sh\n> +++ b/t/t1302-repo-version.sh\n> @@ -87,6 +87,9 @@ allow 1\n> allow 1 noop\n> abort 1 no-such-extension\n> allow 0 no-such-extension\n> +allow 0 noop\n> +abort 0 noop-v1\n> +allow 1 noop-v1\n\nLGTM.\n\nThanks,\n-Stolee\n\n\n"},{"id":"401639","messageId":"xmqqd04vigpy.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200716110007.GD376357@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T16:10:01Z","receivedAt":"2020-07-16T16:10:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I agree with this line of reasoning. I'd prefer to see it\n> addressed now, so that we don't have to remember to do anything later.\n\nVery true. Also the documentation may need some updating,\ne.g. \"These 4 extensions are honored without adding\nrepositoryFormatVersion to your repository (as special cases)\" to\navoid further confusion e.g. \"why doesn't my objectFormat=SHA-3 does\nnot take effect by itself?\".\n\n> I.e., for this patch to put the existing known extensions into the\n> \"good\" list for v0, locking it into place forever, and leaving the\n> objectformat topic with nothing particular it needs to do.\n>\n> But in the name of -rc1 expediency, I'm also OK moving forward with this\n> for now.\n\nI'm OK, too, as I said.\n\nI'd need to kick out bc/sha-2-part-3 topic out of my tree while that\ninfrastructure is in place on top of these two patches, though.\n\nThanks.\n"},{"id":"401640","messageId":"xmqq5zanifoc.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200716122513.GA1050962@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T16:32:35Z","receivedAt":"2020-07-16T16:32:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm, this is actually a bit trickier than I expected because of the way\n> the code is written. It's much easier to complain about extensions in a\n> v0 repository than it is to ignore them. But I'm not sure if that isn't\n> the right way to go anyway.\n>\n> The patch I came up with is below (and goes on top of Jonathan's). Even\n> if we decide this is the right direction, it can definitely happen\n> post-v2.28.\n\nIt must happen already in 'seen' if we want to keep bc/sha-2-part-3\nwith us, though ;-)\n\n> So one option would be to rewrite that handling to record any new\n> extensions (and their values) during the config parse, and then only\n> after proceed to handle new ones only if we're in a v1 repository.\n\nI do not think it would be too bad for read_repository_format() to\ncall git_config_from_file() to collect extensions.* in a string list\nwhile looking for core.repositoryformatversion. Then the function\ncan iterate over the string list to call check_repo_format() itself.\n\n> I'm not sure if it's worth the trouble:\n>\n> - ignoring extensions is likely to end up with broken results anyway\n> (e.g., ignoring a proposed objectformat extension means parsing any\n> object data is likely to encounter errors)\n\nThe primary reason why the safety calls for ignore/reject is the\nnamespace collision. We may decide to use extensions.objectformat\nto record what hash algorithms are used for objects in the\nrepository, while the end user and their tools may use it for\ntotally different purpose (perhaps they have a custom script around\n\"git repack\" that reads the variable to learn what command line\noptions e.g. --window=800 to pass). A new version of Git that\nsupports SHA-2 will suddenly break their configuration, when the\nusers are 100% happy with the current SHA-1 system, with the way\ntheir tool uses extensions.objectformat configuration variable and a\nnewer version of Git that happens to know how to also work with SHA-2,\nusing their v0 repository.\n\nAnd the reasoning 'ignoring would make the problem get noticed\nanyway' does not apply to such users at all, does it?\n\nWe need to declare that any names under \"extensions.*\" is off limits\nby end users regardless and write it in big flasing red letters if\nwe haven't already done so. It is enforced in v1 repositories by\ndying upon seeing an unrecognised extension, but not entirely. When\nthe users are lucky, a known-but-name-collided extension may stop\nwith a type error (e.g. \"extensions.objectformat=frotz\" may say\n\"frotz is not among the accepted hash algorithms\") but that is not\nguaranteed. In v0 repositories, enforcing it after the fact would\ncause the same trouble as the tightening caused.\n\n"},{"id":"401641","messageId":"xmqqv9inh0c5.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200716122513.GA1050962@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T16:49:14Z","receivedAt":"2020-07-16T16:49:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: verify_repository_format(): complain about new extensions in v0 repo\n>\n> We made the mistake in the past of respecting extensions.* even when the\n> repository format version was set to 0. This is bad because forgetting\n> to bump the repository version means that older versions of Git (which\n> do not know about our extensions) won't complain. I.e., it's not a\n> problem in itself, but it means your repository is in a state which does\n> not give you the protection you think you're getting from older\n> versions.\n>\n> For compatibility reasons, we are stuck with that decision for existing\n> extensions. However, we'd prefer not to extend the damage further. We\n> can do that by catching any newly-added extensions and complaining about\n> the repository format.\n\nLooking good overall, but I needed this to build from the source.\n\nThanks.\n\n setup.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex e29659b7b9..e69bd28ed6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -457,10 +457,10 @@ enum extension_result {\n * Do not add new extensions to this function. It handles extensions which are\n * respected even in v0-format repositories for historical compatibility.\n */\n-enum extension_result handle_extension_v0(const char *var,\n-\t\t\t\t\t const char *value,\n-\t\t\t\t\t const char *ext,\n-\t\t\t\t\t struct repository_format *data)\n+static enum extension_result handle_extension_v0(const char *var,\n+\t\t\t\t\t\t const char *value,\n+\t\t\t\t\t\t const char *ext,\n+\t\t\t\t\t\t struct repository_format *data)\n {\n \t\tif (!strcmp(ext, \"noop\")) {\n \t\t\treturn EXTENSION_OK;\n@@ -483,10 +483,10 @@ enum extension_result handle_extension_v0(const char *var,\n /*\n * Record any new extensions in this function.\n */\n-enum extension_result handle_extension(const char *var,\n-\t\t\t\t const char *value,\n-\t\t\t\t const char *ext,\n-\t\t\t\t struct repository_format *data)\n+static enum extension_result handle_extension(const char *var,\n+\t\t\t\t\t const char *value,\n+\t\t\t\t\t const char *ext,\n+\t\t\t\t\t struct repository_format *data)\n {\n \tif (!strcmp(ext, \"noop-v1\")) {\n \t\treturn EXTENSION_OK;\n"},{"id":"401642","messageId":"20200716165341.GA1072075@coredump.intra.peff.net","threadId":"53852","inReplyTo":"xmqq5zanifoc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-16T16:53:41Z","receivedAt":"2020-07-16T16:53:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 09:32:35AM -0700, Junio C Hamano wrote:\n\n> We need to declare that any names under \"extensions.*\" is off limits\n> by end users regardless and write it in big flasing red letters if\n> we haven't already done so.\n\nI thought this was already well-understood, and it was definitely part\nof the plan since 2015. Are other tools really sticking stuff in\nextensions.* that we don't know about?\n\n-Peff\n"},{"id":"401643","messageId":"20200716165651.GB1072075@coredump.intra.peff.net","threadId":"53852","inReplyTo":"xmqqv9inh0c5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-16T16:56:51Z","receivedAt":"2020-07-16T16:56:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 09:49:14AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Subject: verify_repository_format(): complain about new extensions in v0 repo\n> >\n> > We made the mistake in the past of respecting extensions.* even when the\n> > repository format version was set to 0. This is bad because forgetting\n> > to bump the repository version means that older versions of Git (which\n> > do not know about our extensions) won't complain. I.e., it's not a\n> > problem in itself, but it means your repository is in a state which does\n> > not give you the protection you think you're getting from older\n> > versions.\n> >\n> > For compatibility reasons, we are stuck with that decision for existing\n> > extensions. However, we'd prefer not to extend the damage further. We\n> > can do that by catching any newly-added extensions and complaining about\n> > the repository format.\n> \n> Looking good overall, but I needed this to build from the source.\n\nOof, thanks. I did this as a one-off not even on a branch, and my\nconfig.mak magic loosens -Werror in that case (because usually a\ndetached HEAD means I'm investigating some old commit, and quite a few\nof them don't build without warnings these days).\n\nThankfully it seems I only managed a minor error without the compiler\nthere to help me. :)\n\n-Peff\n"},{"id":"401664","messageId":"xmqq4kq7gq8n.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"xmqq5zanifoc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T20:27:20Z","receivedAt":"2020-07-16T20:27:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Hmm, this is actually a bit trickier than I expected because of the way\n>> the code is written. It's much easier to complain about extensions in a\n>> v0 repository than it is to ignore them. But I'm not sure if that isn't\n>> the right way to go anyway.\n>>\n>> The patch I came up with is below (and goes on top of Jonathan's). Even\n>> if we decide this is the right direction, it can definitely happen\n>> post-v2.28.\n>\n> It must happen already in 'seen' if we want to keep bc/sha-2-part-3\n> with us, though ;-)\n\nFWIW, I needed to adjust t0001 while merging the SHA-2 topic. The\ninternal use of \"git config\" via test_config triggers the \"this is\nnot a Git repository as the value of repositoryformatversion and the\ndefined set of extensions are incompatible\".\n\ndiff --cc t/t0001-init.sh\nindex 6d2467995e,34d2064660..ff538c0eed\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n ...\n -test_expect_success 'extensions.objectFormat is not honored with repo version 0' '\n++test_expect_success 'extensions.objectFormat would cause an error in repo version 0' '\n+ \tgit init --object-format=sha256 explicit-v0 &&\n -\ttest_config -C explicit-v0 core.repositoryformatversion 0 &&\n -\tgit -C explicit-v0 rev-parse --show-object-format >actual &&\n -\techo sha1 >expected &&\n -\ttest_cmp expected actual\n++\tv=$(git config --file=explicit-v0/.git/config core.repositoryformatversion 0) &&\n++\ttest_when_finished \"\n++\t\tgit config --file=explicit-v0/.git/config core.repositoryformatversion $v\n++\t\" &&\n++\tgit config --file=explicit-v0/.git/config core.repositoryformatversion 0 &&\n++\ttest_must_fail git -C explicit-v0 rev-parse --show-object-format >actual\n+ '\n\n"},{"id":"401669","messageId":"20200716223719.GA899@gmail.com","threadId":"53852","inReplyTo":"xmqqd04vigpy.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-16T22:37:19Z","receivedAt":"2020-07-16T23:10:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(replying from vacation; back tomorrow)\nJunio C Hamano wrote:\n> Jeff King writes:\n\n>> Yeah, I agree with this line of reasoning. I'd prefer to see it\n>> addressed now, so that we don't have to remember to do anything later.\n>\n> Very true. Also the documentation may need some updating,\n> e.g. \"These 4 extensions are honored without adding\n> repositoryFormatVersion to your repository (as special cases)\" to\n> avoid further confusion e.g. \"why doesn't my objectFormat=SHA-3 does\n> not take effect by itself?\".\n\nYes, I agree, especially about documentation.\n\nFor 2.29, I would like to do or see the following:\n\n- defining the list of repository format v0 supported extensions as\n \"these and no more\", futureproofing along the lines suggested in\n Peff's patch. I like the general approach taken there since it\n allows parsing the relevant config in a single pass, so I think\n it basically takes the right approach. (That said, it might be\n possible to simplify a bit with further changes, e.g. by using the\n configset API.)\n\n When doing this for real, we'd want to document the set of\n supported extensions. That is especially useful to independent\n implementers wanting to support Git's formats, since it tells\n them \"this is the minimum set of extensions that you must\n either handle or error out cleanly on to maintain compatibility\n with Git's repository format v0\".\n \n- improving the behavior when an extension not supported in v0 is\n encountered in a v0 repository. For extensions that are supported\n in v1 and not v0, we should presumably error out so the user can\n repair the repository, and we can put the \"noop\" extension in that\n category for the sake of easy testing. We can also include a check\n in \"git fsck\" for repositories that request the undefined behavior\n of v0 repositories with non-v0 extensions, for faster diagnosis.\n\n What about unrecognized extensions that are potentially extensions\n yet to be defined? Should these be silently ignored to match the\n historical behavior, or should we error out even in repository\n format v0? I lean toward the latter; we'll need to be cautious,\n though, e.g. by making this a separate patch so we can easily tweak\n it if this ends up being disruptive in some unanticipated way.\n\n- making \"git init\" use repository format v1 by default. It's been\n long enough that users can count on Git implementations supporting\n it. This way, users are less likely to run into v0+extensions\n confusion, just because users are less likely to be using v0.\n\nDoes that sound like a good plan to others? If so, are there any\nsteps beyond the two first patches in jn/v0-with-extensions-fix that\nwe would want in order to prepare for it in 2.28?\n\nMy preference would be to move forward in 2.28 with the first two\npatches in that topic branch (i.e., *not* the third yet), since they\ndon't produce any user facing behavior that would create danger for\nusers or clash with this plan. Today, the only extensions we\nrecognize are in that set of extensions that we'll want to continue to\nrecognize in v0 (except possibly the for-testing extension \"noop\").\nThe steps to take with additional extensions are more subtle so it\nseems reasonable for them to bake in \"next\" and then \"master\" for a\n2.29 release.\n\nThanks,\nJonathan\n"},{"id":"401672","messageId":"xmqqh7u7f29h.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200716223719.GA899@gmail.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T23:50:34Z","receivedAt":"2020-07-16T23:50:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> - defining the list of repository format v0 supported extensions as\n> \"these and no more\", futureproofing along the lines suggested in\n> Peff's patch. I like the general approach taken there since it\n> allows parsing the relevant config in a single pass, so I think\n> it basically takes the right approach. (That said, it might be\n> possible to simplify a bit with further changes, e.g. by using the\n> configset API.)\n>\n> When doing this for real, we'd want to document the set of\n> supported extensions. That is especially useful to independent\n> implementers wanting to support Git's formats, since it tells\n> them \"this is the minimum set of extensions that you must\n> either handle or error out cleanly on to maintain compatibility\n> with Git's repository format v0\".\n\nGood.\n\n> - improving the behavior when an extension not supported in v0 is\n> encountered in a v0 repository. For extensions that are supported\n> in v1 and not v0, we should presumably error out so the user can\n> repair the repository, and we can put the \"noop\" extension in that\n> category for the sake of easy testing. We can also include a check\n> in \"git fsck\" for repositories that request the undefined behavior\n> of v0 repositories with non-v0 extensions, for faster diagnosis.\n>\n> What about unrecognized extensions that are potentially extensions\n> yet to be defined? Should these be silently ignored to match the\n> historical behavior, or should we error out even in repository\n> format v0? I lean toward the latter; we'll need to be cautious,\n> though, e.g. by making this a separate patch so we can easily tweak\n> it if this ends up being disruptive in some unanticipated way.\n\nI disagree with your first paragraph. Those that weren't honored by\nmistake back in v0 days, in addition to those that aren't known to us\neven now, should just be silently ignored, not causing an error.\n\n> - making \"git init\" use repository format v1 by default. It's been\n> long enough that users can count on Git implementations supporting\n> it. This way, users are less likely to run into v0+extensions\n> confusion, just because users are less likely to be using v0.\n\nAbsolutely. I would think this is a very good move.\n\n> Does that sound like a good plan to others? If so, are there any\n> steps beyond the two first patches in jn/v0-with-extensions-fix that\n> we would want in order to prepare for it in 2.28?\n>\n> My preference would be to move forward in 2.28 with the first two\n> patches in that topic branch (i.e., *not* the third yet), since they\n> don't produce any user facing behavior that would create danger for\n> users or clash with this plan.\n\nYup, I agree. I'd give another name to the third commit and then\nrewind jn/v0-with-extensions-fix by one to prevent mistakes from\nhappening. Thanks.\n\n\n"},{"id":"401702","messageId":"20200717152251.GA1224964@coredump.intra.peff.net","threadId":"53852","inReplyTo":"20200716223719.GA899@gmail.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-17T15:22:51Z","receivedAt":"2020-07-17T15:22:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 03:37:19PM -0700, Jonathan Nieder wrote:\n\n> For 2.29, I would like to do or see the following:\n> \n> - defining the list of repository format v0 supported extensions as\n> \"these and no more\", futureproofing along the lines suggested in\n> Peff's patch. I like the general approach taken there since it\n> allows parsing the relevant config in a single pass, so I think\n> it basically takes the right approach. (That said, it might be\n> possible to simplify a bit with further changes, e.g. by using the\n> configset API.)\n> \n> When doing this for real, we'd want to document the set of\n> supported extensions. That is especially useful to independent\n> implementers wanting to support Git's formats, since it tells\n> them \"this is the minimum set of extensions that you must\n> either handle or error out cleanly on to maintain compatibility\n> with Git's repository format v0\".\n\nI think we should still consider people setting v0 along with extensions\nto be a bug. It was never documented to work that way and we are being\nnice by keeping the existing behavior, but it is still wrong (and\npre-extension versions of Git will silently ignore them). I don't mind\nmaking other implementers aware of the special status, but we should be\ncareful not to endorse the broken setup.\n\n> - making \"git init\" use repository format v1 by default. It's been\n> long enough that users can count on Git implementations supporting\n> it. This way, users are less likely to run into v0+extensions\n> confusion, just because users are less likely to be using v0.\n\nThat's probably reasonable. It will be mildly annoying for people like\nme who are often testing old versions of Git, but I'm sure I will\nsurvive.\n\nWe should make sure that all major implementations handle v1 reasonably\nfirst, though (and that they did so long enough ago that it's not likely\nto cause problems).\n\n> My preference would be to move forward in 2.28 with the first two\n> patches in that topic branch (i.e., *not* the third yet), since they\n> don't produce any user facing behavior that would create danger for\n> users or clash with this plan. Today, the only extensions we\n> recognize are in that set of extensions that we'll want to continue to\n> recognize in v0 (except possibly the for-testing extension \"noop\").\n> The steps to take with additional extensions are more subtle so it\n> seems reasonable for them to bake in \"next\" and then \"master\" for a\n> 2.29 release.\n\nI'm OK with the plan to ship the first two patches for 2.28, and leave\nmy patch for later (or even soften it from \"die\" to \"ignore with a\nwarning\").\n\nI think leaving \"noop\" in that set of special extensions makes sense,\nsince it lets us test that case easily (and I added a \"noop-v1\" in my\npatch to test the other one; clearly we could also flip it and have\nnoop-v0).\n\n-Peff\n"},{"id":"401703","messageId":"20200717152744.GB1224964@coredump.intra.peff.net","threadId":"53852","inReplyTo":"xmqqh7u7f29h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-17T15:27:44Z","receivedAt":"2020-07-17T15:27:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 04:50:34PM -0700, Junio C Hamano wrote:\n\n> > - improving the behavior when an extension not supported in v0 is\n> > encountered in a v0 repository. For extensions that are supported\n> > in v1 and not v0, we should presumably error out so the user can\n> > repair the repository, and we can put the \"noop\" extension in that\n> > category for the sake of easy testing. We can also include a check\n> > in \"git fsck\" for repositories that request the undefined behavior\n> > of v0 repositories with non-v0 extensions, for faster diagnosis.\n> >\n> > What about unrecognized extensions that are potentially extensions\n> > yet to be defined? Should these be silently ignored to match the\n> > historical behavior, or should we error out even in repository\n> > format v0? I lean toward the latter; we'll need to be cautious,\n> > though, e.g. by making this a separate patch so we can easily tweak\n> > it if this ends up being disruptive in some unanticipated way.\n> \n> I disagree with your first paragraph. Those that weren't honored by\n> mistake back in v0 days, in addition to those that aren't known to us\n> even now, should just be silently ignored, not causing an error.\n\nThat's very much the opposite of my patch. As we add new extensions,\nthose \"unknowns\" will start to die().\n\nI remain unconvinced that there are a bunch of unknown extension.*\nconfig options hanging around in the wild, but maybe I'm being naive.\nIt seems to me more likely that users will be helped by warning about\nextensions that _should_ have had v1 set than that they will be harmed\nbecause they put random crap in their extensions.* config. But maybe you\nknow of a specific example?\n\nAnyway, if we move to \"v1\" as the default for \"git init\" anyway, then\nthe number of people being helped would become much smaller.\n\n> > My preference would be to move forward in 2.28 with the first two\n> > patches in that topic branch (i.e., *not* the third yet), since they\n> > don't produce any user facing behavior that would create danger for\n> > users or clash with this plan.\n> \n> Yup, I agree. I'd give another name to the third commit and then\n> rewind jn/v0-with-extensions-fix by one to prevent mistakes from\n> happening. Thanks.\n\nOK. I was confused to see it still at the tip in the latest What's\nCooking, but I think we're just crossing emails. :)\n\n-Peff\n"},{"id":"401706","messageId":"xmqqr1tadq8k.fsf@gitster.c.googlers.com","threadId":"53852","inReplyTo":"20200717152744.GB1224964@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] repository: allow repository format upgrade with extensions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-17T17:07:55Z","receivedAt":"2020-07-17T17:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Anyway, if we move to \"v1\" as the default for \"git init\" anyway, then\n> the number of people being helped would become much smaller.\n\nYup. So in that sense I do not think I care too deeply either way.\n\n>> > My preference would be to move forward in 2.28 with the first two\n>> > patches in that topic branch (i.e., *not* the third yet), since they\n>> > don't produce any user facing behavior that would create danger for\n>> > users or clash with this plan.\n>> \n>> Yup, I agree. I'd give another name to the third commit and then\n>> rewind jn/v0-with-extensions-fix by one to prevent mistakes from\n>> happening. Thanks.\n>\n> OK. I was confused to see it still at the tip in the latest What's\n> Cooking, but I think we're just crossing emails. :)\n\nYes.\n"}]}