On Tue, Aug 28, 2018 at 09:05:48PM -0700, Jonathan Nieder wrote:
> > 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@xxxxxxxxxxxxxx>
>
> 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.
I'll expand the commit message. They do use SHA-1 for now, but I have a
branch that makes them use SHA-256 instead.
> [...]
> > --- /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?
Sure, if you like.
> [...]
> > --- 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.
Sure.
> > +
> > +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 $.
Good point.
> > + 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.
That's good to know. I will honestly say that looking through the Git
codebase and getting reviews on the list has taught me huge amounts
about the intricacies of shell.
> [...]
> > --- 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?
Yes. Will fix.
> > + while read _tag _rest
>
> This appears to be the first use of this naming convention. I wonder
> if we can use "local" instead.
We probably can. There was a discussion about this elsewhere, and we
determined that it's probably safe, and if it's not, it should be
relatively easy to back out.
> > + 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}"
Yes. I take it that you think that's easier to read, so I'll rewrite it
that way. I will admit a tendency to write code that is more compact,
sometimes (unintentionally) at the cost of readability. Thanks for
providing a sanity check.
I agree that expr is probably better than the echo and egrep.
> > + 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?
Yes. The test -n will return false if the variable is unset, since
${$var+set} evaluates to nothing if the variable is unset and "set" if
it is set. I will admit that I had to look this up in the shell
documentation, so I'm not surprised that this is confusing at first
glance.
Switching to error is probably better.
> Can t/README describe the new test helpers?
Sure. I wasn't aware that there was one to add this to, but now that
you've pointed it out, it makes sense to add them.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
