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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 30, 2021, 23:36 UTC
Message-ID: <xmqqv8z5mzqt.fsf@gitster.g>
In-Reply-To: <patch-v6-1.6-27f267ad555-20211228T143223Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.

Show 28 quoted lines

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

Show 13 quoted lines

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

Show 27 quoted lines

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

Show 10 quoted lines

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