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

From: Patrick Steinhardt <ps@pks.im>
Date: May 28, 2024, 11:55 UTC
Message-ID: <ZlXGI4lkaXcD7_8C@tanuki>
In-Reply-To: <8f488a5eeaaa0cdb525c34c2c165e6ee74ee7691.1716824518.git.gitgitgadget@gmail.com>

On Mon, May 27, 2024 at 03:41:54PM +0000, Xing Xin via GitGitGadget wrote:

> From: Xing Xin <xingxin.xx@bytedance.com>

[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]

Show 6 quoted lines

> 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]

Show 17 quoted lines

> +#########################################################################
> +# 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