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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Subject: [PATCH] t1060: check partial clone of misnamed blob
>
> A recent commit (upload-pack: skip parse-object re-hashing of "want"
> objects, 2022-09-06) loosened the behavior of upload-pack so that it
> does not verify the sha1 of objects it receives directly via "want"
> requests. The existing corruption tests in t1060 aren't affected by
> this: the corruptions are blobs reachable from commits, and the client
> requests the commits.
>
> The more interesting case here is a partial clone, where the client will
> directly ask for the corrupted blob when it does an on-demand fetch of
> the filtered object. And that is not covered at all, so let's add a
> test.
>
> It's important here that we use the "misnamed" corruption and not
> "bit-error". The latter is sufficiently corrupted that upload-pack
> cannot even figure out the type of the object, so it bails identically
> both before and after the recent change. But with "misnamed", with the
> hash-checks enabled it sees the problem (though the error messages are a
> bit confusing because of the inability to create a "struct object" to
> store the flags):

Makes sense.

>   error: hash mismatch d95f3ad14dee633a758d2e331151e950dd13e4ed
>   fatal: git upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed
>   fatal: remote error: upload-pack: not our ref d95f3ad14dee633a758d2e331151e950dd13e4ed
>
> After the change to skip the hash check, the server side happily sends
> the bogus object, but the client correctly realizes that it did not get
> the necessary data:
>
>   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
>
> which is exactly what we expect to happen.

Thanks.  Let's queue it on top.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t1060-object-corruption.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
> index 5b8e47e346..35261afc9d 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 repository' '
> +	test_config -C misnamed uploadpack.allowFilter true &&
> +	git clone --no-local --no-checkout --filter=blob:none \
> +		misnamed corrupt-partial && \
> +	test_must_fail git -C corrupt-partial checkout --force
> +'
> +
>  test_done