{"thread":{"id":"54230","subject":"Posible bug with GIT_DEFAULT_HASH during clone","startedAt":"2020-09-11T16:33:31Z","lastAt":"2020-09-22T16:27:28Z","messageCount":15,"participants":["Matheus Tavares","brian m. carlson","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"405413","messageId":"20200911151717.43475-1-matheus.bernardino@usp.br","threadId":"54230","inReplyTo":null,"subject":"Posible bug with GIT_DEFAULT_HASH during clone","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-09-11T15:17:17Z","receivedAt":"2020-09-11T16:33:31Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, everyone\n\nDocumentation/git.txt mentions that GIT_DEFAULT_HASH is ignored during\nclone, but I think it may not be *totally* ignored, sometimes leaving\nthe config file on the cloned repo in an inconsistent state.\n\nTo reproduce this (tested with current `master` and `seen`):\n\ngit init test\necho F>test/F\ngit -C test add F\ngit -C test commit -m F\nexport GIT_DEFAULT_HASH=sha256\ngit clone test test-clone\ngit -C test-clone status\n\nWhich outputs:\nfatal: repo version is 0, but v1-only extensions found:\n        objectformat\n\nFrom what I could see under gdb, the steps leading to this are:\n\n- First, this call chain gets the GIT_DEFAULT_HASH value:\n cmd_clone() > init_db() > validate_hash_algorithm().\n\n- Then, init_db() calls create_default_files(), which calls\n initialize_repository_version() with GIT_HASH_SHA256, setting these\n configs:\n  * extensions.objectFormat=sha256\n  * core.repositoryFormatVersion=1\n\n- Finally, cmd_clone() later uses the return of\n transport_get_hash_algo() to call initialize_repository_version()\n again, but with GIT_HASH_SHA1, setting:\n * core.repositoryFormatVersion=0\n\nSo we end up with the repository format version as 0 but the\nobjectFormat extension is present.\n\nThanks,\nMatheus\n\n"},{"id":"405459","messageId":"20200911232005.GM241078@camp.crustytoothpaste.net","threadId":"54230","inReplyTo":"20200911151717.43475-1-matheus.bernardino@usp.br","subject":"Re: Posible bug with GIT_DEFAULT_HASH during clone","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-11T23:20:05Z","receivedAt":"2020-09-11T23:20:17Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-09-11 at 15:17:17, Matheus Tavares wrote:\n> Hi, everyone\n> \n> Documentation/git.txt mentions that GIT_DEFAULT_HASH is ignored during\n> clone, but I think it may not be *totally* ignored, sometimes leaving\n> the config file on the cloned repo in an inconsistent state.\n> \n> To reproduce this (tested with current `master` and `seen`):\n> \n> git init test\n> echo F>test/F\n> git -C test add F\n> git -C test commit -m F\n> export GIT_DEFAULT_HASH=sha256\n> git clone test test-clone\n> git -C test-clone status\n> \n> Which outputs:\n> fatal: repo version is 0, but v1-only extensions found:\n>         objectformat\n> \n> From what I could see under gdb, the steps leading to this are:\n> \n> - First, this call chain gets the GIT_DEFAULT_HASH value:\n> cmd_clone() > init_db() > validate_hash_algorithm().\n> \n> - Then, init_db() calls create_default_files(), which calls\n> initialize_repository_version() with GIT_HASH_SHA256, setting these\n> configs:\n>   * extensions.objectFormat=sha256\n>   * core.repositoryFormatVersion=1\n> \n> - Finally, cmd_clone() later uses the return of\n> transport_get_hash_algo() to call initialize_repository_version()\n> again, but with GIT_HASH_SHA1, setting:\n> * core.repositoryFormatVersion=0\n> \n> So we end up with the repository format version as 0 but the\n> objectFormat extension is present.\n\nThanks for the bug report and reproduction steps. There are a couple\npossibilities here:\n\n* First, we just initialize the repository with SHA-1. When I saw this,\n I tried that, but it breaks anytime we have an empty repository (and\n with partial clone, apparently). So I don't think this is going to\n work.\n* We take a reinitialize flag in initialize_repository_version and then\n set the algorithm to \"sha1\". This is extremely easy, but it also\n poses some compatibility problems, because older versions of Git won't\n handle this configuration.\n* We take a reinitialize flag and clear the value, which seems to be the\n best way forward. I'll verify that this doesn't pose any unforeseen\n problems elsewhere in the testsuite and then send a patch.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"405460","messageId":"20200911233815.2808426-1-sandals@crustytoothpaste.net","threadId":"54230","inReplyTo":"20200911151717.43475-1-matheus.bernardino@usp.br","subject":"[PATCH] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-11T23:38:15Z","receivedAt":"2020-09-11T23:39:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"If a user is cloning a SHA-1 repository with GIT_DEFAULT_HASH set to\n\"sha256\", then we can end up with a repository where the repository\nformat version is 0 but the extensions.objectformat key is set to\n\"sha256\". This is both wrong (the user has a SHA-1 repository) and\nnonfunctional (because the extension cannot be used in a v0 repository).\n\nThis happens because in a clone, we initially set up the repository, and\nthen change its algorithm based on what the remote side tells us it's\nusing. We've initially set up the repository as SHA-256 in this case,\nand then later on reset the repository version without clearing the\nextension.\n\nWe could just always set the extension in this case, but that would mean\nthat our SHA-1 repositories weren't compatible with older Git versions,\neven though there's no reason why they shouldn't be. And we also don't\nwant to initialize the repository as SHA-1 initially, since that means\nif we're cloning an empty repository, we'll have failed to honor the\nGIT_DEFAULT_HASH variable and will end up with a SHA-1 repository, not a\nSHA-256 repository.\n\nNeither of those are appealing, so let's tell the repository\ninitialization code if we're doing a reinit like this, and if so, to\nclear the extension if we're using SHA-1. This makes sure we produce a\nvalid and functional repository and doesn't break any of our other use\ncases.\n\nReported-by: Matheus Tavares \nSigned-off-by: brian m. carlson \n---\n builtin/clone.c | 2 +-\n builtin/init-db.c | 6 ++++--\n cache.h | 2 +-\n t/t5601-clone.sh | 10 ++++++++++\n 4 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..925a2e3dd6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1235,7 +1235,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t * Now that we know what algorithm the remote side is using,\n \t\t * let's set ours to the same thing.\n \t\t */\n-\t\tinitialize_repository_version(hash_algo);\n+\t\tinitialize_repository_version(hash_algo, 1);\n \t\trepo_set_hash_algo(the_repository, hash_algo);\n \n \t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex cd3e760541..57e15e93da 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -179,7 +179,7 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)\n \treturn 1;\n }\n \n-void initialize_repository_version(int hash_algo)\n+void initialize_repository_version(int hash_algo, int reinit)\n {\n \tchar repo_version_string[10];\n \tint repo_version = GIT_REPO_VERSION;\n@@ -195,6 +195,8 @@ void initialize_repository_version(int hash_algo)\n \tif (hash_algo != GIT_HASH_SHA1)\n \t\tgit_config_set(\"extensions.objectformat\",\n \t\t\t hash_algos[hash_algo].name);\n+\telse if (reinit)\n+\t\tgit_config_set(\"extensions.objectformat\", NULL);\n }\n \n static int create_default_files(const char *template_path,\n@@ -277,7 +279,7 @@ static int create_default_files(const char *template_path,\n \t\tfree(ref);\n \t}\n \n-\tinitialize_repository_version(fmt->hash_algo);\n+\tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n \tpath = git_path_buf(&buf, \"config\");\ndiff --git a/cache.h b/cache.h\nindex cee8aa5dc3..c0072d43b1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -629,7 +629,7 @@ int path_inside_repo(const char *prefix, const char *path);\n int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *template_dir, int hash_algo,\n \t const char *initial_branch, unsigned int flags);\n-void initialize_repository_version(int hash_algo);\n+void initialize_repository_version(int hash_algo, int reinit);\n \n void sanitize_stdfds(void);\n int daemonize(void);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 15fb64c18d..570d989795 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -631,6 +631,16 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '\n \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n '\n \n+test_expect_success 'clone with GIT_DEFAULT_HASH' '\n+\t(\n+\t\tsane_unset GIT_DEFAULT_HASH &&\n+\t\tgit init test\n+\t) &&\n+\ttest_commit -C test foo &&\n+\tgit clone test test-clone &&\n+\tgit -C test-clone status\n+'\n+\n partial_clone_server () {\n \t SERVER=\"$1\" &&\n \n"},{"id":"405464","messageId":"20200912032448.GB1988@nand.local","threadId":"54230","inReplyTo":"20200911233815.2808426-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-12T03:24:48Z","receivedAt":"2020-09-12T03:24:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 11, 2020 at 11:38:15PM +0000, brian m. carlson wrote:\n> Neither of those are appealing, so let's tell the repository\n> initialization code if we're doing a reinit like this, and if so, to\n> clear the extension if we're using SHA-1. This makes sure we produce a\n> valid and functional repository and doesn't break any of our other use\n> cases.\n\nYour explanation makes sense, and what you're proposing here seems like\na very reasonable path forward. I can't think of anything that would\ncause it not to work as expected.\n\n> -void initialize_repository_version(int hash_algo);\n> +void initialize_repository_version(int hash_algo, int reinit);\n\nI'm not a huge fan of adding a 'reinit' parameter to a function that\nitself begins with the word 'initialize' (why wouldn't you call\n'reinitialize_repository_version()' instead?), but seeing as there are\nonly a couple of callers, maybe it is OK.\n\nAlternatively, I certainly wouldn't complain if you did introduce a new\nfunction and updated the call-site that passes reinit as 1.\n\n> +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n> +\t(\n> +\t\tsane_unset GIT_DEFAULT_HASH &&\n> +\t\tgit init test\n> +\t) &&\n> +\ttest_commit -C test foo &&\n> +\tgit clone test test-clone &&\n> +\tgit -C test-clone status\n> +'\n> +\n\nThis test looks very reasonable, and certainly demonstrates the bug and\nfix. Thanks.\n\nWith or without my nitpick:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"405477","messageId":"20200912195256.GN241078@camp.crustytoothpaste.net","threadId":"54230","inReplyTo":"20200912032448.GB1988@nand.local","subject":"Re: [PATCH] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-12T19:52:56Z","receivedAt":"2020-09-12T19:53:06Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-09-12 at 03:24:48, Taylor Blau wrote:\n> On Fri, Sep 11, 2020 at 11:38:15PM +0000, brian m. carlson wrote:\n> > -void initialize_repository_version(int hash_algo);\n> > +void initialize_repository_version(int hash_algo, int reinit);\n> \n> I'm not a huge fan of adding a 'reinit' parameter to a function that\n> itself begins with the word 'initialize' (why wouldn't you call\n> 'reinitialize_repository_version()' instead?), but seeing as there are\n> only a couple of callers, maybe it is OK.\n> \n> Alternatively, I certainly wouldn't complain if you did introduce a new\n> function and updated the call-site that passes reinit as 1.\n\nI thought about introducing a new function, but since it would share\nalmost all of the code, it seemed a bit wasteful, even if the function\nis small. We do have only two callers, I believe, since I recall\nmaking this function non-static and calling it from clone, so I think\nit's okay.\n\n> > +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n> > +\t(\n> > +\t\tsane_unset GIT_DEFAULT_HASH &&\n> > +\t\tgit init test\n> > +\t) &&\n> > +\ttest_commit -C test foo &&\n> > +\tgit clone test test-clone &&\n> > +\tgit -C test-clone status\n> > +'\n> > +\n> \n> This test looks very reasonable, and certainly demonstrates the bug and\n> fix. Thanks.\n\nThis is essentially Matheus's testcase. I considered using env -u to\navoid the subshell but POSIX doesn't specify that, so we have a\nsubshell.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"405537","messageId":"xmqqo8m811mf.fsf@gitster.c.googlers.com","threadId":"54230","inReplyTo":"20200912195256.GN241078@camp.crustytoothpaste.net","subject":"Re: [PATCH] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T21:37:12Z","receivedAt":"2020-09-14T21:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> Alternatively, I certainly wouldn't complain if you did introduce a new\n>> function and updated the call-site that passes reinit as 1.\n>\n> I thought about introducing a new function, but since it would share\n> almost all of the code, it seemed a bit wasteful, even if the function\n> is small. We do have only two callers, I believe, since I recall\n> making this function non-static and calling it from clone, so I think\n> it's okay.\n\nPerhaps.\n\nFWIW, this seems to have strange interaction with something in\n'seen' when merged; I suspect it is the topic that mucks with the\nset-up sequence for \"git clone\", but didn't check.\n\n\n"},{"id":"405538","messageId":"xmqqk0ww119u.fsf@gitster.c.googlers.com","threadId":"54230","inReplyTo":"xmqqo8m811mf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T21:44:45Z","receivedAt":"2020-09-14T21:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"brian m. carlson\" writes:\n>\n>>> Alternatively, I certainly wouldn't complain if you did introduce a new\n>>> function and updated the call-site that passes reinit as 1.\n>>\n>> I thought about introducing a new function, but since it would share\n>> almost all of the code, it seemed a bit wasteful, even if the function\n>> is small. We do have only two callers, I believe, since I recall\n>> making this function non-static and calling it from clone, so I think\n>> it's okay.\n>\n> Perhaps.\n>\n> FWIW, this seems to have strange interaction with something in\n> 'seen' when merged; I suspect it is the topic that mucks with the\n> set-up sequence for \"git clone\", but didn't check.\n\nActually I have to take it back. I have this directly on top of\nv2.28.0 and it already breaks tests big time. For example, here is\nhow \"cd t && sh t0021-conversion.sh -i -v\" breaks:\n\n ...\n Cloning into 'repo-cloned'...\n fatal: could not unset 'extensions.objectformat'\n fatal: the remote end hung up unexpectedly\n not ok 25 - delayed checkout in process filter\n\nThe story is the same if it is applied on top of 'master' (which is\nexpected, as we haven't done anything to affect this area since\nv2.28.0).\n"},{"id":"405545","messageId":"20200915013244.GF2866143@camp.crustytoothpaste.net","threadId":"54230","inReplyTo":"xmqqk0ww119u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-15T01:32:44Z","receivedAt":"2020-09-15T01:32:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-09-14 at 21:44:45, Junio C Hamano wrote:\n> Actually I have to take it back. I have this directly on top of\n> v2.28.0 and it already breaks tests big time. For example, here is\n> how \"cd t && sh t0021-conversion.sh -i -v\" breaks:\n> \n> ...\n> Cloning into 'repo-cloned'...\n> fatal: could not unset 'extensions.objectformat'\n> fatal: the remote end hung up unexpectedly\n> not ok 25 - delayed checkout in process filter\n> \n> The story is the same if it is applied on top of 'master' (which is\n> expected, as we haven't done anything to affect this area since\n> v2.28.0).\n\nAh, yes. I build with GIT_TEST_DEFAULT_HASH=sha256, but if you run the\ntests with SHA-1, it fails. Feel free to drop it for now and I'll send\nout a v2 soon. Sorry about that.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"405546","messageId":"20200915015845.4149976-1-sandals@crustytoothpaste.net","threadId":"54230","inReplyTo":"20200911233815.2808426-1-sandals@crustytoothpaste.net","subject":"[PATCH v2] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-15T01:58:45Z","receivedAt":"2020-09-15T01:58:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"If a user is cloning a SHA-1 repository with GIT_DEFAULT_HASH set to\n\"sha256\", then we can end up with a repository where the repository\nformat version is 0 but the extensions.objectformat key is set to\n\"sha256\". This is both wrong (the user has a SHA-1 repository) and\nnonfunctional (because the extension cannot be used in a v0 repository).\n\nThis happens because in a clone, we initially set up the repository, and\nthen change its algorithm based on what the remote side tells us it's\nusing. We've initially set up the repository as SHA-256 in this case,\nand then later on reset the repository version without clearing the\nextension.\n\nWe could just always set the extension in this case, but that would mean\nthat our SHA-1 repositories weren't compatible with older Git versions,\neven though there's no reason why they shouldn't be. And we also don't\nwant to initialize the repository as SHA-1 initially, since that means\nif we're cloning an empty repository, we'll have failed to honor the\nGIT_DEFAULT_HASH variable and will end up with a SHA-1 repository, not a\nSHA-256 repository.\n\nNeither of those are appealing, so let's tell the repository\ninitialization code if we're doing a reinit like this, and if so, to\nclear the extension if we're using SHA-1. This makes sure we produce a\nvalid and functional repository and doesn't break any of our other use\ncases.\n\nReported-by: Matheus Tavares \nSigned-off-by: brian m. carlson \n---\nChanges since v1:\n* Use git_config_set_gently to make things work with SHA-1 repos as well\n as SHA-256 repos.\n\nDiff-intervalle contre v1 :\n1: 32d3357460 ! 1: 1becbbbb50 builtin/clone: avoid failure with GIT_DEFAULT_HASH\n @@ builtin/init-db.c: void initialize_repository_version(int hash_algo)\n \t\tgit_config_set(\"extensions.objectformat\",\n \t\t\t hash_algos[hash_algo].name);\n +\telse if (reinit)\n -+\t\tgit_config_set(\"extensions.objectformat\", NULL);\n ++\t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n }\n \n static int create_default_files(const char *template_path,\n\n builtin/clone.c | 2 +-\n builtin/init-db.c | 6 ++++--\n cache.h | 2 +-\n t/t5601-clone.sh | 10 ++++++++++\n 4 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..925a2e3dd6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1235,7 +1235,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t * Now that we know what algorithm the remote side is using,\n \t\t * let's set ours to the same thing.\n \t\t */\n-\t\tinitialize_repository_version(hash_algo);\n+\t\tinitialize_repository_version(hash_algo, 1);\n \t\trepo_set_hash_algo(the_repository, hash_algo);\n \n \t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex cd3e760541..01bc648d41 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -179,7 +179,7 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)\n \treturn 1;\n }\n \n-void initialize_repository_version(int hash_algo)\n+void initialize_repository_version(int hash_algo, int reinit)\n {\n \tchar repo_version_string[10];\n \tint repo_version = GIT_REPO_VERSION;\n@@ -195,6 +195,8 @@ void initialize_repository_version(int hash_algo)\n \tif (hash_algo != GIT_HASH_SHA1)\n \t\tgit_config_set(\"extensions.objectformat\",\n \t\t\t hash_algos[hash_algo].name);\n+\telse if (reinit)\n+\t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n }\n \n static int create_default_files(const char *template_path,\n@@ -277,7 +279,7 @@ static int create_default_files(const char *template_path,\n \t\tfree(ref);\n \t}\n \n-\tinitialize_repository_version(fmt->hash_algo);\n+\tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n \tpath = git_path_buf(&buf, \"config\");\ndiff --git a/cache.h b/cache.h\nindex cee8aa5dc3..c0072d43b1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -629,7 +629,7 @@ int path_inside_repo(const char *prefix, const char *path);\n int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *template_dir, int hash_algo,\n \t const char *initial_branch, unsigned int flags);\n-void initialize_repository_version(int hash_algo);\n+void initialize_repository_version(int hash_algo, int reinit);\n \n void sanitize_stdfds(void);\n int daemonize(void);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 15fb64c18d..570d989795 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -631,6 +631,16 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '\n \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n '\n \n+test_expect_success 'clone with GIT_DEFAULT_HASH' '\n+\t(\n+\t\tsane_unset GIT_DEFAULT_HASH &&\n+\t\tgit init test\n+\t) &&\n+\ttest_commit -C test foo &&\n+\tgit clone test test-clone &&\n+\tgit -C test-clone status\n+'\n+\n partial_clone_server () {\n \t SERVER=\"$1\" &&\n \n"},{"id":"405549","messageId":"xmqq8sdb1x0t.fsf@gitster.c.googlers.com","threadId":"54230","inReplyTo":"20200915015845.4149976-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-15T04:31:14Z","receivedAt":"2020-09-15T04:31:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> If a user is cloning a SHA-1 repository with GIT_DEFAULT_HASH set to\n> \"sha256\", then we can end up with a repository where the repository\n> format version is 0 but the extensions.objectformat key is set to\n> \"sha256\". This is both wrong (the user has a SHA-1 repository) and\n> nonfunctional (because the extension cannot be used in a v0 repository).\n>\n> This happens because in a clone, we initially set up the repository, and\n> then change its algorithm based on what the remote side tells us it's\n> using. We've initially set up the repository as SHA-256 in this case,\n> and then later on reset the repository version without clearing the\n> extension.\n>\n> We could just always set the extension in this case, but that would mean\n> that our SHA-1 repositories weren't compatible with older Git versions,\n> even though there's no reason why they shouldn't be. And we also don't\n> want to initialize the repository as SHA-1 initially, since that means\n> if we're cloning an empty repository, we'll have failed to honor the\n> GIT_DEFAULT_HASH variable and will end up with a SHA-1 repository, not a\n> SHA-256 repository.\n>\n> Neither of those are appealing, so let's tell the repository\n> initialization code if we're doing a reinit like this, and if so, to\n> clear the extension if we're using SHA-1. This makes sure we produce a\n> valid and functional repository and doesn't break any of our other use\n> cases.\n>\n> Reported-by: Matheus Tavares \n> Signed-off-by: brian m. carlson \n> ---\n> Changes since v1:\n> * Use git_config_set_gently to make things work with SHA-1 repos as well\n> as SHA-256 repos.\n\nHmph, the reason why v1's bug weren't caught was because it was only\ntested with GIT_TEST_DEFAULT_HASH=sha256, right? I am wondering if\nadding two new tests that run the same end-user scenario except\nfor the choice of hash algorithms would be a good way to ensure this\nwill stay fixed. Am I mistaken?\n\nThanks anyway for a quick turnaround.\n\n> Diff-intervalle contre v1 :\n> 1: 32d3357460 ! 1: 1becbbbb50 builtin/clone: avoid failure with GIT_DEFAULT_HASH\n> @@ builtin/init-db.c: void initialize_repository_version(int hash_algo)\n> \t\tgit_config_set(\"extensions.objectformat\",\n> \t\t\t hash_algos[hash_algo].name);\n> +\telse if (reinit)\n> -+\t\tgit_config_set(\"extensions.objectformat\", NULL);\n> ++\t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n> }\n> \n> static int create_default_files(const char *template_path,\n>\n> builtin/clone.c | 2 +-\n> builtin/init-db.c | 6 ++++--\n> cache.h | 2 +-\n> t/t5601-clone.sh | 10 ++++++++++\n> 4 files changed, 16 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b087ee40c2..925a2e3dd6 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1235,7 +1235,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\t * Now that we know what algorithm the remote side is using,\n> \t\t * let's set ours to the same thing.\n> \t\t */\n> -\t\tinitialize_repository_version(hash_algo);\n> +\t\tinitialize_repository_version(hash_algo, 1);\n> \t\trepo_set_hash_algo(the_repository, hash_algo);\n> \n> \t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index cd3e760541..01bc648d41 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -179,7 +179,7 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)\n> \treturn 1;\n> }\n> \n> -void initialize_repository_version(int hash_algo)\n> +void initialize_repository_version(int hash_algo, int reinit)\n> {\n> \tchar repo_version_string[10];\n> \tint repo_version = GIT_REPO_VERSION;\n> @@ -195,6 +195,8 @@ void initialize_repository_version(int hash_algo)\n> \tif (hash_algo != GIT_HASH_SHA1)\n> \t\tgit_config_set(\"extensions.objectformat\",\n> \t\t\t hash_algos[hash_algo].name);\n> +\telse if (reinit)\n> +\t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n> }\n> \n> static int create_default_files(const char *template_path,\n> @@ -277,7 +279,7 @@ static int create_default_files(const char *template_path,\n> \t\tfree(ref);\n> \t}\n> \n> -\tinitialize_repository_version(fmt->hash_algo);\n> +\tinitialize_repository_version(fmt->hash_algo, 0);\n> \n> \t/* Check filemode trustability */\n> \tpath = git_path_buf(&buf, \"config\");\n> diff --git a/cache.h b/cache.h\n> index cee8aa5dc3..c0072d43b1 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -629,7 +629,7 @@ int path_inside_repo(const char *prefix, const char *path);\n> int init_db(const char *git_dir, const char *real_git_dir,\n> \t const char *template_dir, int hash_algo,\n> \t const char *initial_branch, unsigned int flags);\n> -void initialize_repository_version(int hash_algo);\n> +void initialize_repository_version(int hash_algo, int reinit);\n> \n> void sanitize_stdfds(void);\n> int daemonize(void);\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 15fb64c18d..570d989795 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -631,6 +631,16 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '\n> \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n> '\n> \n> +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n> +\t(\n> +\t\tsane_unset GIT_DEFAULT_HASH &&\n> +\t\tgit init test\n> +\t) &&\n> +\ttest_commit -C test foo &&\n> +\tgit clone test test-clone &&\n> +\tgit -C test-clone status\n> +'\n> +\n> partial_clone_server () {\n> \t SERVER=\"$1\" &&\n> \n"},{"id":"405585","messageId":"20200915225131.GG2866143@camp.crustytoothpaste.net","threadId":"54230","inReplyTo":"xmqq8sdb1x0t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-15T22:51:31Z","receivedAt":"2020-09-15T22:52:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-09-15 at 04:31:14, Junio C Hamano wrote:\n> > Changes since v1:\n> > * Use git_config_set_gently to make things work with SHA-1 repos as well\n> > as SHA-256 repos.\n> \n> Hmph, the reason why v1's bug weren't caught was because it was only\n> tested with GIT_TEST_DEFAULT_HASH=sha256, right? I am wondering if\n> adding two new tests that run the same end-user scenario except\n> for the choice of hash algorithms would be a good way to ensure this\n> will stay fixed. Am I mistaken?\n\nSure, I can do that. I'll try to get a v3 out soon.\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"406005","messageId":"20200920223541.1299038-1-sandals@crustytoothpaste.net","threadId":"54230","inReplyTo":"20200911233815.2808426-1-sandals@crustytoothpaste.net","subject":"[PATCH v3] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-20T22:35:41Z","receivedAt":"2020-09-20T22:35:51Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"If a user is cloning a SHA-1 repository with GIT_DEFAULT_HASH set to\n\"sha256\", then we can end up with a repository where the repository\nformat version is 0 but the extensions.objectformat key is set to\n\"sha256\". This is both wrong (the user has a SHA-1 repository) and\nnonfunctional (because the extension cannot be used in a v0 repository).\n\nThis happens because in a clone, we initially set up the repository, and\nthen change its algorithm based on what the remote side tells us it's\nusing. We've initially set up the repository as SHA-256 in this case,\nand then later on reset the repository version without clearing the\nextension.\n\nWe could just always set the extension in this case, but that would mean\nthat our SHA-1 repositories weren't compatible with older Git versions,\neven though there's no reason why they shouldn't be. And we also don't\nwant to initialize the repository as SHA-1 initially, since that means\nif we're cloning an empty repository, we'll have failed to honor the\nGIT_DEFAULT_HASH variable and will end up with a SHA-1 repository, not a\nSHA-256 repository.\n\nNeither of those are appealing, so let's tell the repository\ninitialization code if we're doing a reinit like this, and if so, to\nclear the extension if we're using SHA-1. This makes sure we produce a\nvalid and functional repository and doesn't break any of our other use\ncases.\n\nReported-by: Matheus Tavares \nSigned-off-by: brian m. carlson \n---\nDiff-intervalle contre v2 :\n1: 1becbbbb50 ! 1: 004a7b86f8 builtin/clone: avoid failure with GIT_DEFAULT_HASH\n @@ t/t5601-clone.sh: test_expect_success CASE_INSENSITIVE_FS 'colliding file detect\n +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n +\t(\n +\t\tsane_unset GIT_DEFAULT_HASH &&\n -+\t\tgit init test\n ++\t\tgit init --object-format=sha1 test-sha1 &&\n ++\t\tgit init --object-format=sha256 test-sha256\n +\t) &&\n -+\ttest_commit -C test foo &&\n -+\tgit clone test test-clone &&\n -+\tgit -C test-clone status\n ++\ttest_commit -C test-sha1 foo &&\n ++\ttest_commit -C test-sha256 foo &&\n ++\tGIT_DEFAULT_HASH=sha1 git clone test-sha256 test-clone-sha256 &&\n ++\tGIT_DEFAULT_HASH=sha256 git clone test-sha1 test-clone-sha1 &&\n ++\tgit -C test-clone-sha1 status &&\n ++\tgit -C test-clone-sha256 status\n +'\n +\n partial_clone_server () {\n\n builtin/clone.c | 2 +-\n builtin/init-db.c | 6 ++++--\n cache.h | 2 +-\n t/t5601-clone.sh | 14 ++++++++++++++\n 4 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..925a2e3dd6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1235,7 +1235,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t * Now that we know what algorithm the remote side is using,\n \t\t * let's set ours to the same thing.\n \t\t */\n-\t\tinitialize_repository_version(hash_algo);\n+\t\tinitialize_repository_version(hash_algo, 1);\n \t\trepo_set_hash_algo(the_repository, hash_algo);\n \n \t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex cd3e760541..01bc648d41 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -179,7 +179,7 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)\n \treturn 1;\n }\n \n-void initialize_repository_version(int hash_algo)\n+void initialize_repository_version(int hash_algo, int reinit)\n {\n \tchar repo_version_string[10];\n \tint repo_version = GIT_REPO_VERSION;\n@@ -195,6 +195,8 @@ void initialize_repository_version(int hash_algo)\n \tif (hash_algo != GIT_HASH_SHA1)\n \t\tgit_config_set(\"extensions.objectformat\",\n \t\t\t hash_algos[hash_algo].name);\n+\telse if (reinit)\n+\t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n }\n \n static int create_default_files(const char *template_path,\n@@ -277,7 +279,7 @@ static int create_default_files(const char *template_path,\n \t\tfree(ref);\n \t}\n \n-\tinitialize_repository_version(fmt->hash_algo);\n+\tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n \tpath = git_path_buf(&buf, \"config\");\ndiff --git a/cache.h b/cache.h\nindex cee8aa5dc3..c0072d43b1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -629,7 +629,7 @@ int path_inside_repo(const char *prefix, const char *path);\n int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *template_dir, int hash_algo,\n \t const char *initial_branch, unsigned int flags);\n-void initialize_repository_version(int hash_algo);\n+void initialize_repository_version(int hash_algo, int reinit);\n \n void sanitize_stdfds(void);\n int daemonize(void);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 15fb64c18d..b6c8312da1 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -631,6 +631,20 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '\n \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n '\n \n+test_expect_success 'clone with GIT_DEFAULT_HASH' '\n+\t(\n+\t\tsane_unset GIT_DEFAULT_HASH &&\n+\t\tgit init --object-format=sha1 test-sha1 &&\n+\t\tgit init --object-format=sha256 test-sha256\n+\t) &&\n+\ttest_commit -C test-sha1 foo &&\n+\ttest_commit -C test-sha256 foo &&\n+\tGIT_DEFAULT_HASH=sha1 git clone test-sha256 test-clone-sha256 &&\n+\tGIT_DEFAULT_HASH=sha256 git clone test-sha1 test-clone-sha1 &&\n+\tgit -C test-clone-sha1 status &&\n+\tgit -C test-clone-sha256 status\n+'\n+\n partial_clone_server () {\n \t SERVER=\"$1\" &&\n \n"},{"id":"406012","messageId":"xmqq4knrhhzx.fsf@gitster.c.googlers.com","threadId":"54230","inReplyTo":"20200920223541.1299038-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T04:27:14Z","receivedAt":"2020-09-21T04:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n> +\t(\n> +\t\tsane_unset GIT_DEFAULT_HASH &&\n> +\t\tgit init --object-format=sha1 test-sha1 &&\n> +\t\tgit init --object-format=sha256 test-sha256\n> +\t) &&\n> +\ttest_commit -C test-sha1 foo &&\n> +\ttest_commit -C test-sha256 foo &&\n\nUnfortunately, the 'foo' commit is created in test-sha1, but the\nnext step to create 'foo' in test-sha256 fails with\n\n fatal: unknown repository extensions found:\n objectformat\n\n> +\tGIT_DEFAULT_HASH=sha1 git clone test-sha256 test-clone-sha256 &&\n> +\tGIT_DEFAULT_HASH=sha256 git clone test-sha1 test-clone-sha1 &&\n> +\tgit -C test-clone-sha1 status &&\n> +\tgit -C test-clone-sha256 status\n> +'\n> +\n> partial_clone_server () {\n> \t SERVER=\"$1\" &&\n> \n"},{"id":"406117","messageId":"20200922091717.GP67496@camp.crustytoothpaste.net","threadId":"54230","inReplyTo":"xmqq4knrhhzx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-09-22T09:17:17Z","receivedAt":"2020-09-22T09:17:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-09-21 at 04:27:14, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n> > +\t(\n> > +\t\tsane_unset GIT_DEFAULT_HASH &&\n> > +\t\tgit init --object-format=sha1 test-sha1 &&\n> > +\t\tgit init --object-format=sha256 test-sha256\n> > +\t) &&\n> > +\ttest_commit -C test-sha1 foo &&\n> > +\ttest_commit -C test-sha256 foo &&\n> \n> Unfortunately, the 'foo' commit is created in test-sha1, but the\n> next step to create 'foo' in test-sha256 fails with\n> \n> fatal: unknown repository extensions found:\n> objectformat\n\nI'm not seeing that with this series based on master\n(385c171a018f2747b329bcfa6be8eda1709e5abd). I'm doing this:\n\n make -j6 all && (cd t && GIT_TEST_DEFAULT_HASH=sha256 ./t5601-*.sh --verbose)\n make -j6 all && (cd t && GIT_TEST_DEFAULT_HASH=sha1 ./t5601-*.sh --verbose)\n\nAnd getting this output:\n\n Initialized empty Git repository in /home/bmc/checkouts/git/t/trash directory.t5601-clone/test-sha1/.git/\n Initialized empty Git repository in /home/bmc/checkouts/git/t/trash directory.t5601-clone/test-sha256/.git/\n [master (root-commit) 946e985] foo\n Author: A U Thor \n 1 file changed, 1 insertion(+)\n create mode 100644 foo.t\n [master (root-commit) ff872d8] foo\n Author: A U Thor \n 1 file changed, 1 insertion(+)\n create mode 100644 foo.t\n Cloning into 'test-clone-sha256'...\n done.\n Cloning into 'test-clone-sha1'...\n done.\n On branch master\n Your branch is up to date with 'origin/master'.\n \n nothing to commit, working tree clean\n On branch master\n Your branch is up to date with 'origin/master'.\n \n nothing to commit, working tree clean\n\nIs there something I'm missing here?\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"406132","messageId":"xmqqa6xhlqty.fsf@gitster.c.googlers.com","threadId":"54230","inReplyTo":"20200922091717.GP67496@camp.crustytoothpaste.net","subject":"Re: [PATCH v3] builtin/clone: avoid failure with GIT_DEFAULT_HASH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-22T16:27:21Z","receivedAt":"2020-09-22T16:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On 2020-09-21 at 04:27:14, Junio C Hamano wrote:\n>> \"brian m. carlson\" writes:\n>> \n>> > +test_expect_success 'clone with GIT_DEFAULT_HASH' '\n>> > +\t(\n>> > +\t\tsane_unset GIT_DEFAULT_HASH &&\n>> > +\t\tgit init --object-format=sha1 test-sha1 &&\n>> > +\t\tgit init --object-format=sha256 test-sha256\n>> > +\t) &&\n>> > +\ttest_commit -C test-sha1 foo &&\n>> > +\ttest_commit -C test-sha256 foo &&\n>> \n>> Unfortunately, the 'foo' commit is created in test-sha1, but the\n>> next step to create 'foo' in test-sha256 fails with\n>> \n>> fatal: unknown repository extensions found:\n>> objectformat\n>\n> I'm not seeing that with this series based on master\n> (385c171a018f2747b329bcfa6be8eda1709e5abd). I'm doing this:\n>\n> make -j6 all && (cd t && GIT_TEST_DEFAULT_HASH=sha256 ./t5601-*.sh --verbose)\n> make -j6 all && (cd t && GIT_TEST_DEFAULT_HASH=sha1 ./t5601-*.sh --verbose)\n>\n> And getting this output:\n>\n> Initialized empty Git repository in /home/bmc/checkouts/git/t/trash directory.t5601-clone/test-sha1/.git/\n> Initialized empty Git repository in /home/bmc/checkouts/git/t/trash directory.t5601-clone/test-sha256/.git/\n> [master (root-commit) 946e985] foo\n> Author: A U Thor \n> 1 file changed, 1 insertion(+)\n> create mode 100644 foo.t\n> [master (root-commit) ff872d8] foo\n> Author: A U Thor \n> 1 file changed, 1 insertion(+)\n> create mode 100644 foo.t\n> Cloning into 'test-clone-sha256'...\n> done.\n> Cloning into 'test-clone-sha1'...\n> done.\n> On branch master\n> Your branch is up to date with 'origin/master'.\n> \n> nothing to commit, working tree clean\n> On branch master\n> Your branch is up to date with 'origin/master'.\n> \n> nothing to commit, working tree clean\n>\n> Is there something I'm missing here?\n\nI think the issue was because I queued the patch on the same base as\nthe previous round, which was v2.28.0. Whe applied to 385c171a\n(Fifteenth batch, 2020-09-18), I get the same as the above.\n\nSorry about the noise. Will queue again.\n"}]}