On Tue, Jun 12, 2018 at 03:29:47AM -0400, Eric Sunshine wrote:
> On Mon, Jun 11, 2018 at 9:05 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > 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.
> 
> It was surprising to see it used for non-OID's (such as hash
> characteristics), but not hard to deal with.
> 
> One could also view this as a generic key/value cache (not specific to
> OID's) with overriding super-key (the hash algorithm, in this case),
> which would allow for more generic name than test_oid(), but we don't
> have to go there presently.

It is essentially that.  I'm happy with the test_oid name provided
others are.  My only concern is that it would be confusing.

I opted to use the same mechanism for hash characteristics because it
seemed better than creating a lot of one-off functions that might have
duplicative implementations.  But I'm open to arguments that having
test_oid_rawsz, test_oid_hexsz, etc. is better.

> > 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.
> 
> Can you give examples of cases in which values will contain spaces? It
> wasn't obvious from this patch series that such a need would arise.
> 
> Are these values totally free-form? If not, some character (such as
> "_", "-", ".", etc.) could act as a stand-in for space. That shouldn't
> be too hard to handle.

t6030, which tests the bisect porcelain, is sensitive to the hash
algorithm because hash values are used as a secondary sort for the
closest commit.  Without totally gutting the test and redoing it, some
solution to produce something resembling a sane commit message would be
helpful.  We will also have cases where we need to provide strings to
printf(1), such as in some of the pack tests.

I have a minor modification to your code which handles that at the cost
of restricting us to one hash-key-value tuple on a line, which I think
is an acceptable tradeoff.

> > 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.
> 
> Is that just a general concern or do you have specific "weird" keys in mind?

I had originally thought of providing only the SHA-1 value instead of a
named key, which would have necessitated allowing arbitrary inputs, but
I ultimately decided that named keys were better.  I also tend to prefer
dashes in inputs over underscores, since it's a bit easier to type, but
that's really a secondary concern.

I think the benefits of an implementation closer to your outweigh the
downsides.

> My original version of test_oid_cache() actually allowed for that by
> caching _all_ information from the tables rather than only values
> corresponding to $test_hash_algo. It looked like this:
> 
> --- >8 ---
> test_oid_cache () {
>     while read tag rest
>     do
>         case $tag in \#*) continue ;; esac
> 
>         for x in $rest
>         do
>             eval "test_oid_${tag}_${x%:*}=${x#*:}"
>         done
>     done
> }
> --- >8 ---
> 
> The hash algorithm is incorporated into the cache variable name like
> this: "test_oid_hexsz_sha256"

Yeah, I basically reimplemented something similar to that based off your
code.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204