On Sun, Aug 19, 2018 at 1:54 PM brian m. carlson <sand...@crustytoothpaste.net> wrote: > diff --git a/t/oid-info/oid b/t/oid-info/oid > @@ -0,0 +1,29 @@ > +# All zeros or Fs missing one or two hex segments. > +zero_1 sha1:000000000000000000000000000000000000000 > +zero_2 > sha256:000000000000000000000000000000000000000000000000000000000000000 > +zero_2 sha1:00000000000000000000000000000000000000 > +zero_2 > sha256:00000000000000000000000000000000000000000000000000000000000000
Too many zero_2's. I guess the first one should be zero_1. > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > @@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' " > +test_oid_cache hash-info oid > + > +test_expect_success 'test_oid_info provides sane info by default' ' Is "test_oid_info" in the test title outdated? I don't see anything by that name. Same question regarding the other new tests. > + test_oid zero && > + test_oid zero >actual && If the lookup "test_oid zero" fails, it's going to fail whether redirected or not, so the first invocation can be dropped. > + grep "00*" actual && Do you want to anchor this regex? ^00*$ Same comment regarding the other new tests. > + test "$(test_oid hexsz)" -eq $(wc -c <actual) && > + test $(( $(test_oid rawsz) * 2)) -eq "$(test_oid hexsz)" > +' If $(test_oid rawsz) fails to find "rawsz" and returns nothing, this expression will be "*2", which will error out in a less-than-meaningful way. Perhaps it would make more sense to dump the results of the two test_oid() to file and use test_cmp()? Similar comment regarding all the other "test ... -eq ..." expressions here and below: I wonder if it would be better to dump them to files and compare the files. > +test_expect_success 'test_oid_info can look up data for SHA-1' ' > + test_detect_hash sha1 && > + test_oid zero >actual && > + grep "00*" actual && > + test $(wc -c <actual) -eq 40 && > + test "$(test_oid rawsz)" -eq 20 && > + test "$(test_oid hexsz)" -eq 40 > +' > + > +test_expect_success 'test_oid_info can look up data for SHA-256' ' > + test_when_finished "test_detect_hash" && Should the previous test also do this test_when_finished() to protect against someone coming along and re-ordering them some day? Or someone inserting a test between the two? > + test_detect_hash sha256 && > + test_oid zero >actual && > + grep "00*" actual && > + test $(wc -c <actual) -eq 64 && > + test "$(test_oid rawsz)" -eq 32 && > + test "$(test_oid hexsz)" -eq 64 > +' > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > @@ -1147,3 +1147,39 @@ depacketize () { > +test_detect_hash () { > + if test -n "$1" > + then > + test_hash_algo="$1" > + else > + test_hash_algo='sha1' > + fi > +} Mmph. Outside of t0000, do you expect other callers which will need to set the hash algorithm to an explicit value? If not, this sort of polymorphic behavior adds extra, perhaps unnecessary, complexity. Even if you do expect a few other callers, a dedicated test_set_hash() function might be clearer since test_detect_hash() has a very specific meaning. > +test_oid_cache () { > + test -n "$test_hash_algo" || test_detect_hash > + if test -n "$1" > + then > + while test -n "$1" > + do > + test_oid_cache <"$TEST_DIRECTORY/oid-info/$1" What is the benefit of placing test-specific OID info in some dedicated directory? I suppose the idea of lumping all these OID files together in a single oid-info/ directory is that it makes it easier to see at a glance what needs to be changed next time a new hash algorithm is selected. However, that potential long-term benefit comes at the cost of divorcing test-specific information from the tests themselves. I imagine (for myself, at least) that it would be easier to grok a test if its OID's were specified and cached directly in the test script itself (via test_oid_cache <<here-doc). I could even imagine a test script invoking test_oid_cache<<here-doc before each test which has unique OID's in order to localize OID information to the test which needs it, or perhaps even within a test. So, I'm having trouble understanding the benefit of the current approach over merely loading OID's via here-docs in the test scripts themselves. Perhaps the commit message could say something about why the current approach was taken. > + shift > + done > + return $? Why, specifically, return $? here, as opposed to simply returning? > + fi Mmph. This polymorphic, recursive behavior in which test_oid_cache() can load data from a list of files or from its own standard input adds complexity. One alternative would be to have a separate test_oid_cache_file() function. However, I'm wondering why such functionality needs to be built in, in the first place, as opposed to clients merely doing the redirect themselves (test_oid_cache <whatever). I see that you want to support specifying multiple files for a single invocation, but is that likely to come up (aside from t0000)? > + while read _tag _rest > + do > + case $_tag in \#*) > + continue;; > + esac This handles "# comment" lines, but what about blank lines? > + _k="${_rest%:*}" > + _v="${_rest#*:}" Should this be doing any sort of validation, such as complaining if _k or _v is empty? Or if _k contains weird characters? > + eval "test_oid_${_k}_$_tag=\"$_v\"" > + done > +} > + > +test_oid () { > + eval "printf '%s' \"\${test_oid_${test_hash_algo}_$1}\"" > +}