Re: [PATCH 2/3] upload-pack: skip parse-object re-hashing of "want" objects

From: Jeff King <peff@peff.net>
Date: Sep 7, 2022, 20:36 UTC
Message-ID: <YxkAxutS+B8//0WF@coredump.intra.peff.net>
In-Reply-To: <xmqq1qsnugsu.fsf@gitster.g>

On Wed, Sep 07, 2022 at 12:26:41PM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Jeff King <peff@peff.net> 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