{"thread":{"id":"53463","subject":"[PATCH] fetch: allow adding a filter after initial clone.","startedAt":"2020-05-13T20:01:01Z","lastAt":"2020-06-08T17:00:02Z","messageCount":30,"participants":["Xin Li","Junio C Hamano","brian m. carlson","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397754","messageId":"20200513200040.68968-1-delphij@google.com","threadId":"53463","inReplyTo":null,"subject":"[PATCH] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-13T20:00:40Z","receivedAt":"2020-05-13T20:01:01Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Signed-off-by: Xin Li \n---\n builtin/fetch.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3ae52c015d..e5faa17ecd 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1790,8 +1790,16 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !has_promisor_remote())\n-\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n+\tif (filter_options.choice && !has_promisor_remote()) {\n+\t\tchar repo_version_string[10];\n+\n+\t\txsnprintf(repo_version_string, sizeof(repo_version_string),\n+\t\t\t \"%d\", (int)GIT_REPO_VERSION);\n+\t\tgit_config_set(\"core.repositoryformatversion\",\n+\t\t\trepo_version_string);\n+\t\tgit_config_set(\"extensions.partialclone\", \"origin\");\n+\t\tpromisor_remote_reinit();\n+\t}\n \n \tif (all) {\n \t\tif (argc == 1)\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397769","messageId":"xmqqo8qrsh0d.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200513200040.68968-1-delphij@google.com","subject":"Re: [PATCH] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T20:43:14Z","receivedAt":"2020-05-13T20:43:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n(nothnig).\n\nCan you help readers by describing what this change is about?\n\nThis space is reserved for the patch author to describe why this\nchange is a good idea (if this patch is adding a new feature), what\nis already broken without this patch (if this patch is a bugfix),\nand why this change is a safe thing to do (if this patch lifts a\nlimitation we had before that has been protecting us from getting\ninto a bad state).\n\n\n> Signed-off-by: Xin Li \n> ---\n> builtin/fetch.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 3ae52c015d..e5faa17ecd 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1790,8 +1790,16 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \tif (depth || deepen_since || deepen_not.nr)\n> \t\tdeepen = 1;\n> \n> -\tif (filter_options.choice && !has_promisor_remote())\n> -\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n> +\tif (filter_options.choice && !has_promisor_remote()) {\n> +\t\tchar repo_version_string[10];\n> +\n> +\t\txsnprintf(repo_version_string, sizeof(repo_version_string),\n> +\t\t\t \"%d\", (int)GIT_REPO_VERSION);\n> +\t\tgit_config_set(\"core.repositoryformatversion\",\n> +\t\t\trepo_version_string);\n> +\t\tgit_config_set(\"extensions.partialclone\", \"origin\");\n> +\t\tpromisor_remote_reinit();\n> +\t}\n> \n> \tif (all) {\n> \t\tif (argc == 1)\n"},{"id":"397776","messageId":"CAOhzdui-4KeOit74fTy=3YxSncHGZzy+cE+6njyjHPyJ8xFpMA@mail.gmail.com","threadId":"53463","inReplyTo":"xmqqo8qrsh0d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-13T21:41:00Z","receivedAt":"2020-05-13T21:41:16Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"The proposed change would allow converting an existing git clone to\nuse partial clones. For example:\n\n$ git clone --depth=1 https://android.googlesource.com/platform/bionic .\n$ git fetch --unshallow --filter=blob:none origin\n\nPreviously, to allow this one would have to do the following manually\nfirst; the existing code would handle the rest gracefully:\n\n$ git config core.repositoryFormatVersion 1\n$ git config extensions.partialClone origin\n\nAnd the proposed change would have git do it for the user\nautomatically, the existing workflow remains otherwise unchanged.\n"},{"id":"397787","messageId":"xmqqwo5fqyk6.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"CAOhzdui-4KeOit74fTy=3YxSncHGZzy+cE+6njyjHPyJ8xFpMA@mail.gmail.com","subject":"Re: [PATCH] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T22:07:05Z","receivedAt":"2020-05-13T22:07:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> The proposed change would allow converting an existing git clone to\n> use partial clones. For example:\n>\n> $ git clone --depth=1 https://android.googlesource.com/platform/bionic .\n> $ git fetch --unshallow --filter=blob:none origin\n>\n> Previously, to allow this one would have to do the following manually\n> first; the existing code would handle the rest gracefully:\n>\n> $ git config core.repositoryFormatVersion 1\n> $ git config extensions.partialClone origin\n>\n> And the proposed change would have git do it for the user\n> automatically, the existing workflow remains otherwise unchanged.\n\nPlease do not explain these things to the list readers. Instead,\nremember to write them in the proposed log message when you are\nready to send an updated patch, which you would probably want to\nwait before giving other people to comment on the patch.\n\nThanks.\n"},{"id":"397788","messageId":"xmqqsgg3qy1v.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"CAOhzdui-4KeOit74fTy=3YxSncHGZzy+cE+6njyjHPyJ8xFpMA@mail.gmail.com","subject":"Re: [PATCH] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-13T22:18:04Z","receivedAt":"2020-05-13T22:18:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> The proposed change would allow converting an existing git clone to\n> use partial clones. For example:\n>\n> $ git clone --depth=1 https://android.googlesource.com/platform/bionic .\n> $ git fetch --unshallow --filter=blob:none origin\n>\n> Previously, to allow this one would have to do the following manually\n> first; the existing code would handle the rest gracefully:\n>\n> $ git config core.repositoryFormatVersion 1\n> $ git config extensions.partialClone origin\n>\n> And the proposed change would have git do it for the user\n> automatically, the existing workflow remains otherwise unchanged.\n\nActually, do we even know, when we \"upgrade\" the repository with the\npatch, that the 'origin' supports the required protocol extensions\nlike on-demand fetching of objects?\n\nAt \"git clone\" time, \"git clone --filter=...\" against a server that\nis not prepared to serve as a lazy clone's backup remote would fail,\nso we will never write extensions.partialClone=origin when origin\ncannot serve as such. \n\nBut writing these config you suggest to \"do manually\" is already not\na safe thing to do, without making sure it is supported, no? It\ndoes not sound like a good idea to do an unsafe thing for the user\nautomatically.\n\nAdded a few folks on Cc: list, who know and care more about partial\nclones than I do for input.\n\nThanks.\n"},{"id":"397794","messageId":"20200513234435.GG6605@camp.crustytoothpaste.net","threadId":"53463","inReplyTo":"20200513200040.68968-1-delphij@google.com","subject":"Re: [PATCH] fetch: allow adding a filter after initial clone.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-13T23:44:35Z","receivedAt":"2020-05-13T23:44:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-13 at 20:00:40, Xin Li wrote:\n> Signed-off-by: Xin Li \n> ---\n> builtin/fetch.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 3ae52c015d..e5faa17ecd 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1790,8 +1790,16 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \tif (depth || deepen_since || deepen_not.nr)\n> \t\tdeepen = 1;\n> \n> -\tif (filter_options.choice && !has_promisor_remote())\n> -\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n> +\tif (filter_options.choice && !has_promisor_remote()) {\n> +\t\tchar repo_version_string[10];\n> +\n> +\t\txsnprintf(repo_version_string, sizeof(repo_version_string),\n> +\t\t\t \"%d\", (int)GIT_REPO_VERSION);\n> +\t\tgit_config_set(\"core.repositoryformatversion\",\n> +\t\t\trepo_version_string);\n> +\t\tgit_config_set(\"extensions.partialclone\", \"origin\");\n\nSome things stood out to me here. One, is this setting up the\nrepository if it's not already created? If so, we'd probably want to\nuse one of the appropriate functions in setup.c. Even if we're just\nchanging it, we should probably use a helper function.\n\nTwo, it isn't necessarily safe to automatically change the repository\nversion. Keys that start with \"extensions.\" are not special in format\nversion 0, but they are in format version 1. I can technically have an\n\"extensions.tomatoSalad\" in version 0 without any special meaning or\nnegative consequences, but as soon as we change to version 1, Git will\nrefuse to read it, since it doesn't know about the tomatoSalad extension\nand in v1 unknown extensions are fatal.\n\nMy example may sound silly, but since extensions can be set in the\nglobal config, users could well rely on v0 repositories ignoring them\nand having them automatically turned on for their v1 repositories. (I'm\nthinking of the future extensions.objectFormat as something somebody\nmight try to do here, as dangerous as it may be.)\n\nThese aren't insurmountable problems, but they are things we'd need to\ncheck for before just changing the repository version, so we'd want to\nstuff some code in setup.c to handle this case.\n\nThird, I'm not sure that \"origin\" is always the value we want to use\nhere. At a previous employer, the upstream remote was called\n\"upstream\", and your personal fork was called \"origin\", so I'd have\nwanted upstream here.. We'd probably want to use whatever remote the\nuser is already using in this case, and gracefully handle the URL case\nif that isn't allowed here (which it may be; I'm not that familiar with\npartial clone).\n\nI also agree with Junio's assessment that you'd probably want to explain\nmore about this feature in the commit message. For example, I'd want to\nknow what this patch does and have some idea of how I might invoke this\nfeature, why it's safe to change the repository version, how one sets\nthe remote for fetch, and pretty much answers to all the other things I\nasked here. Even if I understand these things now, that doesn't mean a\nfuture developer will in six months' time, and mentioning these things\nin the commit message helps leave a note to them that you considered (or\ndid not consider, as the case may be) certain issues and helps them\nunderstand the code as you saw and wrote it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"398721","messageId":"20200528025359.20931-1-delphij@google.com","threadId":"53463","inReplyTo":"20200513234435.GG6605@camp.crustytoothpaste.net","subject":"[PATCH v2 0/1] fetch: allow adding a filter after initial clone","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-28T02:53:59Z","receivedAt":"2020-05-28T02:56:48Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Previously, to retroactively add filter to an existing (shallow) clone\none would have to manually change the repository configuration to make\ngit to believe that there was an existing promisor, like:\n\n git config core.repositoryFormatVersion 1\n git config extensions.partialClone origin\n git fetch --unshallow --filter=blob:none origin\n\nBecause the code can already set up promisor, it would be safer and more\nconvenient to just do that in git itself.\n\nThis version of change will also prevent the code from making damaging\nrepository upgrades (when non-standard extensions exists) as pointed out\nby earlier reviewers.\n\nXin Li (1):\n fetch: allow adding a filter after initial clone.\n\n builtin/fetch.c | 3 ---\n list-objects-filter-options.c | 3 ++-\n repository.h | 6 ++++++\n setup.c | 30 ++++++++++++++++++++++++++++++\n t/t0410-partial-clone.sh | 21 +++++++++++++++++++++\n 5 files changed, 59 insertions(+), 4 deletions(-)\n\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398722","messageId":"20200528025359.20931-2-delphij@google.com","threadId":"53463","inReplyTo":"20200513234435.GG6605@camp.crustytoothpaste.net","subject":"[PATCH v2 1/1] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-28T02:54:00Z","receivedAt":"2020-05-28T02:57:06Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Retroactively adding filter can be useful for existing shallow clones as\nthey allow users to see earlier change histories without downloading all\ngit objects in a regular --unshallow fetch.\n\nPreviously this is possible by manually amending the repository\nconfiguration to make git think there is an existing promisor. Because\nthe code already does most of the hard work, it's safer for git to\njust perform the configuration change automatically instead.\n\nInstead of bailing out immediately when no promisor is available, make\nthe code check more specific issue (extension became special in\nrepository version 1, while it can have any value in version 0, so\nupgrade should not happen if the repository have an unsupported\nconfiguration that would render it invalid if we upgraded).\n\nSigned-off-by: Xin Li \n---\n builtin/fetch.c | 3 ---\n list-objects-filter-options.c | 3 ++-\n repository.h | 6 ++++++\n setup.c | 30 ++++++++++++++++++++++++++++++\n t/t0410-partial-clone.sh | 21 +++++++++++++++++++++\n 5 files changed, 59 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5788c16bf..3347d578ea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !has_promisor_remote())\n-\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 256bcfbdfe..6d62b60eac 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -326,7 +326,8 @@ void partial_clone_register(\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n-\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n+\t\tif (upgrade_repository_format(the_repository, 1) < 0)\n+\t\t\tdie(_(\"Unable to upgrade repository format to support partial clone\"));\n \n \t\t/* Add promisor config for the remote */\n \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\ndiff --git a/repository.h b/repository.h\nindex 6534fbb7b3..f301f6f456 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n \n void prepare_repo_settings(struct repository *r);\n \n+/*\n+ * Return 1 if upgrade repository format to target_version succeeded,\n+ * 0 if no upgrade is necessary.\n+ */\n+int upgrade_repository_format(struct repository *r, int target_version);\n+\n #endif /* REPOSITORY_H */\ndiff --git a/setup.c b/setup.c\nindex 65fe5ecefb..84da976e07 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -538,6 +538,36 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n+int upgrade_repository_format(struct repository *r, int target_version)\n+{\n+\tconst char *gitdir = get_git_dir();\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct strbuf repo_version = STRBUF_INIT;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\n+\tstrbuf_git_common_path(&sb, r, \"/config\");\n+\tread_repository_format(&repo_fmt, sb.buf);\n+\tstrbuf_release(&sb);\n+\n+\tif (repo_fmt.version >= target_version)\n+\t\treturn 0;\n+\n+\trepo_fmt.version = target_version;\n+\n+\tif (verify_repository_format(&repo_fmt, &err) < 0) {\n+\t\twarning(\"Unable to upgrade repository format to %d: %s\",\n+\t\t target_version, err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_addf(&repo_version, \"%d\", target_version);\n+\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n+\tstrbuf_release(&repo_version);\n+\treturn 1;\n+}\n+\n static void init_repository_format(struct repository_format *format)\n {\n \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..71270d3a53 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,27 @@ test_expect_success 'extensions.partialclone without filter' '\n \tgit -C client fetch origin\n '\n \n+test_expect_success 'convert shallow clone to partial clone' '\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+\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client true remote.origin.promisor &&\n+\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\n+\ttest_cmp_config -C client 1 core.repositoryformatversion\n+'\n+test_expect_success 'convert shallow clone to partial clone must fail with invalid 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+\tgit -C client config extensions.sandwidth true &&\n+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398723","messageId":"20200528032837.GE56118@google.com","threadId":"53463","inReplyTo":"20200528025359.20931-2-delphij@google.com","subject":"Re: [PATCH v2 1/1] fetch: allow adding a filter after initial clone.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-28T03:28:37Z","receivedAt":"2020-05-28T03:28:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nXin Li wrote:\n\n> Retroactively adding filter can be useful for existing shallow clones as\n> they allow users to see earlier change histories without downloading all\n> git objects in a regular --unshallow fetch.\n>\n> Previously this is possible by manually amending the repository\n> configuration to make git think there is an existing promisor. Because\n> the code already does most of the hard work, it's safer for git to\n> just perform the configuration change automatically instead.\n>\n> Instead of bailing out immediately when no promisor is available, make\n> the code check more specific issue (extension became special in\n> repository version 1, while it can have any value in version 0, so\n> upgrade should not happen if the repository have an unsupported\n> configuration that would render it invalid if we upgraded).\n>\n> Signed-off-by: Xin Li \n> ---\n\nnit: the cover letter contains\n\n> Previously, to retroactively add filter to an existing (shallow) clone\n> one would have to manually change the repository configuration to make\n> git to believe that there was an existing promisor, like:\n>\n> git config core.repositoryFormatVersion 1\n> git config extensions.partialClone origin\n> git fetch --unshallow --filter=blob:none origin\n>\n> Because the code can already set up promisor, it would be safer and more\n> convenient to just do that in git itself.\n>\n> This version of change will also prevent the code from making damaging\n> repository upgrades (when non-standard extensions exists) as pointed out\n> by earlier reviewers.\n\nI think that would make a good commit message itself.\n\n[...]\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \tif (depth || deepen_since || deepen_not.nr)\n> \t\tdeepen = 1;\n> \n> -\tif (filter_options.choice && !has_promisor_remote())\n> -\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n> -\n\nMakes sense.\n\n[...]\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -326,7 +326,8 @@ void partial_clone_register(\n> \n> \t/* Check if it is already registered */\n> \tif (!promisor_remote_find(remote)) {\n> -\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n> +\t\tif (upgrade_repository_format(the_repository, 1) < 0)\n> +\t\t\tdie(_(\"Unable to upgrade repository format to support partial clone\"));\n\nnit: Git's error messages tend to use lowercase (e.g., \"fatal: cannot [etc]\"\ninstead of \"fatal: Unable [etc]\").\n\n> \t\t/* Add promisor config for the remote */\n> \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\n\nnot about this patch: By the way, the repository format version bump\nis not sufficient to achieve its intended aim: we also need to set an\nextensions.* setting to ensure Git is new enough to know about partial\nclone. More discussion about this is in [1] (apologies for not having\nfinished solving that). This isn't a regression introduced in this\npatch, and this patch does the right thing in the context of the\ncurrent code.\n\n[...]\n> --- a/repository.h\n> +++ b/repository.h\n> @@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n> \n> void prepare_repo_settings(struct repository *r);\n> \n> +/*\n> + * Return 1 if upgrade repository format to target_version succeeded,\n> + * 0 if no upgrade is necessary.\n> + */\n\nProbably also worth mentioning that this returns -1 on error.\n\n> +int upgrade_repository_format(struct repository *r, int target_version);\n> +\n> #endif /* REPOSITORY_H */\n[...]\n> +++ b/setup.c\n> @@ -538,6 +538,36 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n> \treturn 0;\n> }\n> \n> +int upgrade_repository_format(struct repository *r, int target_version)\n> +{\n> +\tconst char *gitdir = get_git_dir();\n\nUnused variable.\n\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tstruct strbuf repo_version = STRBUF_INIT;\n> +\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n> +\n> +\tstrbuf_git_common_path(&sb, r, \"/config\");\n\nnit: can leave out the '/' to avoid a double-/.\n\n> +\tread_repository_format(&repo_fmt, sb.buf);\n> +\tstrbuf_release(&sb);\n> +\n> +\tif (repo_fmt.version >= target_version)\n> +\t\treturn 0;\n> +\n> +\trepo_fmt.version = target_version;\n> +\n> +\tif (verify_repository_format(&repo_fmt, &err) < 0) {\n> +\t\twarning(\"Unable to upgrade repository format to %d: %s\",\n\nSame nit about capitalization.\n\n> +\t\t target_version, err.buf);\n\nwhitespace nit: this would typically use a tab, to line up with the\nparen on the previous line.\n\n> +\t\tstrbuf_release(&err);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tstrbuf_addf(&repo_version, \"%d\", target_version);\n> +\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n\nAh, I think I misled you: the config_set API hasn't learned to take a\nstruct repository yet, so we should hardcode the_repository in this\nfunction instead of taking a \"struct repository\" parameter.\n\n> +\tstrbuf_release(&repo_version);\n> +\treturn 1;\n> +}\n> +\n> static void init_repository_format(struct repository_format *format)\n> {\n> \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index a3988bd4b8..71270d3a53 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -30,6 +30,27 @@ test_expect_success 'extensions.partialclone without filter' '\n> \tgit -C client fetch origin\n> '\n> \n> +test_expect_success 'convert shallow clone to partial clone' '\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> +\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n> +\ttest_cmp_config -C client true remote.origin.promisor &&\n> +\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\n> +\ttest_cmp_config -C client 1 core.repositoryformatversion\n> +'\n\nnit: Missing blank line.\n\nIs there a different check this test could perform to check the\nuser-facing behavior instead of how the configuration is encoded?\n\n> +test_expect_success 'convert shallow clone to partial clone must fail with invalid 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> +\tgit -C client config extensions.sandwidth true &&\n> +\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n> +'\n> +\n> test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n> \trm -rf repo &&\n> \ttest_create_repo repo &&\n\nWith whatever subset of the mentioned changes makes sense,\nReviewed-by: Jonathan Nieder \n\nThanks.\n\n[1] https://lore.kernel.org/git/20200312230931.GF120942@google.com/.\n\ndiff --git i/list-objects-filter-options.c w/list-objects-filter-options.c\nindex 6d62b60eaca..ce9193c3885 100644\n--- i/list-objects-filter-options.c\n+++ w/list-objects-filter-options.c\n@@ -326,8 +326,8 @@ void partial_clone_register(\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n-\t\tif (upgrade_repository_format(the_repository, 1) < 0)\n-\t\t\tdie(_(\"Unable to upgrade repository format to support partial clone\"));\n+\t\tif (upgrade_repository_format(1) < 0)\n+\t\t\tdie(_(\"cannot enable partial clone support\"));\n \n \t\t/* Add promisor config for the remote */\n \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\ndiff --git i/repository.h w/repository.h\nindex f301f6f4562..14574c6e627 100644\n--- i/repository.h\n+++ w/repository.h\n@@ -197,9 +197,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n void prepare_repo_settings(struct repository *r);\n \n /*\n- * Return 1 if upgrade repository format to target_version succeeded,\n- * 0 if no upgrade is necessary.\n+ * Upgrade the repository format to target_version.\n+ * Returns 1 on success, 0 if no upgrade was necessary, and -1 after\n+ * printing a diagnostic on error.\n */\n-int upgrade_repository_format(struct repository *r, int target_version);\n+int upgrade_repository_format(int target_version);\n \n #endif /* REPOSITORY_H */\ndiff --git i/setup.c w/setup.c\nindex 84da976e077..d1f0aff7d30 100644\n--- i/setup.c\n+++ w/setup.c\n@@ -538,15 +538,14 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n-int upgrade_repository_format(struct repository *r, int target_version)\n+int upgrade_repository_format(int target_version)\n {\n-\tconst char *gitdir = get_git_dir();\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct strbuf repo_version = STRBUF_INIT;\n \tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n \n-\tstrbuf_git_common_path(&sb, r, \"/config\");\n+\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n \tread_repository_format(&repo_fmt, sb.buf);\n \tstrbuf_release(&sb);\n \n@@ -556,8 +555,8 @@ int upgrade_repository_format(struct repository *r, int target_version)\n \trepo_fmt.version = target_version;\n \n \tif (verify_repository_format(&repo_fmt, &err) < 0) {\n-\t\twarning(\"Unable to upgrade repository format to %d: %s\",\n-\t\t target_version, err.buf);\n+\t\twarning(\"cannot upgrade repository format to %d: %s\",\n+\t\t\ttarget_version, err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn -1;\n \t}\ndiff --git i/t/t0410-partial-clone.sh w/t/t0410-partial-clone.sh\nindex 71270d3a539..d580488330f 100755\n--- i/t/t0410-partial-clone.sh\n+++ w/t/t0410-partial-clone.sh\n@@ -41,6 +41,7 @@ test_expect_success 'convert shallow clone to partial clone' '\n \ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\n \ttest_cmp_config -C client 1 core.repositoryformatversion\n '\n+\n test_expect_success 'convert shallow clone to partial clone must fail with invalid extension' '\n \trm -fr server client &&\n \ttest_create_repo server &&\n"},{"id":"398724","messageId":"20200528040812.131224-1-delphij@google.com","threadId":"53463","inReplyTo":"20200528032837.GE56118@google.com","subject":"[PATCH v3] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-28T04:08:12Z","receivedAt":"2020-05-28T04:08:26Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Retroactively adding filter can be useful for existing shallow clones as\nthey allow users to see earlier change histories without downloading all\ngit objects in a regular --unshallow fetch.\n\nWithout this patch, users can make a clone partial by editing the\nrepository configuration to convert the remote into a promisor, like:\n\n git config core.repositoryFormatVersion 1\n git config extensions.partialClone origin\n git fetch --unshallow --filter=blob:none origin\n\nSince the hard part of making this work is already in place and such\nedits can be error-prone, teach Git to perform the required configuration\nchange automatically instead.\n\nInstead of bailing out immediately when no promisor is available, make\nthe code perform a more precise check for any potential problems\n(extensions became special in repository version 1, while it can have\nany value in version 0, so upgrade should not happen if the repository\nhave an unsupported configuration that would render it invalid if we\nupgraded).\n\nSigned-off-by: Xin Li \nReviewed-by: Jonathan Nieder \n---\n builtin/fetch.c | 3 ---\n list-objects-filter-options.c | 3 ++-\n repository.h | 6 ++++++\n setup.c | 29 +++++++++++++++++++++++++++++\n t/t0410-partial-clone.sh | 22 ++++++++++++++++++++++\n 5 files changed, 59 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5788c16bf..3347d578ea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !has_promisor_remote())\n-\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 256bcfbdfe..3553ad7b0a 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -326,7 +326,8 @@ void partial_clone_register(\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n-\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n+\t\tif (upgrade_repository_format(1) < 0)\n+\t\t\tdie(_(\"unable to upgrade repository format to support partial clone\"));\n \n \t\t/* Add promisor config for the remote */\n \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\ndiff --git a/repository.h b/repository.h\nindex 6534fbb7b3..40cc12c7cf 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n \n void prepare_repo_settings(struct repository *r);\n \n+/*\n+ * Return 1 if upgrade repository format to target_version succeeded,\n+ * 0 if no upgrade is necessary; returns -1 when upgrade is not possible.\n+ */\n+int upgrade_repository_format(int target_version);\n+\n #endif /* REPOSITORY_H */\ndiff --git a/setup.c b/setup.c\nindex 65fe5ecefb..123d3d3433 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -538,6 +538,35 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n+int upgrade_repository_format(int target_version)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct strbuf repo_version = STRBUF_INIT;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\n+\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n+\tread_repository_format(&repo_fmt, sb.buf);\n+\tstrbuf_release(&sb);\n+\n+\tif (repo_fmt.version >= target_version)\n+\t\treturn 0;\n+\n+\trepo_fmt.version = target_version;\n+\n+\tif (verify_repository_format(&repo_fmt, &err) < 0) {\n+\t\twarning(\"unable to upgrade repository format to %d: %s\",\n+\t\t target_version, err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_addf(&repo_version, \"%d\", target_version);\n+\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n+\tstrbuf_release(&repo_version);\n+\treturn 1;\n+}\n+\n static void init_repository_format(struct repository_format *format)\n {\n \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..d580488330 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,28 @@ test_expect_success 'extensions.partialclone without filter' '\n \tgit -C client fetch origin\n '\n \n+test_expect_success 'convert shallow clone to partial clone' '\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+\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client true remote.origin.promisor &&\n+\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\n+\ttest_cmp_config -C client 1 core.repositoryformatversion\n+'\n+\n+test_expect_success 'convert shallow clone to partial clone must fail with invalid 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+\tgit -C client config extensions.sandwidth true &&\n+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398743","messageId":"xmqqsgfk3xtd.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200528025359.20931-2-delphij@google.com","subject":"Re: [PATCH v2 1/1] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T15:04:46Z","receivedAt":"2020-05-28T15:04:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> Instead of bailing out immediately when no promisor is available, make\n> the code check more specific issue (extension became special in\n> repository version 1, while it can have any value in version 0, so\n> upgrade should not happen if the repository have an unsupported\n> configuration that would render it invalid if we upgraded).\n\nIt probably has to be a lot stronger than that. In version 0,\nextension did not have any meaning, so an existing repository can\nsafely have \"[extension] worktreeConfig=~/hello\" as long as it is\nversion 0 and nobody would bitch about extension.worktreeConfig not\nbeing a boolean; worse yet, \"[extension] worktreeConfig=true\" in\nversion 0 repository did not make its secondary worktrees to have\nseparate configuration, but if we upgrade to version 1 merely\nbecause the version of Git knows what extension.worktreeConfig means,\nwe broke the repository and its worktrees.\n\nIt would be safe to upgrade version 0 repository when there is *no*\nexisting configuration variable in the \"extension\" section, but \"the\nrepository have an unsupported configuration\" is not a useful or\nsafe criteria to decide if we should refrain from upgrading, I would\nthink.\n\n"},{"id":"398753","messageId":"20200528171924.GC58643@google.com","threadId":"53463","inReplyTo":"xmqqsgfk3xtd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/1] fetch: allow adding a filter after initial clone.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-28T17:19:24Z","receivedAt":"2020-05-28T17:19:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> It would be safe to upgrade version 0 repository when there is *no*\n> existing configuration variable in the \"extension\" section,\n\nYes, that makes sense (also for the case Brian mentioned where someone\nmay try setting extension.objectFormat, see no effect, and then forget\nabout it).\n\nThanks,\nJonathan\n"},{"id":"398776","messageId":"CAOhzdugK7FQSnWruUrX8gQ98AF6kGvNWqPfHmE+qjGjBgzvmiQ@mail.gmail.com","threadId":"53463","inReplyTo":"20200528171924.GC58643@google.com","subject":"Re: [PATCH v2 1/1] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-28T19:12:19Z","receivedAt":"2020-05-28T19:12:38Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"On Thu, May 28, 2020 at 10:19 AM Jonathan Nieder wrote:\n>\n> Junio C Hamano wrote:\n>\n> > It would be safe to upgrade version 0 repository when there is *no*\n> > existing configuration variable in the \"extension\" section,\n>\n> Yes, that makes sense (also for the case Brian mentioned where someone\n> may try setting extension.objectFormat, see no effect, and then forget\n> about it).\n\nI see that's a good point\n\nI found that the current code doesn't really check\nrepositoryformatversion for e.g. partialclone, so without setting\nrepositoryformatversion to 1, just setting extensions.partialclone =\norigin will still make fetch --filter work and some test cases are\ndepending on this, do we want to keep this feature/bug? I have added\nsome additional checks to my local git version and found that we would\nalso need to fix a lot of test cases for worktree, etc. if we would\nenforce the check.\n"},{"id":"398778","messageId":"20200528191739.GA114915@google.com","threadId":"53463","inReplyTo":"CAOhzdugK7FQSnWruUrX8gQ98AF6kGvNWqPfHmE+qjGjBgzvmiQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] fetch: allow adding a filter after initial clone.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-28T19:17:39Z","receivedAt":"2020-05-28T19:17:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Xin Li wrote:\n\n> I found that the current code doesn't really check\n> repositoryformatversion for e.g. partialclone, so without setting\n> repositoryformatversion to 1, just setting extensions.partialclone =\n> origin will still make fetch --filter work and some test cases are\n> depending on this, do we want to keep this feature/bug?\n\nIt's orthogonal to this patch, but no, we don't.\n\nThanks,\nJonathan\n"},{"id":"398819","messageId":"20200529000432.146618-1-delphij@google.com","threadId":"53463","inReplyTo":"20200528191739.GA114915@google.com","subject":"[PATCH v4] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-29T00:04:32Z","receivedAt":"2020-05-29T00:04:58Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"\nRetroactively adding filter can be useful for existing shallow clones as\nthey allow users to see earlier change histories without downloading all\ngit objects in a regular --unshallow fetch.\n\nWithout this patch, users can make a clone partial by editing the\nrepository configuration to convert the remote into a promisor, like:\n\n git config core.repositoryFormatVersion 1\n git config extensions.partialClone origin\n git fetch --unshallow --filter=blob:none origin\n\nSince the hard part of making this work is already in place and such\nedits can be error-prone, teach Git to perform the required configuration\nchange automatically instead.\n\nInstead of bailing out immediately when no promisor is available, make\nthe code perform a more precise check for any potential problems\n(extensions became special in repository version 1, while it can have\nany value in version 0, so upgrade should not happen if the repository\nhave an unsupported configuration that would render it invalid if we\nupgraded).\n\nSigned-off-by: Xin Li \nReviewed-by: Jonathan Nieder \n---\n builtin/fetch.c | 3 --\n builtin/sparse-checkout.c | 2 +\n cache.h | 2 +\n list-objects-filter-options.c | 3 +-\n repository.h | 6 +++\n setup.c | 77 ++++++++++++++++++++++++++++++--\n t/t0410-partial-clone.sh | 23 ++++++++++\n t/t1090-sparse-checkout-scope.sh | 1 -\n t/t2404-worktree-config.sh | 4 +-\n t/t5500-fetch-pack.sh | 1 -\n t/t5702-protocol-v2.sh | 1 -\n 11 files changed, 111 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5788c16bf..3347d578ea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !has_promisor_remote())\n-\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 95d0882417..95669815d4 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -249,6 +249,8 @@ static int set_config(enum sparse_checkout_mode mode)\n {\n \tconst char *config_path;\n \n+\tif (upgrade_repository_format(1) < 0)\n+\t\tdie(_(\"unable to upgrade repository format to enable worktreeConfig\"));\n \tif (git_config_set_gently(\"extensions.worktreeConfig\", \"true\")) {\n \t\terror(_(\"failed to set extensions.worktreeConfig setting\"));\n \t\treturn 1;\ndiff --git a/cache.h b/cache.h\nindex 0f0485ecfe..66dcd2f219 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,6 +1042,7 @@ 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 };\n@@ -1056,6 +1057,7 @@ struct repository_format {\n \t.version = -1, \\\n \t.is_bare = -1, \\\n \t.hash_algo = GIT_HASH_SHA1, \\\n+\t.has_extensions = 0, \\\n \t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n }\n \ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 256bcfbdfe..3553ad7b0a 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -326,7 +326,8 @@ void partial_clone_register(\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n-\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n+\t\tif (upgrade_repository_format(1) < 0)\n+\t\t\tdie(_(\"unable to upgrade repository format to support partial clone\"));\n \n \t\t/* Add promisor config for the remote */\n \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\ndiff --git a/repository.h b/repository.h\nindex 6534fbb7b3..40cc12c7cf 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n \n void prepare_repo_settings(struct repository *r);\n \n+/*\n+ * Return 1 if upgrade repository format to target_version succeeded,\n+ * 0 if no upgrade is necessary; returns -1 when upgrade is not possible.\n+ */\n+int upgrade_repository_format(int target_version);\n+\n #endif /* REPOSITORY_H */\ndiff --git a/setup.c b/setup.c\nindex 65fe5ecefb..0759e9f8f9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -13,6 +13,9 @@ static int work_tree_config_is_bogus;\n static struct startup_info the_startup_info;\n struct startup_info *startup_info = &the_startup_info;\n \n+static int verify_repository_format_eligibility(const struct repository_format *,\n+ struct strbuf *, int);\n+\n /*\n * The input parameter must contain an absolute path, and it must already be\n * normalized.\n@@ -455,6 +458,7 @@ 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@@ -506,9 +510,15 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\tdie(\"%s\", err.buf);\n \t}\n \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+\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 \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n \tif (repository_format_worktree_config) {\n@@ -538,6 +548,34 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n+int upgrade_repository_format(int target_version)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct strbuf repo_version = STRBUF_INIT;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\n+\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n+\tread_repository_format(&repo_fmt, sb.buf);\n+\tstrbuf_release(&sb);\n+\n+\tif (repo_fmt.version >= target_version)\n+\t\treturn 0;\n+\n+\tif (verify_repository_format_eligibility(&repo_fmt, &err,\n+\t target_version) < 0) {\n+\t\twarning(\"unable to 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+\n+\tstrbuf_addf(&repo_version, \"%d\", target_version);\n+\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n+\tstrbuf_release(&repo_version);\n+\treturn 1;\n+}\n+\n static void init_repository_format(struct repository_format *format)\n {\n \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\n@@ -562,7 +600,7 @@ void clear_repository_format(struct repository_format *format)\n \tinit_repository_format(format);\n }\n \n-int verify_repository_format(const struct repository_format *format,\n+static int verify_repository_format_version(const struct repository_format *format,\n \t\t\t struct strbuf *err)\n {\n \tif (GIT_REPO_VERSION_READ < format->version) {\n@@ -571,6 +609,18 @@ int verify_repository_format(const struct repository_format *format,\n \t\treturn -1;\n \t}\n \n+\treturn 0;\n+}\n+\n+int verify_repository_format(const struct repository_format *format,\n+\t\t\t struct strbuf *err)\n+{\n+\tint result;\n+\n+\tresult = verify_repository_format_version(format, err);\n+\tif (result != 0)\n+\t\treturn (result);\n+\n \tif (format->version >= 1 && format->unknown_extensions.nr) {\n \t\tint i;\n \n@@ -585,6 +635,25 @@ int verify_repository_format(const struct repository_format *format,\n \treturn 0;\n }\n \n+static int verify_repository_format_eligibility(const struct repository_format\n+ *format, struct strbuf *err, int target_version)\n+{\n+\tint result;\n+\n+\tresult = verify_repository_format_version(format, err);\n+\tif (result != 0)\n+\t\treturn (result);\n+\n+\tif (format->version <= 0 && format->has_extensions &&\n+\t target_version >= 1) {\n+\t\tstrbuf_addf(err, _(\"extensions found for repository version %d\"),\n+\t\t\t format->version);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n void read_gitfile_error_die(int error_code, const char *path, const char *dir)\n {\n \tswitch (error_code) {\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..463dc3a8be 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,29 @@ test_expect_success 'extensions.partialclone without filter' '\n \tgit -C client fetch origin\n '\n \n+test_expect_success 'convert shallow clone to partial clone' '\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+\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client true remote.origin.promisor &&\n+\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\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+\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+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 40cc004326..f35a73dd20 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -63,7 +63,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n \tgit -C server commit -m message &&\n \n \ttest_config -C client core.sparsecheckout 1 &&\n-\ttest_config -C client extensions.partialclone origin &&\n \techo \"!/*\" >client/.git/info/sparse-checkout &&\n \techo \"/a\" >>client/.git/info/sparse-checkout &&\n \tgit -C client fetch --filter=blob:none origin &&\ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 286121d8de..9536d10919 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n '\n \n test_expect_success 'enable worktreeConfig extension' '\n+\tgit config core.repositoryformatversion 1 &&\n \tgit config extensions.worktreeConfig true &&\n-\ttest_cmp_config true extensions.worktreeConfig\n+\ttest_cmp_config true extensions.worktreeConfig &&\n+\ttest_cmp_config 1 core.repositoryformatversion\n '\n \n test_expect_success 'config is shared as before' '\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8c54e34ef1..0f5ff25179 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -999,7 +999,6 @@ fetch_filter_blob_limit_zero () {\n \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n \n \tgit clone \"$URL\" client &&\n-\ttest_config -C client extensions.partialclone origin &&\n \n \ttest_commit -C \"$SERVER\" two &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..8b27fad6cd 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -348,7 +348,6 @@ test_expect_success 'partial fetch' '\n \trm -rf client \"$(pwd)/trace\" &&\n \tgit init client &&\n \tSERVER=\"file://$(pwd)/server\" &&\n-\ttest_config -C client extensions.partialClone \"$SERVER\" &&\n \n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n \t\tfetch --filter=blob:none \"$SERVER\" master:refs/heads/other &&\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398822","messageId":"xmqqblm71sk1.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200529000432.146618-1-delphij@google.com","subject":"Re: [PATCH v4] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T00:41:18Z","receivedAt":"2020-05-29T00:41:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> Retroactively adding filter can be useful for existing shallow clones as\n> they allow users to see earlier change histories without downloading all\n> git objects in a regular --unshallow fetch.\n>\n> Without this patch, users can make a clone partial by editing the\n> repository configuration to convert the remote into a promisor, like:\n>\n> git config core.repositoryFormatVersion 1\n> git config extensions.partialClone origin\n> git fetch --unshallow --filter=blob:none origin\n>\n> Since the hard part of making this work is already in place and such\n> edits can be error-prone, teach Git to perform the required configuration\n> change automatically instead.\n>\n> Instead of bailing out immediately when no promisor is available, make\n> the code perform a more precise check for any potential problems\n> (extensions became special in repository version 1, while it can have\n> any value in version 0, so upgrade should not happen if the repository\n> have an unsupported configuration that would render it invalid if we\n> upgraded).\n\nUpgrade from v0 to v1 must follow the more strict \"no extension\" rule,\nnot \"no unknown ones\" rule, so the above description must be corrected.\nPerhaps like this?\n\n\t... so upgrade from version 0 should not happen if the\n\trepository has ANY extension. A repository version 1 and\n\tlater make Git fail if there is any unknown extension, so we\n\tneed to fail an upgrade only if there is any extension that\n\tis unknown to us).\n\nYou can drop the second paragraph about upgrading from version 1 to\na later version if you want, as the only interesting use cases in\npractice at this point are upgrading from v0 to v1 and staying at v1.\n\n> Signed-off-by: Xin Li \n> Reviewed-by: Jonathan Nieder \n\nI think the updated design looks good. Let's nitpick some styles ;-)\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b5788c16bf..3347d578ea 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \tif (depth || deepen_since || deepen_not.nr)\n> \t\tdeepen = 1;\n> \n> -\tif (filter_options.choice && !has_promisor_remote())\n> -\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n> -\n> \tif (all) {\n> \t\tif (argc == 1)\n> \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 95d0882417..95669815d4 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -249,6 +249,8 @@ static int set_config(enum sparse_checkout_mode mode)\n> {\n> \tconst char *config_path;\n> \n> +\tif (upgrade_repository_format(1) < 0)\n> +\t\tdie(_(\"unable to upgrade repository format to enable worktreeConfig\"));\n> \tif (git_config_set_gently(\"extensions.worktreeConfig\", \"true\")) {\n> \t\terror(_(\"failed to set extensions.worktreeConfig setting\"));\n> \t\treturn 1;\n\nOK.\n\n\n> diff --git a/cache.h b/cache.h\n> index 0f0485ecfe..66dcd2f219 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1042,6 +1042,7 @@ 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> };\n> @@ -1056,6 +1057,7 @@ struct repository_format {\n> \t.version = -1, \\\n> \t.is_bare = -1, \\\n> \t.hash_algo = GIT_HASH_SHA1, \\\n> +\t.has_extensions = 0, \\\n\nI am on the fence between \"explicitly initializing to zero value is\npointless, especially when we use .designated_initializer\" and\n\"especially with .designated_initializer, it adds a documentation\nvalue to explicitly initialize a field to its zero value\". Unless\nother reviewers weigh in, I am OK to let this stand as-is. \n\n> \t.unknown_extensions = STRING_LIST_INIT_DUP, \\\n> }\n> \n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index 256bcfbdfe..3553ad7b0a 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -326,7 +326,8 @@ void partial_clone_register(\n> \n> \t/* Check if it is already registered */\n> \tif (!promisor_remote_find(remote)) {\n> -\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n> +\t\tif (upgrade_repository_format(1) < 0)\n> +\t\t\tdie(_(\"unable to upgrade repository format to support partial clone\"));\n\nOK.\n\n> diff --git a/repository.h b/repository.h\n> index 6534fbb7b3..40cc12c7cf 100644\n> --- a/repository.h\n> +++ b/repository.h\n> @@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n> \n> void prepare_repo_settings(struct repository *r);\n> \n> +/*\n> + * Return 1 if upgrade repository format to target_version succeeded,\n> + * 0 if no upgrade is necessary; returns -1 when upgrade is not possible.\n> + */\n\nDo we want to start with \"Return\" but say \"returns\" later? \n\n\tReturn 1 if ..., 0 if ..., and -1 when upgrade is not possible.\n\n> +int upgrade_repository_format(int target_version);\n> +\n> #endif /* REPOSITORY_H */\n\n> +int upgrade_repository_format(int target_version)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tstruct strbuf repo_version = STRBUF_INIT;\n> +\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n> +\n> +\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n> +\tread_repository_format(&repo_fmt, sb.buf);\n> +\tstrbuf_release(&sb);\n> +\n> +\tif (repo_fmt.version >= target_version)\n> +\t\treturn 0;\n\nOK. That's \"already up-to-date\" case.\n\n> +\tif (verify_repository_format_eligibility(&repo_fmt, &err,\n> +\t target_version) < 0) {\n\nI do not think this _eligibility thing should be a separate helper\nfunction. One reason is that its name sounds nonsensical (\"eligible\nfor what? it deserives to be verified for its repo format?\"), another\nis it makes it unclear what \"upgrade\" requires by hiding the logic\ninside that decides the eligibility for upgrading. Besides, there\nis only one callsite.\n\nOpen-coding the gist of the helper like this:\n\n\tif (verify_repository_format(&repo_fmt, &err) < 0 ||\n\t (!repo_fmt.version && repo_fmt.has_extensions)) {\n\nshould make it a lot clearer to see. If the repository is unusable\nby the version of Git we are running already, or the repository is\nv0 and has configuration variable(s) in \"extensions.*\" section, we\nrefuse to upgrade.\n\nWhich is slightly different from what you did with the three-way\nsplit of verify_repository_format(), which made the \"eligibility\"\nthing not to care about unknown extensions in a repository v1 and\nhigher. I actually think we should refuse to update v1 or v2\nrepository to v3 with a running Git that knows only about v1\n(i.e. the repository before upgrading may or may not be something we\nunderstand, and if we do not understand it, we shouldn't touch it).\n\n> +\t\twarning(\"unable to 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> + ...\n\nAnd with the suggested change to eliminate \"eligibility\" helper,\nnone of the changes below would become necessary, I would think,\nso I won't say things like \"we do not say 'if (result != 0)';\ninstead we just say 'if (result)'\" ;-)\n\nThanks.\n"},{"id":"398823","messageId":"20200529010119.GE114915@google.com","threadId":"53463","inReplyTo":"20200529000432.146618-1-delphij@google.com","subject":"Re: [PATCH v4] fetch: allow adding a filter after initial clone.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-29T01:01:19Z","receivedAt":"2020-05-29T01:01:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Jonathan Tan, partial clone expert)\nXin Li wrote:\n\n> Subject: fetch: allow adding a filter after initial clone.\n\nnit: should be no period at the end of the subject line\n\n> Signed-off-by: Xin Li \n> Reviewed-by: Jonathan Nieder \n\nLet's see whether I still stand by that. ;-)\n\n[...]\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -13,6 +13,9 @@ static int work_tree_config_is_bogus;\n> static struct startup_info the_startup_info;\n> struct startup_info *startup_info = &the_startup_info;\n> \n> +static int verify_repository_format_eligibility(const struct repository_format *,\n> + struct strbuf *, int);\n\nOdd wrapping, but after Junio's suggestion this goes away.\n\n[...]\n> +\t\twarning(\"unable to upgrade repository format from %d to %d: %s\",\n> +\t\t repo_fmt.version, target_version, err.buf);\n\nSame wrapping nit: this should use a tab to line up right after the\nparenthesis (Git uses 8-space tabs).\n\n[...]\n> --- a/t/t1090-sparse-checkout-scope.sh\n> +++ b/t/t1090-sparse-checkout-scope.sh\n> @@ -63,7 +63,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n> \tgit -C server commit -m message &&\n> \n> \ttest_config -C client core.sparsecheckout 1 &&\n> -\ttest_config -C client extensions.partialclone origin &&\n\nNice.\n\n> \techo \"!/*\" >client/.git/info/sparse-checkout &&\n> \techo \"/a\" >>client/.git/info/sparse-checkout &&\n> \tgit -C client fetch --filter=blob:none origin &&\n> diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\n> index 286121d8de..9536d10919 100755\n> --- a/t/t2404-worktree-config.sh\n> +++ b/t/t2404-worktree-config.sh\n> @@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n> '\n> \n> test_expect_success 'enable worktreeConfig extension' '\n> +\tgit config core.repositoryformatversion 1 &&\n> \tgit config extensions.worktreeConfig true &&\n\nYes, makes sense. Does this patch need it, or could this go in a\nseparate patch?\n\n> -\ttest_cmp_config true extensions.worktreeConfig\n> +\ttest_cmp_config true extensions.worktreeConfig &&\n> +\ttest_cmp_config 1 core.repositoryformatversion\n\nThis (both the existing code and the modified version) is strange: we\njust set the config, so why are we checking it?\n\n[...]\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -999,7 +999,6 @@ fetch_filter_blob_limit_zero () {\n> \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n> \n> \tgit clone \"$URL\" client &&\n> -\ttest_config -C client extensions.partialclone origin &&\n> \n\nNice.\n\n[...]\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -348,7 +348,6 @@ test_expect_success 'partial fetch' '\n> \trm -rf client \"$(pwd)/trace\" &&\n> \tgit init client &&\n> \tSERVER=\"file://$(pwd)/server\" &&\n> -\ttest_config -C client extensions.partialClone \"$SERVER\" &&\n> \n> \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> \t\tfetch --filter=blob:none \"$SERVER\" master:refs/heads/other &&\n\nThis test is a bit unusual, since it's fetching by URL instead of from\na remote. Looks like it comes from\n\n\tcommit ba95710a3bdcb2a80495b1d93a0e482dd69905e1\n\tAuthor: Jonathan Tan \n\tDate: Thu May 3 16:46:56 2018 -0700\n\n\t {fetch,upload}-pack: support filter in protocol v2\n\nJonathan, thoughts about this one? Is making it set\nextensions.partialClone implicitly like the change above a good\nchange?\n\nWith the nits above and the bits Junio described addressed, this would\nindeed be\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"398837","messageId":"20200529064416.37565-1-delphij@google.com","threadId":"53463","inReplyTo":"20200529010119.GE114915@google.com","subject":"[PATCH v5] fetch: allow adding a filter after initial clone","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-29T06:44:16Z","receivedAt":"2020-05-29T06:55:13Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Retroactively adding filter can be useful for existing shallow clones as\nthey allow users to see earlier change histories without downloading all\ngit objects in a regular --unshallow fetch.\n\nWithout this patch, users can make a clone partial by editing the\nrepository configuration to convert the remote into a promisor, like:\n\n git config core.repositoryFormatVersion 1\n git config extensions.partialClone origin\n git fetch --unshallow --filter=blob:none origin\n\nSince the hard part of making this work is already in place and such\nedits can be error-prone, teach Git to perform the required configuration\nchange automatically instead.\n\nInstead of bailing out immediately when no promisor is available, make\nthe code perform a more precise check for any potential problems\n(extensions became special in repository version 1, while it can have\nany value in version 0, so upgrade from version 0 should not happen if\nthe repository has ANY extension).\n\nSigned-off-by: Xin Li \nReviewed-by: Jonathan Nieder \n---\n builtin/fetch.c | 3 ---\n builtin/sparse-checkout.c | 2 ++\n cache.h | 1 +\n list-objects-filter-options.c | 3 ++-\n repository.h | 6 +++++\n setup.c | 41 +++++++++++++++++++++++++++++---\n t/t0410-partial-clone.sh | 23 ++++++++++++++++++\n t/t1090-sparse-checkout-scope.sh | 1 -\n t/t2404-worktree-config.sh | 4 +++-\n t/t5500-fetch-pack.sh | 1 -\n t/t5702-protocol-v2.sh | 1 -\n 11 files changed, 75 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5788c16bf..3347d578ea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !has_promisor_remote())\n-\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 95d0882417..95669815d4 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -249,6 +249,8 @@ static int set_config(enum sparse_checkout_mode mode)\n {\n \tconst char *config_path;\n \n+\tif (upgrade_repository_format(1) < 0)\n+\t\tdie(_(\"unable to upgrade repository format to enable worktreeConfig\"));\n \tif (git_config_set_gently(\"extensions.worktreeConfig\", \"true\")) {\n \t\terror(_(\"failed to set extensions.worktreeConfig setting\"));\n \t\treturn 1;\ndiff --git a/cache.h b/cache.h\nindex 0f0485ecfe..e5885cc9ea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,6 +1042,7 @@ 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/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 256bcfbdfe..3553ad7b0a 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -326,7 +326,8 @@ void partial_clone_register(\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n-\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n+\t\tif (upgrade_repository_format(1) < 0)\n+\t\t\tdie(_(\"unable to upgrade repository format to support partial clone\"));\n \n \t\t/* Add promisor config for the remote */\n \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\ndiff --git a/repository.h b/repository.h\nindex 6534fbb7b3..3c1f7d54bd 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n \n void prepare_repo_settings(struct repository *r);\n \n+/*\n+ * Return 1 if upgrade repository format to target_version succeeded,\n+ * 0 if no upgrade is necessary, and -1 when upgrade is not possible.\n+ */\n+int upgrade_repository_format(int target_version);\n+\n #endif /* REPOSITORY_H */\ndiff --git a/setup.c b/setup.c\nindex 65fe5ecefb..73bfa98431 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,6 +455,7 @@ 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@@ -506,9 +507,15 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\tdie(\"%s\", err.buf);\n \t}\n \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+\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 \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n \tif (repository_format_worktree_config) {\n@@ -538,6 +545,34 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n+int upgrade_repository_format(int target_version)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct strbuf repo_version = STRBUF_INIT;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\n+\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n+\tread_repository_format(&repo_fmt, sb.buf);\n+\tstrbuf_release(&sb);\n+\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 repo_fmt.version, target_version, err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_addf(&repo_version, \"%d\", target_version);\n+\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n+\tstrbuf_release(&repo_version);\n+\treturn 1;\n+}\n+\n static void init_repository_format(struct repository_format *format)\n {\n \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..463dc3a8be 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,29 @@ test_expect_success 'extensions.partialclone without filter' '\n \tgit -C client fetch origin\n '\n \n+test_expect_success 'convert shallow clone to partial clone' '\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+\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client true remote.origin.promisor &&\n+\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\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+\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+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 40cc004326..f35a73dd20 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -63,7 +63,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n \tgit -C server commit -m message &&\n \n \ttest_config -C client core.sparsecheckout 1 &&\n-\ttest_config -C client extensions.partialclone origin &&\n \techo \"!/*\" >client/.git/info/sparse-checkout &&\n \techo \"/a\" >>client/.git/info/sparse-checkout &&\n \tgit -C client fetch --filter=blob:none origin &&\ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 286121d8de..9536d10919 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n '\n \n test_expect_success 'enable worktreeConfig extension' '\n+\tgit config core.repositoryformatversion 1 &&\n \tgit config extensions.worktreeConfig true &&\n-\ttest_cmp_config true extensions.worktreeConfig\n+\ttest_cmp_config true extensions.worktreeConfig &&\n+\ttest_cmp_config 1 core.repositoryformatversion\n '\n \n test_expect_success 'config is shared as before' '\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8c54e34ef1..0f5ff25179 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -999,7 +999,6 @@ fetch_filter_blob_limit_zero () {\n \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n \n \tgit clone \"$URL\" client &&\n-\ttest_config -C client extensions.partialclone origin &&\n \n \ttest_commit -C \"$SERVER\" two &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..8b27fad6cd 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -348,7 +348,6 @@ test_expect_success 'partial fetch' '\n \trm -rf client \"$(pwd)/trace\" &&\n \tgit init client &&\n \tSERVER=\"file://$(pwd)/server\" &&\n-\ttest_config -C client extensions.partialClone \"$SERVER\" &&\n \n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n \t\tfetch --filter=blob:none \"$SERVER\" master:refs/heads/other &&\n-- \n2.27.0.rc0.183.gde8f92d652-goog\n\n"},{"id":"398838","messageId":"CAOhzduh4V4LKgzksTkSdEDrgsv59jTpfYDBTUzZvRxZOaqhhmg@mail.gmail.com","threadId":"53463","inReplyTo":"20200529010119.GE114915@google.com","subject":"Re: [PATCH v4] fetch: allow adding a filter after initial clone.","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-05-29T06:54:59Z","receivedAt":"2020-05-29T06:55:16Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Regarding t/t2404-worktree-config.sh:\n\nOn Thu, May 28, 2020 at 6:01 PM Jonathan Nieder wrote:\n> Xin Li wrote:\n>\n> > diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\n> > index 286121d8de..9536d10919 100755\n> > --- a/t/t2404-worktree-config.sh\n> > +++ b/t/t2404-worktree-config.sh\n> > @@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n> > '\n> >\n> > test_expect_success 'enable worktreeConfig extension' '\n> > + git config core.repositoryformatversion 1 &&\n> > git config extensions.worktreeConfig true &&\n>\n> Yes, makes sense. Does this patch need it, or could this go in a\n> separate patch?\n\nYes, this patch needs setting repositoryformatversion to 1 as we would\nno longer recognize extensions.worktreeConfig=true on version 0\nrepositories.\n\n> > - test_cmp_config true extensions.worktreeConfig\n> > + test_cmp_config true extensions.worktreeConfig &&\n> > + test_cmp_config 1 core.repositoryformatversion\n>\n> This (both the existing code and the modified version) is strange: we\n> just set the config, so why are we checking it?\n\nThe check was mainly to match the existing pattern (which sets\nextensions.worktreeConfig and immediately asserts that they were set).\nThese assertions are not strictly necessary but are harmless, so I\ndon't feel strongly about keeping or removing them.\n"},{"id":"398895","messageId":"xmqqo8q6zkmr.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"xmqqblm71sk1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T18:00:44Z","receivedAt":"2020-05-29T18:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Which is slightly different from what you did with the three-way\n> split of verify_repository_format(), which made the \"eligibility\"\n> thing not to care about unknown extensions in a repository v1 and\n> higher. I actually think we should refuse to update v1 or v2\n> repository to v3 with a running Git that knows only about v1\n> (i.e. the repository before upgrading may or may not be something we\n> understand, and if we do not understand it, we shouldn't touch it).\n\nIt does not change the conclusion, but I think the above sample\nsituation would not make much sense---a caller that asks this\nfunction to upgrade the repository to v3 when the version of Git it\nis linked in does not understand v3 is simply buggy.\n\nBut we should still refuse to update v1 to v2 with a version of Git\nthat understands v2 if the repository has some extension that we do\nnot know about, so \n\n (1) if upgrading from v0, there must be no \"extensions.*\"; and\n\n (2) if upgrading from other versions, there must be no\n \"extensions.*\" we do not recognise.\n\nI suggested would still be the reasonably defensive rule.\n\nThanks.\n\n\n"},{"id":"398896","messageId":"xmqqk10uzkcd.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200529010119.GE114915@google.com","subject":"Re: [PATCH v4] fetch: allow adding a filter after initial clone.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T18:06:58Z","receivedAt":"2020-05-29T18:07:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\n>> index 286121d8de..9536d10919 100755\n>> --- a/t/t2404-worktree-config.sh\n>> +++ b/t/t2404-worktree-config.sh\n>> @@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n>> '\n>> \n>> test_expect_success 'enable worktreeConfig extension' '\n>> +\tgit config core.repositoryformatversion 1 &&\n>> \tgit config extensions.worktreeConfig true &&\n>\n> Yes, makes sense. Does this patch need it, or could this go in a\n> separate patch?\n\nI think it is the consequence of unrelated change in this hunk:\n\n@@ -506,9 +510,15 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\tdie(\"%s\", err.buf);\n \t}\n \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+\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 \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n \tif (repository_format_worktree_config) {\n\nWe used to honor extensions.*, as long as the version of Git\nunderstand it, even in a repository whose version is still v0.\n\nI am not sure if this backward incompatible change is necessary for\nthe purpose of \"allow safe upgrade of repository format version\"\ntopic, but as you hinted, it does smell like it belongs to a\nseparate (and much larger and potentially controversial) patch.\n\nThanks for carefully reading.\n\n"},{"id":"399166","messageId":"20200605091004.208668-1-delphij@google.com","threadId":"53463","inReplyTo":"xmqqk10uzkcd.fsf@gitster.c.googlers.com","subject":"[PATCH v6 0/4] fetch: allow adding a filter after initial clone","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-06-05T09:10:00Z","receivedAt":"2020-06-05T09:10:36Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"This series of patch makes fetch to allow adding a filter after initial\nclone, useful for existing shallow clones as they allow users to see\nearlier change histories without downloading all git objects in a regular\n--unshallow fetch.\n\nThis version of patchset is mostly identical to the v5 draft but splitted\ninto smaller individual patches.\n\nPlease note that the last patch is an incompatible change that would\nmake extensions stop working for version 0 repositories. Currently,\nunknown extensions are silently ignored for version 0, which means the\nuser may see undesirable result when upgraded to a new Git version.\n\nXin Li (4):\n repository: add a helper function to perform repository format upgrade\n fetch: allow adding a filter after initial clone\n sparse-checkout: upgrade repository to version 1 when enabling\n extension\n check_repository_format_gently(): refuse extensions for old\n repositories\n\n builtin/fetch.c | 3 ---\n builtin/sparse-checkout.c | 2 ++\n cache.h | 1 +\n list-objects-filter-options.c | 3 ++-\n repository.h | 6 +++++\n setup.c | 41 +++++++++++++++++++++++++++++---\n t/t0410-partial-clone.sh | 23 ++++++++++++++++++\n t/t1090-sparse-checkout-scope.sh | 1 -\n t/t2404-worktree-config.sh | 4 +++-\n t/t5500-fetch-pack.sh | 1 -\n t/t5702-protocol-v2.sh | 1 -\n 11 files changed, 75 insertions(+), 11 deletions(-)\n\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399167","messageId":"20200605091004.208668-2-delphij@google.com","threadId":"53463","inReplyTo":"20200605091004.208668-1-delphij@google.com","subject":"[PATCH v6 1/4] repository: add a helper function to perform repository format upgrade","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-06-05T09:10:01Z","receivedAt":"2020-06-05T09:10:41Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"In version 1 of repository format, \"extensions\" gained special meaning\nand it is safer to avoid upgrading when there are pre-existing\nextensions.\n\nMake list-objects-filter to use the helper function instead of setting\nrepository version directly as a prerequisite of exposing the upgrade\ncapability.\n\nSigned-off-by: Xin Li \n---\n cache.h | 1 +\n list-objects-filter-options.c | 3 ++-\n repository.h | 6 ++++++\n setup.c | 29 +++++++++++++++++++++++++++++\n 4 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0f0485ecfe..e5885cc9ea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1042,6 +1042,7 @@ 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/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 256bcfbdfe..3553ad7b0a 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -326,7 +326,8 @@ void partial_clone_register(\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n-\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n+\t\tif (upgrade_repository_format(1) < 0)\n+\t\t\tdie(_(\"unable to upgrade repository format to support partial clone\"));\n \n \t\t/* Add promisor config for the remote */\n \t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\ndiff --git a/repository.h b/repository.h\nindex 6534fbb7b3..3c1f7d54bd 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -196,4 +196,10 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);\n \n void prepare_repo_settings(struct repository *r);\n \n+/*\n+ * Return 1 if upgrade repository format to target_version succeeded,\n+ * 0 if no upgrade is necessary, and -1 when upgrade is not possible.\n+ */\n+int upgrade_repository_format(int target_version);\n+\n #endif /* REPOSITORY_H */\ndiff --git a/setup.c b/setup.c\nindex 65fe5ecefb..597b41b822 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -455,6 +455,7 @@ 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@@ -538,6 +539,34 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \treturn 0;\n }\n \n+int upgrade_repository_format(int target_version)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tstruct strbuf repo_version = STRBUF_INIT;\n+\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\n+\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n+\tread_repository_format(&repo_fmt, sb.buf);\n+\tstrbuf_release(&sb);\n+\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+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\n+\tstrbuf_addf(&repo_version, \"%d\", target_version);\n+\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n+\tstrbuf_release(&repo_version);\n+\treturn 1;\n+}\n+\n static void init_repository_format(struct repository_format *format)\n {\n \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399168","messageId":"20200605091004.208668-3-delphij@google.com","threadId":"53463","inReplyTo":"20200605091004.208668-1-delphij@google.com","subject":"[PATCH v6 2/4] fetch: allow adding a filter after initial clone","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-06-05T09:10:02Z","receivedAt":"2020-06-05T09:10:44Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Retroactively adding a filter can be useful for existing shallow clones as\nthey allow users to see earlier change histories without downloading all\ngit objects in a regular --unshallow fetch.\n\nWithout this patch, users can make a clone partial by editing the\nrepository configuration to convert the remote into a promisor, like:\n\n  git config core.repositoryFormatVersion 1\n  git config extensions.partialClone origin\n  git fetch --unshallow --filter=blob:none origin\n\nSince the hard part of making this work is already in place and such\nedits can be error-prone, teach Git to perform the required configuration\nchange automatically instead.\n\nNote that this change does not modify the existing git behavior which\nrecognizes setting extensions.partialClone without changing\nrepositoryFormatVersion.\n\nSigned-off-by: Xin Li \n---\n builtin/fetch.c | 3 ---\n t/t0410-partial-clone.sh | 12 ++++++++++++\n t/t1090-sparse-checkout-scope.sh | 1 -\n t/t5500-fetch-pack.sh | 1 -\n t/t5702-protocol-v2.sh | 1 -\n 5 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5788c16bf..3347d578ea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !has_promisor_remote())\n-\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n-\n \tif (all) {\n \t\tif (argc == 1)\n \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..16ad000382 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,18 @@ test_expect_success 'extensions.partialclone without filter' '\n \tgit -C client fetch origin\n '\n \n+test_expect_success 'convert shallow clone to partial clone' '\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+\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n+\ttest_cmp_config -C client true remote.origin.promisor &&\n+\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\n+\ttest_cmp_config -C client 1 core.repositoryformatversion\n+'\n+\n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 40cc004326..f35a73dd20 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -63,7 +63,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n \tgit -C server commit -m message &&\n \n \ttest_config -C client core.sparsecheckout 1 &&\n-\ttest_config -C client extensions.partialclone origin &&\n \techo \"!/*\" >client/.git/info/sparse-checkout &&\n \techo \"/a\" >>client/.git/info/sparse-checkout &&\n \tgit -C client fetch --filter=blob:none origin &&\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 8c54e34ef1..0f5ff25179 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -999,7 +999,6 @@ fetch_filter_blob_limit_zero () {\n \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n \n \tgit clone \"$URL\" client &&\n-\ttest_config -C client extensions.partialclone origin &&\n \n \ttest_commit -C \"$SERVER\" two &&\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..8b27fad6cd 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -348,7 +348,6 @@ test_expect_success 'partial fetch' '\n \trm -rf client \"$(pwd)/trace\" &&\n \tgit init client &&\n \tSERVER=\"file://$(pwd)/server\" &&\n-\ttest_config -C client extensions.partialClone \"$SERVER\" &&\n \n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n \t\tfetch --filter=blob:none \"$SERVER\" master:refs/heads/other &&\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399169","messageId":"20200605091004.208668-4-delphij@google.com","threadId":"53463","inReplyTo":"20200605091004.208668-1-delphij@google.com","subject":"[PATCH v6 3/4] sparse-checkout: upgrade repository to version 1 when enabling extension","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-06-05T09:10:03Z","receivedAt":"2020-06-05T09:10:49Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"The 'extensions' configuration variable gets special meaning in the new\nrepository version, so when enabling the extension we should upgrade the\nrepository to version 1.\n\nSigned-off-by: Xin Li \n---\n builtin/sparse-checkout.c | 2 ++\n t/t2404-worktree-config.sh | 4 +++-\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 95d0882417..95669815d4 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -249,6 +249,8 @@ static int set_config(enum sparse_checkout_mode mode)\n {\n \tconst char *config_path;\n \n+\tif (upgrade_repository_format(1) < 0)\n+\t\tdie(_(\"unable to upgrade repository format to enable worktreeConfig\"));\n \tif (git_config_set_gently(\"extensions.worktreeConfig\", \"true\")) {\n \t\terror(_(\"failed to set extensions.worktreeConfig setting\"));\n \t\treturn 1;\ndiff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\nindex 286121d8de..9536d10919 100755\n--- a/t/t2404-worktree-config.sh\n+++ b/t/t2404-worktree-config.sh\n@@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n '\n \n test_expect_success 'enable worktreeConfig extension' '\n+\tgit config core.repositoryformatversion 1 &&\n \tgit config extensions.worktreeConfig true &&\n-\ttest_cmp_config true extensions.worktreeConfig\n+\ttest_cmp_config true extensions.worktreeConfig &&\n+\ttest_cmp_config 1 core.repositoryformatversion\n '\n \n test_expect_success 'config is shared as before' '\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399170","messageId":"20200605091004.208668-5-delphij@google.com","threadId":"53463","inReplyTo":"20200605091004.208668-1-delphij@google.com","subject":"[PATCH v6 4/4] check_repository_format_gently(): refuse extensions for old repositories","fromName":"Xin Li","fromEmail":"delphij@google.com","sentAt":"2020-06-05T09:10:04Z","receivedAt":"2020-06-05T09:10:54Z","isPatch":true,"sender":{"key":"delphij@google.com","avatar":"https://avatars.githubusercontent.com/u/1045626?v=4"},"body":"Previously, extensions were recognized regardless of repository format\nversion.  If the user sets an undefined \"extensions\" value on a\nrepository of version 0 and that value is used by a future git version,\nthey might get an undesired result.\n\nBecause all extensions now also upgrade repository versions, tightening\nthe check would help avoid this for future extensions.\n\nSigned-off-by: Xin Li \n---\n setup.c | 12 +++++++++---\n t/t0410-partial-clone.sh | 11 +++++++++++\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 597b41b822..eb066db6d8 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -507,9 +507,15 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\tdie(\"%s\", err.buf);\n \t}\n \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+\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 \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 16ad000382..463dc3a8be 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -42,6 +42,17 @@ 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+\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+\ttest_must_fail git -C client fetch --unshallow --filter=\"blob:none\"\n+'\n+\n test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399209","messageId":"xmqqy2p1minn.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200605091004.208668-2-delphij@google.com","subject":"Re: [PATCH v6 1/4] repository: add a helper function to perform repository format upgrade","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-05T19:12:12Z","receivedAt":"2020-06-05T19:12:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> In version 1 of repository format, \"extensions\" gained special meaning\n> and it is safer to avoid upgrading when there are pre-existing\n> extensions.\n\nI am tempted to suggest s/upgrading/& from version 0/ but if we\nassume everybody knows there are only v0 and v1, that may be\nunnecessary. I dunno.\n\n> Make list-objects-filter to use the helper function instead of setting\n> repository version directly as a prerequisite of exposing the upgrade\n> capability.\n>\n> Signed-off-by: Xin Li \n> ---\n> cache.h | 1 +\n> list-objects-filter-options.c | 3 ++-\n> repository.h | 6 ++++++\n> setup.c | 29 +++++++++++++++++++++++++++++\n> 4 files changed, 38 insertions(+), 1 deletion(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 0f0485ecfe..e5885cc9ea 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1042,6 +1042,7 @@ 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> };\n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index 256bcfbdfe..3553ad7b0a 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -326,7 +326,8 @@ void partial_clone_register(\n> \n> \t/* Check if it is already registered */\n> \tif (!promisor_remote_find(remote)) {\n> -\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n> +\t\tif (upgrade_repository_format(1) < 0)\n> +\t\t\tdie(_(\"unable to upgrade repository format to support partial clone\"));\n\nThe idea is that builtin/fetch.c calls this before the actual fetch\noperation happens, which sounds sensible.\n\n\nThe other caller of this function is in builtin/clone.c; at that\npoint, we have set up our $GIT_DIR/config file enough to be able to\nwrite the refspec into it, so we should be able to tell what version\nnumber we wrote to the repository. Could we have written version 0\nthere in today's code, though (just being curious, as we should be\nable to upgrade it to version 1 with this code)?\n\n> +int upgrade_repository_format(int target_version)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\tstruct strbuf repo_version = STRBUF_INIT;\n> +\tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n> +\n> +\tstrbuf_git_common_path(&sb, the_repository, \"config\");\n> +\tread_repository_format(&repo_fmt, sb.buf);\n> +\tstrbuf_release(&sb);\n> +\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\n\"If we do not understand the extensions, or if the original is v0\nand has any extensions, we shouldn't be upgrading\"---makes sense.\n\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> +\t\treturn -1;\n> +\t}\n> +\n> +\tstrbuf_addf(&repo_version, \"%d\", target_version);\n> +\tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n> +\tstrbuf_release(&repo_version);\n> +\treturn 1;\n> +}\n> +\n> static void init_repository_format(struct repository_format *format)\n> {\n> \tconst struct repository_format fresh = REPOSITORY_FORMAT_INIT;\n"},{"id":"399210","messageId":"xmqqtuzpmiij.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200605091004.208668-3-delphij@google.com","subject":"Re: [PATCH v6 2/4] fetch: allow adding a filter after initial clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-05T19:15:16Z","receivedAt":"2020-06-05T19:15:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> Retroactively adding a filter can be useful for existing shallow clones as\n> they allow users to see earlier change histories without downloading all\n> git objects in a regular --unshallow fetch.\n>\n> Without this patch, users can make a clone partial by editing the\n> repository configuration to convert the remote into a promisor, like:\n>\n>   git config core.repositoryFormatVersion 1\n>   git config extensions.partialClone origin\n>   git fetch --unshallow --filter=blob:none origin\n>\n> Since the hard part of making this work is already in place and such\n> edits can be error-prone, teach Git to perform the required configuration\n> change automatically instead.\n>\n> Note that this change does not modify the existing git behavior which\n> recognizes setting extensions.partialClone without changing\n> repositoryFormatVersion.\n\nHmph, I am not sure which part of the above corresponds to the\nremoval of \"in a repository without extensions.partialClone you\ncannot use the --filter option\" logic. With or without this patch,\nusers can butcher $GIT_DIR/config and pretend that the repository\nwas created with the pertial clone extension from the beginning, but\nthis step is not about automating those tasks, it seems.\n\n>\n> Signed-off-by: Xin Li \n> ---\n> builtin/fetch.c | 3 ---\n> t/t0410-partial-clone.sh | 12 ++++++++++++\n> t/t1090-sparse-checkout-scope.sh | 1 -\n> t/t5500-fetch-pack.sh | 1 -\n> t/t5702-protocol-v2.sh | 1 -\n> 5 files changed, 12 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b5788c16bf..3347d578ea 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1790,9 +1790,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \tif (depth || deepen_since || deepen_not.nr)\n> \t\tdeepen = 1;\n> \n> -\tif (filter_options.choice && !has_promisor_remote())\n> -\t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n> -\n> \tif (all) {\n> \t\tif (argc == 1)\n> \t\t\tdie(_(\"fetch --all does not take a repository argument\"));\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index a3988bd4b8..16ad000382 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -30,6 +30,18 @@ test_expect_success 'extensions.partialclone without filter' '\n> \tgit -C client fetch origin\n> '\n> \n> +test_expect_success 'convert shallow clone to partial clone' '\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> +\tgit -C client fetch --unshallow --filter=\"blob:none\" &&\n> +\ttest_cmp_config -C client true remote.origin.promisor &&\n> +\ttest_cmp_config -C client blob:none remote.origin.partialclonefilter &&\n> +\ttest_cmp_config -C client 1 core.repositoryformatversion\n> +'\n> +\n> test_expect_success 'missing reflog object, but promised by a commit, passes fsck' '\n> \trm -rf repo &&\n> \ttest_create_repo repo &&\n> diff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\n> index 40cc004326..f35a73dd20 100755\n> --- a/t/t1090-sparse-checkout-scope.sh\n> +++ b/t/t1090-sparse-checkout-scope.sh\n> @@ -63,7 +63,6 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n> \tgit -C server commit -m message &&\n> \n> \ttest_config -C client core.sparsecheckout 1 &&\n> -\ttest_config -C client extensions.partialclone origin &&\n> \techo \"!/*\" >client/.git/info/sparse-checkout &&\n> \techo \"/a\" >>client/.git/info/sparse-checkout &&\n> \tgit -C client fetch --filter=blob:none origin &&\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 8c54e34ef1..0f5ff25179 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -999,7 +999,6 @@ fetch_filter_blob_limit_zero () {\n> \ttest_config -C \"$SERVER\" uploadpack.allowfilter 1 &&\n> \n> \tgit clone \"$URL\" client &&\n> -\ttest_config -C client extensions.partialclone origin &&\n> \n> \ttest_commit -C \"$SERVER\" two &&\n> \n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 5039e66dc4..8b27fad6cd 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -348,7 +348,6 @@ test_expect_success 'partial fetch' '\n> \trm -rf client \"$(pwd)/trace\" &&\n> \tgit init client &&\n> \tSERVER=\"file://$(pwd)/server\" &&\n> -\ttest_config -C client extensions.partialClone \"$SERVER\" &&\n> \n> \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> \t\tfetch --filter=blob:none \"$SERVER\" master:refs/heads/other &&\n"},{"id":"399211","messageId":"xmqqpnadmi8e.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200605091004.208668-4-delphij@google.com","subject":"Re: [PATCH v6 3/4] sparse-checkout: upgrade repository to version 1 when enabling extension","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-05T19:21:21Z","receivedAt":"2020-06-05T19:21:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> The 'extensions' configuration variable gets special meaning in the new\n> repository version, so when enabling the extension we should upgrade the\n> repository to version 1.\n>\n> Signed-off-by: Xin Li \n> ---\n> builtin/sparse-checkout.c | 2 ++\n> t/t2404-worktree-config.sh | 4 +++-\n> 2 files changed, 5 insertions(+), 1 deletion(-)\n\nThe other place that \"extensions.*\" is referred to is in\nbuiltin/init-db.c::initialize_repository_version() and it already\nmakes sure that extensions.objectformat is set only in a repository\nwhose verseion is GIT_REPO_VERSION_READ (which is 1---by the way I\nsuspect we want a better name than _READ for the symbol---it wants\nto use the highest version supported by this binary), so with this\npatch we covered everything, hopefully.\n\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 95d0882417..95669815d4 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -249,6 +249,8 @@ static int set_config(enum sparse_checkout_mode mode)\n> {\n> \tconst char *config_path;\n> \n> +\tif (upgrade_repository_format(1) < 0)\n> +\t\tdie(_(\"unable to upgrade repository format to enable worktreeConfig\"));\n> \tif (git_config_set_gently(\"extensions.worktreeConfig\", \"true\")) {\n> \t\terror(_(\"failed to set extensions.worktreeConfig setting\"));\n> \t\treturn 1;\n> diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh\n> index 286121d8de..9536d10919 100755\n> --- a/t/t2404-worktree-config.sh\n> +++ b/t/t2404-worktree-config.sh\n> @@ -23,8 +23,10 @@ test_expect_success 'config --worktree without extension' '\n> '\n> \n> test_expect_success 'enable worktreeConfig extension' '\n> +\tgit config core.repositoryformatversion 1 &&\n> \tgit config extensions.worktreeConfig true &&\n> -\ttest_cmp_config true extensions.worktreeConfig\n> +\ttest_cmp_config true extensions.worktreeConfig &&\n> +\ttest_cmp_config 1 core.repositoryformatversion\n> '\n> \n> test_expect_success 'config is shared as before' '\n"},{"id":"399305","messageId":"xmqqpna9lchh.fsf@gitster.c.googlers.com","threadId":"53463","inReplyTo":"20200605091004.208668-5-delphij@google.com","subject":"Re: [PATCH v6 4/4] check_repository_format_gently(): refuse extensions for old repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T16:59:54Z","receivedAt":"2020-06-08T17:00:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Xin Li writes:\n\n> Previously, extensions were recognized regardless of repository format\n> version.  If the user sets an undefined \"extensions\" value on a\n> repository of version 0 and that value is used by a future git version,\n> they might get an undesired result.\n>\n> Because all extensions now also upgrade repository versions, tightening\n> the check would help avoid this for future extensions.\n>\n> Signed-off-by: Xin Li \n> ---\n> setup.c | 12 +++++++++---\n> t/t0410-partial-clone.sh | 11 +++++++++++\n> 2 files changed, 20 insertions(+), 3 deletions(-)\n\nThanks.\n\nPeople, I think this is the only bit that is potentially\ncontroversial with the risk of breaking existing set-up. \nIts potentially negative effect can be seen in the patch to\ntest in the previous step.\n\n test_expect_success 'enable worktreeConfig extension' '\n +\tgit config core.repositoryformatversion 1 &&\n git config extensions.worktreeConfig true &&\n\nIt used to be that, by mistake, you could just say\n\n git config extensions.worktreeConfig true\n\nin an existing repository (with extra worktrees) and expect that\nalone is enough to let you use per-worktree configuration files.\n\nWith this patch, that is no longer true, because the extensions.*\nwill not take effect unless you upgrade core.repositoryformatversion\nto 1.\n\nIt is the right thing to do in the longer term (we could even\nconsider it a bugfix), and the workaround is simple enough.\n\nUnless the user is somehow using a configuration variable whose name\nbegins with \"extensions.\" for whatever random purpose, that is. If\nthe user's setup uses extensions.bar, there is no workaround\npossible other than renaming the variable to put it outside\nextensions.* hierarchy, and update whatever tool that is using that\nvariable to use the renamed variable.\n\nComments?\n"}]}