Re: [PATCH v7 1/3] bundle-uri: verify oid before writing refs

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2024, 17:37 UTC
Message-ID: <xmqqr0cuyww9.fsf@gitster.g>
In-Reply-To: <fc9f44fda0032ab1e5ee0c9bcc2886ddb8e17722.1718632536.git.gitgitgadget@gmail.com>

"Xing Xin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> @@ -33,6 +46,16 @@ test_expect_success 'clone with path bundle' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'clone with bundle that has bad header' '
> +	# Write bundle ref fails, but clone can still proceed.
> +	git clone --bundle-uri="clone-from/bad-header.bundle" \
> +		clone-from clone-bad-header 2>err &&
> +	commit_b=$(git -C clone-from rev-parse B) &&
> +	test_grep "trying to write ref '\''refs/bundles/topic'\'' with nonexistent object $commit_b" err &&
> +	git -C clone-bad-header for-each-ref --format="%(refname)" >refs &&
> +	! grep "refs/bundles/" refs

Why not "test_grep !" here? There are other uses of bare grep in the newly added lines, but I won't repeat them here; the same comment applies to them.

Show 5 quoted lines

> +	test_write_lines refs/bundles/topic >expect &&
> +	test_cmp expect actual &&
> +	# Ensure that refs/bundles/topic are sent as "have".
> +	test_grep "clone> have $(git -C clone-from rev-parse A)" trace-packet.txt
> +'

Can this rev-parse fail (the failure would be hidden from the test)? If so,

	...
	test_cmp expect actual &&
	# Ensure that refs/bundles/topic is sent as "have"
	tip=$(git -C clone-from rev-parse A) &&
	test_grep "clone> have $tip" trace-packet.txt

would catch such a failure. You are doing so in the previous test in the hunk starting at 33/46 above with commit_b variable already.

There are other uses of git command in $(command substitution) whose exit status are ignored in the newly added lines, but I won't repeat them here; the same comment applies to them.

Thanks.