{"thread":{"id":"59550","subject":"git clone of empty repositories doesn't preserve hash","startedAt":"2023-04-05T10:35:53Z","lastAt":"2023-05-19T15:32:49Z","messageCount":58,"participants":["Adam Majer","Junio C Hamano","Jeff King","brian m. carlson","Felipe Contreras","demerphq","Oswald Buddenhagen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"474836","messageId":"e7a8957e-6251-39f1-5109-87d4dd382e81@zombino.com","threadId":"59550","inReplyTo":null,"subject":"git clone of empty repositories doesn't preserve hash","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-04-05T10:28:12Z","receivedAt":"2023-04-05T10:35:53Z","isPatch":false,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"Hi all,\n\nI've noticed while adding support for sha256 repositories for Gitea that,\n\ngit init --bare --object-format=sha256 a\ngit clone a b\n\nThen the repository b is initialized as default hash, so sha1. It seems \nthat receive-pack will list the null OID in the header if there are no \nrefs available, but the upload-pack doesn't list anything and hence the \nheader with capabilities and the hash function is missing\n\ngit receive-pack a\ngit upload-pack a\n\nWhat is the right approach here? Could upload-pack send a NULL OID \nfollowed by header info that is then used by clone?\n\nThere is a workaround to specify the hash via GIT_DEFAULT_HASH. Not \nideal though.\n\nThanks,\nAdam\n\n"},{"id":"474876","messageId":"xmqqr0syw3pe.fsf@gitster.g","threadId":"59550","inReplyTo":"e7a8957e-6251-39f1-5109-87d4dd382e81@zombino.com","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T19:04:29Z","receivedAt":"2023-04-05T19:04:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Majer writes:\n\n> I've noticed while adding support for sha256 repositories for Gitea that,\n>\n> git init --bare --object-format=sha256 a\n> git clone a b\n>\n> Then the repository b is initialized as default hash, so sha1. It\n> seems that receive-pack will list the null OID in the header if there\n> are no refs available, but the upload-pack doesn't list anything and\n> hence the header with capabilities and the hash function is missing\n>\n> git receive-pack a\n> git upload-pack a\n>\n> What is the right approach here? Could upload-pack send a NULL OID\n> followed by header info that is then used by clone?\n\nInteresting.\n\nDoes such a clone copy the name of the primary branch from the\nremote repository to the newly created repository? I recall we had\nseen such a feature request but offhand I do not recall how we\nsolved it. The need to transmit a capability even when there is no\nconcrete reference to be fetched to satisfy such a feature request\nshould be the same as yours, so there may already be a good place to\nadd that information. Or we may have ended up not solving the \"what\nis the name of the primary branch in this empty repository?\", in\nwhich case, the solution for this \"what is the hash function to be\nused in this empty repository?\" should be designed to be extensive\nenough to allow us later support that feature easily.\n\nThanks for raising the issue.\n\n\n"},{"id":"474881","messageId":"d04c430e-b609-b0a1-fd0f-0f3734d5c3b1@zombino.com","threadId":"59550","inReplyTo":"xmqqr0syw3pe.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-04-05T19:47:55Z","receivedAt":"2023-04-05T19:48:13Z","isPatch":false,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"On 4/5/23 21:04, Junio C Hamano wrote:\n> Does such a clone copy the name of the primary branch from the\n> remote repository to the newly created repository?\n\nYes it does.\n\n# git init -b maestro --object-format=sha256 a\n# git clone a b\n# cat b/.git/HEAD\nref: refs/heads/maestro\n\n- Adam\n\n"},{"id":"474883","messageId":"20230405200153.GA525125@coredump.intra.peff.net","threadId":"59550","inReplyTo":"d04c430e-b609-b0a1-fd0f-0f3734d5c3b1@zombino.com","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-05T20:01:53Z","receivedAt":"2023-04-05T20:02:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 09:47:55PM +0200, Adam Majer wrote:\n\n> On 4/5/23 21:04, Junio C Hamano wrote:\n> > Does such a clone copy the name of the primary branch from the\n> > remote repository to the newly created repository?\n> \n> Yes it does.\n> \n> # git init -b maestro --object-format=sha256 a\n> # git clone a b\n> # cat b/.git/HEAD\n> ref: refs/heads/maestro\n\nYeah, we send a special capability line in that case. If you do:\n\n GIT_TRACE_PACKET=1 git clone a b\n\nyou can see that upload-pack indicates that ls-refs understands the\n\"unborn\" capability:\n\n packet: upload-pack> version 2\n packet: upload-pack> agent=git/2.40.0.824.g7b678b1f643\n packet: upload-pack> ls-refs=unborn\n packet: upload-pack> fetch=shallow wait-for-done\n packet: upload-pack> server-option\n packet: upload-pack> object-format=sha256\n packet: upload-pack> object-info\n packet: upload-pack> 0000\n\nAnd then clone asks for it say \"yes, I also understand unborn\":\n\n packet: clone> command=ls-refs\n packet: clone> agent=git/2.40.0.824.g7b678b1f643\n packet: clone> object-format=sha256\n packet: clone> 0001\n packet: clone> peel\n packet: clone> symrefs\n packet: clone> unborn\n packet: clone> ref-prefix HEAD\n packet: clone> ref-prefix refs/heads/\n packet: clone> ref-prefix refs/tags/\n packet: clone> 0000\n\nAnd then upload-pack can send us the extra information:\n\n packet: upload-pack> unborn HEAD symref-target:refs/heads/maestro\n packet: upload-pack> 0000\n\nI think we'd need to do something similar here for object-format.\n\n-Peff\n"},{"id":"474885","messageId":"xmqqa5zmukp5.fsf@gitster.g","threadId":"59550","inReplyTo":"20230405200153.GA525125@coredump.intra.peff.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T20:40:22Z","receivedAt":"2023-04-05T20:41:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, we send a special capability line in that case. If you do:\n>\n> GIT_TRACE_PACKET=1 git clone a b\n>\n> you can see that upload-pack indicates that ls-refs understands the\n> \"unborn\" capability:\n>\n> packet: upload-pack> version 2\n> packet: upload-pack> agent=git/2.40.0.824.g7b678b1f643\n> packet: upload-pack> ls-refs=unborn\n> packet: upload-pack> fetch=shallow wait-for-done\n> packet: upload-pack> server-option\n> packet: upload-pack> object-format=sha256\n> packet: upload-pack> object-info\n> packet: upload-pack> 0000\n>\n> And then clone asks for it say \"yes, I also understand unborn\":\n>\n> packet: clone> command=ls-refs\n> packet: clone> agent=git/2.40.0.824.g7b678b1f643\n> packet: clone> object-format=sha256\n> packet: clone> 0001\n> packet: clone> peel\n> packet: clone> symrefs\n> packet: clone> unborn\n> packet: clone> ref-prefix HEAD\n> packet: clone> ref-prefix refs/heads/\n> packet: clone> ref-prefix refs/tags/\n> packet: clone> 0000\n>\n> And then upload-pack can send us the extra information:\n>\n> packet: upload-pack> unborn HEAD symref-target:refs/heads/maestro\n> packet: upload-pack> 0000\n>\n> I think we'd need to do something similar here for object-format.\n\nI guess we only need to touch \"git clone\" then. Without being\nasked, it advertsizes object-format=sha256 already, and when the\nmaestro repository is prepared without --object-format=sha256,\nupload-pack advertises object-format=sha1 instead. So it probably\nis just the matter of capturing it and using it to populate the\nextensions.objectformat with an appropriate value.\n\n"},{"id":"474886","messageId":"xmqq355euj2i.fsf@gitster.g","threadId":"59550","inReplyTo":"xmqqa5zmukp5.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T21:15:33Z","receivedAt":"2023-04-05T21:15:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I guess we only need to touch \"git clone\" then. Without being\n> asked, it advertsizes object-format=sha256 already, and when the\n> maestro repository is prepared without --object-format=sha256,\n> upload-pack advertises object-format=sha1 instead. So it probably\n> is just the matter of capturing it and using it to populate the\n> extensions.objectformat with an appropriate value.\n\nIt turns out that there was a readily mimickable example in 3d8314f8\n(clone: propagate empty remote HEAD even with other branches,\n2022-07-07). The commit lifted code out of a block, in which we\nknow we are copying from a non-empty repository, to execute also\nwhen talking with an empty repository. The recording of the\nhash-algorithm in the extensions section is done in the same way, so\nwe can do the same \"fix\".\n\n----- >8 -----\nSubject: [PATCH] clone: propagate object-format when cloning from void\n\nA user could prepare an empty repository and set it to use SHA256 as\nthe object format. The new repository created by \"git clone\" from\nsuch a repository however would not record that it is expecting\nobjects in the same SHA256 format. This works as expected if the\nsource repository is not empty.\n\nJust like we started copying the name of the primary branch from the\nremote repository even if it is unborn in 3d8314f8 (clone: propagate\nempty remote HEAD even with other branches, 2022-07-07), lift the\ncode that records the object format out of the block executed only\nwhen cloning from an instantiated repository, so that it works also\nwhen cloning from an empty repository.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/clone.c | 11 ++++++-----\n t/t5702-protocol-v2.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git c/builtin/clone.c w/builtin/clone.c\nindex 462c286274..8f16d18a43 100644\n--- c/builtin/clone.c\n+++ w/builtin/clone.c\n@@ -910,6 +910,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n \tint filter_submodules = 0;\n+\tint hash_algo;\n \n \tstruct transport_ls_refs_options transport_ls_refs_options =\n \t\tTRANSPORT_LS_REFS_OPTIONS_INIT;\n@@ -1298,15 +1299,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (mapped_refs) {\n-\t\tint hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n-\n \t\t/*\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, 1);\n-\t\trepo_set_hash_algo(the_repository, hash_algo);\n+\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n+\tinitialize_repository_version(hash_algo, 1);\n+\trepo_set_hash_algo(the_repository, hash_algo);\n+\n+\tif (mapped_refs) {\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\ndiff --git c/t/t5702-protocol-v2.sh w/t/t5702-protocol-v2.sh\nindex 71aabe30b7..6af5c2062f 100755\n--- c/t/t5702-protocol-v2.sh\n+++ w/t/t5702-protocol-v2.sh\n@@ -269,6 +269,17 @@ test_expect_success 'clone propagates unborn HEAD from non-empty repo' '\n \tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n '\n \n+test_expect_success 'clone propagates object-format from empty repo' '\n+\ttest_when_finished \"rm -fr src256 dst256\" &&\n+\n+\techo sha256 >expect &&\n+\tgit init --object-format=sha256 src256 &&\n+\tgit clone src256 dst256 &&\n+\tgit -C dst256 rev-parse --show-object-format >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bare clone propagates unborn HEAD from non-empty repo' '\n \ttest_when_finished \"rm -rf file_unborn_parent file_unborn_child.git\" &&\n \n"},{"id":"474888","messageId":"20230405212301.GA529421@coredump.intra.peff.net","threadId":"59550","inReplyTo":"xmqqa5zmukp5.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-05T21:23:01Z","receivedAt":"2023-04-05T21:23:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 01:40:22PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, we send a special capability line in that case. If you do:\n> >\n> > GIT_TRACE_PACKET=1 git clone a b\n> >\n> > you can see that upload-pack indicates that ls-refs understands the\n> > \"unborn\" capability:\n> >\n> > packet: upload-pack> version 2\n> > packet: upload-pack> agent=git/2.40.0.824.g7b678b1f643\n> > packet: upload-pack> ls-refs=unborn\n> > packet: upload-pack> fetch=shallow wait-for-done\n> > packet: upload-pack> server-option\n> > packet: upload-pack> object-format=sha256\n> > packet: upload-pack> object-info\n> > packet: upload-pack> 0000\n> >\n> > And then clone asks for it say \"yes, I also understand unborn\":\n> >\n> > packet: clone> command=ls-refs\n> > packet: clone> agent=git/2.40.0.824.g7b678b1f643\n> > packet: clone> object-format=sha256\n> > packet: clone> 0001\n> > packet: clone> peel\n> > packet: clone> symrefs\n> > packet: clone> unborn\n> > packet: clone> ref-prefix HEAD\n> > packet: clone> ref-prefix refs/heads/\n> > packet: clone> ref-prefix refs/tags/\n> > packet: clone> 0000\n> >\n> > And then upload-pack can send us the extra information:\n> >\n> > packet: upload-pack> unborn HEAD symref-target:refs/heads/maestro\n> > packet: upload-pack> 0000\n> >\n> > I think we'd need to do something similar here for object-format.\n> \n> I guess we only need to touch \"git clone\" then. Without being\n> asked, it advertsizes object-format=sha256 already, and when the\n> maestro repository is prepared without --object-format=sha256,\n> upload-pack advertises object-format=sha1 instead. So it probably\n> is just the matter of capturing it and using it to populate the\n> extensions.objectformat with an appropriate value.\n\nAh, yeah, you're right. I was thinking that capability advertisement was\n\"by the way, I understand sha256\". But I may just be showing my\nignorance of the current state of the hash-transition protocol\nextensions. :)\n\nI'm actually surprised this does not Just Work already. The client gets\nthe intended algorithm from that capability line, which is how we know\nhow to parse any ls-refs output at all (we are not just guessing from\nthe length). But we only bother to set it in the local config if we have\nrefs to fetch, which seems like a bug.\n\nSo the solution is maybe something like this:\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 462c286274c..5eca95cb892 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1296,19 +1296,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tclear_bundle_list(transport->bundles);\n \t\t\tFREE_AND_NULL(transport->bundles);\n \t\t}\n \t}\n \n-\tif (mapped_refs) {\n+\t{\n \t\tint hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n \n \t\t/*\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, 1);\n \t\trepo_set_hash_algo(the_repository, hash_algo);\n+\t}\n+\n+\tif (mapped_refs) {\n \t\t/*\n \t\t * transport_get_remote_refs() may return refs with null sha-1\n \t\t * in mapped_refs (see struct transport->get_refs_list\n \t\t * comment). In that case we need fetch it early because\n \t\t * remote_head code below relies on it.\n"},{"id":"474889","messageId":"20230405212617.GB529421@coredump.intra.peff.net","threadId":"59550","inReplyTo":"xmqq355euj2i.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-05T21:26:17Z","receivedAt":"2023-04-05T21:26:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 05, 2023 at 02:15:33PM -0700, Junio C Hamano wrote:\n\n> ----- >8 -----\n> Subject: [PATCH] clone: propagate object-format when cloning from void\n> \n> A user could prepare an empty repository and set it to use SHA256 as\n> the object format. The new repository created by \"git clone\" from\n> such a repository however would not record that it is expecting\n> objects in the same SHA256 format. This works as expected if the\n> source repository is not empty.\n> \n> Just like we started copying the name of the primary branch from the\n> remote repository even if it is unborn in 3d8314f8 (clone: propagate\n> empty remote HEAD even with other branches, 2022-07-07), lift the\n> code that records the object format out of the block executed only\n> when cloning from an instantiated repository, so that it works also\n> when cloning from an empty repository.\n\nHeh, our mails just crossed, but I stumbled upon the same patch. So yes,\nthis looks good to me.\n\nI suspect that setting the flag in the_repository might not matter,\nsince we do not have any refs or objects to manipulate, and what we care\nabout here is that initialize_repository_version() is run. But I agree\nthat the two conceptually belong together, and hoisting both out of the\nblock is the right thing to do.\n\n> diff --git c/t/t5702-protocol-v2.sh w/t/t5702-protocol-v2.sh\n> index 71aabe30b7..6af5c2062f 100755\n> --- c/t/t5702-protocol-v2.sh\n> +++ w/t/t5702-protocol-v2.sh\n> @@ -269,6 +269,17 @@ test_expect_success 'clone propagates unborn HEAD from non-empty repo' '\n> \tgrep \"warning: remote HEAD refers to nonexistent ref\" stderr\n> '\n> \n> +test_expect_success 'clone propagates object-format from empty repo' '\n> +\ttest_when_finished \"rm -fr src256 dst256\" &&\n> +\n> +\techo sha256 >expect &&\n> +\tgit init --object-format=sha256 src256 &&\n> +\tgit clone src256 dst256 &&\n> +\tgit -C dst256 rev-parse --show-object-format >actual &&\n> +\n> +\ttest_cmp expect actual\n> +'\n\nAnd this test looks straightforward. Very nice.\n\n-Peff\n"},{"id":"474894","messageId":"ZC36xXGBIAHcetZW@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"xmqq355euj2i.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-05T22:48:37Z","receivedAt":"2023-04-05T22:48:44Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-05 at 21:15:33, Junio C Hamano wrote:\n> A user could prepare an empty repository and set it to use SHA256 as\n> the object format. The new repository created by \"git clone\" from\n> such a repository however would not record that it is expecting\n> objects in the same SHA256 format. This works as expected if the\n> source repository is not empty.\n> \n> Just like we started copying the name of the primary branch from the\n> remote repository even if it is unborn in 3d8314f8 (clone: propagate\n> empty remote HEAD even with other branches, 2022-07-07), lift the\n> code that records the object format out of the block executed only\n> when cloning from an instantiated repository, so that it works also\n> when cloning from an empty repository.\n\nYeah, this looks like the right thing to do. I know this did work\noriginally, at least for protocol v2, but I may have neglected to add a\ntest when I wrote it and it regressed. Thanks for the patch, which I\nthink is obviously correct, and adding a test for this case.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"474932","messageId":"2da9d073-874b-b2b4-8e4a-8ee2254c69b7@zombino.com","threadId":"59550","inReplyTo":"xmqq355euj2i.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-04-06T13:11:55Z","receivedAt":"2023-04-06T13:12:05Z","isPatch":false,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"On 4/5/23 23:15, Junio C Hamano wrote:\n> Subject: [PATCH] clone: propagate object-format when cloning from void\n> \n\nThank you, this fixes this issue.\n\n- Adam\n"},{"id":"476074","messageId":"ZEhHsJh20gtiDBd9@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"xmqq355euj2i.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-25T21:35:44Z","receivedAt":"2023-04-25T21:35:50Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-05 at 21:15:33, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> ----- >8 -----\n> Subject: [PATCH] clone: propagate object-format when cloning from void\n> \n> A user could prepare an empty repository and set it to use SHA256 as\n> the object format. The new repository created by \"git clone\" from\n> such a repository however would not record that it is expecting\n> objects in the same SHA256 format. This works as expected if the\n> source repository is not empty.\n> \n> Just like we started copying the name of the primary branch from the\n> remote repository even if it is unborn in 3d8314f8 (clone: propagate\n> empty remote HEAD even with other branches, 2022-07-07), lift the\n> code that records the object format out of the block executed only\n> when cloning from an instantiated repository, so that it works also\n> when cloning from an empty repository.\n> \n> Signed-off-by: Junio C Hamano \n\nSo it looks like this has made it into master and works for v2 but\nbreaks things for v0 and v1. I noticed because the Git LFS testsuite is\nbroken and the following test demonstrates it:\n\n----- %< -----\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 6c8d4c6cf1..3be5057579 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -244,6 +244,17 @@ test_expect_success 'push with ssh:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'clone propagates object-format from empty repo' '\n+\ttest_when_finished \"rm -fr src256 dst256\" &&\n+\n+\techo sha256 >expect &&\n+\tgit init --object-format=sha256 src256 &&\n+\tgit clone src256 dst256 &&\n+\tgit -C dst256 rev-parse --show-object-format >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n # Test protocol v1 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n----- %< -----\n\nIf nobody looks at this, I'll take a look tomorrow and hopefully send a\npatch. I just wanted to point this out to the list right away in the\ninterest of getting it noticed.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476075","messageId":"xmqqcz3rzjkg.fsf@gitster.g","threadId":"59550","inReplyTo":"ZEhHsJh20gtiDBd9@tapette.crustytoothpaste.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T22:24:47Z","receivedAt":"2023-04-25T22:24:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> So it looks like this has made it into master and works for v2 but\n> breaks things for v0 and v1. I noticed because the Git LFS testsuite is\n> broken and the following test demonstrates it:\n>\n> ----- %< -----\n> diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\n> index 6c8d4c6cf1..3be5057579 100755\n> --- a/t/t5700-protocol-v1.sh\n> +++ b/t/t5700-protocol-v1.sh\n> @@ -244,6 +244,17 @@ test_expect_success 'push with ssh:// using protocol v1' '\n> \tgrep \"push< version 1\" log\n> '\n> \n> +test_expect_success 'clone propagates object-format from empty repo' '\n> +\ttest_when_finished \"rm -fr src256 dst256\" &&\n> +\n> +\techo sha256 >expect &&\n> +\tgit init --object-format=sha256 src256 &&\n> +\tgit clone src256 dst256 &&\n> +\tgit -C dst256 rev-parse --show-object-format >actual &&\n> +\n> +\ttest_cmp expect actual\n> +'\n> +\n> # Test protocol v1 with 'http://' transport\n> #\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> ----- %< -----\n\nInteresting. I applied\n\n - the above addition to the t/t5700\n\n - reverse of 96f4113a (Merge branch 'jc/clone-object-format-from-void',\n 2023-04-11), but only the part outside t/\n\non top of today's 'master', and tried to run t5700 and t5702 (the\nlatter has new tests added by 96f4113a to protect the change in the\ntopic from future breakage for v2). It seems both t5700 and t5702\nfails.\n\nThe latter failing is very much expected; the code change reverted\nby the above experiment is what made it work in the first place.\n\nBut the former not working, after reverting the change, is totally\nunexpected---doesn't it mean that the change in question does not\nhave much to do in the breakage?\n\nIn fact, the new test to 5700 applied on top of v2.40.1 fails, too.\n\nOr are you complaining that the fix merged to 'master' only covers\nthe current protocol and not historical v0/v1 protocol? I couldn't\ntell, and didn't get that impression from the phrasing \"breaks\",\nimplying whatever that was \"broken\" used to be working.\n\nPuzzled.\n"},{"id":"476076","messageId":"xmqqcz3ry2sw.fsf@gitster.g","threadId":"59550","inReplyTo":"ZEhHsJh20gtiDBd9@tapette.crustytoothpaste.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T23:12:15Z","receivedAt":"2023-04-25T23:12:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> If nobody looks at this, I'll take a look tomorrow and hopefully send a\n> patch. I just wanted to point this out to the list right away in the\n> interest of getting it noticed.\n\nThanks. \n\nThe topic in question has been in 'master' for 2 weeks, since it was\nmerged at 96f4113a (Merge branch 'jc/clone-object-format-from-void',\n2023-04-11), and as I said, what your test demonstrates is not a\nregression caused by the topic but \"was broken, did not get\naddressed, is still broken\". So it does not sound like it needs\n\"right away\" kind of attention.\n\n> +test_expect_success 'clone propagates object-format from empty repo' '\n> +\ttest_when_finished \"rm -fr src256 dst256\" &&\n> +\n> +\techo sha256 >expect &&\n> +\tgit init --object-format=sha256 src256 &&\n> +\tgit clone src256 dst256 &&\n\nThis needs to be at least \"git clone --no-local\" for it to be v0/v1\nprotocol test. It is testing the local optimization codepath.\n\nWe could peek the original repository and copy the hash function\nname to the target as part of the local cloning. That obviously\nis outside the scope of the earlier fix that worked only at the\nprotocol level.\n\nAnd I think even with \"--no-local\" to make it about v0/v1 protocol,\nthe outcome is still pretty much expected. If we make the above\ncommand line to\n\n\tGIT_TRACE_PACKET=1 \\\n\tgit clone --no-local src256 dst256 &&\n\nto clone over the on-the-wire protocol, then we see\n\n Cloning into 'dst256'...\n packet: upload-pack> 0000\n packet: clone< 0000\n warning: You appear to have cloned an empty repository.\n packet: clone> 0000\n packet: upload-pack< 0000\n --- expect 2023-04-25 22:55:39.771850195 +0000\n ...\n\nin the output of \"sh t5700-*.sh -i -v\". Without any ref, v0/v1 can\nnot carry any capabilities, because there is no ref information to\ntuck the capabilities on.\n\nI unfortunately doubt that any solution would exist that does not\nbreak compatibility with the deployed clients that expect the\ncurrent v0/v1.\n\nThanks.\n"},{"id":"476080","messageId":"ZEhuMML6n8F+cNLg@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"xmqqcz3ry2sw.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T00:20:00Z","receivedAt":"2023-04-26T00:20:06Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-25 at 23:12:15, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > If nobody looks at this, I'll take a look tomorrow and hopefully send a\n> > patch. I just wanted to point this out to the list right away in the\n> > interest of getting it noticed.\n> \n> Thanks. \n> \n> The topic in question has been in 'master' for 2 weeks, since it was\n> merged at 96f4113a (Merge branch 'jc/clone-object-format-from-void',\n> 2023-04-11), and as I said, what your test demonstrates is not a\n> regression caused by the topic but \"was broken, did not get\n> addressed, is still broken\". So it does not sound like it needs\n> \"right away\" kind of attention.\n\nIn my case, the clone is over HTTP, so this may not be the ideal way to\nreproduce it and it may need a better testcase, but it does bisect to\nthe patch above and it is new in master (and doesn't reproduce in\n2.40.0). Note that in our case in the Git LFS testsuite, we're using\nGIT_DEFAULT_HASH=sha256.\n\nI believe what is happening is that for some reason, the object-format\ndata in v0 and v1 is not being read properly, and so we're now setting\nit to sha1 whereas before we were reading the value from the default\nsetting of the repository (sha256).\n\nIt very well may be that it's always been broken and this has just made\nit obvious that it's broken, but I'll look tomorrow and probably send a\npatch. I don't think we should revert this change, but I do think we\nneed to fix it before 2.41, since I think it means right now that all\nclones over protocol v0 and v1 end up with a SHA-1 repository.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476095","messageId":"20230426105134.GA130148@coredump.intra.peff.net","threadId":"59550","inReplyTo":"xmqqcz3ry2sw.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-26T10:51:34Z","receivedAt":"2023-04-26T10:53:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 25, 2023 at 04:12:15PM -0700, Junio C Hamano wrote:\n\n> And I think even with \"--no-local\" to make it about v0/v1 protocol,\n> the outcome is still pretty much expected. If we make the above\n> command line to\n> \n> \tGIT_TRACE_PACKET=1 \\\n> \tgit clone --no-local src256 dst256 &&\n> \n> to clone over the on-the-wire protocol, then we see\n> \n> Cloning into 'dst256'...\n> packet: upload-pack> 0000\n> packet: clone< 0000\n> warning: You appear to have cloned an empty repository.\n> packet: clone> 0000\n> packet: upload-pack< 0000\n> --- expect 2023-04-25 22:55:39.771850195 +0000\n> ...\n> \n> in the output of \"sh t5700-*.sh -i -v\". Without any ref, v0/v1 can\n> not carry any capabilities, because there is no ref information to\n> tuck the capabilities on.\n> \n> I unfortunately doubt that any solution would exist that does not\n> break compatibility with the deployed clients that expect the\n> current v0/v1.\n\nWe could send a capabilities^{} line, which Git has supported on the\nclient side since eb398797cd (connect: advertized capability is not a\nref, 2016-09-09). So sending it should not break even old clients\n(though we would have to check what alternate implementations like\nlibgit2 or dulwich do; we know JGit supports it).\n\nHowever, the object-format support here was broken until the very recent\n13e67aa39b (v0 protocol: fix sha1/sha256 confusion for capabilities^{},\n2023-04-14), so it would only be useful going forward (before then we'd\ndie(), but maybe that is preferable to having the wrong object format?).\n\nI'm not sure it's worth the effort, though. If you want to use sha256\neverywhere and tell the other side about it, you need a modern client\nanyway, and that means the ability to speak v2. So this would only\nmatter if for some reason the v2 probe was being ignored (e.g., proxies\neating it, ssh refusing environment variable, etc), which itself are\nthings that ideally would be fixed (and can maybe one day even go away\nif we optimistically default to v2).\n\n-Peff\n"},{"id":"476099","messageId":"20230426112508.GB130148@coredump.intra.peff.net","threadId":"59550","inReplyTo":"ZEhuMML6n8F+cNLg@tapette.crustytoothpaste.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-26T11:25:08Z","receivedAt":"2023-04-26T11:25:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2023 at 12:20:00AM +0000, brian m. carlson wrote:\n\n> In my case, the clone is over HTTP, so this may not be the ideal way to\n> reproduce it and it may need a better testcase, but it does bisect to\n> the patch above and it is new in master (and doesn't reproduce in\n> 2.40.0). Note that in our case in the Git LFS testsuite, we're using\n> GIT_DEFAULT_HASH=sha256.\n> \n> I believe what is happening is that for some reason, the object-format\n> data in v0 and v1 is not being read properly, and so we're now setting\n> it to sha1 whereas before we were reading the value from the default\n> setting of the repository (sha256).\n\nI'm having trouble finding any breakage at all. E.g., this test passes:\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 0908534f25..95b10288e7 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -704,4 +704,24 @@ test_expect_success 'no empty path components' '\n \t! grep \"//\" log\n '\n \n+test_expect_success 'v0 clone over http recognizes object-format' '\n+\tgit init --bare --object-format=sha256 \\\n+\t\t\"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\n+\t# do not test an empty repo. In v0, we have no way for an\n+\t# empty server to report its object format, so we would\n+\t# always default to sha1. We could in theory test that\n+\t# a client who wants to default to sha256 will realize\n+\t# the other side is sha1, but we have no way to set that local\n+\t# default. Unlike git-init, git-clone does not support\n+\t# --object-format, nor GIT_DEFAULT_HASH.\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" --work-tree=. \\\n+\t\tcommit --allow-empty -m foo &&\n+\n+\tgit -c protocol.version=0 clone $HTTPD_URL/smart/sha256.git sha256 &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\techo sha256 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nI'd expect it to break in the empty-repo case, for the reasons given in\nthe comment (v0 cannot communicate object-format in an empty repo). But\nthat is nothing new.\n\nIt sounds from your description that your test is running in a mode\nwhere the client defaults to sha256 (though I'm not sure how, since we\nexplicitly document that GIT_DEFAULT_HASH should not affect clone), and\nthen you clone an empty sha256 repository via v0, expecting the result\nto be sha256.\n\nBut I think that is a wrong expectation, at least from the\nclient's perspective. An empty repository cannot communicate its\nobject-format over v0, so the client should assume its v0, and should\nthen itself become v0. And that last \"should itself become\" is what\nJunio's patch fixed.\n\nThe first part, \"empty repository cannot communicate its object-format\nover v0\" is the part is \"it's always been broken\". We could fix it, but\nI'm not sure if it is worth the trouble (see my other message).\n\n> It very well may be that it's always been broken and this has just made\n> it obvious that it's broken, but I'll look tomorrow and probably send a\n> patch. I don't think we should revert this change, but I do think we\n> need to fix it before 2.41, since I think it means right now that all\n> clones over protocol v0 and v1 end up with a SHA-1 repository.\n\nHopefully my guess at what your test is doing is correct, and I didn't\njust leave us off on a tangent. ;)\n\nBut if it is, then I think that everything in Git is OK. Non-empty repos\nover v0 work correctly both before and after Junio's patch. Empty ones\nbefore his patch were erroneously using sha256 if they preferred it\nlocally, even when the other side really was sha1. They _also_ were\nusing sha256 erroneously when the other side was sha256 but wasn't able\nto report it (because of v0 limitations). Which is counter-intuitive,\nperhaps, but was still the wrong thing for a client to do.\n\n-Peff\n"},{"id":"476111","messageId":"xmqqcz3qwuj7.fsf@gitster.g","threadId":"59550","inReplyTo":"20230426112508.GB130148@coredump.intra.peff.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T15:08:28Z","receivedAt":"2023-04-26T15:08:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It sounds from your description that your test is running in a mode\n> where the client defaults to sha256 (though I'm not sure how, since we\n> explicitly document that GIT_DEFAULT_HASH should not affect clone), and\n> then you clone an empty sha256 repository via v0, expecting the result\n> to be sha256.\n\nThanks for coming up with an excellent guess that helped come up\nwith a reproduction.\n\nWith Brian's patch a bit tweaked (attached below), the test does\nfail with the current 'master' and passes before the merge in\nquestion. And the trace clearly shows that without being told\nanything about the object format via the capability, the client\nchooses to honor GIT_DEFAULT_HASH to initialize the new repository\nwith sha256.\n\nThere is this entry in the release notes of 2.29.0:\n\n * \"git clone\" that clones from SHA-1 repository, while\n GIT_DEFAULT_HASH set to use SHA-256 already, resulted in an\n unusable repository that half-claims to be SHA-256 repository\n with SHA-1 objects and refs. This has been corrected.\n\nThis refers to 47ac9703 (builtin/clone: avoid failure with\nGIT_DEFAULT_HASH, 2020-09-20). It seems that the \"fix\" described\nthere was incomplete and did not address \"clone a void\" case, and\nthe patch under discussion finished the incomplete fix without\nknowing by accident. The test that was added by 47ac9703 to t5601\ndoes use non-empty repositories (one with SHA-1, the other with\nSHA-256) and tries to check the interaction with \"clone\" with the\nenvironment variable. With the patch under discussion, this test\ndid not break and \"clone\" is still ignoring the GIT_DEFAULT_HASH\nvariable.\n\nAlso the description in the document of GIT_DEFAULT_HASH makes\nreaders ambivalent:\n\n`GIT_DEFAULT_HASH`::\n\tIf this variable is set, the default hash algorithm for new\n\trepositories will be set to this value. This value is currently\n\tignored when cloning; the setting of the remote repository\n\tis used instead. The default is \"sha1\". THIS VARIABLE IS\n\tEXPERIMENTAL! See `--object-format` in linkgit:git-init[1].\n\nTo me, \"is currently ignored\" hints that the author, or somebody\nclose to the author, of this entry feels that it is a bug that\n\"clone\" does not honor it. The log message of 47ac9703 also phrases\n\"failed to honor the GIT_DEFAULT_HASH\" as if it were a bad thing [*].\n\nOn the other hand, it is clearly documented as experimental so\nanything that has been relying on the behaviour of any command with\na particular value set to this variable are waiting to be broken.\n\nI am torn about this. Even though we may have been very clear that\nGIT_DEFAULT_HASH should not kick in in this case, \"clone\" was buggy\n(in the sense that it did not behave as documented in an empty\nrepository) and whatever thing that changed behaviour that Brian\nnoticed was ignoring the documentation and taking advantage of that\n\"bug\", and Hyrum's law ensues.\n\nIn the longer term, after/when we allow incremental/over-the-wire\nmigration of object-format, i.e. clone from SHA-1 repository to\ncreate SHA-256 repository (or vice versa) and fetching and pushing\nbetween them would bidirectionally convert the object format on the\nfly, it is likely we would teach a new option \"--object-format\" to\n\"git clone\" to say \"you would use whatever object format the origin\nuses by default, but this time, I am telling you to use this format\non our side, doing on-the-fly object format conversion as needed\".\n\nSo it would be OK to make sure that GIT_DEFAULT_HASH is ignored by\n\"clone\" as documented now, and even after on-the-fly object-format\nmigration is implemented, I would think.\n\n\n[Footnote]\n\n* ... but I think it was misguided. What it says there is this:\n\n And we also don't want to initialize the repository as SHA-1\n initially, since that means if we're cloning an empty\n repository, we'll have failed to honor the GIT_DEFAULT_HASH\n variable and will end up with a SHA-1 repository, not a SHA-256\n repository.\n\nIf this were \"When we're cloning an empty repository, we'd have\nfailed to honor the object format the other side has chosen and will\nend up with a SHA-1 repository, not a SHA-256 repository.\", then it\nis very much in line with the reality before the patch under\ndiscussion and also in line with the official stance that \"clone\"\nshould not honor GIT_DEFAULT_HASH.\n\nWhere the original description breaks down is when the other side is\nSHA-1 and this side has GIT_DEFAULT_HASH set to SHA-256. If we\nhonored the variable, we'd create a SHA-256 repository that will\ntalk to SHA-1 repository before the rest of the system is ready.\n\n t/t5700-protocol-v1.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git i/t/t5700-protocol-v1.sh w/t/t5700-protocol-v1.sh\nindex 6c8d4c6cf1..2f39dd2e05 100755\n--- i/t/t5700-protocol-v1.sh\n+++ w/t/t5700-protocol-v1.sh\n@@ -244,6 +244,19 @@ test_expect_success 'push with ssh:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'clone propagates object-format from empty repo' '\n+\ttest_when_finished \"rm -fr src256 dst256\" &&\n+\n+\techo sha256 >expect &&\n+\tgit init --object-format=sha256 src256 &&\n+\tGIT_DEFAULT_HASH=sha256 \\\n+\tGIT_TRACE_PACKET=1 \\\n+\tgit clone --no-local src256 dst256 &&\n+\tgit -C dst256 rev-parse --show-object-format >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n # Test protocol v1 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n"},{"id":"476112","messageId":"xmqqzg6uvfpo.fsf_-_@gitster.g","threadId":"59550","inReplyTo":"xmqqcz3qwuj7.fsf@gitster.g","subject":"[PATCH] doc: GIT_DEFAULT_HASH is and will be ignored during \"clone\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T15:13:55Z","receivedAt":"2023-04-26T15:14:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The phrasing \"is currently ignored\" was prone to be misinterpreted\nas if we were wishing if it were honored. Rephrase it to make it\nclear that the experimental variable will be ignored.\n\nIn the longer term, after/when we allow incremental/over-the-wire\nmigration of the object-format, i.e. cloning from an SHA-1\nrepository to create an SHA-256 repository (or vice versa) and\nfetching and pushing between them would bidirectionally convert the\nobject format on the fly, it is likely that we would teach a new\noption \"--object-format\" to \"git clone\" to say \"you would use\nwhatever object format the origin uses by default, but this time, I\nam telling you to use this format on our side, doing on-the-fly\nobject format conversion as needed\". So it is perfectly OK to\nignore the settings of this experimental variable, even after such\nan extension happens that makes it necessary for us to have a way to\ncreate a new repository that uses different object format from the\norigin repository.\n\nSigned-off-by: Junio C Hamano \n---\n\n * An obvious follow-up to the previous discussion.\n\n Documentation/git.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git c/Documentation/git.txt w/Documentation/git.txt\nindex 74973d3cc4..54b043899f 100644\n--- c/Documentation/git.txt\n+++ w/Documentation/git.txt\n@@ -546,9 +546,9 @@ double-quotes and respecting backslash escapes. E.g., the value\n \n `GIT_DEFAULT_HASH`::\n \tIf this variable is set, the default hash algorithm for new\n-\trepositories will be set to this value. This value is currently\n-\tignored when cloning; the setting of the remote repository\n-\tis used instead. The default is \"sha1\". THIS VARIABLE IS\n+\trepositories will be set to this value. This value is\n+\tignored when cloning and the setting of the remote repository\n+\tis always used. The default is \"sha1\". THIS VARIABLE IS\n \tEXPERIMENTAL! See `--object-format` in linkgit:git-init[1].\n \n Git Commits\n"},{"id":"476115","messageId":"xmqq8reeveee.fsf@gitster.g","threadId":"59550","inReplyTo":"20230426105134.GA130148@coredump.intra.peff.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T15:42:17Z","receivedAt":"2023-04-26T15:42:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We could send a capabilities^{} line, which Git has supported on the\n> client side since eb398797cd (connect: advertized capability is not a\n> ref, 2016-09-09). So sending it should not break even old clients\n> (though we would have to check what alternate implementations like\n> libgit2 or dulwich do; we know JGit supports it).\n\nAh, I forgot all about that JGit workaround. Yes, we can exploit\nit, and any implementation that does not understand it correctly\n(including git before 13e67aa3 (v0 protocol: fix sha1/sha256\nconfusion for capabilities^{}, 2023-04-14)) does not work with JGit\nwhen cloning an empty repository anyway, so it is not all that bad.\n\nBut I tend to agree with your conclusion that it may be an update\nfor the sake of completeness to retrofit v0/v1 on the serving side.\nIt would not help any real-world use cases all that much.\n\nThanks.\n"},{"id":"476145","messageId":"ZEmMUFR7AJn+v7jV@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"20230426105134.GA130148@coredump.intra.peff.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T20:40:48Z","receivedAt":"2023-04-26T20:40:54Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-26 at 10:51:34, Jeff King wrote:\n> We could send a capabilities^{} line, which Git has supported on the\n> client side since eb398797cd (connect: advertized capability is not a\n> ref, 2016-09-09). So sending it should not break even old clients\n> (though we would have to check what alternate implementations like\n> libgit2 or dulwich do; we know JGit supports it).\n\nI have a patch which does exactly this, which I will be sending shortly.\nI've confirmed that libgit2 and JGit support it, which is unsurprising,\nsince all of the implementations, Git included, share the same code. In\naddition, this is the behaviour we document as supporting, so all\nimplementations should support it.\n\n> However, the object-format support here was broken until the very recent\n> 13e67aa39b (v0 protocol: fix sha1/sha256 confusion for capabilities^{},\n> 2023-04-14), so it would only be useful going forward (before then we'd\n> die(), but maybe that is preferable to having the wrong object format?).\n\nI think it's better to die than to silently have the wrong object\nformat, and it also prevents the problem if other clients using v0 or v1\n(which effectively have to be supported for compatibility, while v2 is\noptional) try to clone from a fixed server.\n\n> I'm not sure it's worth the effort, though. If you want to use sha256\n> everywhere and tell the other side about it, you need a modern client\n> anyway, and that means the ability to speak v2. So this would only\n> matter if for some reason the v2 probe was being ignored (e.g., proxies\n> eating it, ssh refusing environment variable, etc), which itself are\n> things that ideally would be fixed (and can maybe one day even go away\n> if we optimistically default to v2).\n\nUsing v2 everywhere is difficult because many SSH servers still don't\npass GIT_PROTOCOL by default, meaning that we're stuck with v0 and v1.\nIn retrospect, sending an environment variable here was not a great\ndecision, but we're stuck with it now.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476146","messageId":"20230426205324.326501-2-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"20230426205324.326501-1-sandals@crustytoothpaste.net","subject":"[PATCH 1/2] http: advertise capabilities when cloning empty repos","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T20:53:23Z","receivedAt":"2023-04-26T20:53:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" \n\nWhen cloning an empty repository, the HTTP protocol version 0 currently\noffers nothing but the header and flush packets for the /info/refs\nendpoint. This means that no capabilities are provided, so the client\nside doesn't know what capabilities are present.\n\nHowever, this does pose a problem when working with SHA-256\nrepositories, since we use the capabilities to know the remote side's\nobject format (hash algorithm). It used to be possible to set the\ncorrect algorithm with `GIT_DEFAULT_HASH` (which is what the Git LFS\ntestsuite did), but this no longer works as of 8b214c2e9d (\"clone:\npropagate object-format when cloning from void\", 2023-04-05), since\nthere we always read the hash algorithm from the remote. If there is no\nhash algorithm provided, we default to SHA-1 for backwards\ncompatibility.\n\nFortunately, the push version of the protocol already indicates a clue\nfor how to solve this. When the /info/refs endpoint is accessed for a\npush and the remote is empty, we include a dummy \"capabilities^{}\" ref\npointing to the all-zeros object ID. The protocol documentation already\nindicates this should _always_ be sent, even for fetches and clones, so\nlet's just do that, which means we'll properly announce the hash\nalgorithm as part of the capabilities. This just works with the\nexisting code because we share the same ref code for fetches and clones,\nand libgit2 does as well.\n\nSigned-off-by: brian m. carlson \n---\n t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n t/t5700-protocol-v1.sh | 21 +++++++++++++++++++--\n upload-pack.c | 4 ++++\n 3 files changed, 50 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 0908534f25..21b7767cbd 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -611,6 +611,33 @@ test_expect_success 'client falls back from v2 to v0 to match server' '\n \tgrep symref=HEAD:refs/heads/ trace\n '\n \n+test_expect_success 'create empty http-accessible SHA-256 repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\t git --bare init --object-format=sha256\n+\t)\n+'\n+\n+test_expect_success 'clone empty SHA-256 repository with protocol v2' '\n+\trm -fr sha256 &&\n+\techo sha256 >expected &&\n+\tgit -c protocol.version=2 clone \"$HTTPD_URL/smart/sha256.git\" &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp actual expected &&\n+\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'clone empty SHA-256 repository with protocol v0' '\n+\trm -fr sha256 &&\n+\techo sha256 >expected &&\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone \"$HTTPD_URL/smart/sha256.git\" &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp actual expected &&\n+\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'passing hostname resolution information works' '\n \tBOGUS_HOST=gitbogusexamplehost.invalid &&\n \tBOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 6c8d4c6cf1..3cd9db9012 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -249,10 +249,12 @@ test_expect_success 'push with ssh:// using protocol v1' '\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-test_expect_success 'create repo to be served by http:// transport' '\n+test_expect_success 'create repos to be served by http:// transport' '\n \tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n \tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" config http.receivepack true &&\n-\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one &&\n+\tgit init --object-format=sha256 \"$HTTPD_DOCUMENT_ROOT_PATH/sha256\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/sha256\" config http.receivepack true\n '\n \n test_expect_success 'clone with http:// using protocol v1' '\n@@ -269,6 +271,21 @@ test_expect_success 'clone with http:// using protocol v1' '\n \tgrep \"git< version 1\" log\n '\n \n+test_expect_success 'clone with http:// using protocol v1 with empty SHA-256 repo' '\n+\tGIT_TRACE_PACKET=1 GIT_TRACE_CURL=1 git -c protocol.version=1 \\\n+\t\tclone \"$HTTPD_URL/smart/sha256\" sha256 2>log &&\n+\n+\tcat log &&\n+\techo sha256 >expect &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Client requested to use protocol v1\n+\tgrep \"Git-Protocol: version=1\" log &&\n+\t# Server responded using protocol v1\n+\tgrep \"git< version 1\" log\n+'\n+\n test_expect_success 'fetch with http:// using protocol v1' '\n \ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" two &&\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 08633dc121..5ef9b162b6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -120,6 +120,7 @@ struct upload_pack_data {\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n \tunsigned advertise_sid : 1;\n+\tunsigned sent_capabilities : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -1240,6 +1241,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n+\t\tdata->sent_capabilities = 1;\n \t} else {\n \t\tpacket_fwrite_fmt(stdout, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n@@ -1379,6 +1381,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n \t\t\tdata.no_done = 1;\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\tif (!data.sent_capabilities && advertise_refs)\n+\t\t\tsend_ref(\"capabilities^{}\", null_oid(), 0, &data);\n \t\t/*\n \t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n \t\t * uses stdio.\n"},{"id":"476147","messageId":"20230426205324.326501-1-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"ZEmMUFR7AJn+v7jV@tapette.crustytoothpaste.net","subject":"[PATCH 0/2] Fix empty SHA-256 clones with v0 and v1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T20:53:22Z","receivedAt":"2023-04-26T20:53:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We recently fixed empty clones with SHA-256 over protocol v2 by\nhonouring the hash algorithm specified even when no refs are present.\nHowever, in doing so, we made it impossible to set up a v0 or v1\nrepository by cloning from an empty SHA-256 repository. In doing so, we\nalso broke the Git LFS testsuite for SHA-256 repositories.\n\nThis series consists of two patches. The first introduces the dummy\n`capabilities^{}` entry for fetches and clones from an empty repository\nfor v0 and v1, just as we do for clones. This is already supported by\nolder versions of Git, as well as libgit2 and JGit.\n\nThe second introduces some backwards compatibility to avoid regressing\nthe old behaviour of using GIT_DEFAULT_HASH to initialize the proper\nhash in this case. We add a flag to see if we explicitly obtained a\nhash algorithm from the remote side, and if not, we honour\nGIT_DEFAULT_HASH, as before.\n\nbrian m. carlson (2):\n http: advertise capabilities when cloning empty repos\n Honor GIT_DEFAULT_HASH for empty clones without remote algo\n\n Documentation/git.txt | 10 +++++++---\n builtin/clone.c | 8 +++++---\n connect.c | 5 ++++-\n pkt-line.h | 2 ++\n t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n t/t5700-protocol-v1.sh | 32 ++++++++++++++++++++++++++++++--\n transport-helper.c | 1 +\n transport.c | 14 ++++++++++++++\n transport.h | 14 ++++++++++++++\n upload-pack.c | 4 ++++\n 10 files changed, 108 insertions(+), 9 deletions(-)\n\n"},{"id":"476148","messageId":"20230426205324.326501-3-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"20230426205324.326501-1-sandals@crustytoothpaste.net","subject":"[PATCH 2/2] Honor GIT_DEFAULT_HASH for empty clones without remote algo","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T20:53:24Z","receivedAt":"2023-04-26T20:53:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" \n\nThe previous commit introduced a change that allows HTTP v0 and v1\noperations to determine the hash of an empty remote repository by\nsending capabilities. However, there are still some cases, such as when\ncloning locally, where the capabilities are not sent. This is because\nfor local operations, we don't strip out the fake \"capabilities^{}\" ref,\nand thus \"git ls-remote\" would produce incorrect values if we did.\n\nHowever, up until 8b214c2e9d (\"clone: propagate object-format when\ncloning from void\", 2023-04-05), we honored GIT_DEFAULT_HASH in this\ncase, so let's continue to do that. Check whether the hash algorithm\nwas explicitly set, and if so, continue to use that value. If not, use\nthe default value for GIT_DEFAULT_HASH to ensure that we can at least\nproperly configure an empty clone whose hash algorithm we know.\n\nNote that without this patch, git clone cannot create a SHA-256\nrepository from an empty remote without protocol v2 (except over HTTP,\nas in the previous patch).\n---\n Documentation/git.txt | 10 +++++++---\n builtin/clone.c | 8 +++++---\n connect.c | 5 ++++-\n pkt-line.h | 2 ++\n t/t5700-protocol-v1.sh | 11 +++++++++++\n transport-helper.c | 1 +\n transport.c | 14 ++++++++++++++\n transport.h | 14 ++++++++++++++\n 8 files changed, 58 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 74973d3cc4..48eda9f883 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -547,9 +547,13 @@ double-quotes and respecting backslash escapes. E.g., the value\n `GIT_DEFAULT_HASH`::\n \tIf this variable is set, the default hash algorithm for new\n \trepositories will be set to this value. This value is currently\n-\tignored when cloning; the setting of the remote repository\n-\tis used instead. The default is \"sha1\". THIS VARIABLE IS\n-\tEXPERIMENTAL! See `--object-format` in linkgit:git-init[1].\n+\tignored when cloning if the remote value can be definitively\n+\tdetermined; the setting of the remote repository is used\n+\tinstead. The value is honored if the remote repository's\n+\talgorithm cannot be determined, such as some cases when\n+\tthe remote repository is empty. The default is \"sha1\".\n+\tTHIS VARIABLE IS EXPERIMENTAL! See `--object-format`\n+\tin linkgit:git-init[1].\n \n Git Commits\n ~~~~~~~~~~~\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 186845ef0b..c207798de9 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1316,13 +1316,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (transport_get_hash_algo_explicit(transport)) {\n \t\t/*\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-\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n-\tinitialize_repository_version(hash_algo, 1);\n-\trepo_set_hash_algo(the_repository, hash_algo);\n+\t\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n+\t\tinitialize_repository_version(hash_algo, 1);\n+\t\trepo_set_hash_algo(the_repository, hash_algo);\n+\t}\n \n \tif (mapped_refs) {\n \t\t/*\ndiff --git a/connect.c b/connect.c\nindex 3a0186280c..40cb9bf261 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -243,8 +243,10 @@ static void process_capabilities(struct packet_reader *reader, int *linelen)\n \tif (feat_val) {\n \t\tchar *hash_name = xstrndup(feat_val, feat_len);\n \t\tint hash_algo = hash_algo_by_name(hash_name);\n-\t\tif (hash_algo != GIT_HASH_UNKNOWN)\n+\t\tif (hash_algo != GIT_HASH_UNKNOWN) {\n \t\t\treader->hash_algo = &hash_algos[hash_algo];\n+\t\t\treader->hash_algo_explicit = 1;\n+\t\t}\n \t\tfree(hash_name);\n \t} else {\n \t\treader->hash_algo = &hash_algos[GIT_HASH_SHA1];\n@@ -493,6 +495,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)\n \t\tif (hash_algo == GIT_HASH_UNKNOWN)\n \t\t\tdie(_(\"unknown object format '%s' specified by server\"), hash_name);\n \t\treader->hash_algo = &hash_algos[hash_algo];\n+\t\treader->hash_algo_explicit = 1;\n \t\tpacket_write_fmt(fd_out, \"object-format=%s\", reader->hash_algo->name);\n \t} else {\n \t\treader->hash_algo = &hash_algos[GIT_HASH_SHA1];\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 8e9846f315..10700a9d8c 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -190,6 +190,8 @@ struct packet_reader {\n \tint line_peeked;\n \n \tunsigned use_sideband : 1;\n+\t/* indicates if we saw an explicit capability */\n+\tunsigned hash_algo_explicit : 1;\n \tconst char *me;\n \n \t/* hash algorithm in use */\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 3cd9db9012..ad24c7fe64 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -244,6 +244,17 @@ test_expect_success 'push with ssh:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'clone propagates object-format from empty repo' '\n+\ttest_when_finished \"rm -fr src256 dst256\" &&\n+\n+\techo sha256 >expect &&\n+\tgit init --object-format=sha256 src256 &&\n+\tGIT_DEFAULT_HASH=sha256 git -c protocol.version=1 clone --no-local src256 dst256 &&\n+\tgit -C dst256 rev-parse --show-object-format >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n # Test protocol v1 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6b816940dc..c65cf7c620 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1236,6 +1236,7 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \t\t\t\t\tdie(_(\"unsupported object format '%s'\"),\n \t\t\t\t\t value);\n \t\t\t\ttransport->hash_algo = &hash_algos[algo];\n+\t\t\t\ttransport->hash_algo_explicit = 1;\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\ndiff --git a/transport.c b/transport.c\nindex 67afdae57c..7774487e8d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -147,6 +147,12 @@ static void get_refs_from_bundle_inner(struct transport *transport)\n \t\tdie(_(\"could not read bundle '%s'\"), transport->url);\n \n \ttransport->hash_algo = data->header.hash_algo;\n+\t/*\n+\t * This is always set, even if we didn't get an explicit object-format\n+\t * capability, since we know that a missing capability or a v2 bundle\n+\t * definitively indicates SHA-1.\n+\t */\n+\ttransport->hash_algo_explicit = 1;\n }\n \n static struct ref *get_refs_from_bundle(struct transport *transport,\n@@ -190,6 +196,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \tret = unbundle(the_repository, &data->header, data->fd,\n \t\t &extra_index_pack_args, 0);\n \ttransport->hash_algo = data->header.hash_algo;\n+\ttransport->hash_algo_explicit = 1;\n \treturn ret;\n }\n \n@@ -360,6 +367,7 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \t}\n \tdata->finished_handshake = 1;\n \ttransport->hash_algo = reader.hash_algo;\n+\ttransport->hash_algo_explicit = reader.hash_algo_explicit;\n \n \tif (reader.line_peeked)\n \t\tBUG(\"buffer must be empty at the end of handshake()\");\n@@ -1190,6 +1198,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t}\n \n \tret->hash_algo = &hash_algos[GIT_HASH_SHA1];\n+\tret->hash_algo_explicit = 0;\n \n \treturn ret;\n }\n@@ -1199,6 +1208,11 @@ const struct git_hash_algo *transport_get_hash_algo(struct transport *transport)\n \treturn transport->hash_algo;\n }\n \n+int transport_get_hash_algo_explicit(struct transport *transport)\n+{\n+\treturn transport->hash_algo_explicit;\n+}\n+\n int transport_set_option(struct transport *transport,\n \t\t\t const char *name, const char *value)\n {\ndiff --git a/transport.h b/transport.h\nindex 6393cd9823..ce67eefc58 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -128,6 +128,11 @@ struct transport {\n \t * in transport_set_verbosity().\n \t **/\n \tunsigned progress : 1;\n+\t/*\n+\t * Indicates whether the hash algorithm was initialized explicitly as\n+\t * opposed to using a fallback.\n+\t */\n+\tunsigned hash_algo_explicit : 1;\n \t/*\n \t * If transport is at least potentially smart, this points to\n \t * git_transport_options structure to use in case transport\n@@ -305,6 +310,15 @@ int transport_get_remote_bundle_uri(struct transport *transport);\n * This can only be called after fetching the remote refs.\n */\n const struct git_hash_algo *transport_get_hash_algo(struct transport *transport);\n+/*\n+ * Fetch whether the hash algorithm provided was explicitly set.\n+ *\n+ * If this value is false, \"transport_get_hash_algo\" will always return a value\n+ * of SHA-1, which is the default algorithm if none is specified.\n+ *\n+ * This can only be called after fetching the remote refs.\n+ */\n+int transport_get_hash_algo_explicit(struct transport *transport);\n int transport_fetch_refs(struct transport *transport, struct ref *refs);\n \n /*\n"},{"id":"476149","messageId":"ZEmSZmfUpIcZAM6c@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"xmqqzg6uvfpo.fsf_-_@gitster.g","subject":"Re: [PATCH] doc: GIT_DEFAULT_HASH is and will be ignored during \"clone\"","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T21:06:46Z","receivedAt":"2023-04-26T21:06:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-26 at 15:13:55, Junio C Hamano wrote:\n> The phrasing \"is currently ignored\" was prone to be misinterpreted\n> as if we were wishing if it were honored. Rephrase it to make it\n> clear that the experimental variable will be ignored.\n> \n> In the longer term, after/when we allow incremental/over-the-wire\n> migration of the object-format, i.e. cloning from an SHA-1\n> repository to create an SHA-256 repository (or vice versa) and\n> fetching and pushing between them would bidirectionally convert the\n> object format on the fly, it is likely that we would teach a new\n> option \"--object-format\" to \"git clone\" to say \"you would use\n> whatever object format the origin uses by default, but this time, I\n> am telling you to use this format on our side, doing on-the-fly\n> object format conversion as needed\". So it is perfectly OK to\n> ignore the settings of this experimental variable, even after such\n> an extension happens that makes it necessary for us to have a way to\n> create a new repository that uses different object format from the\n> origin repository.\n\nI have a different proposal which clarifies when it will and will not be\nhonoured in my series. I think we would want to honour this variable\nonce we have SHA-1 and SHA-256 interop, and can convert on the fly, so I\nthink keeping the \"currently\" here is a good idea.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476150","messageId":"xmqqsfcmqrer.fsf@gitster.g","threadId":"59550","inReplyTo":"20230426205324.326501-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH 0/2] Fix empty SHA-256 clones with v0 and v1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T21:12:28Z","receivedAt":"2023-04-26T21:12:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> The second introduces some backwards compatibility to avoid regressing\n> the old behaviour of using GIT_DEFAULT_HASH to initialize the proper\n> hash in this case. We add a flag to see if we explicitly obtained a\n> hash algorithm from the remote side, and if not, we honour\n> GIT_DEFAULT_HASH, as before.\n\nI am fairly negative on this half of the series. The first one is\nexcellent, though.\n\nTHanks.\n"},{"id":"476152","messageId":"xmqqo7naqrb6.fsf@gitster.g","threadId":"59550","inReplyTo":"20230426205324.326501-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T21:14:37Z","receivedAt":"2023-04-26T21:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> From: \"brian m. carlson\" \n>\n> When cloning an empty repository, the HTTP protocol version 0 currently\n> offers nothing but the header and flush packets for the /info/refs\n> endpoint. This means that no capabilities are provided, so the client\n> side doesn't know what capabilities are present.\n>\n> However, this does pose a problem when working with SHA-256\n> repositories, since we use the capabilities to know the remote side's\n> object format (hash algorithm). It used to be possible to set the\n> correct algorithm with `GIT_DEFAULT_HASH` (which is what the Git LFS\n> testsuite did), but this no longer works as of 8b214c2e9d (\"clone:\n\n\"this no longer works as of\" -> \"this was a mistake and was fixed by\".\n\n> propagate object-format when cloning from void\", 2023-04-05), since\n> there we always read the hash algorithm from the remote. If there is no\n> hash algorithm provided, we default to SHA-1 for backwards\n> compatibility.\n\nOther than that, looks good to me.\n\nThanks. Will queue.\n"},{"id":"476154","messageId":"xmqqjzxyqr4t.fsf@gitster.g","threadId":"59550","inReplyTo":"20230426205324.326501-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH 2/2] Honor GIT_DEFAULT_HASH for empty clones without remote algo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T21:18:26Z","receivedAt":"2023-04-26T21:18:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> However, up until 8b214c2e9d (\"clone: propagate object-format when\n> cloning from void\", 2023-04-05), we honored GIT_DEFAULT_HASH in this\n> case, so let's continue to do that.\n\nLet's not. Once we identified the bug of mistakenly honoring a\nwrong variable, let's fix it and keep it fixed.\n\nThe local optimization, if necessary, can be taught to peek the\nsource repository and propagating the object format selection to the\ndestination repository.\n"},{"id":"476155","messageId":"ZEmXf4m8Hoz0KyOX@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"xmqqo7naqrb6.fsf@gitster.g","subject":"Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-26T21:28:31Z","receivedAt":"2023-04-26T21:28:36Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-26 at 21:14:37, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > From: \"brian m. carlson\" \n> >\n> > When cloning an empty repository, the HTTP protocol version 0 currently\n> > offers nothing but the header and flush packets for the /info/refs\n> > endpoint. This means that no capabilities are provided, so the client\n> > side doesn't know what capabilities are present.\n> >\n> > However, this does pose a problem when working with SHA-256\n> > repositories, since we use the capabilities to know the remote side's\n> > object format (hash algorithm). It used to be possible to set the\n> > correct algorithm with `GIT_DEFAULT_HASH` (which is what the Git LFS\n> > testsuite did), but this no longer works as of 8b214c2e9d (\"clone:\n> \n> \"this no longer works as of\" -> \"this was a mistake and was fixed by\".\n\nI tend to disagree. While I agree that change is valuable because it\nfixes v2, which we want, it does cause a change in user-visible\nbehaviour, which broke the Git LFS testsuite. Whether we like things\nworking that way or not, clearly there were people relying on it.\n\nFortunately, in that case, Git LFS can just enable protocol v2 and\nthings work again, but I think \"this no longer works\" is accurate and\nmore neutral, and addresses the issue. We wouldn't have to deal with\nthat issue if we could gracefully handle git clone --local with older\nversions of the protocol, but one of the tests fails when we do that.\nI'll take some more time to see if I can come up with a nice way to\ngracefully handle that, and if so, I'll send a v2.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476156","messageId":"xmqqbkjaqqfp.fsf@gitster.g","threadId":"59550","inReplyTo":"20230426205324.326501-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH 2/2] Honor GIT_DEFAULT_HASH for empty clones without remote algo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-26T21:33:30Z","receivedAt":"2023-04-26T21:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> `GIT_DEFAULT_HASH`::\n> \tIf this variable is set, the default hash algorithm for new\n> \trepositories will be set to this value. This value is currently\n> +\tignored when cloning if the remote value can be definitively\n> +\tdetermined; the setting of the remote repository is used\n> +\tinstead. The value is honored if the remote repository's\n> +\talgorithm cannot be determined, such as some cases when\n> +\tthe remote repository is empty. The default is \"sha1\".\n> +\tTHIS VARIABLE IS EXPERIMENTAL! See `--object-format`\n> +\tin linkgit:git-init[1].\n\nWe'd need to evantually cover all the transports (and non-transport\nlike the \"--local\" optimization) so that the object-format and other\nchoices are communicated from the origin to a new clone anyway, so\nthis extra complexity \"until X is fixed, it behaves this way, but\notherwise the variable is read in the meantime\" may be a disservice\nto the end users, even though it may make it easier in the shorter\nterm for maintainers of programs that rely on the buggy \"git clone\"\nthat partially honored this environment variable.\n\nIn short, I am still not convinced that the above is a good design\nchoice in the longer term.\n\nThanks.\n"},{"id":"476163","messageId":"20230427044633.GA982277@coredump.intra.peff.net","threadId":"59550","inReplyTo":"xmqqcz3qwuj7.fsf@gitster.g","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-27T04:46:33Z","receivedAt":"2023-04-27T04:46:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2023 at 08:08:28AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It sounds from your description that your test is running in a mode\n> > where the client defaults to sha256 (though I'm not sure how, since we\n> > explicitly document that GIT_DEFAULT_HASH should not affect clone), and\n> > then you clone an empty sha256 repository via v0, expecting the result\n> > to be sha256.\n> \n> Thanks for coming up with an excellent guess that helped come up\n> with a reproduction.\n> \n> With Brian's patch a bit tweaked (attached below), the test does\n> fail with the current 'master' and passes before the merge in\n> question. And the trace clearly shows that without being told\n> anything about the object format via the capability, the client\n> chooses to honor GIT_DEFAULT_HASH to initialize the new repository\n> with sha256.\n\nAh, OK. The fact that we do respect GIT_DEFAULT_HASH there solves my\nremaining confusion. And in retrospect it makes sense, because clone is\ngoing to use the equivalent of \"git init\" under the hood. So we'll start\nwith _something_, and then adjust it based on what we read from the\nother side. And that \"something\" respects GIT_DEFAULT_HASH.\n\nSort of a side note:\n\n This behavior is somewhat due to the fact that clone is implemented as\n \"init, then fetch into the new repository\". It could also conceptually\n be \"talk to the remote, then init, then actually fetch\". It never\n mattered before, but with options like object-format, the \"init\" step\n may be affected by things the remote said.\n\n So in our world, clone has to \"fix up\" parts of the repository that\n were initialized by tweaking the object-format config of the live\n repo. In a world where the very first thing it did was talk to the\n remote, then it would pass the option along to init in the first\n place.\n\n It's probably not worth trying to re-architect clone, though (not just\n in terms of work, but who knows what other subtle assumptions are\n baked into the current ordering). And it's not like it _solves_ this\n issue. It just might have made finding it a little less confusing.\n\n> [Footnote]\n> \n> * ... but I think it was misguided. What it says there is this:\n> \n> And we also don't want to initialize the repository as SHA-1\n> initially, since that means if we're cloning an empty\n> repository, we'll have failed to honor the GIT_DEFAULT_HASH\n> variable and will end up with a SHA-1 repository, not a SHA-256\n> repository.\n> \n> If this were \"When we're cloning an empty repository, we'd have\n> failed to honor the object format the other side has chosen and will\n> end up with a SHA-1 repository, not a SHA-256 repository.\", then it\n> is very much in line with the reality before the patch under\n> discussion and also in line with the official stance that \"clone\"\n> should not honor GIT_DEFAULT_HASH.\n> \n> Where the original description breaks down is when the other side is\n> SHA-1 and this side has GIT_DEFAULT_HASH set to SHA-256. If we\n> honored the variable, we'd create a SHA-256 repository that will\n> talk to SHA-1 repository before the rest of the system is ready.\n\nExactly. We are (and always have been) wrong 50% of the time in an empty\nrepo for v0. Your recent patch fixed v2 in an empty repo, but in doing\nso flipped which halves of v0 were wrong.\n\n-Peff\n"},{"id":"476164","messageId":"20230427045643.GB982277@coredump.intra.peff.net","threadId":"59550","inReplyTo":"ZEmMUFR7AJn+v7jV@tapette.crustytoothpaste.net","subject":"Re: git clone of empty repositories doesn't preserve hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-27T04:56:43Z","receivedAt":"2023-04-27T04:56:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2023 at 08:40:48PM +0000, brian m. carlson wrote:\n\n> On 2023-04-26 at 10:51:34, Jeff King wrote:\n> > We could send a capabilities^{} line, which Git has supported on the\n> > client side since eb398797cd (connect: advertized capability is not a\n> > ref, 2016-09-09). So sending it should not break even old clients\n> > (though we would have to check what alternate implementations like\n> > libgit2 or dulwich do; we know JGit supports it).\n> \n> I have a patch which does exactly this, which I will be sending shortly.\n> I've confirmed that libgit2 and JGit support it, which is unsurprising,\n> since all of the implementations, Git included, share the same code. In\n> addition, this is the behaviour we document as supporting, so all\n> implementations should support it.\n\nYeah, I was worried about how accurate that \"should\" is. :)\n\nSince you checked the others, I peeked at dulwich's code, and it appears\nto support it, too (I didn't actually run a test, but the code is pretty\nclear). It doesn't support sha256 at all, but that's OK. What I'd be\nconcerned about is breaking clients when there is no useful capability\nto advertise (though of course we could decide to send capabilities^{}\nonly when there is something useful to say).\n\n> > However, the object-format support here was broken until the very recent\n> > 13e67aa39b (v0 protocol: fix sha1/sha256 confusion for capabilities^{},\n> > 2023-04-14), so it would only be useful going forward (before then we'd\n> > die(), but maybe that is preferable to having the wrong object format?).\n> \n> I think it's better to die than to silently have the wrong object\n> format, and it also prevents the problem if other clients using v0 or v1\n> (which effectively have to be supported for compatibility, while v2 is\n> optional) try to clone from a fixed server.\n\nOK, good. So we can ignore that recently fixed bug (which only affects\nthe case where older versions are cloning something whose hash format\ndoes not match theirs).\n\n> > I'm not sure it's worth the effort, though. If you want to use sha256\n> > everywhere and tell the other side about it, you need a modern client\n> > anyway, and that means the ability to speak v2. So this would only\n> > matter if for some reason the v2 probe was being ignored (e.g., proxies\n> > eating it, ssh refusing environment variable, etc), which itself are\n> > things that ideally would be fixed (and can maybe one day even go away\n> > if we optimistically default to v2).\n> \n> Using v2 everywhere is difficult because many SSH servers still don't\n> pass GIT_PROTOCOL by default, meaning that we're stuck with v0 and v1.\n> In retrospect, sending an environment variable here was not a great\n> decision, but we're stuck with it now.\n\nTrue (I even got bit by this at one point, not realizing that I wasn't\nusing v2 with one of my personal servers). I certainly don't have an\nobjection if you're willing to do the work.\n\nIt would also allow us to fix the long-broken \"unborn HEAD\" problem for\nv0 in an empty repo (though I am also fine if we don't go that far).\n\n-Peff\n"},{"id":"476165","messageId":"20230427050047.GC982277@coredump.intra.peff.net","threadId":"59550","inReplyTo":"ZEmXf4m8Hoz0KyOX@tapette.crustytoothpaste.net","subject":"Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-27T05:00:47Z","receivedAt":"2023-04-27T05:00:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2023 at 09:28:31PM +0000, brian m. carlson wrote:\n\n> > > However, this does pose a problem when working with SHA-256\n> > > repositories, since we use the capabilities to know the remote side's\n> > > object format (hash algorithm). It used to be possible to set the\n> > > correct algorithm with `GIT_DEFAULT_HASH` (which is what the Git LFS\n> > > testsuite did), but this no longer works as of 8b214c2e9d (\"clone:\n> > \n> > \"this no longer works as of\" -> \"this was a mistake and was fixed by\".\n> \n> I tend to disagree. While I agree that change is valuable because it\n> fixes v2, which we want, it does cause a change in user-visible\n> behaviour, which broke the Git LFS testsuite. Whether we like things\n> working that way or not, clearly there were people relying on it.\n> \n> Fortunately, in that case, Git LFS can just enable protocol v2 and\n> things work again, but I think \"this no longer works\" is accurate and\n> more neutral, and addresses the issue. We wouldn't have to deal with\n> that issue if we could gracefully handle git clone --local with older\n> versions of the protocol, but one of the tests fails when we do that.\n> I'll take some more time to see if I can come up with a nice way to\n> gracefully handle that, and if so, I'll send a v2.\n\nReiterating what I said upthread, I think it was always 50% broken.\nTaking the local hash format over the remote one was always the wrong\nthing to do, but it sometimes worked out (because we happened to match\nthe remote).\n\nBut the opposite case:\n\n git init --object-format=sha1 dst.git\n GIT_DEFAULT_HASH=sha256 git clone dst.git\n\nwould previously have done the wrong thing. We just flipped which half\nwas broken and which was not.\n\n-Peff\n"},{"id":"476167","messageId":"20230427053016.GD982277@coredump.intra.peff.net","threadId":"59550","inReplyTo":"20230426205324.326501-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-27T05:30:16Z","receivedAt":"2023-04-27T05:30:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2023 at 08:53:23PM +0000, brian m. carlson wrote:\n\n> From: \"brian m. carlson\" \n> \n> When cloning an empty repository, the HTTP protocol version 0 currently\n> offers nothing but the header and flush packets for the /info/refs\n> endpoint. This means that no capabilities are provided, so the client\n> side doesn't know what capabilities are present.\n\nIs this really an HTTP problem?\n\nIf I do:\n\n git init --bare --object-format=sha256 remote.git\n git -c protocol.version=0 clone --bare remote.git local.git\n git -C local.git rev-parse --show-object-format\n\nI will get sha1, which is wrong. Likewise with GIT_DEFAULT_HASH=sha256\non the clone (after Junio's recent patch), regardless of what the server\nclaims. This is really a git-protocol issue that affects all transports.\n\nSo I think in this hunk:\n\n> @@ -1379,6 +1381,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n> \t\t\tdata.no_done = 1;\n> \t\thead_ref_namespaced(send_ref, &data);\n> \t\tfor_each_namespaced_ref(send_ref, &data);\n> +\t\tif (!data.sent_capabilities && advertise_refs)\n> +\t\t\tsend_ref(\"capabilities^{}\", null_oid(), 0, &data);\n> \t\t/*\n> \t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n> \t\t * uses stdio.\n\nyou would want to drop the \"&& advertise_refs\" bit, after which both of\nthe cases above would yield a sha256 repository.\n\nThere is one other catch, though. Doing as I suggest results in a\nfailure in t5509, because the new code does not interact correctly with\nnamespaces. That is true of your version, as well; it's just that the\ntest suite does not cover the combination of namespaces, http, and empty\nrepos.\n\nThe issue is that send_ref() will try to strip the namespace, and end up\nwith NULL (which on my glibc system ends up with a ref named \"(null)\",\nbut obviously could segfault, too).\n\nSomething like this fixes it:\n\ndiff --git a/environment.c b/environment.c\nindex 8a96997539..37cd66b295 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -234,6 +234,8 @@ const char *get_git_namespace(void)\n const char *strip_namespace(const char *namespaced_ref)\n {\n \tconst char *out;\n+\tif (!strcmp(namespaced_ref, \"capabilities^{}\"))\n+\t\treturn namespaced_ref; /* magic ref */\n \tif (skip_prefix(namespaced_ref, get_git_namespace(), &out))\n \t\treturn out;\n \treturn NULL;\n\nbut I suspect it would be cleaner to refactor send_ref() to allow\nsending a name more directly.\n\n(As an aside, it feels like send_ref() is also wrong not to check for\nNULL from strip_namespace(), but I guess in practice we do not feed\nit names outside of the namespace. Might be a good candidate for a BUG()\ncheck or other assertion).\n\n> +test_expect_success 'clone empty SHA-256 repository with protocol v0' '\n> +\trm -fr sha256 &&\n> +\techo sha256 >expected &&\n> +\tGIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone \"$HTTPD_URL/smart/sha256.git\" &&\n> +\tgit -C sha256 rev-parse --show-object-format >actual &&\n> +\ttest_cmp actual expected &&\n> +\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n> +\ttest_must_be_empty actual\n> +'\n\nThis looks reasonable, though I think if we do not need HTTP to\ndemonstrate the issue (and I don't think we do), then we should probably\navoid it, just to get test coverage on platforms that don't support\nHTTP.\n\n-Peff\n"},{"id":"476168","messageId":"20230427054343.GE982277@coredump.intra.peff.net","threadId":"59550","inReplyTo":"xmqqbkjaqqfp.fsf@gitster.g","subject":"Re: [PATCH 2/2] Honor GIT_DEFAULT_HASH for empty clones without remote algo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-27T05:43:43Z","receivedAt":"2023-04-27T05:43:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2023 at 02:33:30PM -0700, Junio C Hamano wrote:\n\n> \"brian m. carlson\" writes:\n> \n> > `GIT_DEFAULT_HASH`::\n> > \tIf this variable is set, the default hash algorithm for new\n> > \trepositories will be set to this value. This value is currently\n> > +\tignored when cloning if the remote value can be definitively\n> > +\tdetermined; the setting of the remote repository is used\n> > +\tinstead. The value is honored if the remote repository's\n> > +\talgorithm cannot be determined, such as some cases when\n> > +\tthe remote repository is empty. The default is \"sha1\".\n> > +\tTHIS VARIABLE IS EXPERIMENTAL! See `--object-format`\n> > +\tin linkgit:git-init[1].\n> \n> We'd need to evantually cover all the transports (and non-transport\n> like the \"--local\" optimization) so that the object-format and other\n> choices are communicated from the origin to a new clone anyway, so\n> this extra complexity \"until X is fixed, it behaves this way, but\n> otherwise the variable is read in the meantime\" may be a disservice\n> to the end users, even though it may make it easier in the shorter\n> term for maintainers of programs that rely on the buggy \"git clone\"\n> that partially honored this environment variable.\n> \n> In short, I am still not convinced that the above is a good design\n> choice in the longer term.\n\nI also think it is working against the backwards-compatible design of\nthe hash function transition. If we do not see an object-format line\nfrom the remote, then either:\n\n 1. They sent us capabilities, but it did not include object-format. So\n if we are in GIT_DEFAULT_HASH=sha256 mode locally, but the other\n side is an older version of Git (or even a current version of other\n implementations, like Dulwich) that do not send object-format at\n all, then we will not correctly fall back to assuming they are\n sha1. In a non-empty repo, this means we'll fail to parse their ref\n advertisement (we'll expect sha256 hashes but get sha1), and\n cloning will be broken.\n\n 2. They did not send us capabilities, because the repo is empty (and\n the server does not have brian's patch 1). The hash transition doc\n says we're supposed to assume they're sha1. It's _sort of_\n academic, in that they also are not telling us about any refs on\n their side. But we may end up mis-matched with the server (again,\n this is the 50/50 thing; we don't know what their format is).\n Presumably that bites us later when we try to push up new objects\n (but would eventually work when we support interop).\n\nI think handling (2) is iffy as a goal, but the collateral damage of (1)\nis a complete show-stopper for this patch. If we wanted to do (2) by\nitself, we'd have to distinguish \"did they even send us a capabilities\nline\" as a separate case (but I tend to agree with you that it is not\nworth doing for now).\n\n-Peff\n"},{"id":"476232","messageId":"xmqq8redjbyg.fsf@gitster.g","threadId":"59550","inReplyTo":"20230427053016.GD982277@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] http: advertise capabilities when cloning empty repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-27T20:40:23Z","receivedAt":"2023-04-27T20:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think in this hunk:\n>\n>> @@ -1379,6 +1381,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n>> \t\t\tdata.no_done = 1;\n>> \t\thead_ref_namespaced(send_ref, &data);\n>> \t\tfor_each_namespaced_ref(send_ref, &data);\n>> +\t\tif (!data.sent_capabilities && advertise_refs)\n>> +\t\t\tsend_ref(\"capabilities^{}\", null_oid(), 0, &data);\n>> \t\t/*\n>> \t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n>> \t\t * uses stdio.\n>\n> you would want to drop the \"&& advertise_refs\" bit, after which both of\n> the cases above would yield a sha256 repository.\n\nGood suggestion.\n\n>> +test_expect_success 'clone empty SHA-256 repository with protocol v0' '\n>> +\trm -fr sha256 &&\n>> +\techo sha256 >expected &&\n>> +\tGIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone \"$HTTPD_URL/smart/sha256.git\" &&\n>> +\tgit -C sha256 rev-parse --show-object-format >actual &&\n>> +\ttest_cmp actual expected &&\n>> +\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n>> +\ttest_must_be_empty actual\n>> +'\n>\n> This looks reasonable, though I think if we do not need HTTP to\n> demonstrate the issue (and I don't think we do), then we should probably\n> avoid it, just to get test coverage on platforms that don't support\n> HTTP.\n\nHTTP tests tend to be more cumbersome to set up and harder to debug\nthan the plain vanilla \"over the pipe on the same machine\"\ntransport, so I tend to agree with the statement.\n\nThey however represent a more common use case, so having HTTP tests\nin addition to non-HTTP tests would be nicer, if we can afford to.\n\nThanks.\n\n"},{"id":"476356","messageId":"20230501170018.1410567-1-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"ZEmMUFR7AJn+v7jV@tapette.crustytoothpaste.net","subject":"[PATCH v2 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-01T17:00:17Z","receivedAt":"2023-05-01T17:07:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We recently fixed empty clones with SHA-256 over protocol v2 by\nhonouring the hash algorithm specified even when no refs are present.\nHowever, in doing so, we made it impossible to set up a v0 or v1\nrepository by cloning from an empty SHA-256 repository. In doing so, we\nalso broke the Git LFS testsuite for SHA-256 repositories.\n\nThis series introduces the dummy `capabilities^{}` entry for fetches and\nclones from an empty repository for v0 and v1, just as we do for clones.\nThis is already supported by older versions of Git, as well as libgit2,\ndulwich, and JGit.\n\nUnlike in v1, we wire this up for all protocols and fix the NULL pointer\ndereference that would occur in that case, as well as add some more\ntests. The second patch has been dropped, since it is no longer needed\nand was not very popular.\n\nChanges since v1:\n* Drop patch to honour GIT_DEFAULT_HASH\n* Support all requests, not just HTTP.\n* Add more tests.\n* Fix NULL pointer dereference.\n\nbrian m. carlson (1):\n upload-pack: advertise capabilities when cloning empty repos\n\n t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n t/t5700-protocol-v1.sh | 31 +++++++++++++++++++++++++++++--\n upload-pack.c | 7 ++++++-\n 3 files changed, 62 insertions(+), 3 deletions(-)\n\n"},{"id":"476357","messageId":"20230501170018.1410567-2-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"20230501170018.1410567-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-01T17:00:18Z","receivedAt":"2023-05-01T17:07:47Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" \n\nWhen cloning an empty repository, protocol versions 0 and 1 currently\noffer nothing but the header and flush packets for the /info/refs\nendpoint. This means that no capabilities are provided, so the client\nside doesn't know what capabilities are present.\n\nHowever, this does pose a problem when working with SHA-256\nrepositories, since we use the capabilities to know the remote side's\nobject format (hash algorithm). As of 8b214c2e9d (\"clone: propagate\nobject-format when cloning from void\", 2023-04-05), this has been fixed\nfor protocol v2, since there we always read the hash algorithm from the\nremote.\n\nFortunately, the push version of the protocol already indicates a clue\nfor how to solve this. When the /info/refs endpoint is accessed for a\npush and the remote is empty, we include a dummy \"capabilities^{}\" ref\npointing to the all-zeros object ID. The protocol documentation already\nindicates this should _always_ be sent, even for fetches and clones, so\nlet's just do that, which means we'll properly announce the hash\nalgorithm as part of the capabilities. This just works with the\nexisting code because we share the same ref code for fetches and clones,\nand libgit2, JGit, and dulwich do as well.\n\nThere is one minor issue to fix, though. When we call send_ref with\nnamespaces, we would return NULL with the capabilities entry, which\nwould cause a crash. Instead, let's make sure we don't try to strip the\nnamespace if we're using our special capabilities entry.\n\nAdd several sets of tests for HTTP as well as for local clones.\n\nSigned-off-by: brian m. carlson \n---\n t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n t/t5700-protocol-v1.sh | 31 +++++++++++++++++++++++++++++--\n upload-pack.c | 7 ++++++-\n 3 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 0908534f25..21b7767cbd 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -611,6 +611,33 @@ test_expect_success 'client falls back from v2 to v0 to match server' '\n \tgrep symref=HEAD:refs/heads/ trace\n '\n \n+test_expect_success 'create empty http-accessible SHA-256 repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\t git --bare init --object-format=sha256\n+\t)\n+'\n+\n+test_expect_success 'clone empty SHA-256 repository with protocol v2' '\n+\trm -fr sha256 &&\n+\techo sha256 >expected &&\n+\tgit -c protocol.version=2 clone \"$HTTPD_URL/smart/sha256.git\" &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp actual expected &&\n+\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'clone empty SHA-256 repository with protocol v0' '\n+\trm -fr sha256 &&\n+\techo sha256 >expected &&\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone \"$HTTPD_URL/smart/sha256.git\" &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp actual expected &&\n+\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'passing hostname resolution information works' '\n \tBOGUS_HOST=gitbogusexamplehost.invalid &&\n \tBOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 6c8d4c6cf1..a73b4d4ff6 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -244,15 +244,28 @@ test_expect_success 'push with ssh:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'clone propagates object-format from empty repo' '\n+\ttest_when_finished \"rm -fr src256 dst256\" &&\n+\n+\techo sha256 >expect &&\n+\tgit init --object-format=sha256 src256 &&\n+\tgit clone --no-local src256 dst256 &&\n+\tgit -C dst256 rev-parse --show-object-format >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n # Test protocol v1 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-test_expect_success 'create repo to be served by http:// transport' '\n+test_expect_success 'create repos to be served by http:// transport' '\n \tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n \tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" config http.receivepack true &&\n-\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one &&\n+\tgit init --object-format=sha256 \"$HTTPD_DOCUMENT_ROOT_PATH/sha256\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/sha256\" config http.receivepack true\n '\n \n test_expect_success 'clone with http:// using protocol v1' '\n@@ -269,6 +282,20 @@ test_expect_success 'clone with http:// using protocol v1' '\n \tgrep \"git< version 1\" log\n '\n \n+test_expect_success 'clone with http:// using protocol v1 with empty SHA-256 repo' '\n+\tGIT_TRACE_PACKET=1 GIT_TRACE_CURL=1 git -c protocol.version=1 \\\n+\t\tclone \"$HTTPD_URL/smart/sha256\" sha256 2>log &&\n+\n+\techo sha256 >expect &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Client requested to use protocol v1\n+\tgrep \"Git-Protocol: version=1\" log &&\n+\t# Server responded using protocol v1\n+\tgrep \"git< version 1\" log\n+'\n+\n test_expect_success 'fetch with http:// using protocol v1' '\n \ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" two &&\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 08633dc121..d7b31d0527 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -120,6 +120,7 @@ struct upload_pack_data {\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n \tunsigned advertise_sid : 1;\n+\tunsigned sent_capabilities : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -1212,7 +1213,8 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow deepen-since deepen-not\"\n \t\t\" deepen-relative no-progress include-tag multi_ack_detailed\";\n-\tconst char *refname_nons = strip_namespace(refname);\n+\tconst char *refname_nons = !strcmp(refname, \"capabilities^{}\") ?\n+\t\t\t\t refname : strip_namespace(refname);\n \tstruct object_id peeled;\n \tstruct upload_pack_data *data = cb_data;\n \n@@ -1240,6 +1242,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n+\t\tdata->sent_capabilities = 1;\n \t} else {\n \t\tpacket_fwrite_fmt(stdout, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n@@ -1379,6 +1382,8 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n \t\t\tdata.no_done = 1;\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\tif (!data.sent_capabilities)\n+\t\t\tsend_ref(\"capabilities^{}\", null_oid(), 0, &data);\n \t\t/*\n \t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n \t\t * uses stdio.\n"},{"id":"476366","messageId":"xmqqv8hc54xb.fsf@gitster.g","threadId":"59550","inReplyTo":"20230501170018.1410567-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T17:37:20Z","receivedAt":"2023-05-01T17:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Changes since v1:\n> * Support all requests, not just HTTP.\n> * Add more tests.\n> * Fix NULL pointer dereference.\n\nGreat. Will queue.\n\nLet's merge it down to 'next' soonish.\n\nThanks.\n"},{"id":"476393","messageId":"20230501224038.GA1174291@coredump.intra.peff.net","threadId":"59550","inReplyTo":"20230501170018.1410567-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-01T22:40:38Z","receivedAt":"2023-05-01T22:40:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2023 at 05:00:18PM +0000, brian m. carlson wrote:\n\n> There is one minor issue to fix, though. When we call send_ref with\n> namespaces, we would return NULL with the capabilities entry, which\n> would cause a crash. Instead, let's make sure we don't try to strip the\n> namespace if we're using our special capabilities entry.\n\nThanks, this hunk:\n\n> @@ -1212,7 +1213,8 @@ static int send_ref(const char *refname, const struct object_id *oid,\n> \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n> \t\t\" side-band-64k ofs-delta shallow deepen-since deepen-not\"\n> \t\t\" deepen-relative no-progress include-tag multi_ack_detailed\";\n> -\tconst char *refname_nons = strip_namespace(refname);\n> +\tconst char *refname_nons = !strcmp(refname, \"capabilities^{}\") ?\n> +\t\t\t\t refname : strip_namespace(refname);\n> \tstruct object_id peeled;\n> \tstruct upload_pack_data *data = cb_data;\n\nlooks much better than sticking it in strip_namespace() as I did\nearlier. I did wonder about refactoring further:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex d7b31d0527..e1d75d7c3c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1207,19 +1207,17 @@ static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {\n \t\tstrbuf_addf(buf, \" session-id=%s\", trace2_session_id());\n }\n \n-static int send_ref(const char *refname, const struct object_id *oid,\n-\t\t int flag UNUSED, void *cb_data)\n+static void write_v0_ref(struct upload_pack_data *data,\n+\t\t\t const char *refname, const char *refname_nons,\n+\t\t\t const struct object_id *oid)\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow deepen-since deepen-not\"\n \t\t\" deepen-relative no-progress include-tag multi_ack_detailed\";\n-\tconst char *refname_nons = !strcmp(refname, \"capabilities^{}\") ?\n-\t\t\t\t refname : strip_namespace(refname);\n \tstruct object_id peeled;\n-\tstruct upload_pack_data *data = cb_data;\n \n \tif (mark_our_ref(refname_nons, refname, oid, &data->hidden_refs))\n-\t\treturn 0;\n+\t\treturn;\n \n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n@@ -1249,6 +1247,12 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n \t\tpacket_fwrite_fmt(stdout, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+}\n+\n+static int send_ref(const char *refname, const struct object_id *oid,\n+\t\t int flag UNUSED, void *cb_data)\n+{\n+\twrite_v0_ref(cb_data, refname, strip_namespace(refname), oid);\n \treturn 0;\n }\n \n@@ -1382,8 +1386,10 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n \t\t\tdata.no_done = 1;\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n-\t\tif (!data.sent_capabilities)\n-\t\t\tsend_ref(\"capabilities^{}\", null_oid(), 0, &data);\n+\t\tif (!data.sent_capabilities) {\n+\t\t\tconst char *ref = \"capabilities^{}\";\n+\t\t\twrite_v0_ref(&data, ref, ref, null_oid());\n+\t\t}\n \t\t/*\n \t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n \t\t * uses stdio.\n\nwhich avoids doing an extra strcmp() on every ref. But probably it is\nnot that big a deal either way.\n\n> Add several sets of tests for HTTP as well as for local clones.\n\nThis part puzzled me a bit. There's a local test in t5700, which is\ngood. But it also gets HTTP tests. What do they offer versus the ones in\nt5551 (or vice versa)?\n\n-Peff\n"},{"id":"476394","messageId":"xmqqzg6n1x8g.fsf@gitster.g","threadId":"59550","inReplyTo":"20230501224038.GA1174291@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T22:51:43Z","receivedAt":"2023-05-01T22:51:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@ -1382,8 +1386,10 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n> \t\t\tdata.no_done = 1;\n> \t\thead_ref_namespaced(send_ref, &data);\n> \t\tfor_each_namespaced_ref(send_ref, &data);\n> -\t\tif (!data.sent_capabilities)\n> -\t\t\tsend_ref(\"capabilities^{}\", null_oid(), 0, &data);\n> +\t\tif (!data.sent_capabilities) {\n> +\t\t\tconst char *ref = \"capabilities^{}\";\n> +\t\t\twrite_v0_ref(&data, ref, ref, null_oid());\n> +\t\t}\n\nAh, this separation of duties wrt the namespace stripping makes the\nresult easier to read.\n\nThe version brian posted looked good enough to me, but if we are to\nhave another iteration, incorporating this change would be nice.\n\nThanks.\n\n"},{"id":"476475","messageId":"6451a0ba5c3fb_200ae2945b@chronos.notmuch","threadId":"59550","inReplyTo":"20230427054343.GE982277@coredump.intra.peff.net","subject":"Is GIT_DEFAULT_HASH flawed?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-02T23:46:02Z","receivedAt":"2023-05-02T23:46:09Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nChanging the subject as this message seems like a different topic.\n\nJeff King wrote:\n> On Wed, Apr 26, 2023 at 02:33:30PM -0700, Junio C Hamano wrote:\n> > \"brian m. carlson\" writes:\n> > \n> > > `GIT_DEFAULT_HASH`::\n> > > \tIf this variable is set, the default hash algorithm for new\n> > > \trepositories will be set to this value. This value is currently\n> > > +\tignored when cloning if the remote value can be definitively\n> > > +\tdetermined; the setting of the remote repository is used\n> > > +\tinstead. The value is honored if the remote repository's\n> > > +\talgorithm cannot be determined, such as some cases when\n> > > +\tthe remote repository is empty. The default is \"sha1\".\n> > > +\tTHIS VARIABLE IS EXPERIMENTAL! See `--object-format`\n> > > +\tin linkgit:git-init[1].\n> > \n> > We'd need to evantually cover all the transports (and non-transport\n> > like the \"--local\" optimization) so that the object-format and other\n> > choices are communicated from the origin to a new clone anyway, so\n> > this extra complexity \"until X is fixed, it behaves this way, but\n> > otherwise the variable is read in the meantime\" may be a disservice\n> > to the end users, even though it may make it easier in the shorter\n> > term for maintainers of programs that rely on the buggy \"git clone\"\n> > that partially honored this environment variable.\n> > \n> > In short, I am still not convinced that the above is a good design\n> > choice in the longer term.\n> \n> I also think it is working against the backwards-compatible design of\n> the hash function transition.\n\nTo be honest this whole approach seems to be completely flawed to me and\nagainst the whole design of git in the first place.\n\nIn a recent email Linus Torvalds explained why object ids were\ncalculated based {type, size, data} [1], and he explained very clearly\nthat two objects with exactly the same data are not supposed to have the\nsame id if the type is different.\n\nIf even the tiniest change such as adding a period to a commit messange\nchanges the object id (and thus semantically makes it a different\nobject), then it makes sense that changing the type of an object also\nchanges the object id (and thus it's also a different object).\n\nAnd because the id of the parent is included in the content of every\ncommit, the top-level id ensures the integrity of the whole graph.\n\nBut then comes this notion that the hash algorithm is a property of the\nrepository, and not part of the object storage, which means changing the\nwhole hash algorithm of a repository is considered less of a change than\nadding a period to the commit message, worse: not a change at all.\n\nI am reminded of the warning Sam Smith gave to the Git project [2] which\nseemed to be unheard, but the notion of cryptographic algorithm agility\nmakes complete sense to me.\n\nIn my view one repository should be able to have part SHA-1 history,\npart SHA3-256 history, and part BLAKE2b history.\n\nChanging the hash algorithm of one commit should change the object id of\nthat commit, and thus make it semantically a different commit.\n\nIn other words: an object of type \"blob\" should never be confused with\nan object of type \"blob:sha-256\", even if the content is exactly the\nsame.\n\nThe fact that apparently it's so easy to clone a repository with\nthe wrong hash algorithm should give developers pause, as it means the\nwhole point of using cryptographic hash algorithms to ensure the\nintegrity of the commit history is completely gone.\n\nI have not been following the SHA-1 -> OID discussions, but I\ndistinctively recall Linus Torvalds mentioning that the choice of using\nSHA-1 wasn't even for security purposes, it was to ensure integrity.\nWhen I do a `git fetch` as long as the new commits have the same SHA-1\nas parent as the SHA-1s I have in my repository I can be relatively\ncertain the repository has not been tampered with. Which means that if I\ndo a `git fetch` that suddenly brings SHA-256 commits, some of them must\nhave SHA-1 parents that match the ones I currently have. Otherwise how\ndo I know it's the same history?\n\nMaybe that's one of the reasons people don't seem particularly eager to\nmove away from SHA-1:\n\nBetter the SHA-1 you know, than the SHA-256 you don't.\n\nCheers.\n\n[1] https://lore.kernel.org/git/CAHk-=wjr-CMLX2Jo2++rwcv0VNr+HmZqXEVXNsJGiPRUwNxzBQ@mail.gmail.com/\n[2] https://lore.kernel.org/git/D433038A-2643-4F63-8677-CA8AB6904AE1@samuelsmith.org/\n\n-- \nFelipe Contreras\n"},{"id":"476493","messageId":"70103746-6980-baed-13d9-afeae6cee464@zombino.com","threadId":"59550","inReplyTo":"6451a0ba5c3fb_200ae2945b@chronos.notmuch","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-05-03T09:03:47Z","receivedAt":"2023-05-03T09:03:54Z","isPatch":false,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"On 5/3/23 01:46, Felipe Contreras wrote:\n> To be honest this whole approach seems to be completely flawed to me and\n> against the whole design of git in the first place.\n\nThe discussion above is mostly moot now since this has been fixed in \nlater patches in this thread, AFAIK. It's also moot for other reasons, \nlike the hash function transition plan is not really implemented, yet.\n\nAlso, this was about corner-case, like it often is.\n\n\n> In a recent email Linus Torvalds explained why object ids were\n> calculated based {type, size, data} [1], and he explained very clearly\n> that two objects with exactly the same data are not supposed to have the\n> same id if the type is different.\n\nThis is different. But aside, type + size + data are not really much \ndifferent from just having data in a hash function. There are plenty of \nhash collisions where\n\n HASH(type + size + data) == HASH(type + size + data')\n\nby definition of how these functions work. The problem is always in \nfinding these collisions. But anyway...\n\n> In my view one repository should be able to have part SHA-1 history,\n> part SHA3-256 history, and part BLAKE2b history.\n\nYes, that would be great. Please provide patch series for this :-)\n\n> I have not been following the SHA-1 -> OID discussions, but I\n> distinctively recall Linus Torvalds mentioning that the choice of using\n> SHA-1 wasn't even for security purposes, it was to ensure integrity.\n\nThese are different sides of the same coin. Hashes are used to provide \nintegrity. Hashes like MD4, MD5, SHA1, SHA256 are there for integrity. \nSome of these are no longer recommended and some are completely broken.\n\n> Better the SHA-1 you know, than the SHA-256 you don't.\n\nWrong conclusion ;) Also, we know SHA-256\n\nThe problem in git-core and virtually all clients and other \nimplementations is/was that SHA1 was hardcoded and assumed to be THE ONE \nand ONLY hash. It will take quite a bit of work outside of git-core to \nremove this one assumption (remember two digit year and 2000? - yes I'm \nold). Once this hash assumption is removed, you can start talking about \nadding other hashes and interop.\n\nKeep in mind -- hashes are there for object reference. They are the glue \nin git. But there is really nothing stopping us from recalculating them \n\"on the fly\". If you have SHA1 repo, you can calculate a SHA256 or \nwhatever hash for any type object. That's not the problem, conceptually \nspeaking.\n\nFinally, let not have a \"bike shed\" discussion about this. The \nGIT_DEFAULT_HASH is meant to be used by `git init` in-lieu of \n--object-format parameter, so it's not flawed. When used in other \napplications, it probably indicates a bug. But we can't fix all the bugs \nat once :-)\n\nCheers,\n- Adam\n"},{"id":"476494","messageId":"CANgJU+UasufF7-B8ukEMm_Lv8gu4wUpaVKa9AOBacDHJvi7fxQ@mail.gmail.com","threadId":"59550","inReplyTo":"6451a0ba5c3fb_200ae2945b@chronos.notmuch","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"demerphq","fromEmail":"demerphq@gmail.com","sentAt":"2023-05-03T09:09:35Z","receivedAt":"2023-05-03T09:11:15Z","isPatch":false,"sender":{"key":"demerphq@gmail.com","avatar":null},"body":"On Wed, 3 May 2023 at 02:17, Felipe Contreras\n wrote:\n>\n> Hi,\n>\n> Changing the subject as this message seems like a different topic.\n>\n> Jeff King wrote:\n> > On Wed, Apr 26, 2023 at 02:33:30PM -0700, Junio C Hamano wrote:\n> > > \"brian m. carlson\" writes:\n> > >\n> > > > `GIT_DEFAULT_HASH`::\n> > > > If this variable is set, the default hash algorithm for new\n> > > > repositories will be set to this value. This value is currently\n> > > > + ignored when cloning if the remote value can be definitively\n> > > > + determined; the setting of the remote repository is used\n> > > > + instead. The value is honored if the remote repository's\n> > > > + algorithm cannot be determined, such as some cases when\n> > > > + the remote repository is empty. The default is \"sha1\".\n> > > > + THIS VARIABLE IS EXPERIMENTAL! See `--object-format`\n> > > > + in linkgit:git-init[1].\n> > >\n> > > We'd need to evantually cover all the transports (and non-transport\n> > > like the \"--local\" optimization) so that the object-format and other\n> > > choices are communicated from the origin to a new clone anyway, so\n> > > this extra complexity \"until X is fixed, it behaves this way, but\n> > > otherwise the variable is read in the meantime\" may be a disservice\n> > > to the end users, even though it may make it easier in the shorter\n> > > term for maintainers of programs that rely on the buggy \"git clone\"\n> > > that partially honored this environment variable.\n> > >\n> > > In short, I am still not convinced that the above is a good design\n> > > choice in the longer term.\n> >\n> > I also think it is working against the backwards-compatible design of\n> > the hash function transition.\n>\n> To be honest this whole approach seems to be completely flawed to me and\n> against the whole design of git in the first place.\n>\n> In a recent email Linus Torvalds explained why object ids were\n> calculated based {type, size, data} [1], and he explained very clearly\n> that two objects with exactly the same data are not supposed to have the\n> same id if the type is different.\n\nHe said:\n\n--- quote-begin ---\nThe \"no aliasing\" means that no two distinct pointers can point to the\nsame data. So a tagged pointer of type \"commit\" can not point to the\nsame object as a tagged pointer of type \"blob\". They are distinct\npointers, even if (maybe) the commit object encoding ends up then\nbeing identical to a blob object.\n--- quote-end ---\n\nAs far as I could tell he didn't really explain *why* he wanted this,\nand IMO it is non-obvious why he would care if a blob and a commit had\nthe same text, and thus the same ID. He just said he didnt want it to\nhappen, not why. I can imagine some aesthetic reasons why you might\nwant to ensure that no blob has the same ID as a commit, and I can\nimagine it might make debugging easier at certain points, but it seems\nunnecessary given the data is write once.\n\n> If even the tiniest change such as adding a period to a commit messange\n> changes the object id (and thus semantically makes it a different\n> object), then it makes sense that changing the type of an object also\n> changes the object id (and thus it's also a different object).\n>\n> And because the id of the parent is included in the content of every\n> commit, the top-level id ensures the integrity of the whole graph.\n>\n> But then comes this notion that the hash algorithm is a property of the\n> repository, and not part of the object storage, which means changing the\n> whole hash algorithm of a repository is considered less of a change than\n> adding a period to the commit message, worse: not a change at all.\n\nI really dont understand why you think having two hash functions\nproducing different results for the same data is comparable to a\nsingle hash producing different results for different data. In one\ncase you have two different continuum of identifiers, with one ID per\ncontinuum, and in the other you have two different identifiers in the\nsame continuum, and if you a continuum you would have 4 different\nidentifiers right? Eg, the two cases are really quite different at a\nfundamental level.\n\n> I am reminded of the warning Sam Smith gave to the Git project [2] which\n> seemed to be unheard, but the notion of cryptographic algorithm agility\n> makes complete sense to me.\n>\n> In my view one repository should be able to have part SHA-1 history,\n> part SHA3-256 history, and part BLAKE2b history.\n\nIsn't this orthagonal to your other points?\n\n> Changing the hash algorithm of one commit should change the object id of\n> that commit, and thus make it semantically a different commit.\n>\n> In other words: an object of type \"blob\" should never be confused with\n> an object of type \"blob:sha-256\", even if the content is exactly the\n> same.\n\nThis doesn't make sense to me. As long as we can distinguish the\nhashes produced by the different hash functions in use we can create a\nmapping of the data that is hashed such that we have a 1:1 mapping of\nidentifiers of each type at which point it really doesn't matter which\nhash function is used.\n\n> The fact that apparently it's so easy to clone a repository with\n> the wrong hash algorithm should give developers pause, as it means the\n> whole point of using cryptographic hash algorithms to ensure the\n> integrity of the commit history is completely gone.\n\nThis is a leap too far. The fact that it is \"so easy to clone a repo\nwith the wrong hash algorithm\" is completely orthogonal to the\nfundamental principles of hash identifiers from strong hash functions.\nYou seem to be deriving grand conclusions from what sounds to me like\na simple bug/design-oversight.\n\n> I have not been following the SHA-1 -> OID discussions, but I\n> distinctively recall Linus Torvalds mentioning that the choice of using\n> SHA-1 wasn't even for security purposes, it was to ensure integrity.\n> When I do a `git fetch` as long as the new commits have the same SHA-1\n> as parent as the SHA-1s I have in my repository I can be relatively\n> certain the repository has not been tampered with. Which means that if I\n> do a `git fetch` that suddenly brings SHA-256 commits, some of them must\n> have SHA-1 parents that match the ones I currently have. Otherwise how\n> do I know it's the same history?\n\nSo consider what /could/ happen here. You fetch a commit which uses\nSHA-256 into a repo where all of your local commits use SHA-1. The\ncommit you fetched says its parent is some SHA-256 ID you don't know\nabout as all your ID's are SHA-1. So git then could go and construct\nan index, hashing each item using SHA-256 instead of SHA-1, and using\nthe result to build a bi-directional mapping from SHA-1 to SHA-256 and\nback. All it has to do then is look into the mapping to find if the\nSHA-256 parent id is present in your repo. If it is then you know it's\nthe same history.\n\nThe key point here is that if you ignore SHAttered artifacts (which\nseems reasonable as you can detect the attack during hashing) you can\nbuild a 1:1 map of SHA-1 and SHA-256 ids. Once you have that mapping\nit doesn't matter which ID is used.\n\n> Maybe that's one of the reasons people don't seem particularly eager to\n> move away from SHA-1:\n\nMaybe, but it doesn't make sense to me. You seem to be putting undue\nweight on an unnecessary aspect of the git design: there doesn't seem\nto be a reason for Linuses \"no aliasing\" policy, and it seems like one\ncould build a git-a-like without it without suffering any significant\npenalties. Regardless, provided that the hash functions allow a 1:1\nmapping of ID's (which is assumed by using \"collision free hash\nfunctions\"), it seems like it really doesn't matter which hash is used\nat any given time.\n\ncheers,\nYves\n\n-- \nperl -Mre=debug -e \"/just|another|perl|hacker/\"\n"},{"id":"476513","messageId":"64528158cdd1e_68229498@chronos.notmuch","threadId":"59550","inReplyTo":"70103746-6980-baed-13d9-afeae6cee464@zombino.com","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-03T15:44:24Z","receivedAt":"2023-05-03T15:44:31Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Adam Majer wrote:\n> On 5/3/23 01:46, Felipe Contreras wrote:\n> > To be honest this whole approach seems to be completely flawed to me and\n> > against the whole design of git in the first place.\n> \n> The discussion above is mostly moot now since this has been fixed in \n> later patches in this thread, AFAIK.\n\nThat particular isssue might be fixed, but that issue should never have\nhappened in the first place if the design was correct.\n\nA bad design makes certain errors prone to happen, a good design makes the same\nerrors happen rarely, a great design makes those errors impossible.\n\nGit was designed to make it *impossible* to confuse two commits with similar\ndata.\n\nThe symptom might have been fixed, that doesn't mean there's no underlying\nproblem.\n\n> It's also moot for other reasons, like the hash function transition plan is\n> not really implemented, yet.\n\nThe implemention of the plan isn't the problem, it's the plan itself.\n\n> Also, this was about corner-case, like it often is.\n\nA corner-case that should be impossible.\n\n> > In a recent email Linus Torvalds explained why object ids were\n> > calculated based {type, size, data} [1], and he explained very clearly\n> > that two objects with exactly the same data are not supposed to have the\n> > same id if the type is different.\n> \n> This is different. But aside, type + size + data are not really much \n> different from just having data in a hash function.\n\nIt's completely different.\n\n> There are plenty of hash collisions where\n> \n> HASH(type + size + data) == HASH(type + size + data')\n> \n> by definition of how these functions work. The problem is always in \n> finding these collisions. But anyway...\n\nI don't think you understand why Linus Torvalds chose to hash objects.\n\n> > In my view one repository should be able to have part SHA-1 history,\n> > part SHA3-256 history, and part BLAKE2b history.\n> \n> Yes, that would be great. Please provide patch series for this :-)\n\nI have hundreds of patches being ignored, why would I write yet another patch\nseries that will be ignored?\n\n> > I have not been following the SHA-1 -> OID discussions, but I\n> > distinctively recall Linus Torvalds mentioning that the choice of using\n> > SHA-1 wasn't even for security purposes, it was to ensure integrity.\n> \n> These are different sides of the same coin. Hashes are used to provide \n> integrity. Hashes like MD4, MD5, SHA1, SHA256 are there for integrity. \n> Some of these are no longer recommended and some are completely broken.\n\nThere are different philosophical views of what \"security\" means, and it seems\npretty clear to me that your view does not align with the view of Linus\nTorvalds.\n\n> > Better the SHA-1 you know, than the SHA-256 you don't.\n> \n> Wrong conclusion ;) Also, we know SHA-256\n\nYou don't understand what is being said.\n\nWhich hash is more trustworthy?\n\n a. 69c786637d7a7fe3b2b8f7d989af095f5f49c3a8\n b. d891b12414e1d9331f8cbb15acfe690671974f27ba76e2b423294cfb7a055f2f\n\nIf you answer b just beacuse it's SHA-256 you don't understand security.\n\nb is a random commit I generated, a is the current git.git master.\n\nA SHA-1 hash from a source you trust is inifinitely more trustworthy than a\nrandom SHA-256 hash. Even a known MD5 hash is better in this respect.\n\n> Keep in mind -- hashes are there for object reference.\n\nNo. I don't think you understand why Linus Torvalds used hashes.\n\n> If you have SHA1 repo, you can calculate a SHA256 or whatever hash for any\n> type object.\n\nI know it *can* be done, I understand how hash algorithms work, but just\nbecause something *can* be done doesn't mean it *should*.\n\nYou *can* generate a SHA-1 of a blob's data, instead of a SHA-1 of a blob's\n`type + size + data`, does that mean we should? No.\n\n> Finally, let not have a \"bike shed\" discussion about this.\n\nDiscussing the original design of git's object storage which has withstood the\ntest of time for 18 years is not \"bike sheding\".\n\nI don't even think you understand what I'm trying to say.\n\n---\n\nWhy do you think these commands generate different hashes?\n\n git hash-object -t blob /dev/null\n git hash-object -t tree /dev/null\n\n-- \nFelipe Contreras\n"},{"id":"476526","messageId":"31868D65-0456-4594-AB2C-C4735B8F1D75@zombino.com","threadId":"59550","inReplyTo":"64528158cdd1e_68229498@chronos.notmuch","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Adam Majer","fromEmail":"adamm@zombino.com","sentAt":"2023-05-03T17:21:48Z","receivedAt":"2023-05-03T17:21:58Z","isPatch":false,"sender":{"key":"adamm@zombino.com","avatar":"https://avatars.githubusercontent.com/u/1211498?v=4"},"body":"\n\nOn May 3, 2023 5:44:24 p.m. GMT+02:00, Felipe Contreras wrote:\n>Git was designed to make it *impossible* to confuse two commits with similar\n>data.\n\nThat was never ever the problem here.\n\n\n\n>> This is different. But aside, type + size + data are not really much \n>> different from just having data in a hash function.\n>\n>It's completely different.\n\nHow so? Type and size are just about 2 and a dozen bits of data, respectfully.\n\n\n>There are different philosophical views of what \"security\" means, and it seems\n>pretty clear to me that your view does not align with the view of Linus\n>Torvalds.\n\n\nI'm not sure why you are name dropping Linus everywhere or assuming you know more than anyone here about hash functions.\n\nYour explanation is quite clear to me (and probably everyone else here). But I'll just leave it at that.\n\nCheers,\nAdam\n\n"},{"id":"476535","messageId":"6452a5fdf2bd9_6822945f@chronos.notmuch","threadId":"59550","inReplyTo":"CANgJU+UasufF7-B8ukEMm_Lv8gu4wUpaVKa9AOBacDHJvi7fxQ@mail.gmail.com","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-03T18:20:45Z","receivedAt":"2023-05-03T18:20:57Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"demerphq wrote:\n> On Wed, 3 May 2023 at 02:17, Felipe Contreras\n> wrote:\n> > Changing the subject as this message seems like a different topic.\n> > Jeff King wrote:\n> > > On Wed, Apr 26, 2023 at 02:33:30PM -0700, Junio C Hamano wrote:\n> > > > \"brian m. carlson\" writes:\n> > > >\n> > > > > `GIT_DEFAULT_HASH`::\n> > > > > If this variable is set, the default hash algorithm for new\n> > > > > repositories will be set to this value. This value is currently\n> > > > > + ignored when cloning if the remote value can be definitively\n> > > > > + determined; the setting of the remote repository is used\n> > > > > + instead. The value is honored if the remote repository's\n> > > > > + algorithm cannot be determined, such as some cases when\n> > > > > + the remote repository is empty. The default is \"sha1\".\n> > > > > + THIS VARIABLE IS EXPERIMENTAL! See `--object-format`\n> > > > > + in linkgit:git-init[1].\n> > > >\n> > > > We'd need to evantually cover all the transports (and non-transport\n> > > > like the \"--local\" optimization) so that the object-format and other\n> > > > choices are communicated from the origin to a new clone anyway, so\n> > > > this extra complexity \"until X is fixed, it behaves this way, but\n> > > > otherwise the variable is read in the meantime\" may be a disservice\n> > > > to the end users, even though it may make it easier in the shorter\n> > > > term for maintainers of programs that rely on the buggy \"git clone\"\n> > > > that partially honored this environment variable.\n> > > >\n> > > > In short, I am still not convinced that the above is a good design\n> > > > choice in the longer term.\n> > >\n> > > I also think it is working against the backwards-compatible design of\n> > > the hash function transition.\n> >\n> > To be honest this whole approach seems to be completely flawed to me and\n> > against the whole design of git in the first place.\n> >\n> > In a recent email Linus Torvalds explained why object ids were\n> > calculated based {type, size, data} [1], and he explained very clearly\n> > that two objects with exactly the same data are not supposed to have the\n> > same id if the type is different.\n> \n> He said:\n> \n> --- quote-begin ---\n> The \"no aliasing\" means that no two distinct pointers can point to the\n> same data. So a tagged pointer of type \"commit\" can not point to the\n> same object as a tagged pointer of type \"blob\". They are distinct\n> pointers, even if (maybe) the commit object encoding ends up then\n> being identical to a blob object.\n> --- quote-end ---\n> \n> As far as I could tell he didn't really explain *why* he wanted this,\n> and IMO it is non-obvious why he would care if a blob and a commit had\n> the same text, and thus the same ID. He just said he didnt want it to\n> happen, not why.\n\nBut we don't need to understand why to know it's part of the core design.\n\nIf something is part of the core design as a rule it's better to not\nmess with it.\n\n> I can imagine some aesthetic reasons why you might want to ensure that\n> no blob has the same ID as a commit, and I can imagine it might make\n> debugging easier at certain points, but it seems unnecessary given the\n> data is write once.\n\nI don't know, but to me separating objects makes sense not just conceptually,\nbut in practice there's a whole class of potential errors that could be\navoided.\n\nFor example, I can think of an implementation of `git prune` that would check\ncommits first, then trees, then blobs, and the blobs that not reachable from\nany trees are removed. But if a commit can have the same id as a blob, you have\nto think of a different implementation.\n\nIf that's not possible, then you just forget about those potential issues.\n\n> > If even the tiniest change such as adding a period to a commit messange\n> > changes the object id (and thus semantically makes it a different\n> > object), then it makes sense that changing the type of an object also\n> > changes the object id (and thus it's also a different object).\n> >\n> > And because the id of the parent is included in the content of every\n> > commit, the top-level id ensures the integrity of the whole graph.\n> >\n> > But then comes this notion that the hash algorithm is a property of the\n> > repository, and not part of the object storage, which means changing the\n> > whole hash algorithm of a repository is considered less of a change than\n> > adding a period to the commit message, worse: not a change at all.\n> \n> I really dont understand why you think having two hash functions\n> producing different results for the same data is comparable to a\n> single hash producing different results for different data.\n\nThat depends on what you consider the \"data\" to be.\n\nIf you consider the content of a blob to be the data, then you wouldn't\nhave different results if a commit has the same data: it would be the\nsame id.\n\nIf instead you consider the data to be `type+content`, then you would\nhave different results.\n\n> In one case you have two different continuum of identifiers, with one\n> ID per continuum, and in the other you have two different identifiers\n> in the same continuum, and if you a continuum you would have 4\n> different identifiers right? Eg, the two cases are really quite\n> different at a fundamental level.\n\nThat entirely depends on what data you hash.\n\nIf you hash `algo+type+size+data` there's only one id per object.\nPeriod.\n\n> > I am reminded of the warning Sam Smith gave to the Git project [2] which\n> > seemed to be unheard, but the notion of cryptographic algorithm agility\n> > makes complete sense to me.\n> >\n> > In my view one repository should be able to have part SHA-1 history,\n> > part SHA3-256 history, and part BLAKE2b history.\n> \n> Isn't this orthagonal to your other points?\n\nNot if you consider changing the hash algorithm of a repository to be an\nimportant part of its history (more important than adding a period to a\ncommit).\n\n> > Changing the hash algorithm of one commit should change the object id of\n> > that commit, and thus make it semantically a different commit.\n> >\n> > In other words: an object of type \"blob\" should never be confused with\n> > an object of type \"blob:sha-256\", even if the content is exactly the\n> > same.\n> \n> This doesn't make sense to me. As long as we can distinguish the\n> hashes produced by the different hash functions in use we can create a\n> mapping of the data that is hashed such that we have a 1:1 mapping of\n> identifiers of each type at which point it really doesn't matter which\n> hash function is used.\n\nYes, we *can*, that doesn't mean we *should*.\n\nIf you do `git commit --ammend --signoff` to add your `Signed-off-by` to\na commit, there's a 1:1 mapping from the original commit, to the new\none, but conceptually in git they are different objects.\n\nI recall Linus Torvalds mentioned he used Monotone as a guideline of\nwhat *not* to do. In Monotone you could add the equivalent of\n`Signed-off-by` without changing the hash of the commit, in fact, you\ncould add any metadata if I recall correctly. But this opens a whole can\nof worms because now how do you know you have all the metadata relevant\nto the commit?\n\nMaking all the metadata of a commit part of the commit solves the\nintegrity problem Monotone had at the cost of making git commits\nessentially immutable: any change means it's a different commit.\n\nIf making *any* change in the object, makes it conceptually a different\nobject, including the type of the object, how on Earth is changing the\nhash algorithm not considered a change?\n\nThis object:\n\n ❯ git hash-object -t blob /dev/null\n e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\nIs considered different from this object:\n\n ❯ git hash-object -t tree /dev/null\n 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\nThat's why they have a different hash.\n\nWhy would these objects be considered the same?\n\n ❯ git hash-object -t blob /dev/null\n e69de29bb2d1d6434b8b29ae775ad8c2e48c5391\n\n ❯ git hash-object -t blob /dev/null\n 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813\n\nIt makes *zero* sense that adding a period changes the object, adding a\ns-o-b changes the object, changing the type changes the object, but\nchanging the hash algorithm does not.\n\n> > The fact that apparently it's so easy to clone a repository with\n> > the wrong hash algorithm should give developers pause, as it means the\n> > whole point of using cryptographic hash algorithms to ensure the\n> > integrity of the commit history is completely gone.\n> \n> This is a leap too far. The fact that it is \"so easy to clone a repo\n> with the wrong hash algorithm\" is completely orthogonal to the\n> fundamental principles of hash identifiers from strong hash functions.\n\nOnly if you think changing the hash algorithm is a less important part\nof an object than adding a period.\n\nDo you honestly think these two should be considered the same object?\n\na)\n\n tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n author Felipe Contreras 0 -0600\n committer Felipe Contreras 0 -0600\n\n Initial commit\n\nb)\n\n tree 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321\n author Felipe Contreras 0 -0600\n committer Felipe Contreras 0 -0600\n\n Initial commit\n\n> You seem to be deriving grand conclusions from what sounds to me like\n> a simple bug/design-oversight.\n\nI think you are dismissing the brilliant idea that made git's object\nstorage model so successful.\n\n> > I have not been following the SHA-1 -> OID discussions, but I\n> > distinctively recall Linus Torvalds mentioning that the choice of using\n> > SHA-1 wasn't even for security purposes, it was to ensure integrity.\n> > When I do a `git fetch` as long as the new commits have the same SHA-1\n> > as parent as the SHA-1s I have in my repository I can be relatively\n> > certain the repository has not been tampered with. Which means that if I\n> > do a `git fetch` that suddenly brings SHA-256 commits, some of them must\n> > have SHA-1 parents that match the ones I currently have. Otherwise how\n> > do I know it's the same history?\n> \n> So consider what /could/ happen here. You fetch a commit which uses\n> SHA-256 into a repo where all of your local commits use SHA-1. The\n> commit you fetched says its parent is some SHA-256 ID you don't know\n> about as all your ID's are SHA-1. So git then could go and construct\n> an index, hashing each item using SHA-256 instead of SHA-1, and using\n> the result to build a bi-directional mapping from SHA-1 to SHA-256 and\n> back. All it has to do then is look into the mapping to find if the\n> SHA-256 parent id is present in your repo. If it is then you know it's\n> the same history.\n\nYeah, that *could* happen. Doesn't mean it *should*.\n\n> The key point here is that if you ignore SHAttered artifacts (which\n> seems reasonable as you can detect the attack during hashing) you can\n> build a 1:1 map of SHA-1 and SHA-256 ids. Once you have that mapping\n> it doesn't matter which ID is used.\n\nIt may not matter to you. It matters to me.\n\n> > Maybe that's one of the reasons people don't seem particularly eager to\n> > move away from SHA-1:\n> \n> Maybe, but it doesn't make sense to me. You seem to be putting undue\n> weight on an unnecessary aspect of the git design: there doesn't seem\n> to be a reason for Linuses \"no aliasing\" policy, and it seems like one\n> could build a git-a-like without it without suffering any significant\n> penalties.\n\nThe fact you don't see a reason doesn't mean there isn't one. This is an\nargument from ignorance fallacy.\n\nThe appendix was considered a vestigial organ because nobody could see a\nreason for it. Did that mean it served no puprpose? No.\n\n> Regardless, provided that the hash functions allow a 1:1 mapping of\n> ID's (which is assumed by using \"collision free hash functions\"), it\n> seems like it really doesn't matter which hash is used at any given\n> time.\n\nPeople who designed CVS, Subversion, Monotone, and Mercurial didn't see\na reason for many of Git's design choices either.\n\nI'd argue they were wrong.\n\nI think changing the hash algorithm of a commit matters.\n\nCheers.\n\n-- \nFelipe Contreras"},{"id":"476578","messageId":"ZFLmGYXgvyydLB5E@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"6451a0ba5c3fb_200ae2945b@chronos.notmuch","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-03T22:54:17Z","receivedAt":"2023-05-03T22:54:34Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-05-02 at 23:46:02, Felipe Contreras wrote:\n> In my view one repository should be able to have part SHA-1 history,\n> part SHA3-256 history, and part BLAKE2b history.\n\nThat is practically very difficult and it means that it's hard to have\nconfidence in the later history because SHA-1 is weak and you have to\nrely on it to verify the SHA-256 history later. Since attacks always\nget better, SHA-1 will eventually be so weak that collisions can be\ncomputed in the amount of time we now take for MD4 or MD5 collisions\n(i.e., seconds), and with your plan, we'd have to retain that history\nforever with the resulting lack of confidence in part of the history.\n\nThis also doesn't work with various structures like trees, the index,\nand pack and index formats, which have no indication of the algorithm\nused and simply rely on fixed-size, often 4-byte aligned object IDs\nwithout any metadata. In addition, the internals of the code often\ndon't pass around enough data to make these values variable and thus\nthis approach would substantially complicate the code in many ways.\n\nAlso, we've already decided on the current design a long time ago with\nthe transition plan after extensive, thoughtful discussion by many\npeople. Very few people other than me have worked on sending patches to\nwork on the hash function transition, and that work up to now has all\nbeen done on my personal time, without compensation of any sort, out of\na desire to improve the project. Lots of people have opined on how it\nshould have been different without sending any patches.\n\nIf you would like to propose patches for the extensive amount of work to\nimplement your solution, then we could consider them, although I will\nwarn you that your approach will likely require at least several hundred\npatches. However, I refer you to the list archives to determine why\nyour approach is not the one we chose and is not, in my view, the best\npath forward. I should also be clear that I have no intention of\nsubmitting patches to change our approach now or in the future, or\nredoing the patches I've already sent.\n\n> The fact that apparently it's so easy to clone a repository with\n> the wrong hash algorithm should give developers pause, as it means the\n> whole point of using cryptographic hash algorithms to ensure the\n> integrity of the commit history is completely gone.\n\nNo, it doesn't. It means that our empty repositories until recently\nlacked any indication of the algorithm or other capabilities, which was\na mistake in our original protocol design that has now been corrected.\n\nIf you interact with the repository later on when it has data, then if\nyou're using the wrong hash algorithm, you'll find that you get a\nhelpful error message that that's not yet supported. If you patched Git\nto ignore that check, you'd find that your repository would just be very\nbroken in many ways with lots of random crashing and seemingly unrelated\nerror messages instead of subtly using the wrong algorithm.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476735","messageId":"645843afce409_4e6129427@chronos.notmuch","threadId":"59550","inReplyTo":"31868D65-0456-4594-AB2C-C4735B8F1D75@zombino.com","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-08T00:34:55Z","receivedAt":"2023-05-08T00:35:03Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Adam Majer wrote:\n> On May 3, 2023 5:44:24 p.m. GMT+02:00, Felipe Contreras wrote:\n> >Git was designed to make it *impossible* to confuse two commits with similar\n> >data.\n> \n> That was never ever the problem here.\n\nBut it will be.\n\n> >> This is different. But aside, type + size + data are not really much \n> >> different from just having data in a hash function.\n> >\n> >It's completely different.\n> \n> How so? Type and size are just about 2 and a dozen bits of data, respectfully.\n\nDo you understand how checksums work?\n\nCompare these two objects:\n\n 1. 0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33\n 2. 6ba62a7c5e3e9a260c5a30adf2756882c02f12a6\n\nAre they a) \"not much different\", or b) \"completely different\"?\n\nAnswer: doesn't matter, they are *different*. Period.\n\n> >There are different philosophical views of what \"security\" means, and it seems\n> >pretty clear to me that your view does not align with the view of Linus\n> >Torvalds.\n> \n> \n> I'm not sure why you are name dropping Linus everywhere\n\nI don't know if you are aware, but Linus Torvalds is the author of git.\n\nHe also happens to be the author of the most successful software project\nin history: Linux.\n\nSo generally his design choices are considered to be good.\n\n> or assuming you know more than anyone here about hash functions.\n\nI don't assume such a thing.\n\nBut I'm pretty certain not many people are aware of the integrity issues\nVCSs presented circa 2004, that git hashes solved in 2005, because if\nthey did, they could have created an object model storage similar to\ngit's, and no one did (except Linus Torvalds).\n\n> Your explanation is quite clear to me (and probably everyone else\n> here). But I'll just leave it at that.\n\nIs it? Then you would have no trouble steel manning my argument, which\nyou haven't done.\n\n-- \nFelipe Contreras\n"},{"id":"476736","messageId":"645857d8e8fd7_4e6129477@chronos.notmuch","threadId":"59550","inReplyTo":"ZFLmGYXgvyydLB5E@tapette.crustytoothpaste.net","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-05-08T02:00:56Z","receivedAt":"2023-05-08T02:01:04Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"brian m. carlson wrote:\n> On 2023-05-02 at 23:46:02, Felipe Contreras wrote:\n> > In my view one repository should be able to have part SHA-1 history,\n> > part SHA3-256 history, and part BLAKE2b history.\n> \n> That is practically very difficult and it means that it's hard to have\n> confidence in the later history because SHA-1 is weak and you have to\n> rely on it to verify the SHA-256 history later.\n\nWhy would I have to rely on SHA-1 to verify the SHA-256 history later\non?\n\n> Since attacks always get better, SHA-1 will eventually be so weak that\n> collisions can be computed in the amount of time we now take for MD4\n> or MD5 collisions (i.e., seconds), and with your plan, we'd have to\n> retain that history forever with the resulting lack of confidence in\n> part of the history.\n\nWe have to do the same with your plan as well.\n\nYour plan relies on SHA-256 being interchangeable with SHA-1, so if the\nGit project decided to switch *today* to SHA-256, we would have two\nobject ids:\n\n 1. 69c786637d7a7fe3b2b8f7d989af095f5f49c3a8\n 2. 2b4ebdace10518280172449c012af17b51e9d46e023a91a5d3dd3a8ad9e4a116\n\nThis object would refer to a tree and a parent object with SHA-1 ids,\nwhich would be OK, because they would be interchangeable with some\ncorresponding SHA-256 ids.\n\nIsn't that your plan?\n\nTherefore the SHA-1 of the parent of the commit, and the tree of the\ncommit would be trusted and retained forever.\n\n> This also doesn't work with various structures like trees, the index,\n> and pack and index formats, which have no indication of the algorithm\n> used and simply rely on fixed-size, often 4-byte aligned object IDs\n> without any metadata.\n\nSo? The index and pack objects can be regenerated, so at any point in\ntime they could be regenerated for SHA-1 or SHA-256.\n\nThe tree object is a no-brainer. For an object of type \"commit:256\" you\nrequire a tree of type \"tree:256\". Easy.\n\n> In addition, the internals of the code often don't pass around enough\n> data to make these values variable and thus this approach would\n> substantially complicate the code in many ways.\n\nReally? `enum object_type` is not passed around?\n\n> Also, we've already decided on the current design a long time ago with\n> the transition plan after extensive, thoughtful discussion by many\n> people.\n\nWho is \"we\"?\n\nI've participated in many discussions in the git mailing list where the\nconsensus is that 99% of people decide to do something, and that\nsomething never happens.\n\nThe fact that \"we\" have decided something doesn't carry as much weight\nas you seem to think it does.\n\nMoreover, haven't \"we\" decided that this transitioning plan is\n*tentantive*, and the SHA-256 feature is *experimental*?\n\n> Very few people other than me have worked on sending patches to\n> work on the hash function transition, and that work up to now has all\n> been done on my personal time, without compensation of any sort, out of\n> a desire to improve the project.\n\nWhich seems to suggest if there is a need, it's not very pressing.\n\nDoesn't it?\n\n> Lots of people have opined on how it should have been different\n> without sending any patches.\n\nAs is typical.\n\n> If you would like to propose patches for the extensive amount of work\n> to implement your solution, then we could consider them, although I\n> will warn you that your approach will likely require at least several\n> hundred patches.\n\nThat's not an issue. I've started projects with several hundred patches\njust to prove that something is possible.\n\n> However, I refer you to the list archives to determine why\n> your approach is not the one we chose and is not, in my view, the best\n> path forward.\n\nYeah? Provide me with *one* mail proposing my approach.\n\n> I should also be clear that I have no intention of submitting patches\n> to change our approach now or in the future, or redoing the patches\n> I've already sent.\n\nYou don't have to. (and it's not really necessary as it's typically the\ncase that people don't provide patches for designs that compete against\ntheir own).\n\n> > The fact that apparently it's so easy to clone a repository with\n> > the wrong hash algorithm should give developers pause, as it means the\n> > whole point of using cryptographic hash algorithms to ensure the\n> > integrity of the commit history is completely gone.\n> \n> No, it doesn't. It means that our empty repositories until recently\n> lacked any indication of the algorithm or other capabilities, which was\n> a mistake in our original protocol design that has now been corrected.\n\nYes it does.\n\nCan I clone a repository that already transitioned to SHA-256, and then\npush a SHA-1 commit?\n\nWell, of course I can't because that's not currently implemented, but if\nwe followed the current plan that apparently \"we\" have decided on, it\nshould be.\n\n> If you interact with the repository later on when it has data, then if\n> you're using the wrong hash algorithm, you'll find that you get a\n> helpful error message that that's not yet supported.\n\nThat isn't true according to your plan.\n\nSHA-1 would be interchangeable with SHA-256, would it not? So according\nto the current plan, I would be able to push a SHA-1 commit on a SHA-256\nrepository.\n\n---\n\nIs it not the case that the current plan aims to have support for SHA-1\nand SHA-256 object ids at the same time?\n\nIn other words: in your ideal world, the following object ids would\n*both* refer to the same git object:\n\n 1. 69c786637d7a7fe3b2b8f7d989af095f5f49c3a8\n 2. 2b4ebdace10518280172449c012af17b51e9d46e023a91a5d3dd3a8ad9e4a116\n\nWould they not?\n\n-- \nFelipe Contreras\n"},{"id":"476816","messageId":"ZFlr8PWOPRuLuP6E@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"645857d8e8fd7_4e6129477@chronos.notmuch","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-08T21:38:56Z","receivedAt":"2023-05-08T21:39:55Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-05-08 at 02:00:56, Felipe Contreras wrote:\n> brian m. carlson wrote:\n> > On 2023-05-02 at 23:46:02, Felipe Contreras wrote:\n> > > In my view one repository should be able to have part SHA-1 history,\n> > > part SHA3-256 history, and part BLAKE2b history.\n> > \n> > That is practically very difficult and it means that it's hard to have\n> > confidence in the later history because SHA-1 is weak and you have to\n> > rely on it to verify the SHA-256 history later.\n> \n> Why would I have to rely on SHA-1 to verify the SHA-256 history later\n> on?\n\nIf your history contains mixed and matched hash algorithms, you'll need\nto be able to verify those commits to the root to have any confidence in\na signed commit or tag, which means trusting SHA-1 if you have any SHA-1\ncommits in the repository.\n\n> > Since attacks always get better, SHA-1 will eventually be so weak that\n> > collisions can be computed in the amount of time we now take for MD4\n> > or MD5 collisions (i.e., seconds), and with your plan, we'd have to\n> > retain that history forever with the resulting lack of confidence in\n> > part of the history.\n> \n> We have to do the same with your plan as well.\n> \n> Your plan relies on SHA-256 being interchangeable with SHA-1, so if the\n> Git project decided to switch *today* to SHA-256, we would have two\n> object ids:\n> \n> 1. 69c786637d7a7fe3b2b8f7d989af095f5f49c3a8\n> 2. 2b4ebdace10518280172449c012af17b51e9d46e023a91a5d3dd3a8ad9e4a116\n> \n> This object would refer to a tree and a parent object with SHA-1 ids,\n> which would be OK, because they would be interchangeable with some\n> corresponding SHA-256 ids.\n> \n> Isn't that your plan?\n\nFor a period of time, yes. At some point, people will abandon SHA-1 and\nwon't use it anymore for a particular repository, and then its security\ndoesn't matter.\n\n> Therefore the SHA-1 of the parent of the commit, and the tree of the\n> commit would be trusted and retained forever.\n\nNope. At some point, we just turn off SHA-1.\n\n> > This also doesn't work with various structures like trees, the index,\n> > and pack and index formats, which have no indication of the algorithm\n> > used and simply rely on fixed-size, often 4-byte aligned object IDs\n> > without any metadata.\n> \n> So? The index and pack objects can be regenerated, so at any point in\n> time they could be regenerated for SHA-1 or SHA-256.\n\nRight, and that point you've basically converted the repository over.\n\n> The tree object is a no-brainer. For an object of type \"commit:256\" you\n> require a tree of type \"tree:256\". Easy.\n\nThat doesn't work with the pack format because there are only seven\nvalid types of objects, and five of them are used.\n\n> > Also, we've already decided on the current design a long time ago with\n> > the transition plan after extensive, thoughtful discussion by many\n> > people.\n> \n> Who is \"we\"?\n\nThe list members.\n\n> I've participated in many discussions in the git mailing list where the\n> consensus is that 99% of people decide to do something, and that\n> something never happens.\n> \n> The fact that \"we\" have decided something doesn't carry as much weight\n> as you seem to think it does.\n\nJonathan Nieder decided to propose an approach for how we'd go about\nthis, and it was discussed extensively on the list and the parts that\nI've implemented have almost completely conformed to that documentation.\n\nI think his approach was very thoughtful and addressed many questions\nabout how the project was to proceed, and I appreciate that he sent it\nand that others contributed in a helpful way.\n\nThe project wouldn't have been possible unless we had a clear decision\non how to implement things. There was an opportunity at the time to\ncomment on the approach and propose alternatives, and we didn't choose\nto adopt any.\n\n> Moreover, haven't \"we\" decided that this transitioning plan is\n> *tentantive*, and the SHA-256 feature is *experimental*?\n\nWe've documented that it's experimental because it's not seeing wide\nuse. I expect that will change, at which point it will no longer be\nexperimental. The design is not tentative and there are no plans to\nchange it.\n\n> > Very few people other than me have worked on sending patches to\n> > work on the hash function transition, and that work up to now has all\n> > been done on my personal time, without compensation of any sort, out of\n> > a desire to improve the project.\n> \n> Which seems to suggest if there is a need, it's not very pressing.\n> \n> Doesn't it?\n\nNo, I don't agree. An approach to continue to use SHA-1 indefinitely\nmeans that Git will not be viable at many major organizations and\nin many major governments which have restrictions on using insecure\ncryptography.\n\nI think this ends the point at which I'd like to respond to your\nproposal further. I continue to feel that it's argumentative,\nunproductive, and not in the best interests of the project, and I don't\nthink continuing here is going to shed any more light on the topic.\n\nUltimately, I feel that my previous statement that we're not going to\nsee eye to eye on most things continues to be correct. I also don't\nthink arguing with you is a productive use of my time or a positive\ncontribution to the community, and in general, I'm going to avoid\ncommenting on your patches or proposals because I can't see a way that\nwe can interact in a useful and helpful way on the list or elsewhere. As\na result, I'll kindly ask you to refrain from CCing me on further emails\nto the list or otherwise emailing me for the indefinite future, and I'll\ndo the same for you after this email.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"476870","messageId":"ZFohLeq1AfdVKqfY@ugly","threadId":"59550","inReplyTo":"ZFlr8PWOPRuLuP6E@tapette.crustytoothpaste.net","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-05-09T10:32:13Z","receivedAt":"2023-05-09T10:32:34Z","isPatch":false,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, May 08, 2023 at 09:38:56PM +0000, brian m. carlson wrote:\n>On 2023-05-08 at 02:00:56, Felipe Contreras wrote:\n>> brian m. carlson wrote:\n>> > On 2023-05-02 at 23:46:02, Felipe Contreras wrote:\n>> > > In my view one repository should be able to have part SHA-1 history,\n>> > > part SHA3-256 history, and part BLAKE2b history.\n>> > \n>> > That is practically very difficult and it means that it's hard to have\n>> > confidence in the later history because SHA-1 is weak and you have to\n>> > rely on it to verify the SHA-256 history later.\n>> \n>> Why would I have to rely on SHA-1 to verify the SHA-256 history later\n>> on?\n>\n>If your history contains mixed and matched hash algorithms, you'll need\n>to be able to verify those commits to the root to have any confidence in\n>a signed commit or tag, which means trusting SHA-1 if you have any SHA-1\n>commits in the repository.\n>\nthe history is traversed from the end anyway, so having sha-1 in the \nhistory is entirely irrelevant for verifying sha-256 commits, assuming \none may only upgrade the algorithm.\n\nthe transition plan implies the intent to ultimately get rid of old \nalgos, but this is a non-starter, because old histories need to remain \naccessible indefinitely (you can't rewrite all external references, and \neven for in-history references this would be unreliable and would \nfalsify historical builds).\n\ni won't try making an argument for mixed histories, as i'm assuming i \nwouldn't add anything that hasn't already been written.\n\n-- ossi\n"},{"id":"476889","messageId":"xmqqild11mg6.fsf@gitster.g","threadId":"59550","inReplyTo":"ZFohLeq1AfdVKqfY@ugly","subject":"Re: Is GIT_DEFAULT_HASH flawed?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-09T16:47:21Z","receivedAt":"2023-05-09T16:47:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n>>If your history contains mixed and matched hash algorithms, you'll need\n>>to be able to verify those commits to the root to have any confidence in\n>>a signed commit or tag, which means trusting SHA-1 if you have any SHA-1\n>>commits in the repository.\n>>\n> the history is traversed from the end anyway, so having sha-1 in the\n> history is entirely irrelevant for verifying sha-256 commits, assuming\n> one may only upgrade the algorithm.\n\nThat depends on what is meant by \"verify a commit\". If we are only\ninterested in the tree contents, then the newer commits whose trees\nare hashed with a more secure hash algorithm recursively down to the\nblobs would lack any weak link hashed by a less secure algorithm.\nMost people do not care as deeply how their project tree came to the\nshape it has today as they care about what is in the recent trees,\nso this is an acceptable stance to take.\n\nIf the less secure algorithm becomes so weak that the history, up to\nthe last commit that was signed by it, can be rewritten arbitrarily,\nhowever, an attacker can lie about why the code that survives to\nthis day looks the way it does by forging old parts of the history,\nand mislead today's developers to do wrong things. The only way to\nprevent that kind of attack is to verify a commit by recursively\nmaking sure not just the commit and its tree, but its parents are\nall authentic, and less secure algorithm may make it impossible.\n\nThe old history hashed with the old algorithm needs to be kept to\nhelp external references, but I think as a mitigation, those who\ncare about that part of history can create a copy of the history\nhashed with the new algorithm and publish the correspondence between\nthe two parallel histories to assure the integrity of that part of\nthe old history.\n\n"},{"id":"477477","messageId":"20230517192443.1149190-1-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"ZEmMUFR7AJn+v7jV@tapette.crustytoothpaste.net","subject":"[PATCH v3 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-17T19:24:42Z","receivedAt":"2023-05-17T19:25:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We recently fixed empty clones with SHA-256 over protocol v2 by\nhonouring the hash algorithm specified even when no refs are present.\nHowever, in doing so, we made it impossible to set up a v0 or v1\nrepository by cloning from an empty SHA-256 repository. In doing so, we\nalso broke the Git LFS testsuite for SHA-256 repositories.\n\nThis series introduces the dummy `capabilities^{}` entry for fetches and\nclones from an empty repository for v0 and v1, just as we do for clones.\nThis is already supported by older versions of Git, as well as libgit2,\ndulwich, and JGit.\n\nChanges since v2:\n* Move advertisement of fake capabilities ref to a separate function to\n avoid an extra strcmp.\n\nChanges since v1:\n* Drop patch to honour GIT_DEFAULT_HASH\n* Support all requests, not just HTTP.\n* Add more tests.\n* Fix NULL pointer dereference.\n\nbrian m. carlson (1):\n upload-pack: advertise capabilities when cloning empty repos\n\n t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n t/t5700-protocol-v1.sh | 31 +++++++++++++++++++++++++++++--\n upload-pack.c | 22 +++++++++++++++++-----\n 3 files changed, 73 insertions(+), 7 deletions(-)\n\n"},{"id":"477478","messageId":"20230517192443.1149190-2-sandals@crustytoothpaste.net","threadId":"59550","inReplyTo":"20230517192443.1149190-1-sandals@crustytoothpaste.net","subject":"[PATCH v3 1/1] upload-pack: advertise capabilities when cloning empty repos","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-17T19:24:43Z","receivedAt":"2023-05-17T19:25:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"From: \"brian m. carlson\" \n\nWhen cloning an empty repository, protocol versions 0 and 1 currently\noffer nothing but the header and flush packets for the /info/refs\nendpoint. This means that no capabilities are provided, so the client\nside doesn't know what capabilities are present.\n\nHowever, this does pose a problem when working with SHA-256\nrepositories, since we use the capabilities to know the remote side's\nobject format (hash algorithm). As of 8b214c2e9d (\"clone: propagate\nobject-format when cloning from void\", 2023-04-05), this has been fixed\nfor protocol v2, since there we always read the hash algorithm from the\nremote.\n\nFortunately, the push version of the protocol already indicates a clue\nfor how to solve this. When the /info/refs endpoint is accessed for a\npush and the remote is empty, we include a dummy \"capabilities^{}\" ref\npointing to the all-zeros object ID. The protocol documentation already\nindicates this should _always_ be sent, even for fetches and clones, so\nlet's just do that, which means we'll properly announce the hash\nalgorithm as part of the capabilities. This just works with the\nexisting code because we share the same ref code for fetches and clones,\nand libgit2, JGit, and dulwich do as well.\n\nThere is one minor issue to fix, though. If we called send_ref with\nnamespaces, we would return NULL with the capabilities entry, which\nwould cause a crash. Instead, let's refactor out a function to print\njust the ref itself without stripping the namespace and use it for our\nspecial capabilities entry.\n\nAdd several sets of tests for HTTP as well as for local clones. The\nbehavior can be slightly different for HTTP versus a local or SSH clone\nbecause of the stateless-rpc functionality, so it's worth testing both.\n\nSigned-off-by: brian m. carlson \n---\n t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n t/t5700-protocol-v1.sh | 31 +++++++++++++++++++++++++++++--\n upload-pack.c | 22 +++++++++++++++++-----\n 3 files changed, 73 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 0908534f25..21b7767cbd 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -611,6 +611,33 @@ test_expect_success 'client falls back from v2 to v0 to match server' '\n \tgrep symref=HEAD:refs/heads/ trace\n '\n \n+test_expect_success 'create empty http-accessible SHA-256 repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/sha256.git\" &&\n+\t git --bare init --object-format=sha256\n+\t)\n+'\n+\n+test_expect_success 'clone empty SHA-256 repository with protocol v2' '\n+\trm -fr sha256 &&\n+\techo sha256 >expected &&\n+\tgit -c protocol.version=2 clone \"$HTTPD_URL/smart/sha256.git\" &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp actual expected &&\n+\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'clone empty SHA-256 repository with protocol v0' '\n+\trm -fr sha256 &&\n+\techo sha256 >expected &&\n+\tGIT_TRACE=1 GIT_TRACE_PACKET=1 git -c protocol.version=0 clone \"$HTTPD_URL/smart/sha256.git\" &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp actual expected &&\n+\tgit ls-remote \"$HTTPD_URL/smart/sha256.git\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_expect_success 'passing hostname resolution information works' '\n \tBOGUS_HOST=gitbogusexamplehost.invalid &&\n \tBOGUS_HTTPD_URL=$HTTPD_PROTO://$BOGUS_HOST:$LIB_HTTPD_PORT &&\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex 6c8d4c6cf1..a73b4d4ff6 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -244,15 +244,28 @@ test_expect_success 'push with ssh:// using protocol v1' '\n \tgrep \"push< version 1\" log\n '\n \n+test_expect_success 'clone propagates object-format from empty repo' '\n+\ttest_when_finished \"rm -fr src256 dst256\" &&\n+\n+\techo sha256 >expect &&\n+\tgit init --object-format=sha256 src256 &&\n+\tgit clone --no-local src256 dst256 &&\n+\tgit -C dst256 rev-parse --show-object-format >actual &&\n+\n+\ttest_cmp expect actual\n+'\n+\n # Test protocol v1 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-test_expect_success 'create repo to be served by http:// transport' '\n+test_expect_success 'create repos to be served by http:// transport' '\n \tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" &&\n \tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" config http.receivepack true &&\n-\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one\n+\ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" one &&\n+\tgit init --object-format=sha256 \"$HTTPD_DOCUMENT_ROOT_PATH/sha256\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/sha256\" config http.receivepack true\n '\n \n test_expect_success 'clone with http:// using protocol v1' '\n@@ -269,6 +282,20 @@ test_expect_success 'clone with http:// using protocol v1' '\n \tgrep \"git< version 1\" log\n '\n \n+test_expect_success 'clone with http:// using protocol v1 with empty SHA-256 repo' '\n+\tGIT_TRACE_PACKET=1 GIT_TRACE_CURL=1 git -c protocol.version=1 \\\n+\t\tclone \"$HTTPD_URL/smart/sha256\" sha256 2>log &&\n+\n+\techo sha256 >expect &&\n+\tgit -C sha256 rev-parse --show-object-format >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Client requested to use protocol v1\n+\tgrep \"Git-Protocol: version=1\" log &&\n+\t# Server responded using protocol v1\n+\tgrep \"git< version 1\" log\n+'\n+\n test_expect_success 'fetch with http:// using protocol v1' '\n \ttest_commit -C \"$HTTPD_DOCUMENT_ROOT_PATH/http_parent\" two &&\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 08633dc121..d3312006a3 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -120,6 +120,7 @@ struct upload_pack_data {\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n \tunsigned advertise_sid : 1;\n+\tunsigned sent_capabilities : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -1206,18 +1207,17 @@ static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) {\n \t\tstrbuf_addf(buf, \" session-id=%s\", trace2_session_id());\n }\n \n-static int send_ref(const char *refname, const struct object_id *oid,\n-\t\t int flag UNUSED, void *cb_data)\n+static void write_v0_ref(struct upload_pack_data *data,\n+\t\t\tconst char *refname, const char *refname_nons,\n+\t\t\tconst struct object_id *oid)\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow deepen-since deepen-not\"\n \t\t\" deepen-relative no-progress include-tag multi_ack_detailed\";\n-\tconst char *refname_nons = strip_namespace(refname);\n \tstruct object_id peeled;\n-\tstruct upload_pack_data *data = cb_data;\n \n \tif (mark_our_ref(refname_nons, refname, oid, &data->hidden_refs))\n-\t\treturn 0;\n+\t\treturn;\n \n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n@@ -1240,12 +1240,20 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\t\t git_user_agent_sanitized());\n \t\tstrbuf_release(&symref_info);\n \t\tstrbuf_release(&session_id);\n+\t\tdata->sent_capabilities = 1;\n \t} else {\n \t\tpacket_fwrite_fmt(stdout, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n \tif (!peel_iterated_oid(oid, &peeled))\n \t\tpacket_fwrite_fmt(stdout, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n+\treturn;\n+}\n+\n+static int send_ref(const char *refname, const struct object_id *oid,\n+\t\t int flag UNUSED, void *cb_data)\n+{\n+\twrite_v0_ref(cb_data, refname, strip_namespace(refname), oid);\n \treturn 0;\n }\n \n@@ -1379,6 +1387,10 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,\n \t\t\tdata.no_done = 1;\n \t\thead_ref_namespaced(send_ref, &data);\n \t\tfor_each_namespaced_ref(send_ref, &data);\n+\t\tif (!data.sent_capabilities) {\n+\t\t\tconst char *refname = \"capabilities^{}\";\n+\t\t\twrite_v0_ref(&data, refname, refname, null_oid());\n+\t\t}\n \t\t/*\n \t\t * fflush stdout before calling advertise_shallow_grafts because send_ref\n \t\t * uses stdio.\n"},{"id":"477491","messageId":"xmqqv8gqoci0.fsf@gitster.g","threadId":"59550","inReplyTo":"20230517192443.1149190-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-17T21:48:39Z","receivedAt":"2023-05-17T21:48:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> We recently fixed empty clones with SHA-256 over protocol v2 by\n> honouring the hash algorithm specified even when no refs are present.\n> However, in doing so, we made it impossible to set up a v0 or v1\n> repository by cloning from an empty SHA-256 repository. In doing so, we\n> also broke the Git LFS testsuite for SHA-256 repositories.\n>\n> This series introduces the dummy `capabilities^{}` entry for fetches and\n> clones from an empty repository for v0 and v1, just as we do for clones.\n> This is already supported by older versions of Git, as well as libgit2,\n> dulwich, and JGit.\n>\n> Changes since v2:\n> * Move advertisement of fake capabilities ref to a separate function to\n> avoid an extra strcmp.\n\nWe want this in -rc2 if not -rc1 for the upcoming release, right?\nI've read the patch again and it all looked sensible.\n\nThanks.\n\n\n> Changes since v1:\n> * Drop patch to honour GIT_DEFAULT_HASH\n> * Support all requests, not just HTTP.\n> * Add more tests.\n> * Fix NULL pointer dereference.\n>\n> brian m. carlson (1):\n> upload-pack: advertise capabilities when cloning empty repos\n>\n> t/t5551-http-fetch-smart.sh | 27 +++++++++++++++++++++++++++\n> t/t5700-protocol-v1.sh | 31 +++++++++++++++++++++++++++++--\n> upload-pack.c | 22 +++++++++++++++++-----\n> 3 files changed, 73 insertions(+), 7 deletions(-)\n"},{"id":"477499","messageId":"ZGVVEafBY3Rs65uD@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"xmqqv8gqoci0.fsf@gitster.g","subject":"Re: [PATCH v3 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-17T22:28:33Z","receivedAt":"2023-05-17T22:28:41Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-05-17 at 21:48:39, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > We recently fixed empty clones with SHA-256 over protocol v2 by\n> > honouring the hash algorithm specified even when no refs are present.\n> > However, in doing so, we made it impossible to set up a v0 or v1\n> > repository by cloning from an empty SHA-256 repository. In doing so, we\n> > also broke the Git LFS testsuite for SHA-256 repositories.\n> >\n> > This series introduces the dummy `capabilities^{}` entry for fetches and\n> > clones from an empty repository for v0 and v1, just as we do for clones.\n> > This is already supported by older versions of Git, as well as libgit2,\n> > dulwich, and JGit.\n> >\n> > Changes since v2:\n> > * Move advertisement of fake capabilities ref to a separate function to\n> > avoid an extra strcmp.\n> \n> We want this in -rc2 if not -rc1 for the upcoming release, right?\n> I've read the patch again and it all looked sensible.\n\nIf it's possible, that would be great. I understand you just put out\n-rc0, and I apologize for the delay in getting back to you, but it would\nbe ideal to avoid having the problem we're fixing in the release.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"477535","messageId":"20230518182838.GB557383@coredump.intra.peff.net","threadId":"59550","inReplyTo":"20230517192443.1149190-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-18T18:28:38Z","receivedAt":"2023-05-18T18:28:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2023 at 07:24:42PM +0000, brian m. carlson wrote:\n\n> Changes since v2:\n> * Move advertisement of fake capabilities ref to a separate function to\n> avoid an extra strcmp.\n\nThanks, the code change looks good to me.\n\nI'm still a little puzzled why we need http tests both in t5551 and\nt5700. Not too big a deal to have redundant tests, obviously, but I\nwonder if I am missing something.\n\n-Peff\n"},{"id":"477598","messageId":"ZGeWmZzbPFviQBmB@tapette.crustytoothpaste.net","threadId":"59550","inReplyTo":"20230518182838.GB557383@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/1] Fix empty SHA-256 clones with v0 and v1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-05-19T15:32:41Z","receivedAt":"2023-05-19T15:32:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-05-18 at 18:28:38, Jeff King wrote:\n> I'm still a little puzzled why we need http tests both in t5551 and\n> t5700. Not too big a deal to have redundant tests, obviously, but I\n> wonder if I am missing something.\n\nTechnically, they work with v0 and v1, which are separate, and I wanted\nto test both, not just v0. I realize that practically, they are very\nsimilar, but I prefer to be a bit more thorough.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"}]}