Re: [PATCH v3 01/11] t: add tool to translate hash-related values

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 29, 2018, 04:05 UTC
Message-ID: <20180829040548.GB197350@aiede.svl.corp.google.com>
In-Reply-To: <20180829005642.980617-2-sandals@crustytoothpaste.net>

Hi,

brian m. carlson wrote:

> Add a test function helper, test_oid, that produces output that varies
> depending on the hash in use.

Cool!

Show 9 quoted lines

>                                Add two additional helpers,
> test_oid_cache, which can be used to load data for test_oid from
> standard input, and test_oid_init, which can be used to load certain
> fixed values from lookup charts.  Check that these functions work in
> t0000, as the rest of the testsuite will soon come to depend on them.
>
> Implement two basic lookup charts, one for common invalid or synthesized
> object IDs, and one for various facts about the hash function in use.
> Provide versions for both SHA-1 and SHA-256.

What do test_oid_cache and test_oid_init do?  How can I use them?

Judging from t0000-basic.sh, the idea looks something like

	Add a test function helper, test_oid, that ...

	test_oid allows looking up arbitrary information about an object format:
	the length of object ids, values of well known object ids, etc.  Before
	calling it, a test script must invoke test_oid_cache (either directly
	or indirectly through test_oid_init) to load the lookup charts.

	See t0000 for an example, which also serves as a sanity-check that
	these functions work in preparation for using them in the rest of the
	test suite.

	There are two basic lookup charts for now: oid-info/oid, with common
	invalid or synthesized object IDs; and oid-info/hash-info, with facts
	such as object id length about the formats in use.  The charts contain
	information about both SHA-1 and SHA-256.

	So now you can update existing tests to be format-independent by (1)
	adding an invocation of test_oid_init to test setup and (2) replacing
	format dependencies with $(test_oid foo).

	Since values are stored as shell variables, names used for lookup can
	only consist of shell identifier characters.  If this is a problem in
	the future, we can hash the names before use.

	Improved-by: Eric Sunshine <sunshine@sunshineco.com>

Do these always use sha1 for now? Ah, t0000 answers but it might be worth mentioning in the commit message, too:

	test_set_hash allows setting which object format test_oid should look
	up information for, and test_detect_hash returns to the default format.

[...]

Show 5 quoted lines

> --- /dev/null
> +++ b/t/oid-info/hash-info
> @@ -0,0 +1,8 @@
> +rawsz sha1:20
> +rawsz sha256:32

Can there be a README in this directory describing the files and format?

[...]

Show 7 quoted lines

> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -821,6 +821,41 @@ test_expect_success 'tests clean up even on failures' "
>  	EOF
>  "
>  
> +test_oid_init

Can this be wrapped in test_expect_success? That way, if it fails or prints an error message then the usual test machinery would handle it.

> +
> +test_expect_success 'test_oid provides sane info by default' '
> +	test_oid zero >actual &&
> +	grep "^00*$" actual &&

nit: can save the reader some confusion by escaping the $.

> +	rawsz="$(test_oid rawsz)" &&
> +	hexsz="$(test_oid hexsz)" &&

optional: no need for these quotation marks --- a command substitution assigned to a shell variable is treated as if it were quoted.

> +	test "$hexsz" -eq $(wc -c <actual) &&
> +	test $(( $rawsz * 2)) -eq "$hexsz"

Makes sense.

[...]

> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1155,3 +1155,47 @@ depacketize () {

[...]

> +test_oid_cache () {
> +	test -n "$test_hash_algo" || test_detect_hash

Should this use an uninterrupted &&-chain?

> +	while read _tag _rest

This appears to be the first use of this naming convention. I wonder if we can use "local" instead.

Show 11 quoted lines

> +	do
> +		case $_tag in
> +		\#*)
> +			continue;;
> +		?*)
> +			# non-empty
> +			;;
> +		*)
> +			# blank line
> +			continue;;
> +

unnecessary blank line here

Show 7 quoted lines

> +		esac &&
> +
> +		_k="${_rest%:*}" &&
> +		_v="${_rest#*:}" &&
> +		{ echo "$_k" | egrep '^[a-z0-9]+$' >/dev/null ||
> +			error 'bug in the test script: bad hash algorithm'; } &&
> +		eval "test_oid_${_k}_$_tag=\"\$_v\"" || return 1

This is dense, so I'm having trouble taking it in at a glance.

I think the idea is

		key=${rest%%:*} &&
		val=${rest#*:} &&

		if ! expr "$key" : '[a-z0-9]*$' >/dev/null
		then
			error ...
		fi &&
		eval "test_oid_${key}_${tag}=\${val}"

Show 8 quoted lines

> +	done
> +}
> +
> +test_oid () {
> +	eval "
> +		test -n \"\${test_oid_${test_hash_algo}_$1+set}\" &&
> +		printf '%s' \"\${test_oid_${test_hash_algo}_$1}\"
> +	"

I'm also having trouble taking this one in. Maybe splitting into two evals would work?

	var=test_oid_${test_hash_algo}_$1 &&

	eval "test -n \"\${$var+set}\"" &&
	eval "printf '%s\n' \"\${$var}\""

What is the initial test meant to do? Can this function get a documentation comment? Are we relying on "test -n" to return a failing result if the variable is unset, or could the test be omitted (relying on "\${$var}" to evaluate to "" when the variable is unset)? Should this call 'error' when the variable is unset?

Can t/README describe the new test helpers?

Thanks, Jonathan