Re: [RFC PATCH 0/2] alternate hash test

2018-01-31 Thread brian m. carlson 
On Tue, Jan 30, 2018 at 05:04:52PM -0800, Stefan Beller wrote:
> Thanks for writing this, I chose a slightly different approach at
> https://public-inbox.org/git/20170728171817.21458-2-sbel...@google.com/
> which might be quicker for local testing.

I'm actually going to rework the patches to be much more similar to
that.  I've been convinced that's an easier approach to the series, and
I like the simplicity.

My goal is not only to identify failing tests, but to exercise the hash
algorithm logic so that we find any hidden dependencies outside of that
code.  For example, I noticed some of our Perl scripts have hard-coded
empty-tree object IDs in them, which obviously will need to be fixed.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [RFC PATCH 0/2] alternate hash test

2018-01-30 Thread Stefan Beller 
On Sun, Jan 28, 2018 at 9:06 AM, brian m. carlson
 wrote:
> This series wires up an alternate hash implementation, namely
> BLAKE2b-160.  The goal is to allow us to identify tests which rely on
> the hash algorithm in use so that we can fix those tests.
>
> For this test, I picked BLAKE2b-160 for a couple reasons:
> * Debian ships a libb2-1 package which can be used easily (in other
>   words, I was lazy and didn't want to add a crypto implementation just
>   for test purposes);
> * The API of the libb2 package easily allows arbitrary hash lengths, so
>   I didn't have to manage truncation myself;
> * Our codebase isn't yet ready for a hash function larger than 20 bytes,
>   as there's still more work to do on the object_id conversions.
>
> This isn't an endorsement for or against any particular algorithm
> choice, just an artifact of the tools that were easily available to me.
> Provoking discussion of which hash to pick for NewHash is explicitly
> *not* a goal for this series.  I'm only interested in the ability to
> identify and fix tests.
>
> The first patch does no feature detection and just assumes you have
> libb2 installed.  For obvious reasons, this series is not meant for
> production use.

Thanks for writing this, I chose a slightly different approach at
https://public-inbox.org/git/20170728171817.21458-2-sbel...@google.com/
which might be quicker for local testing.

Thanks for bringing this discussion back on the list,
Stefan

Re: [RFC PATCH 0/2] alternate hash test

2018-01-30 Thread Junio C Hamano 
"brian m. carlson"  writes:

> This series wires up an alternate hash implementation, namely
> BLAKE2b-160.  The goal is to allow us to identify tests which rely on
> the hash algorithm in use so that we can fix those tests.

Yay.

> Provoking discussion of which hash to pick for NewHash is explicitly
> *not* a goal for this series.  I'm only interested in the ability to
> identify and fix tests.

Double yay.

Re: [RFC PATCH 0/2] alternate hash test

2018-01-28 Thread brian m. carlson 
On Sun, Jan 28, 2018 at 07:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Jan 28, 2018 at 6:06 PM, brian m. carlson
>  wrote:
> If the goal is to smoke out hardcoded SHA1s in tests, isn't it easier
> to instrument SHA-1 (e.g. our blk_sha1 copy, or our wrappers) to
> pretend that whenever we ask for the hash for STRING to pretend we
> asked for SOME_PREFIX + STRING?
> 
> Such an approach would have the advantage of being more portable
> (easier to run these mock test), and also that if we ever move to
> NewHash we could still test for this, we'd just always set the prefix
> to compilation time(), and could thus guarantee that the hashes would
> change every time git was built.

That's certainly a possibility.  We could simply call the update
function from the init function and prepend a NUL byte or something like
that, which would definitely produce different results.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [RFC PATCH 0/2] alternate hash test

2018-01-28 Thread Ævar Arnfjörð Bjarmason 
On Sun, Jan 28, 2018 at 6:06 PM, brian m. carlson
 wrote:
> For this test, I picked BLAKE2b-160 for a couple reasons:
> * Debian ships a libb2-1 package which can be used easily (in other
>   words, I was lazy and didn't want to add a crypto implementation just
>   for test purposes);
> [...]
> This isn't an endorsement for or against any particular algorithm
> choice, just an artifact of the tools that were easily available to me.
> Provoking discussion of which hash to pick for NewHash is explicitly
> *not* a goal for this series.  I'm only interested in the ability to
> identify and fix tests.

If the goal is to smoke out hardcoded SHA1s in tests, isn't it easier
to instrument SHA-1 (e.g. our blk_sha1 copy, or our wrappers) to
pretend that whenever we ask for the hash for STRING to pretend we
asked for SOME_PREFIX + STRING?

Such an approach would have the advantage of being more portable
(easier to run these mock test), and also that if we ever move to
NewHash we could still test for this, we'd just always set the prefix
to compilation time(), and could thus guarantee that the hashes would
change every time git was built.

Re: [RFC PATCH 0/2] alternate hash test

2018-01-28 Thread SZEDER Gábor 
> This series wires up an alternate hash implementation, namely
> BLAKE2b-160.  The goal is to allow us to identify tests which rely on
> the hash algorithm in use so that we can fix those tests.

> t9903-bash-prompt.sh (Wstat: 256 Tests: 66 
> Failed: 53)
>   Failed tests:  4, 6-10, 14-34, 36, 39-62, 65
>   Non-zero exit status: 1

I didn't recall seeing hardcoded SHA-1s in the bash prompt tests, so
went to have a look.  And indeed, these failures are not the test's
fault, but the result of a segfault in 'git checkout' while trying to
return from an orphan branch in test 4.  The rest of the failures are
mostly fallout caused by the wrong branch being checked out or the
leftover index.lock.

$ ./t9903-bash-prompt.sh -v -i
<...>
expecting success: 
  printf " (unborn)" >expected &&
  git checkout --orphan unborn &&
  test_when_finished "git checkout master" &&
  __git_ps1 >"$actual" &&
  test_cmp expected "$actual"

Switched to a new branch 'unborn'
./test-lib.sh: line 609: 29342 Segmentation fault  git checkout master
not ok 4 - prompt - unborn branch


$ gdb --args ../../git checkout master
<...>
Program received signal SIGSEGV, Segmentation fault.
0x0041b636 in merge_working_tree (opts=0x7fffd200, 
old=0x7fffd0d0, new=0x7fffd1e0,
writeout_error=0x7fffd0c0)
at builtin/checkout.c:525
525 init_tree_desc([0], tree->buffer,
tree->size);
(gdb) p tree 
$1 = (struct tree *) 0x0


The root cause is that patch 2/2 misses places where EMPTY_TREE_SHA1_*
constants should have been replaced with their EMPTY_TREE_SBLAKE2B_*
counterparts.

[RFC PATCH 0/2] alternate hash test

2018-01-28 Thread brian m. carlson 
This series wires up an alternate hash implementation, namely
BLAKE2b-160.  The goal is to allow us to identify tests which rely on
the hash algorithm in use so that we can fix those tests.

For this test, I picked BLAKE2b-160 for a couple reasons:
* Debian ships a libb2-1 package which can be used easily (in other
  words, I was lazy and didn't want to add a crypto implementation just
  for test purposes);
* The API of the libb2 package easily allows arbitrary hash lengths, so
  I didn't have to manage truncation myself;
* Our codebase isn't yet ready for a hash function larger than 20 bytes,
  as there's still more work to do on the object_id conversions.

This isn't an endorsement for or against any particular algorithm
choice, just an artifact of the tools that were easily available to me.
Provoking discussion of which hash to pick for NewHash is explicitly
*not* a goal for this series.  I'm only interested in the ability to
identify and fix tests.

The first patch does no feature detection and just assumes you have
libb2 installed.  For obvious reasons, this series is not meant for
production use.

This series applies on top of the object_id part 11 series I just sent
out.

Below is a list of the prove output indicating which tests failed on my
system with these patches.

Test Summary Report
---
t0002-gitfile.sh (Wstat: 256 Tests: 14 Failed: 
3)
  Failed tests:  12-14
  Non-zero exit status: 1
t0005-signals.sh (Wstat: 256 Tests: 5 Failed: 2)
  Failed tests:  4-5
  Non-zero exit status: 1
t-basic.sh   (Wstat: 256 Tests: 78 Failed: 
12)
  Failed tests:  47, 54, 56, 58, 60, 62, 64, 66, 71, 74-76
  Non-zero exit status: 1
t1007-hash-object.sh (Wstat: 256 Tests: 38 Failed: 
15)
  Failed tests:  6, 8, 10-11, 19-29
  Non-zero exit status: 1
t1011-read-tree-sparse-checkout.sh   (Wstat: 256 Tests: 21 Failed: 
3)
  Failed tests:  1-2, 5
  Non-zero exit status: 1
t1300-repo-config.sh (Wstat: 256 Tests: 146 Failed: 
1)
  Failed test:  144
  Non-zero exit status: 1
t1304-default-acl.sh (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
t1405-main-ref-store.sh  (Wstat: 256 Tests: 17 Failed: 
1)
  Failed test:  10
  Non-zero exit status: 1
t1411-reflog-show.sh (Wstat: 256 Tests: 18 Failed: 
6)
  Failed tests:  3-5, 7, 10, 13
  Non-zero exit status: 1
t1450-fsck.sh(Wstat: 256 Tests: 71 Failed: 
3)
  Failed tests:  18, 26, 59
  Non-zero exit status: 1
t1507-rev-parse-upstream.sh  (Wstat: 256 Tests: 28 Failed: 
2)
  Failed tests:  25-26
  Non-zero exit status: 1
t1512-rev-parse-disambiguation.sh(Wstat: 256 Tests: 33 Failed: 
19)
  Failed tests:  2-5, 7-12, 16, 22, 26-32
  Non-zero exit status: 1
t1700-split-index.sh (Wstat: 256 Tests: 22 Failed: 
9)
  Failed tests:  1-2, 5-7, 13-16
  Non-zero exit status: 1
t2011-checkout-invalid-head.sh   (Wstat: 256 Tests: 10 Failed: 
5)
  Failed tests:  3, 6-7, 9-10
  Non-zero exit status: 1
t2015-checkout-unborn.sh (Wstat: 256 Tests: 6 Failed: 2)
  Failed tests:  3-4
  Non-zero exit status: 1
t1013-read-tree-submodule.sh (Wstat: 256 Tests: 64 Failed: 
15)
  Failed tests:  1-2, 4-7, 19-20, 22-25, 30, 34-35
  Non-zero exit status: 1
t2017-checkout-orphan.sh (Wstat: 256 Tests: 13 Failed: 
7)
  Failed tests:  7-13
  Non-zero exit status: 1
t2020-checkout-detach.sh (Wstat: 256 Tests: 24 Failed: 
2)
  Failed tests:  23-24
  Non-zero exit status: 1
t2101-update-index-reupdate.sh   (Wstat: 256 Tests: 7 Failed: 6)
  Failed tests:  1-3, 5-7
  Non-zero exit status: 1
t2107-update-index-basic.sh  (Wstat: 256 Tests: 9 Failed: 1)
  Failed test:  9
  Non-zero exit status: 1
t2203-add-intent.sh  (Wstat: 256 Tests: 16 Failed: 
4)
  Failed tests:  3, 12, 15-16
  Non-zero exit status: 1
t3033-merge-toplevel.sh  (Wstat: 256 Tests: 13 Failed: 
11)
  Failed tests:  3-13
  Non-zero exit status: 1
t3103-ls-tree-misc.sh(Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
t3201-branch-contains.sh (Wstat: 256 Tests: 19 Failed: 
1)
  Failed test:  18
  Non-zero exit status: 1
t3301-notes.sh   (Wstat: 256 Tests: 134 Failed: 
50)
  Failed tests:  10, 18-23, 42-44, 46, 55-57, 60, 65-78
80, 83-84, 86-91, 93-104
  Non-zero exit status: 1
t2013-checkout-submodule.sh  (Wstat: 256 Tests: 70 Failed: 
16)
  Failed tests:  7-8, 10-13, 19, 25-26, 28-31, 36, 40-41
  Non-zero exit status: 1