Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant

"brian m. carlson" [off-list ref] writes:

One approach I had considered taking is having a helper of some sort
that wrapped a simple key/value store.  We could pass the wrapper the
SHA-1 value (or, if necessary, an arbitrary key) and have it return the
proper value based on the given hash function.

That does have the downsides that the values may not present in the
tests themselves, and that people adding new tests will of course need
to run the test suite twice.  But it does make the tests easier to read.

Opinions on the desirability of this approach are of course welcome.

I am not quite sure if I follow.  There was a proposal to tweak the
commit format that uses the new hash in such a way that we can tell
what SHA-1 would have been used if everything were SHA-1 (I think it
was from Jonathan, but I may be mistaken), and I recall that
generally the list were receptive to the idea.  But I have a feeling
that your "helper of some sort" is something else.

If your <key,value> is about letting us store something like

 - If you hash "hello\n" the resulting blob in SHA-1 world has this
   object name, and with that, you can find out the equivalent
   object name in SHA-256 world.

 - If you have a tree with the above blob at path P and nothing
   else, then the object name of that tree in the SHA-1 world and
   SHA-256 world are different and we can map between them.

 - Likewise for a commit that points at the above tree with fixed
   date, author and message.

I am not sure how much it would help.  Are you aiming to make it
easier and more structured to create a patch like what Stefan did
recently for t8008 in 0ba9c9a0 ("t8008: rely on rev-parse'd HEAD
instead of sha1 value", 2017-07-26)?

I also suspect that tests like t1512 and t6500 would not benefit
that much from such a mapping.  In these tests, the object names by
themselves are not interesting.  These tests are about what Git does
when the names of the objects involved in them happen to share a
certain prefix.  We are not interested in using the same payload in
these tests using different hash, which is likely to destroy the
aspect of the object names that these tests are interested in,
namely, they share the same prefix.  When updating these tests to
adjust for the SHA-256 world, we want to preserve that the resulting
object names happen to share the same prefix by tweaking the payload
strings (i.e. "263 and 410" in t6500 are chosen to cause the
resulting objects to share "17/" prefix and fall inside a same
fan-out directory as loose objects.  We want to choose different
strings so that the names of the resulting objects share the same
prefix, not necessarily "17/" but preferrably so, in the SHA-256
world.  Similarly, Random-looking strings like "a2onsxbvj" in t1512
are chosen to cause blobs, trees, commits and tags that are involved
in the test to all share the same prefix "000000..."; we want to
choose different set of such random-looking strings that cause all
objects involved to hash to the same prefix, not necessarily but
preferrably "000000...").

On Sun, Jul 30, 2017 at 02:21:50PM -0700, Junio C Hamano wrote:

"brian m. carlson" [off-list ref] writes:

quoted

One approach I had considered taking is having a helper of some sort
that wrapped a simple key/value store.  We could pass the wrapper the
SHA-1 value (or, if necessary, an arbitrary key) and have it return the
proper value based on the given hash function.

That does have the downsides that the values may not present in the
tests themselves, and that people adding new tests will of course need
to run the test suite twice.  But it does make the tests easier to read.

Opinions on the desirability of this approach are of course welcome.

I am not quite sure if I follow.  There was a proposal to tweak the
commit format that uses the new hash in such a way that we can tell
what SHA-1 would have been used if everything were SHA-1 (I think it
was from Jonathan, but I may be mistaken), and I recall that
generally the list were receptive to the idea.  But I have a feeling
that your "helper of some sort" is something else.

If your <key,value> is about letting us store something like

 - If you hash "hello\n" the resulting blob in SHA-1 world has this
   object name, and with that, you can find out the equivalent
   object name in SHA-256 world.

 - If you have a tree with the above blob at path P and nothing
   else, then the object name of that tree in the SHA-1 world and
   SHA-256 world are different and we can map between them.

 - Likewise for a commit that points at the above tree with fixed
   date, author and message.

I am not sure how much it would help.  Are you aiming to make it
easier and more structured to create a patch like what Stefan did
recently for t8008 in 0ba9c9a0 ("t8008: rely on rev-parse'd HEAD
instead of sha1 value", 2017-07-26)?

Yes, basically, but a bit more generally.  There will always be cases in
which we need to specify an object ID or an arbitrary string and the
behavior will need to vary based on the hash.  That can be something
like, in this case, the two blob contents that would have the similar
prefix.

So in this case, we pass the helper the string "263 410" and get back a
value for either the hacked SHA-1 hash or the SHA-256 or whatever we're
using.

This was basically a nicer way of wrapping the case statement that you
had given as an example.  Of course, it doesn't relieve us of doing the
hard work of analyzing the tests, which Stefan is doing, and with which
I don't want to interfere.

It was simply a proposal for a future direction which we could take if
we found ourselves needing to write a large number of hash-specific case
statements.  I'm happy to wait to actually implement that code until we
decide we need such a thing.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:

Yes, basically, but a bit more generally.  There will always be cases in
which we need to specify an object ID or an arbitrary string and the
behavior will need to vary based on the hash.  That can be something
like, in this case, the two blob contents that would have the similar
prefix.

So in this case, we pass the helper the string "263 410" and get back a
value for either the hacked SHA-1 hash or the SHA-256 or whatever we're
using.

I realize this was worded poorly.  So for my example, in this case, we'd
do:

test-helper-hash-string "263 410"

For SHA-1, we'd get "263 410".  For SHA-256, we'd get "313 481" (which,
as SHA-256 blobs, both start with "17" in their hex representation).
Presumably we'd read some environment variable to determine the proper
value.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

On Mon, Jul 31, 2017 at 1:24 AM, brian m. carlson
[off-list ref] wrote:

On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:

quoted

Yes, basically, but a bit more generally.  There will always be cases in
which we need to specify an object ID or an arbitrary string and the
behavior will need to vary based on the hash.  That can be something
like, in this case, the two blob contents that would have the similar
prefix.

So in this case, we pass the helper the string "263 410" and get back a
value for either the hacked SHA-1 hash or the SHA-256 or whatever we're
using.

I realize this was worded poorly.  So for my example, in this case, we'd
do:

test-helper-hash-string "263 410"

For SHA-1, we'd get "263 410".  For SHA-256, we'd get "313 481" (which,
as SHA-256 blobs, both start with "17" in their hex representation).
Presumably we'd read some environment variable to determine the proper
value.

I've been mostly out of the loop on this hash transition plan, but
don't we expect to be compiling a git that knows about both SHA-1 and
whatever the $newhash is? If so it seems better to just test all N
hashes we have:

    test_expect_success_hash $desc_description '
        hash_value=$(test-helper-hash-string $CURRENT_HASH)
        ....
    '

Then test_expect_success_hash would run N times for the N hashes we have.

This would obviously be slightly more hassle to write & convert, but I
think it would be worth it, particularly with something like Travis
where we can test all hashes, instead of being in some mode where we
fragment on all of hashes/gettext poison and whatever other
compilation option we have that really requires compiling a new git
version...

On Sun, Jul 30, 2017 at 4:24 PM, brian m. carlson
[off-list ref] wrote:

On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:

quoted

Yes, basically, but a bit more generally.  There will always be cases in
which we need to specify an object ID or an arbitrary string and the
behavior will need to vary based on the hash.  That can be something
like, in this case, the two blob contents that would have the similar
prefix.

So in this case, we pass the helper the string "263 410" and get back a
value for either the hacked SHA-1 hash or the SHA-256 or whatever we're
using.

I realize this was worded poorly.  So for my example, in this case, we'd
do:

test-helper-hash-string "263 410"

For SHA-1, we'd get "263 410".  For SHA-256, we'd get "313 481" (which,
as SHA-256 blobs, both start with "17" in their hex representation).
Presumably we'd read some environment variable to determine the proper
value.

This is what Junio proposed in the first message, except that we defer that
to a shell script as for each test we may need different things, so a helper may
be of little value?

On Mon, Jul 31, 2017 at 1:17 PM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Mon, Jul 31, 2017 at 1:24 AM, brian m. carlson
[off-list ref] wrote:

quoted

On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:

quoted

Yes, basically, but a bit more generally.  There will always be cases in
which we need to specify an object ID or an arbitrary string and the
behavior will need to vary based on the hash.  That can be something
like, in this case, the two blob contents that would have the similar
prefix.

So in this case, we pass the helper the string "263 410" and get back a
value for either the hacked SHA-1 hash or the SHA-256 or whatever we're
using.

I realize this was worded poorly.  So for my example, in this case, we'd
do:

test-helper-hash-string "263 410"

For SHA-1, we'd get "263 410".  For SHA-256, we'd get "313 481" (which,
as SHA-256 blobs, both start with "17" in their hex representation).
Presumably we'd read some environment variable to determine the proper
value.

I've been mostly out of the loop on this hash transition plan, but
don't we expect to be compiling a git that knows about both SHA-1 and
whatever the $newhash is?

That is my understanding as well.

If so it seems better to just test all N
hashes we have:

    test_expect_success_hash $desc_description '
        hash_value=$(test-helper-hash-string $CURRENT_HASH)
        ....
    '

Then test_expect_success_hash would run N times for the N hashes we have.

I think that is just adding more workload without furthering the stated goal
which is usually reached with just one hash function. The tests we're talking
about here are not trying to test correctness of hashes but some other
functionality
(correct abbreviation length, collisions in prefix, etc.) that would not change
depending on the hash function used, I imagine.

For t0000 we want to have multiple versions, one for each hash.

This would obviously be slightly more hassle to write & convert, but I
think it would be worth it, particularly with something like Travis
where we can test all hashes, instead of being in some mode where we
fragment on all of hashes/gettext poison and whatever other
compilation option we have that really requires compiling a new git
version...

On Mon, Jul 31, 2017 at 01:26:40PM -0700, Stefan Beller wrote:

On Sun, Jul 30, 2017 at 4:24 PM, brian m. carlson
[off-list ref] wrote:

quoted

I realize this was worded poorly.  So for my example, in this case, we'd
do:

test-helper-hash-string "263 410"

For SHA-1, we'd get "263 410".  For SHA-256, we'd get "313 481" (which,
as SHA-256 blobs, both start with "17" in their hex representation).
Presumably we'd read some environment variable to determine the proper
value.

This is what Junio proposed in the first message, except that we defer that
to a shell script as for each test we may need different things, so a helper may
be of little value?

I think a shell script may end up being a fine helper for our needs.  In
any case, I think we're in violent agreement.  My proposal was just an
idea I had considered when thinking about how to do this, and any
suitable solution is fine with me.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204