Re: [PATCH v4 01/12] t: add test functions to translate hash-related values

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 4, 2018, 00:01 UTC
Message-ID: <CAPig+cQZMEvJZ2b3RjtXvy9ZtTjRKUvye_xT0nibOqcHtpApZQ@mail.gmail.com>
In-Reply-To: <20180903232515.336397-2-sandals@crustytoothpaste.net>

On Mon, Sep 3, 2018 at 7:25 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 9 quoted lines

> Add several test functions to make working with various hash-related
> values easier.
> [...]
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -1155,3 +1155,70 @@ depacketize () {
> +# Detect the hash algorithm in use.
> +test_detect_hash () {
> +       test_hash_algo='sha1'
> +}

Not worth a re-roll itself, but if you do re-roll for some reason, perhaps include a comment inside the function saying that the actual "detect" part will be fleshed out later.

> +test_oid_cache () {
> +       local tag reset k v

s/reset/rest/

It probably wouldn't hurt to &&-chain the above, as well, to future-proof it against someone coming along and inserting code before the 'local' line.

Show 13 quoted lines

> +       test -n "$test_hash_algo" || test_detect_hash &&
> +       while read tag rest
> +       do
> +               case $tag in
> +               [...]
> +               esac &&
> +
> +               k="${rest%:*}" &&
> +               v="${rest#*:}" &&
> +
> +               if ! expr "$k" : '[a-z0-9][a-z0-9]*$' >/dev/null
> +               then
> +                       error 'bug in the test script: bad hash algorithm'

Did you want to redirect this to stderr like you do with error() in test_oid()?

> +               fi &&
> +               eval "test_oid_${k}_$tag=\"\$v\""
> +       done
> +}