On Wed, Apr 26, 2023 at 12:20:00AM +0000, brian m. carlson wrote: > In my case, the clone is over HTTP, so this may not be the ideal way to > reproduce it and it may need a better testcase, but it does bisect to > the patch above and it is new in master (and doesn't reproduce in > 2.40.0). Note that in our case in the Git LFS testsuite, we're using > GIT_DEFAULT_HASH=sha256. > > I believe what is happening is that for some reason, the object-format > data in v0 and v1 is not being read properly, and so we're now setting > it to sha1 whereas before we were reading the value from the default > setting of the repository (sha256). I'm having trouble finding any breakage at all. E.g., this test passes: diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 0908534f25..95b10288e7 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -704,4 +704,24 @@ test_expect_success 'no empty path components' ' ! grep "//" log ' +test_expect_success 'v0 clone over http recognizes object-format' ' + git init --bare --object-format=sha256 \ + "$HTTPD_DOCUMENT_ROOT_PATH/sha256.git" && + + # do not test an empty repo. In v0, we have no way for an + # empty server to report its object format, so we would + # always default to sha1. We could in theory test that + # a client who wants to default to sha256 will realize + # the other side is sha1, but we have no way to set that local + # default. Unlike git-init, git-clone does not support + # --object-format, nor GIT_DEFAULT_HASH. + git -C "$HTTPD_DOCUMENT_ROOT_PATH/sha256.git" --work-tree=. \ + commit --allow-empty -m foo && + + git -c protocol.version=0 clone $HTTPD_URL/smart/sha256.git sha256 && + git -C sha256 rev-parse --show-object-format >actual && + echo sha256 >expect && + test_cmp expect actual +' + test_done I'd expect it to break in the empty-repo case, for the reasons given in the comment (v0 cannot communicate object-format in an empty repo). But that is nothing new. It sounds from your description that your test is running in a mode where the client defaults to sha256 (though I'm not sure how, since we explicitly document that GIT_DEFAULT_HASH should not affect clone), and then you clone an empty sha256 repository via v0, expecting the result to be sha256. But I think that is a wrong expectation, at least from the client's perspective. An empty repository cannot communicate its object-format over v0, so the client should assume its v0, and should then itself become v0. And that last "should itself become" is what Junio's patch fixed. The first part, "empty repository cannot communicate its object-format over v0" is the part is "it's always been broken". We could fix it, but I'm not sure if it is worth the trouble (see my other message). > It very well may be that it's always been broken and this has just made > it obvious that it's broken, but I'll look tomorrow and probably send a > patch. I don't think we should revert this change, but I do think we > need to fix it before 2.41, since I think it means right now that all > clones over protocol v0 and v1 end up with a SHA-1 repository. Hopefully my guess at what your test is doing is correct, and I didn't just leave us off on a tangent. ;) But if it is, then I think that everything in Git is OK. Non-empty repos over v0 work correctly both before and after Junio's patch. Empty ones before his patch were erroneously using sha256 if they preferred it locally, even when the other side really was sha1. They _also_ were using sha256 erroneously when the other side was sha256 but wasn't able to report it (because of v0 limitations). Which is counter-intuitive, perhaps, but was still the wrong thing for a client to do. -Peff
