On Wed, Sep 07, 2022 at 12:26:41PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > The exception for both is if --verify-objects is used. In that case, > > we'll skip this optimization, and the new test makes sure we do this > > correctly. > > I wondered if we want to test the change on the "upload-pack" side > by going in to the swapped-commits repository, running upload-pack > manually and seeing that it spews unusable output without failing, > but it probably is not worth the effort. We have plenty of tests > that exercises upload-pack in "good" cases. What might be a good > test is to try fetching from swapped-commits repository and make > sure that index-pack on the receiving end notices, but I suspect we > already have such a "fetch/clone from a corrupt repository" test, > in which case we do not have to add one. There are some tests in t1060 that cover transfer of corrupted objects. But one of the tricky things about corruption is that it can be somewhat arbitrary where and when we notice. I.e., whether upload-pack notices the problem and bails or whether it spews an invalid result, we're OK with either outcome, and a test for it can end up rather fragile. What we do care about is that _somebody_ notices the problem. t1060 covers that for some cases, though of course there's no partial clone there. If we add one like this: diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index 5b8e47e346..fd18a8b29e 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -139,4 +139,11 @@ test_expect_success 'internal tree objects are not "missing"' ' ) ' +test_expect_success 'partial clone of corrupted reository' ' + test_config -C bit-error uploadpack.allowFilter true && + git clone --no-local --no-checkout --filter=blob:none \ + bit-error corrupt-partial && \ + test_must_fail git -C corrupt-partial checkout --force +' + test_done we can see that the initial blob:none clone is OK (since neither side looks at the corrupted blob at all). And then the checkout does barf as expected. But it looks like we catch the error in upload-pack, probably because the loose object is so corrupted that we cannot even access its type header. I tried moving the corruption to further in the file, but I think it's simply so small that zlib will read the whole input and complain about the bogus crc. Hmm. Looks like that script has another corrupted repo where an object is misnamed. So if we s/bit-error/misnamed/ in the test above, then we do trigger the new code. Before we get: error: hash mismatch d95f3ad14dee633a758d2e331151e950dd13e4ed fatal: git upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed fatal: remote error: upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed (the error is a bit misleading, but I guess the inability to create the object struct means our "is it our ref" code gets confused). After my patches, we get: remote: Enumerating objects: 1, done. remote: Counting objects: 100% (1/1), done. remote: Total 1 (delta 0), reused 0 (delta 0), pack-reused 0 Receiving objects: 100% (1/1), 49 bytes | 49.00 KiB/s, done. fatal: bad revision 'd95f3ad14dee633a758d2e331151e950dd13e4ed' error: [...]/misnamed did not send all necessary objects Is that worth having? I dunno. It's kind of brittle in that a later change could mean we're finding the corruption elsewhere, and not checking exactly what we think we are. OTOH, it probably doesn't hurt to cover more cases here, and it's not a very expensive test. -Peff
