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

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

> [...]
> > --- /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?

> [...]
> > --- 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.

> [...]
> > --- 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?

> > +		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}"

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