{"thread":{"id":"53850","subject":"[PATCH] setup: warn if extensions exist on old format","startedAt":"2020-07-13T19:20:59Z","lastAt":"2020-07-14T20:24:13Z","messageCount":15,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Junio C Hamano","Derrick Stolee","Jonathan Nieder","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401500","messageId":"pull.674.git.1594668051847.gitgitgadget@gmail.com","threadId":"53850","inReplyTo":null,"subject":"[PATCH] setup: warn if extensions exist on old format","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-13T19:20:51Z","receivedAt":"2020-07-13T19:20:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nPrior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions\nfor old repositories, 2020-06-05), Git was honoring configured\nextensions, even if core.repositoryFormatVersion was 0 (or unset). This\nwas incorrect, and is now fixed.\n\nThe issue now is that users who relied on that previously bad behavior\nwill upgrade to the next version of Git and suddently be in a bad\nsituation. In particular, users of the 'git sparse-checkout' builting\nwill rely on the extensions.worktreeConfig for the core.sparseCheckout\nand core.sparseCheckoutCone config options. Without that extension,\nthese users will suddenly have repositories stop acting like a sparse\nrepo.\n\nWhat is worse is that a user will be confronted with the following\nerror if they try to run 'git sparse-checkout init' again:\n\n\twarning: unable to upgrade repository format from 0 to 1\n\nThis is because the logic in 14c7fa269e4 refuses to upgrae repos when\nthe version is unset but extensions exist.\n\nWhile it is important to correct this improper behavior, create a\nwarning so users in this situation can correct themselves without too\nmuch guesswork. By creating a warning in\ncheck_repository_format_gently(), we can alert the user if they have a\nocnfigured extension but not a configured repository version.\n\nSigned-off-by: Derrick Stolee \n---\n setup: warn if extensions exist on old format\n \n This is based on xl/upgrade-repo-format.\n \n Can this be considered for v2.28.0-rc1? I think this is an important\n shift in behavior that will disrupt many users if they upgrade without\n it!\n \n If not this warning or change, then how else can we help users who are\n in this situation? How can we save those who adopted the sparse-checkout\n builtin in recent versions from terrible post-upgrade headaches?\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-674%2Fderrickstolee%2Fsparse-checkout-warning-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-674/derrickstolee/sparse-checkout-warning-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/674\n\n setup.c | 5 +++++\n t/t1091-sparse-checkout-builtin.sh | 9 +++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex eb066db6d8..6ff6c54d39 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -542,6 +542,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t}\n \n+\tif (candidate->version == 0 && candidate->has_extensions) {\n+\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n+\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 88cdde255c..d249428f44 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -68,6 +68,15 @@ test_expect_success 'git sparse-checkout init' '\n \tcheck_files repo a\n '\n \n+test_expect_success 'warning about core.repositoryFormatVersion' '\n+\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n+\tgit -C repo status 2>err &&\n+\ttest_must_be_empty err &&\n+\tgit -C repo config --local core.repositoryFormatVersion 0 &&\n+\tgit -C repo status 2>err &&\n+\ttest_i18ngrep \"some extensions are enabled, but core.repositoryFormatVersion=0\" err\n+'\n+\n test_expect_success 'git sparse-checkout list after init' '\n \tgit -C repo sparse-checkout list >actual &&\n \tcat >expect <<-\\EOF &&\n\nbase-commit: 14c7fa269e42df4133edd9ae7763b678ed6594cd\n-- \ngitgitgadget\n"},{"id":"401501","messageId":"20200713193405.GC77607@syl.lan","threadId":"53850","inReplyTo":"pull.674.git.1594668051847.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup: warn if extensions exist on old format","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-13T19:34:05Z","receivedAt":"2020-07-13T19:34:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 13, 2020 at 07:20:51PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> Prior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions\n> for old repositories, 2020-06-05), Git was honoring configured\n> extensions, even if core.repositoryFormatVersion was 0 (or unset). This\n> was incorrect, and is now fixed.\n>\n> The issue now is that users who relied on that previously bad behavior\n> will upgrade to the next version of Git and suddently be in a bad\n> situation. In particular, users of the 'git sparse-checkout' builting\n> will rely on the extensions.worktreeConfig for the core.sparseCheckout\n> and core.sparseCheckoutCone config options. Without that extension,\n> these users will suddenly have repositories stop acting like a sparse\n> repo.\n>\n> What is worse is that a user will be confronted with the following\n> error if they try to run 'git sparse-checkout init' again:\n>\n> \twarning: unable to upgrade repository format from 0 to 1\n>\n> This is because the logic in 14c7fa269e4 refuses to upgrae repos when\n\ns/upgrae/upgrade\n\n> the version is unset but extensions exist.\n\nI'm not sure that I want to get into a discussion about whether or not\nthis logic is right while -rc0 is already out, but it seems like\n14c7fa269e4 could be tweaked to be a little more tolerant of this case.\n\nOn the other hand, I think that the approach here is perfectly sensible,\nabsent of a more invasive change to the logic in 14c7fa269e4.\n\n> While it is important to correct this improper behavior, create a\n> warning so users in this situation can correct themselves without too\n> much guesswork. By creating a warning in\n> check_repository_format_gently(), we can alert the user if they have a\n> ocnfigured extension but not a configured repository version.\n\ns/ocnfigured/configured\n\n> Signed-off-by: Derrick Stolee \n> ---\n> setup: warn if extensions exist on old format\n>\n> This is based on xl/upgrade-repo-format.\n>\n> Can this be considered for v2.28.0-rc1? I think this is an important\n> shift in behavior that will disrupt many users if they upgrade without\n> it!\n\nI would be happy to see something like this in -rc1, unless Junio has\nreservations about making this change at this point in the release cycle\n(I do not have any such concerns).\n\n> If not this warning or change, then how else can we help users who are\n> in this situation? How can we save those who adopted the sparse-checkout\n> builtin in recent versions from terrible post-upgrade headaches?\n>\n> Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-674%2Fderrickstolee%2Fsparse-checkout-warning-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-674/derrickstolee/sparse-checkout-warning-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/674\n>\n> setup.c | 5 +++++\n> t/t1091-sparse-checkout-builtin.sh | 9 +++++++++\n> 2 files changed, 14 insertions(+)\n>\n> diff --git a/setup.c b/setup.c\n> index eb066db6d8..6ff6c54d39 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -542,6 +542,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n> \t\t}\n> \t}\n>\n> +\tif (candidate->version == 0 && candidate->has_extensions) {\n> +\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n> +\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n> +\t}\n> +\n> \treturn 0;\n> }\n>\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 88cdde255c..d249428f44 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -68,6 +68,15 @@ test_expect_success 'git sparse-checkout init' '\n> \tcheck_files repo a\n> '\n>\n> +test_expect_success 'warning about core.repositoryFormatVersion' '\n> +\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n> +\tgit -C repo status 2>err &&\n> +\ttest_must_be_empty err &&\n> +\tgit -C repo config --local core.repositoryFormatVersion 0 &&\n\nI don't think it's that difficult to see that this patch is correct, but\nit might be worth testing this for the case that\n'core.repositoryFormatVersion' is unset, too.\n\n> +\tgit -C repo status 2>err &&\n> +\ttest_i18ngrep \"some extensions are enabled, but core.repositoryFormatVersion=0\" err\n> +'\n> +\n> test_expect_success 'git sparse-checkout list after init' '\n> \tgit -C repo sparse-checkout list >actual &&\n> \tcat >expect <<-\\EOF &&\n>\n> base-commit: 14c7fa269e42df4133edd9ae7763b678ed6594cd\n> --\n> gitgitgadget\n\nThanks,\nTaylor\n"},{"id":"401502","messageId":"xmqqblkjnr5m.fsf@gitster.c.googlers.com","threadId":"53850","inReplyTo":"pull.674.git.1594668051847.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup: warn if extensions exist on old format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-13T19:36:37Z","receivedAt":"2020-07-13T19:36:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> Prior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions\n> for old repositories, 2020-06-05), Git was honoring configured\n> extensions, even if core.repositoryFormatVersion was 0 (or unset). This\n> was incorrect, and is now fixed.\n>\n> The issue now is that users who relied on that previously bad behavior\n> will upgrade to the next version of Git and suddently be in a bad\n> situation. In particular, users of the 'git sparse-checkout' builting\n> will rely on the extensions.worktreeConfig for the core.sparseCheckout\n> and core.sparseCheckoutCone config options. Without that extension,\n> these users will suddenly have repositories stop acting like a sparse\n> repo.\n\ns/builting/command/ perhaps???\n\n>\n> What is worse is that a user will be confronted with the following\n> error if they try to run 'git sparse-checkout init' again:\n>\n> \twarning: unable to upgrade repository format from 0 to 1\n>\n> This is because the logic in 14c7fa269e4 refuses to upgrae repos when\n> the version is unset but extensions exist.\n>\n> While it is important to correct this improper behavior, create a\n> warning so users in this situation can correct themselves without too\n> much guesswork. By creating a warning in\n> check_repository_format_gently(), we can alert the user if they have a\n> ocnfigured extension but not a configured repository version.\n\ns/ocn/con/\n\n> Signed-off-by: Derrick Stolee \n> ---\n\nThanks for a thoughtful analysis of the situation and coming up with\na plan to remedy.\n\n> +\tif (candidate->version == 0 && candidate->has_extensions) {\n> +\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n> +\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n> +\t}\n\nThat reads well.\n\nAn alternative may be to grandfather some extensions that were\nenabled by git by mistake without updating the format version, and\nwe update the repository even if the repository has extensions that\nshould not exist, but those offending extensions are limited only to\nthose that we decide to special case. That would make the end-user\nexperience even smoother.\n\nIs extenions.worktreeCOnfig the only one that needs this escape\nhatch?\n\n\n"},{"id":"401504","messageId":"95c338d0-dbf0-669d-59a3-8c2d6c87c8fc@gmail.com","threadId":"53850","inReplyTo":"20200713193405.GC77607@syl.lan","subject":"Re: [PATCH] setup: warn if extensions exist on old format","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-13T19:41:20Z","receivedAt":"2020-07-13T19:41:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/13/2020 3:34 PM, Taylor Blau wrote:\n> s/upgrae/upgrade\n> s/ocnfigured/configured\n\nOh man, what was going on with me when I was typing this message.\n\nThanks for the thorough inspection.\n\n>> diff --git a/setup.c b/setup.c\n>> index eb066db6d8..6ff6c54d39 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -542,6 +542,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n>> \t\t}\n>> \t}\n>>\n>> +\tif (candidate->version == 0 && candidate->has_extensions) {\n>> +\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n>> +\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>>\n>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n>> index 88cdde255c..d249428f44 100755\n>> --- a/t/t1091-sparse-checkout-builtin.sh\n>> +++ b/t/t1091-sparse-checkout-builtin.sh\n>> @@ -68,6 +68,15 @@ test_expect_success 'git sparse-checkout init' '\n>> \tcheck_files repo a\n>> '\n>>\n>> +test_expect_success 'warning about core.repositoryFormatVersion' '\n>> +\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n>> +\tgit -C repo status 2>err &&\n>> +\ttest_must_be_empty err &&\n>> +\tgit -C repo config --local core.repositoryFormatVersion 0 &&\n> \n> I don't think it's that difficult to see that this patch is correct, but\n> it might be worth testing this for the case that\n> 'core.repositoryFormatVersion' is unset, too.\n\nYou were absolutely right to check this, since this diff causes\nthe test to fail:\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex d249428f44..b5de141292 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -74,6 +74,9 @@ test_expect_success 'warning about core.repositoryFormatVersion' '\n test_must_be_empty err &&\n git -C repo config --local core.repositoryFormatVersion 0 &&\n git -C repo status 2>err &&\n+ test_i18ngrep \"some extensions are enabled, but core.repositoryFormatVersion=0\" err &&\n+ git -C repo config --local --unset core.repositoryFormatVersion 0 &&\n+ git -C repo status 2>err &&\n test_i18ngrep \"some extensions are enabled, but core.repositoryFormatVersion=0\" err\n '\n \n\nI'll investigate why the \"unset\" case is different than the \"0\" case.\n\nHopefully the \"should we do this?\" question can continue being discussed\nwhile I work on a v2.\n\nThanks,\n-Stolee\n"},{"id":"401509","messageId":"xmqqy2nnmbh8.fsf@gitster.c.googlers.com","threadId":"53850","inReplyTo":"xmqqblkjnr5m.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] setup: warn if extensions exist on old format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-13T20:00:35Z","receivedAt":"2020-07-13T20:00:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> An alternative may be to grandfather some extensions that were\n> enabled by git by mistake without updating the format version, and\n> we update the repository even if the repository has extensions that\n> should not exist, but those offending extensions are limited only to\n> those that we decide to special case. That would make the end-user\n> experience even smoother.\n>\n> Is extenions.worktreeCOnfig the only one that needs this escape\n> hatch?\n\nAssuming that worktreeconfig is the only thing, the change may look\nlike this. With this change, we might want to drop the new warning\nin hunk ll.542- to avoid encouraging people to muck with their\nrepository with random configuration variables that happen to share\nextensions.* prefix with us.\n\n cache.h | 2 +-\n setup.c | 17 +++++++++++++----\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e5885cc9ea..8ff46857f6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,8 +1042,8 @@ struct repository_format {\n \tint worktree_config;\n \tint is_bare;\n \tint hash_algo;\n-\tint has_extensions;\n \tchar *work_tree;\n+\tint has_unallowed_extensions;\n \tstruct string_list unknown_extensions;\n };\n \ndiff --git a/setup.c b/setup.c\nindex eb066db6d8..5f4786d3b9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,12 +455,13 @@ 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 \t\t * check_repository_format will complain\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@@ -469,10 +470,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\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} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\telse\n+\t\t\tis_unallowed_extension = 0;\n+\t\t} else\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\n+\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\n@@ -542,6 +546,11 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t}\n \n+\tif (candidate->version == 0 && candidate->has_unallowed_extensions) {\n+\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n+\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n+\t}\n+\n \treturn 0;\n }\n \n@@ -560,7 +569,7 @@ int upgrade_repository_format(int 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 (!repo_fmt.version && repo_fmt.has_unallowed_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 \t\tstrbuf_release(&err);\n"},{"id":"401512","messageId":"xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com","threadId":"53850","inReplyTo":"xmqqy2nnmbh8.fsf@gitster.c.googlers.com","subject":"[PATCH] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-13T20:18:06Z","receivedAt":"2020-07-13T20:18:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"fail and warn\" approach introduced in the previous step may be\nwith smaller impact to the codebase, but\n\n - it requires the end-user to read, understand and execute the\n manual upgrade\n\n - it encourages to follow the same procedure blindly, making the\n protection even less useful\n\nLet's instead keep failing hard without teaching how to bypass the\nrepository protection, but allow upgrading even when only the\nworktreeconfig extension exists in an old repository, which is\nlikely to be set by a broke version of Git that did not update the\nrepository version when setting the extension.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This time with a log message to explain and justify the change,\n with a matching tweak to the test script, designed to be applied\n on top, but feel free to squash it in if you agree with me that\n we do not need two separate commits for this.\n\n Thanks.\n\n cache.h | 2 +-\n setup.c | 17 ++++++++---------\n t/t1091-sparse-checkout-builtin.sh | 9 ---------\n 3 files changed, 9 insertions(+), 19 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e5885cc9ea..8ff46857f6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,8 +1042,8 @@ struct repository_format {\n \tint worktree_config;\n \tint is_bare;\n \tint hash_algo;\n-\tint has_extensions;\n \tchar *work_tree;\n+\tint has_unallowed_extensions;\n \tstruct string_list unknown_extensions;\n };\n \ndiff --git a/setup.c b/setup.c\nindex 6ff6c54d39..65270440a9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,12 +455,13 @@ 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 \t\t * check_repository_format will complain\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@@ -469,10 +470,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\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} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\telse\n+\t\t\tis_unallowed_extension = 0;\n+\t\t} else\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\n+\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\n@@ -542,11 +546,6 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t}\n \t}\n \n-\tif (candidate->version == 0 && candidate->has_extensions) {\n-\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n-\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n-\t}\n-\n \treturn 0;\n }\n \n@@ -565,7 +564,7 @@ int upgrade_repository_format(int 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 (!repo_fmt.version && repo_fmt.has_unallowed_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 \t\tstrbuf_release(&err);\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex d249428f44..88cdde255c 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -68,15 +68,6 @@ test_expect_success 'git sparse-checkout init' '\n \tcheck_files repo a\n '\n \n-test_expect_success 'warning about core.repositoryFormatVersion' '\n-\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n-\tgit -C repo status 2>err &&\n-\ttest_must_be_empty err &&\n-\tgit -C repo config --local core.repositoryFormatVersion 0 &&\n-\tgit -C repo status 2>err &&\n-\ttest_i18ngrep \"some extensions are enabled, but core.repositoryFormatVersion=0\" err\n-'\n-\n test_expect_success 'git sparse-checkout list after init' '\n \tgit -C repo sparse-checkout list >actual &&\n \tcat >expect <<-\\EOF &&\n-- \n2.28.0-rc0\n\n"},{"id":"401516","messageId":"b8ae18f7-a0af-bd62-2553-92989e1dbe7e@gmail.com","threadId":"53850","inReplyTo":"xmqqpn8zmao1.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-07-13T20:46:57Z","receivedAt":"2020-07-13T20:47:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/13/2020 4:18 PM, Junio C Hamano wrote:\n> The \"fail and warn\" approach introduced in the previous step may be\n> with smaller impact to the codebase, but\n> \n> - it requires the end-user to read, understand and execute the\n> manual upgrade\n> \n> - it encourages to follow the same procedure blindly, making the\n> protection even less useful\n> \n> Let's instead keep failing hard without teaching how to bypass the\n> repository protection, but allow upgrading even when only the\n> worktreeconfig extension exists in an old repository, which is\n> likely to be set by a broke version of Git that did not update the\n> repository version when setting the extension.\n\nThis is a more subtle way to handle the case. In fact, it\nsilently makes extensions.worktreeConfig work as it did in 2.27,\nwhich means users will not have any troubles after upgrading.\n\nI also like that you are actually fixing the case where a user in\nthe bad state _can_ get out using \"git sparse-checkout init\".\n\nThis can be verified by adding this test:\n\ntest_expect_success 'git sparse-checkout works if repository format is wrong' '\n\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n\tgit -C repo sparse-checkout init &&\n\tgit -C repo config core.repositoryFormatVersion >actual &&\n\techo 1 >expect &&\n\ttest_cmp expect actual &&\n\tgit -C repo config core.repositoryFormatVersion 0 &&\n\tgit -C repo sparse-checkout init &&\n\tgit -C repo config core.repositoryFormatVersion >actual &&\n\ttest_cmp expect actual\n'\n\n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * This time with a log message to explain and justify the change,\n> with a matching tweak to the test script, designed to be applied\n> on top, but feel free to squash it in if you agree with me that\n> we do not need two separate commits for this.\n\nSince this commit removes all evidence of the previous one, I would\nrecommend just squashing them together.\n\nThanks for your fast feedback!\n-Stolee\n"},{"id":"401518","messageId":"xmqqlfjnm6ll.fsf@gitster.c.googlers.com","threadId":"53850","inReplyTo":"b8ae18f7-a0af-bd62-2553-92989e1dbe7e@gmail.com","subject":"Re: [PATCH] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-13T21:45:58Z","receivedAt":"2020-07-13T21:46:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I also like that you are actually fixing the case where a user in\n> the bad state _can_ get out using \"git sparse-checkout init\".\n>\n> This can be verified by adding this test:\n>\n> test_expect_success 'git sparse-checkout works if repository format is wrong' '\n> \ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n> \tgit -C repo sparse-checkout init &&\n> \tgit -C repo config core.repositoryFormatVersion >actual &&\n> \techo 1 >expect &&\n> \ttest_cmp expect actual &&\n> \tgit -C repo config core.repositoryFormatVersion 0 &&\n> \tgit -C repo sparse-checkout init &&\n> \tgit -C repo config core.repositoryFormatVersion >actual &&\n> \ttest_cmp expect actual\n> '\n>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> \n>> * This time with a log message to explain and justify the change,\n>> with a matching tweak to the test script, designed to be applied\n>> on top, but feel free to squash it in if you agree with me that\n>> we do not need two separate commits for this.\n>\n> Since this commit removes all evidence of the previous one, I would\n> recommend just squashing them together.\n\nAlright, then care to do the honors ;-)? Let's make sure we have it\nin -rc1 to avoid nasty \"regression\" in the upcoming release.\n\nThanks for raising the issue and exploring the solution space.\n\n"},{"id":"401524","messageId":"pull.674.v2.git.1594690017.gitgitgadget@gmail.com","threadId":"53850","inReplyTo":"pull.674.git.1594668051847.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] setup: warn if extensions exist on old format","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-14T01:26:55Z","receivedAt":"2020-07-14T01:27:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is based on xl/upgrade-repo-format.\n\nThanks, Taylor and Junio for jumping in with helpful review.\n\nUpdates in v2:\n\n 1. My initial patch is essentially dropped in its entirety, with Junio's\n patch here instead. I added an extra test and kept some of my commit\n message.\n \n \n 2. A second patch has joined the fray, hopefully answering the concerned\n raise by Johannes [1].\n \n \n\n[1] \nhttps://lore.kernel.org/git/pull.675.git.1594677321039.gitgitgadget@gmail.com/\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n setup: tweak upgrade policy to grandfather worktreeconfig\n config: provide extra detail about worktree config\n\n builtin/config.c | 5 +++--\n cache.h | 2 +-\n setup.c | 12 ++++++++----\n t/t1091-sparse-checkout-builtin.sh | 12 ++++++++++++\n t/t2404-worktree-config.sh | 12 ++++++++++++\n 5 files changed, 36 insertions(+), 7 deletions(-)\n\n\nbase-commit: 14c7fa269e42df4133edd9ae7763b678ed6594cd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-674%2Fderrickstolee%2Fsparse-checkout-warning-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-674/derrickstolee/sparse-checkout-warning-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/674\n\nRange-diff vs v1:\n\n 1: 113cb7cda2 ! 1: 1b26d9710a setup: warn if extensions exist on old format\n @@ Metadata\n Author: Derrick Stolee \n \n ## Commit message ##\n - setup: warn if extensions exist on old format\n + setup: tweak upgrade policy to grandfather worktreeconfig\n \n Prior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions\n for old repositories, 2020-06-05), Git was honoring configured\n @@ Commit message\n \n The issue now is that users who relied on that previously bad behavior\n will upgrade to the next version of Git and suddently be in a bad\n - situation. In particular, users of the 'git sparse-checkout' builting\n + situation. In particular, users of the 'git sparse-checkout' command\n will rely on the extensions.worktreeConfig for the core.sparseCheckout\n and core.sparseCheckoutCone config options. Without that extension,\n these users will suddenly have repositories stop acting like a sparse\n @@ Commit message\n This is because the logic in 14c7fa269e4 refuses to upgrae repos when\n the version is unset but extensions exist.\n \n - While it is important to correct this improper behavior, create a\n - warning so users in this situation can correct themselves without too\n - much guesswork. By creating a warning in\n - check_repository_format_gently(), we can alert the user if they have a\n - ocnfigured extension but not a configured repository version.\n + One possible way to alert a user of this issue is to warn them when Git\n + notices an extension exists, but core.repositoryFormatVersion is not a\n + correct value. However,\n \n + - it requires the end-user to read, understand and execute the\n + manual upgrade\n +\n + - it encourages to follow the same procedure blindly, making the\n + protection even less useful\n +\n + Let's instead keep failing hard without teaching how to bypass the\n + repository protection, but allow upgrading even when only the\n + worktreeconfig extension exists in an old repository, which is\n + likely to be set by a broke version of Git that did not update the\n + repository version when setting the extension.\n +\n + This change of behavior is made visible by testing how 'git\n + sparse-checkout init' behaves to upgrade the repository format version\n + even if the extension.worktreeConfig is already set. This would\n + previously fail without a clear way forward.\n +\n + Signed-off-by: Junio C Hamano \n Signed-off-by: Derrick Stolee \n \n + ## cache.h ##\n +@@ cache.h: struct repository_format {\n + \tint worktree_config;\n + \tint is_bare;\n + \tint hash_algo;\n +-\tint has_extensions;\n + \tchar *work_tree;\n ++\tint has_unallowed_extensions;\n + \tstruct string_list unknown_extensions;\n + };\n + \n +\n ## setup.c ##\n -@@ setup.c: static int check_repository_format_gently(const char *gitdir, struct repository_\n - \t\t}\n +@@ setup.c: 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 + \t\t * check_repository_format will complain\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 +@@ setup.c: static int check_repo_format(const char *var, const char *value, void *vdata)\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} else if (!strcmp(ext, \"worktreeconfig\")) {\n + \t\t\tdata->worktree_config = git_config_bool(var, value);\n +-\t\telse\n ++\t\t\tis_unallowed_extension = 0;\n ++\t\t} else\n + \t\t\tstring_list_append(&data->unknown_extensions, ext);\n ++\n ++\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n \t}\n \n -+\tif (candidate->version == 0 && candidate->has_extensions) {\n -+\t\twarning(_(\"some extensions are enabled, but core.repositoryFormatVersion=0\"));\n -+\t\twarning(_(\"if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'\"));\n -+\t}\n -+\n - \treturn 0;\n - }\n + \treturn read_worktree_config(var, value, vdata);\n +@@ setup.c: int upgrade_repository_format(int 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 (!repo_fmt.version && repo_fmt.has_unallowed_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 + \t\tstrbuf_release(&err);\n \n ## t/t1091-sparse-checkout-builtin.sh ##\n @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'git sparse-checkout init' '\n \tcheck_files repo a\n '\n \n -+test_expect_success 'warning about core.repositoryFormatVersion' '\n ++test_expect_success 'git sparse-checkout works if repository format is wrong' '\n +\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n -+\tgit -C repo status 2>err &&\n -+\ttest_must_be_empty err &&\n -+\tgit -C repo config --local core.repositoryFormatVersion 0 &&\n -+\tgit -C repo status 2>err &&\n -+\ttest_i18ngrep \"some extensions are enabled, but core.repositoryFormatVersion=0\" err\n ++\tgit -C repo config --unset core.repositoryFormatVersion &&\n ++\tgit -C repo sparse-checkout init &&\n ++\tgit -C repo config core.repositoryFormatVersion >actual &&\n ++\techo 1 >expect &&\n ++\tgit -C repo config core.repositoryFormatVersion 0 &&\n ++\tgit -C repo sparse-checkout init &&\n ++\tgit -C repo config core.repositoryFormatVersion >actual &&\n ++\ttest_cmp expect actual\n +'\n +\n test_expect_success 'git sparse-checkout list after init' '\n -: ---------- > 2: e11e973c6f config: provide extra detail about worktree config\n\n-- \ngitgitgadget\n"},{"id":"401525","messageId":"1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com","threadId":"53850","inReplyTo":"pull.674.v2.git.1594690017.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-14T01:26:56Z","receivedAt":"2020-07-14T01:27:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nPrior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions\nfor old repositories, 2020-06-05), Git was honoring configured\nextensions, even if core.repositoryFormatVersion was 0 (or unset). This\nwas incorrect, and is now fixed.\n\nThe issue now is that users who relied on that previously bad behavior\nwill upgrade to the next version of Git and suddently be in a bad\nsituation. In particular, users of the 'git sparse-checkout' command\nwill rely on the extensions.worktreeConfig for the core.sparseCheckout\nand core.sparseCheckoutCone config options. Without that extension,\nthese users will suddenly have repositories stop acting like a sparse\nrepo.\n\nWhat is worse is that a user will be confronted with the following\nerror if they try to run 'git sparse-checkout init' again:\n\n\twarning: unable to upgrade repository format from 0 to 1\n\nThis is because the logic in 14c7fa269e4 refuses to upgrae repos when\nthe version is unset but extensions exist.\n\nOne possible way to alert a user of this issue is to warn them when Git\nnotices an extension exists, but core.repositoryFormatVersion is not a\ncorrect value. However,\n\n - it requires the end-user to read, understand and execute the\n manual upgrade\n\n - it encourages to follow the same procedure blindly, making the\n protection even less useful\n\nLet's instead keep failing hard without teaching how to bypass the\nrepository protection, but allow upgrading even when only the\nworktreeconfig extension exists in an old repository, which is\nlikely to be set by a broke version of Git that did not update the\nrepository version when setting the extension.\n\nThis change of behavior is made visible by testing how 'git\nsparse-checkout init' behaves to upgrade the repository format version\neven if the extension.worktreeConfig is already set. This would\npreviously fail without a clear way forward.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Derrick Stolee \n---\n cache.h | 2 +-\n setup.c | 12 ++++++++----\n t/t1091-sparse-checkout-builtin.sh | 12 ++++++++++++\n 3 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e5885cc9ea..8ff46857f6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,8 +1042,8 @@ struct repository_format {\n \tint worktree_config;\n \tint is_bare;\n \tint hash_algo;\n-\tint has_extensions;\n \tchar *work_tree;\n+\tint has_unallowed_extensions;\n \tstruct string_list unknown_extensions;\n };\n \ndiff --git a/setup.c b/setup.c\nindex eb066db6d8..65270440a9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,12 +455,13 @@ 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 \t\t * check_repository_format will complain\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@@ -469,10 +470,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\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} else if (!strcmp(ext, \"worktreeconfig\")) {\n \t\t\tdata->worktree_config = git_config_bool(var, value);\n-\t\telse\n+\t\t\tis_unallowed_extension = 0;\n+\t\t} else\n \t\t\tstring_list_append(&data->unknown_extensions, ext);\n+\n+\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n \t}\n \n \treturn read_worktree_config(var, value, vdata);\n@@ -560,7 +564,7 @@ int upgrade_repository_format(int 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 (!repo_fmt.version && repo_fmt.has_unallowed_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 \t\tstrbuf_release(&err);\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 88cdde255c..6e339c7c8e 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 \tcheck_files repo a\n '\n \n+test_expect_success 'git sparse-checkout works if repository format is wrong' '\n+\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n+\tgit -C repo config --unset core.repositoryFormatVersion &&\n+\tgit -C repo sparse-checkout init &&\n+\tgit -C repo config core.repositoryFormatVersion >actual &&\n+\techo 1 >expect &&\n+\tgit -C repo config core.repositoryFormatVersion 0 &&\n+\tgit -C repo sparse-checkout init &&\n+\tgit -C repo config core.repositoryFormatVersion >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git sparse-checkout list after init' '\n \tgit -C repo sparse-checkout list >actual &&\n \tcat >expect <<-\\EOF &&\n-- \ngitgitgadget\n\n"},{"id":"401526","messageId":"e11e973c6fff6a523da090f7294234902e65a9d0.1594690017.git.gitgitgadget@gmail.com","threadId":"53850","inReplyTo":"pull.674.v2.git.1594690017.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] config: provide extra detail about worktree config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-14T01:26:57Z","receivedAt":"2020-07-14T01:27:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \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\nThis warning is specifically placed inside an existing error message\nfor 'git config --worktree' that already fails if the repository format\nversion is not 1.\n\nReported-by: Johannes Schindelin \nSigned-off-by: Derrick Stolee \n---\n builtin/config.c | 5 +++--\n t/t2404-worktree-config.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ee4aef6a35..2cdc293ae5 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 \t\telse if (worktrees[0] && worktrees[1])\n \t\t\tdie(_(\"--worktree cannot be used with multiple \"\n \t\t\t \"working trees unless the config\\n\"\n-\t\t\t \"extension worktreeConfig is enabled. \"\n-\t\t\t \"Please read \\\"CONFIGURATION FILE\\\"\\n\"\n+\t\t\t \"extension worktreeConfig is enabled \"\n+\t\t\t \"and core.repositoryFormatVersion is at least\\n\"\n+\t\t\t \"1. Please read \\\"CONFIGURATION FILE\\\"\"\n \t\t\t \"section in \\\"git help worktree\\\" for details\"));\n \t\telse\n \t\t\tgiven_config_source.file = git_pathdup(\"config\");\ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 9536d10919..303a2644bd 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -77,5 +77,17 @@ 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+\ttest_config core.repositoryformatversion 1 &&\n+\ttest_config extensions.worktreeConfig true &&\n+\tgit config --worktree test.one true &&\n+\ttest_cmp_config true test.one &&\n+\ttest_config core.repositoryformatversion 0 &&\n+\ttest_must_fail git config --worktree test.two true 2>err &&\n+\ttest_i18ngrep \"extension worktreeConfig is enabled\" err &&\n+\tgit config --unset core.repositoryformatversion &&\n+\ttest_must_fail git config --worktree test.three true 2>err &&\n+\ttest_i18ngrep \"core.repositoryFormatVersion is at least\" err\n+'\n \n test_done\n-- \ngitgitgadget\n"},{"id":"401531","messageId":"20200714040616.GA2208896@google.com","threadId":"53850","inReplyTo":"b8ae18f7-a0af-bd62-2553-92989e1dbe7e@gmail.com","subject":"Re: [PATCH] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-07-14T04:06:16Z","receivedAt":"2020-07-14T04:06:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n> On 7/13/2020 4:18 PM, Junio C Hamano wrote:\n\n>> The \"fail and warn\" approach introduced in the previous step may be\n>> with smaller impact to the codebase, but\n>>\n>> - it requires the end-user to read, understand and execute the\n>> manual upgrade\n>>\n>> - it encourages to follow the same procedure blindly, making the\n>> protection even less useful\n>>\n>> Let's instead keep failing hard without teaching how to bypass the\n>> repository protection, but allow upgrading even when only the\n>> worktreeconfig extension exists in an old repository, which is\n>> likely to be set by a broke version of Git that did not update the\n>> repository version when setting the extension.\n>\n> This is a more subtle way to handle the case. In fact, it\n> silently makes extensions.worktreeConfig work as it did in 2.27,\n> which means users will not have any troubles after upgrading.\n\nI'd like to propose a different endgame:\n\nInstead of looking at `extensions.*` settings one by one to see how\nvarious implementations handled them with repositoryFormatVersion=0,\nwhat if we treat repositoryFormatVersion=0 as a synonym of version=1?\n\nThat is:\n\n1. in new repositories, set repositoryFormatVersion = 1, since (1) Git\n versions in the wild should all already know how to handle it and\n (2) as we've learned, other Git implementations need to understand\n some of extensions.* anyway\n\n2. whenever setting any extensions.*, automatically upgrade to\n repositoryFormatVersion = 1\n\n3. when in an existing repository with extensions.* set and\n repositoryFormatVersion = 0, act as though repositoryFormatVersion = 1\n\n4. document this new behavior with repositoryFormatVersion = 0 in\n Documentation/technical/repository-version.txt\n\nThis way, the result is simpler than where we started.\n\nUnfortunately, we are not even that consistent about what to do with\nextensions.* settings when repositoryFormatVersion = 0 today. So\nwe'll have to exercise some care and analyze them one by one to make\nsure this is safe (and if it isn't, at *that* point we'd come up with\na more complex variant on (2) and (3) above).\n\nIt's too late to go that far for 2.28. It would be tempting to try a\nsimple revert of 14c7fa269e4 (check_repository_format_gently(): refuse\nextensions for old repositories, 2020-06-05) to get back to tried and\ntrue behavior but that does not do enough --- it still produces an\nerror when trying to upgrade repository format when any extensions are\nset. So how about such a revert plus Junio's patch plus the analogous\nchange to Junio's patch for\n\n extensions.preciousObjects\n extensions.partialClone\n\n?\n\nThanks,\nJonathan\n"},{"id":"401534","messageId":"xmqqa702n2kv.fsf@gitster.c.googlers.com","threadId":"53850","inReplyTo":"20200714040616.GA2208896@google.com","subject":"Re: [PATCH] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-14T04:27:28Z","receivedAt":"2020-07-14T04:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> It's too late to go that far for 2.28. It would be tempting to try a\n> simple revert of 14c7fa269e4 (check_repository_format_gently(): refuse\n> extensions for old repositories, 2020-06-05) to get back to tried and\n> true behavior but that does not do enough --- it still produces an\n> error when trying to upgrade repository format when any extensions are\n> set. So how about such a revert plus Junio's patch plus the analogous\n> change to Junio's patch for\n>\n> extensions.preciousObjects\n> extensions.partialClone\n\nMy illustration patch was done \"assuming that worktreeconfig is the\nonly thing we wrote by mistake without updating the format version\",\nand if these two also share the same problem, I obviously is 100%\nfine with covering these other ones with the same approach.\n\nI like your \"v0 and v1 are the same, but the repository is declared\nto be corrupt if the extentions that are not known by today's code\nis found in v0 repository\", by the way. Assuming that the two you\nlisted above plus worktreeconfig are the only ones known by today's\ncode, that is. We seem to also know about \"noop\", so shouldn't it\nalso be grandfathered in?\n\n"},{"id":"401559","messageId":"nycvar.QRO.7.76.6.2007142221370.52@tvgsbejvaqbjf.bet","threadId":"53850","inReplyTo":"1b26d9710a7ffaca0bad1f4e1c1729f501ed1559.1594690017.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] setup: tweak upgrade policy to grandfather worktreeconfig","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-14T20:22:25Z","receivedAt":"2020-07-14T20:22:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Tue, 14 Jul 2020, Derrick Stolee via GitGitGadget wrote:\n\n> This is because the logic in 14c7fa269e4 refuses to upgrae repos when\n> the version is unset but extensions exist.\n\ns/upgrae/upgrade/\n\nThe rest looks good to me.\n\nThank you for working on this,\nDscho\n\n> One possible way to alert a user of this issue is to warn them when Git\n> notices an extension exists, but core.repositoryFormatVersion is not a\n> correct value. However,\n>\n> - it requires the end-user to read, understand and execute the\n> manual upgrade\n>\n> - it encourages to follow the same procedure blindly, making the\n> protection even less useful\n>\n> Let's instead keep failing hard without teaching how to bypass the\n> repository protection, but allow upgrading even when only the\n> worktreeconfig extension exists in an old repository, which is\n> likely to be set by a broke version of Git that did not update the\n> repository version when setting the extension.\n>\n> This change of behavior is made visible by testing how 'git\n> sparse-checkout init' behaves to upgrade the repository format version\n> even if the extension.worktreeConfig is already set. This would\n> previously fail without a clear way forward.\n>\n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Derrick Stolee \n> ---\n> cache.h | 2 +-\n> setup.c | 12 ++++++++----\n> t/t1091-sparse-checkout-builtin.sh | 12 ++++++++++++\n> 3 files changed, 21 insertions(+), 5 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index e5885cc9ea..8ff46857f6 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1042,8 +1042,8 @@ struct repository_format {\n> \tint worktree_config;\n> \tint is_bare;\n> \tint hash_algo;\n> -\tint has_extensions;\n> \tchar *work_tree;\n> +\tint has_unallowed_extensions;\n> \tstruct string_list unknown_extensions;\n> };\n>\n> diff --git a/setup.c b/setup.c\n> index eb066db6d8..65270440a9 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -455,12 +455,13 @@ 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> \t\t * check_repository_format will complain\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> @@ -469,10 +470,13 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\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} else if (!strcmp(ext, \"worktreeconfig\")) {\n> \t\t\tdata->worktree_config = git_config_bool(var, value);\n> -\t\telse\n> +\t\t\tis_unallowed_extension = 0;\n> +\t\t} else\n> \t\t\tstring_list_append(&data->unknown_extensions, ext);\n> +\n> +\t\tdata->has_unallowed_extensions |= is_unallowed_extension;\n> \t}\n>\n> \treturn read_worktree_config(var, value, vdata);\n> @@ -560,7 +564,7 @@ int upgrade_repository_format(int 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 (!repo_fmt.version && repo_fmt.has_unallowed_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> \t\tstrbuf_release(&err);\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 88cdde255c..6e339c7c8e 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> \tcheck_files repo a\n> '\n>\n> +test_expect_success 'git sparse-checkout works if repository format is wrong' '\n> +\ttest_when_finished git -C repo config core.repositoryFormatVersion 1 &&\n> +\tgit -C repo config --unset core.repositoryFormatVersion &&\n> +\tgit -C repo sparse-checkout init &&\n> +\tgit -C repo config core.repositoryFormatVersion >actual &&\n> +\techo 1 >expect &&\n> +\tgit -C repo config core.repositoryFormatVersion 0 &&\n> +\tgit -C repo sparse-checkout init &&\n> +\tgit -C repo config core.repositoryFormatVersion >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'git sparse-checkout list after init' '\n> \tgit -C repo sparse-checkout list >actual &&\n> \tcat >expect <<-\\EOF &&\n> --\n> gitgitgadget\n>\n>\n"},{"id":"401560","messageId":"nycvar.QRO.7.76.6.2007142223320.52@tvgsbejvaqbjf.bet","threadId":"53850","inReplyTo":"pull.674.v2.git.1594690017.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] setup: warn if extensions exist on old format","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-14T20:24:01Z","receivedAt":"2020-07-14T20:24:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Tue, 14 Jul 2020, Derrick Stolee via GitGitGadget wrote:\n\n> 2. A second patch has joined the fray, hopefully answering the concerned\n> raise by Johannes [1].\n\nIt answers my concern!\n\nThank you for providing these patches,\nDscho\n"}]}