Subject: Re: [PATCH v3 1/4] bundle-uri: verify oid before writing refs

From: Patrick Steinhardt <[email-masked]>
Date: Tue, 28 May 2024 13:55:15 +0200

On Mon, May 27, 2024 at 03:41:54PM +0000, Xing Xin via GitGitGadget wrote:
> From: Xing Xin <[email-masked]>
[snip]
> 5. The `verify_bundle` will call `parse_object`, within which the
>    `prepare_packed_git` or `reprepare_packed_git` is eventually called,
>    which means that the `raw_object_store->packed_git` data gets filled
>    in and ``packed_git_initialized` is set. This also means consecutive

s/``/`/

[snip]
> This commit fixes this bug by dropping the `REF_SKIP_OID_VERIFICATION`
> flag when writing bundle refs, so we can:
> 
> 1. Ensure that the bundle refs we are writing are pointing to valid
>    objects.
> 2. Ensure all the tips from bundle refs can be correctly parsed.

I think one angle that your explanation doesn't cover is why exactly
dropping the flag fixes the observed issue.

> And a set of negotiation related tests for bundle-uri are added.

s/And/Add/

[snip]
> +#########################################################################
> +# Clone negotiation related tests begin here
> +
> +test_expect_success 'negotiation: bundle with part of wanted commits' '
> +	test_when_finished rm -rf trace*.txt &&
> +	GIT_TRACE_PACKET="$(pwd)/trace-packet.txt" \
> +	git clone --no-local --bundle-uri="clone-from/A.bundle" \
> +		clone-from nego-bundle-part &&
> +	git -C nego-bundle-part for-each-ref --format="%(refname)" >refs &&
> +	grep "refs/bundles/" refs >actual &&
> +	cat >expect <<-\EOF &&
> +	refs/bundles/topic
> +	EOF
> +	test_cmp expect actual &&
> +	# Ensure that refs/bundles/topic are sent as "have".
> +	grep "clone> have $(git -C clone-from rev-parse A)" trace-packet.txt
> +'

As far as I can see there is no test that verifies the case where the
bundle contains refs, but misses the objects to satisfy the refs. Can we
craft such a bundle and exercise this new failure mode?

Patrick