Re: [PATCH 01/10] t: add tool to translate hash-related values
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Jun 14, 2018, 00:22 UTC
- Message-ID
- <20180614002205.GN38834@genre.crustytoothpaste.net>
- In-Reply-To
- <CAPig+cSEpCTg+rYQYNQ5wUz3PL+51dr7bQdn-2cOWgg2ScdXNA@mail.gmail.com>
On Tue, Jun 12, 2018 at 03:29:47AM -0400, Eric Sunshine wrote:
Show 13 quoted lines
> 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.
Show 14 quoted lines
> > 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.
Show 5 quoted lines
> > 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.
Show 20 quoted lines
> 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