Re: [PATCH 01/10] t: add tool to translate hash-related values
On Tue, Jul 24, 2018 at 6:17 PM brian m. carlson
wrote:
> On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote:
> > Here's what I had envisioned when reading your emails about OID lookup
> > table functionality:
> >
> > --- >8 ---
> > 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"
> > }
> > --- >8 ---
>
> I'd like to use this as a basis for my v2, but I would need your
> sign-off in order to do that. Would that be okay?
You can have my sign-off for the code above.
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote:
> 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"
> }
> --- >8 ---
I'd like to use this as a basis for my v2, but I would need your
sign-off in order to do that. Would that be okay?
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 01/10] t: add tool to translate hash-related values
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
> 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
signature.asc
Description: PGP signature
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Mon, Jun 11, 2018 at 9:05 PM, brian m. carlson
wrote:
> On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote:
>> 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.
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.
>> 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.
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.
>> Here's what I had envisioned when reading your emails about OID lookup
>> table functionality:
>>
>> --- >8 ---
>> 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
>> }
>
> 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?
>> 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.
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"
>> 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.
See above about possibly using a stand-in character for space.
>> 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.
Indeed, this isn't a real selling point; I only just thought of it
while composing the mail. Going through the API is more robust.
(Although, see above how the revised test_oid_cache() incorporates the
hash algorithm into the cache variable name.)
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Mon, Jun 11, 2018 at 03:47:43AM -0400, Eric Sunshine wrote:
> On Mon, Jun 04, 2018 at 11:52:20PM +, 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
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Mon, Jun 04, 2018 at 11:52:20PM +, 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()?
> Signed-off-by: brian m. carlson
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -1147,3 +1147,43 @@ depacketize () {
> +test_translate_f_ () {
> + local file="$TEST_DIRECTORY/translate/$2" &&
> + perl -e '
> + $delim = "\t";
> + ($hoidlen, $file, $arg) = @ARGV;
> + open($fh, "<", $file) or die "open: $!";
> + while (<$fh>) {
> + # Allow specifying other delimiters.
> + $delim = $1 if /^#!\sdelimiter\s(.)/;
> + next if /^#/;
> + @fields = split /$delim/, $_, 3;
> + if ($fields[0] eq $arg) {
> + print($hoidlen == 40 ? $fields[1] : $fields[2]);
> + last;
> + }
> + }
> + ' "$1" "$file" "$3"
> +}
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.
> +# Without -f, print the first argument if we are using SHA-1 and the second
> if
> +# we're using NewHash.
> +# With -f FILE ARG, read the (by default) tab-delimited file from
> +# t/translate/FILE, finding the first field matching ARG and printing either
> the
> +# second or third field depending on the hash in use.
> +test_translate () {
> + local hoidlen=$(printf "%s" "$EMPTY_BLOB" | wc -c) &&
> + if [ "$1" = "-f" ]
> + then
> + shift &&
> + test_translate_f_ "$hoidlen" "$@"
> + else
> + if [ "$hoidlen" -eq 40 ]
> + then
> + printf "%s" "$1"
> + else
> + printf "%s" "$2"
> + fi
> + fi
> +}
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.
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"
}
--- >8 ---
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.
test_oid_cache() reads a table of OID's and caches them so that
subsequent look-ups are fast. It would be called (possibly multiple
times) by any script needing to "translate" OID's. Each line of the
table has form "tag algo:value ...". Lines starting with '#' are
comments (as in your implementation). Since it reads stdin, it works
equally well reading OID tables from files and here-docs, which
provides extra flexibility for test authors. For instance:
test_oid_cache
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Thu, Jun 07, 2018 at 12:57:04AM +, brian m. carlson wrote: > > > Unless I'm wrong, we don't use the "local" keyword ? > > > > We've got a test balloon out; see 01d3a526ad (t: check whether the > > shell supports the "local" keyword, 2017-10-26). I think it's reasonable > > to consider starting its use. > > I used it because it's already in use earlier in the file in some of the > mingw_* functions. Perhaps we happen to know that our mingw systems > will always have a suitable /bin/sh, but I suppose some less capable > shells would still have choked on it by now. We do know in that case; it's always bash under mingw. That said... > I can remove it if necessary, but it didn't seem necessary. I feel OK about starting to use it, with the knowledge that we may get a late-comer who hasn't even tested v2.16.0 yet and says "no, wait! My shell doesn't support local!". And then we'd have to deal with it then. But I suspect that won't happen, or it will turn out that the shell in question is unusable for some other reason anyway. -Peff
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Wed, Jun 06, 2018 at 04:58:46PM -0400, Jeff King wrote:
> On Wed, Jun 06, 2018 at 08:19:27AM +0200, Torsten Bögershausen wrote:
>
> > > +test_translate_f_ () {
> > > + local file="$TEST_DIRECTORY/translate/$2" &&
> >
> > Unless I'm wrong, we don't use the "local" keyword ?
>
> We've got a test balloon out; see 01d3a526ad (t: check whether the
> shell supports the "local" keyword, 2017-10-26). I think it's reasonable
> to consider starting its use.
I used it because it's already in use earlier in the file in some of the
mingw_* functions. Perhaps we happen to know that our mingw systems
will always have a suitable /bin/sh, but I suppose some less capable
shells would still have choked on it by now.
I can remove it if necessary, but it didn't seem necessary.
> > > + perl -e '
> >
> > The bare "perl" is better spelled as "$PERL_PATH"
>
> This use is OK. Since a0e0ec9f7d (t: provide a perl() function which
> uses $PERL_PATH, 2013-10-28), most common uses handle this automatically
> (there are some exceptions, covered in t/README).
This was exactly my reasoning.
> > > + if [ "$1" = "-f" ]
> >
> > Style nit, please avoid [] and use test:
> > if test "$1" = "-f"
>
> This I agree with. :)
Yeah, I forgot that that's our style. I'll fix that.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 01/10] t: add tool to translate hash-related values
On Wed, Jun 06, 2018 at 08:19:27AM +0200, Torsten Bögershausen wrote:
> > +test_translate_f_ () {
> > + local file="$TEST_DIRECTORY/translate/$2" &&
>
> Unless I'm wrong, we don't use the "local" keyword ?
We've got a test balloon out; see 01d3a526ad (t: check whether the
shell supports the "local" keyword, 2017-10-26). I think it's reasonable
to consider starting its use.
> > + perl -e '
>
> The bare "perl" is better spelled as "$PERL_PATH"
This use is OK. Since a0e0ec9f7d (t: provide a perl() function which
uses $PERL_PATH, 2013-10-28), most common uses handle this automatically
(there are some exceptions, covered in t/README).
> > + if [ "$1" = "-f" ]
>
> Style nit, please avoid [] and use test:
> if test "$1" = "-f"
This I agree with. :)
-Peff
Re: [PATCH 01/10] t: add tool to translate hash-related values
Some style nite inline
On Mon, Jun 04, 2018 at 11:52:20PM +, 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.
>
> For the moment, use the length of the empty blob to determine the hash
> in use. In the future, we can change this code so that it can use the
> configuration and learn about the difference in input, output, and
> on-disk formats.
>
> Implement two basic lookup charts, one for common invalid or synthesized
> object IDs, and one for various facts about the hash function in use.
>
> Signed-off-by: brian m. carlson
> ---
> t/test-lib-functions.sh | 40
> t/translate/hash-info | 9 +
> t/translate/oid | 15 +++
> 3 files changed, 64 insertions(+)
> create mode 100644 t/translate/hash-info
> create mode 100644 t/translate/oid
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 2b2181dca0..0e7067460b 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1147,3 +1147,43 @@ depacketize () {
> }
> '
> }
> +
> +test_translate_f_ () {
> + local file="$TEST_DIRECTORY/translate/$2" &&
Unless I'm wrong, we don't use the "local" keyword ?
> + perl -e '
The bare "perl" is better spelled as "$PERL_PATH"
> + $delim = "\t";
> + ($hoidlen, $file, $arg) = @ARGV;
> + open($fh, "<", $file) or die "open: $!";
> + while (<$fh>) {
> + # Allow specifying other delimiters.
> + $delim = $1 if /^#!\sdelimiter\s(.)/;
> + next if /^#/;
> + @fields = split /$delim/, $_, 3;
> + if ($fields[0] eq $arg) {
> + print($hoidlen == 40 ? $fields[1] : $fields[2]);
> + last;
> + }
> + }
> + ' "$1" "$file" "$3"
> +}
> +
> +# Without -f, print the first argument if we are using SHA-1 and the second
> if
> +# we're using NewHash.
> +# With -f FILE ARG, read the (by default) tab-delimited file from
> +# t/translate/FILE, finding the first field matching ARG and printing either
> the
> +# second or third field depending on the hash in use.
> +test_translate () {
> + local hoidlen=$(printf "%s" "$EMPTY_BLOB" | wc -c) &&
> + if [ "$1" = "-f" ]
Style nit, please avoid [] and use test:
if test "$1" = "-f"
And more [] below
> + then
> + shift &&
> + test_translate_f_ "$hoidlen" "$@"
> + else
> + if [ "$hoidlen" -eq 40 ]
> + then
> + printf "%s" "$1"
> + else
> + printf "%s" "$2"
> + fi
> + fi
> +}
> diff --git a/t/translate/hash-info b/t/translate/hash-info
> new file mode 100644
> index 00..36cbd9a8eb
> --- /dev/null
> +++ b/t/translate/hash-info
> @@ -0,0 +1,9 @@
> +# Various facts about the hash algorithm in use for easy access in tests.
> +#
> +# Several aliases are provided for easy recall.
> +rawsz20 32
> +oidlen 20 32
> +hexsz40 64
> +hexoidlen40 64
> +zero
>
> +zero-oid
>
> diff --git a/t/translate/oid b/t/translate/oid
> new file mode 100644
> index 00..8de0fd64af
> --- /dev/null
> +++ b/t/translate/oid
> @@ -0,0 +1,15 @@
> +# These are some common invalid and partial object IDs used in tests.
> +001 0001
> 0001
> +002 0002
> 0002
> +003 0003
> 0003
> +004 0004
> 0004
> +005 0005
> 0005
> +006 0006
> 0006
> +007 0007
> 0007
> +# All zeros or Fs missing one or two hex segments.
> +zero-1 000
> 000
> +zero-2 00
>
