On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote: > On Mon, Jun 04, 2018 at 11:52:20PM +0000, brian m. carlson wrote: > > Add a test function helper, test_translate, that will produce its first > > argument if the hash in use is SHA-1 and the second if its argument is > > NewHash. Implement a mode that can read entries from a file as well for > > reusability across tests. > > The word "translate" is very generic and is (at least in my mind) > strongly associated with i18n/l10n, so the name test_translate() may > be confusing for readers. Perhaps test_oid_lookup() or test_oid_get() > or even just test_oid()?
test_oid would be fine. One note is that this doesn't always produce OIDs; sometimes it will produce other values, but as long as you don't think that's too confusing, I'm fine with it. > This is a very expensive lookup since it invokes a heavyweight command > (perl, in this case) for *every* OID it needs to retrieve from the > file. Windows users, especially, will likely not be happy about this. > See below for an alternative. I agree perl would be expensive if it were invoked frequently, but excepting SHA1-prerequisite tests, this function is invoked 32 times in the entire testsuite. One of the reasons I chose perl was because we have a variety of cases where we'll need spaces in values, and those tend to be complex in shell. > This is less flexible than I had expected, allowing for only SHA1 and > NewHash. When you had written about OID lookup table functionality in > email previously, my impression was that the tables would allow values > for arbitrary hash algorithms. Such flexibility would allow people to > experiment with hash algorithms without having to once again retrofit > the test suite machinery. I wasn't thinking of that as a goal, but that would be a nice improvement. > Here's what I had envisioned when reading your emails about OID lookup > table functionality: > > --- >8 --- > test_detect_hash () { > test_hash_algo=... > } > > test_oid_cache () { > while read tag rest > do > case $tag in \#*) continue ;; esac > > for x in $rest > do > k=${x%:*} > v=${x#*:} > if test "$k" = $test_hash_algo > then > eval "test_oid_$tag=$v" > break > fi > done > done > } > > test_oid () { > if test $# -gt 1 > then > test_oid_cache <<-EOF > $* > EOF > fi > eval "echo \$test_oid_$1" > } Using shell variables like this does have the downside that we're restricted to only characters allowed in shell variables. That was something I was trying to avoid, but it certainly isn't fatal. > test_detect_hash() would detect the hash algorithm and record it > instead of having to determine it each time an OID needs to be > "translated". It probably would be called by test-lib.sh. We'll probably have to deal with multiple hashes in the future, including for input and output, but this could probably be coerced to handle that case. > And, when specifying values from which to choose based upon hash > algorithm: > > $(test_oid bored sha1:deadbeef NewHash:feedface) This syntax won't exactly be usable, because we have to deal with spaces in values. It shouldn't be too much of a problem to just use test_oid_cache at the top of the file, though. > A nice property of how this implementation caches values is that you > don't need test_oid() for really simple cases. You can just access the > variable directly. For instance: $test_oid_hexsz Because we're going to need multiple hash support in the future, for input, output, and on-disk, I feel like this is not a good direction for us to go in the testsuite. Internally, those variable names are likely to change. > Another nice property of how caching is implemented is that someone > testing a new hash algorithm doesn't have edit the existing tables to > tack the value for the new algorithm onto the end of each line. It > works equally well to place those values in a new file or new here-doc > or simply append new lines to existing files or here-docs. For > instance, someone testing algorithm "NewShiny" can just add those > lines without having to modify existing lines: That would be convenient. I like a lot of things about your implementation. This has been helpful feedback; let me think about it some and reroll. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature