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

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

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

> @@ -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.

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