Subject: Re: [PATCH v6 1/6] object-name tests: add tests for ambiguous object blind spots

From: Junio C Hamano <[email-masked]>
Date: Thu, 30 Dec 2021 15:36:42 -0800

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> +test_cmp_failed_rev_parse () {
> +	dir=$1
> +	rev=$2
> +	shift

What are we shifting away?

> +	test_must_fail git -C "$dir" rev-parse "$rev" 2>actual.raw &&
> +	sed "s/\($rev\)[0-9a-f]*/\1.../g" <actual.raw >actual &&

I wonder if we need to ensure not to mistakenly produce second hit
in an object name that has $rev twice, e.g. "cafe123cafe..."?

> +	test_cmp expect actual
> +}

It is a bit confusing to _depend_ on the caller to prepare a
fixed-name file, like this.  We've avoided such confusion in
different ways in other tests, like (A) make the helper take
the expected output from its standard input, or (B) make the
helper take the name of the file that has expected output as
its argument.

> +test_expect_success 'ambiguous blob output' '
> +	git init --bare blob.prefix &&
> +	(
> +		cd blob.prefix &&
> +
> +		# Both start with "dead..", under both SHA-1 and SHA-256
> +		echo brocdnra | git hash-object -w --stdin &&
> +		echo brigddsv | git hash-object -w --stdin &&
> +
> +		# Both start with "beef.."
> +		echo 1agllotbh | git hash-object -w --stdin &&
> +		echo 1bbfctrkc | git hash-object -w --stdin
> +	) &&
> +
> +	test_must_fail git -C blob.prefix rev-parse dead &&
> +	cat >expect <<-\EOF &&
> +	error: short object ID beef... is ambiguous
> +	hint: The candidates are:
> +	hint:   beef... blob
> +	hint:   beef... blob
> +	fatal: ambiguous argument '\''beef...'\'': unknown revision or path not in the working tree.
> +	Use '\''--'\'' to separate paths from revisions, like this:
> +	'\''git <command> [<revision>...] -- [<file>...]'\''
> +	EOF
> +	test_cmp_failed_rev_parse blob.prefix beef
> +'
> +
> +test_expect_success 'ambiguous loose blob parsed as OBJ_BAD' '

"loose bad object", as they aren't even blobs, perhaps?

> +	git init --bare blob.bad &&
> +	(
> +		cd blob.bad &&
> +
> +		# Both have the prefix "bad0"
> +		echo xyzfaowcoh | git hash-object -t bad -w --stdin --literally &&
> +		echo xyzhjpyvwl | git hash-object -t bad -w --stdin --literally
> +	) &&
> +
> +	cat >expect <<-\EOF &&
> +	error: short object ID bad0... is ambiguous
> +	hint: The candidates are:
> +	fatal: invalid object type

That indeed is not very nice.

> +	EOF
> +	test_cmp_failed_rev_parse blob.bad bad0
> +'
> +
> +test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
> +	git init --bare blob.corrupt &&
> +	(
> +		cd blob.corrupt &&
> +
> +		# Both have the prefix "cafe"
> +		echo bnkxmdwz | git hash-object -w --stdin &&
> +		oid=$(echo bmwsjxzi | git hash-object -w --stdin) &&
> +
> +		oidf=objects/$(test_oid_to_path "$oid") &&
> +		chmod 755 $oidf &&
> +		echo broken >$oidf
> +	) &&
> +
> +	cat >expect <<-\EOF &&
> +	error: short object ID cafe... is ambiguous
> +	hint: The candidates are:
> +	error: inflate: data stream error (incorrect header check)
> +	error: unable to unpack cafe... header
> +	error: inflate: data stream error (incorrect header check)
> +	error: unable to unpack cafe... header
> +	hint:   cafe... unknown type
> +	hint:   cafe... blob

This is an interesting one.  I _think_ it is clear enough for the
readers that the inflate errors are for the object that immediately
follows them, so as long as we show these hints one by one, the
above output is perfectly fine.  But we'll see.

> +	fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
> +	Use '\''--'\'' to separate paths from revisions, like this:
> +	'\''git <command> [<revision>...] -- [<file>...]'\''
> +	EOF
> +	test_cmp_failed_rev_parse blob.corrupt cafe
> +'
> +
>  if ! test_have_prereq SHA1
>  then
>  	skip_all='not using SHA-1 for objects'