[03/10] t1450: make hash size independent
diff mbox series

Message ID 20190609224400.41557-4-sandals@crustytoothpaste.net
State New
Headers show
Series
  • Hash-independent tests, part 4
Related show

Commit Message

brian m. carlson June 9, 2019, 10:43 p.m. UTC
Replace several hard-coded full and partial object IDs with variables or
computed values.  Create junk data to stuff inside an invalid tree that
can be either 20 or 32 bytes long.  Compute a binary all-zeros object ID
instead of hard-coding a 20-byte length.

Additionally, compute various object IDs by using test_oid and
$EMPTY_BLOB so that this test works with multiple hash algorithms.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t1450-fsck.sh | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

Comments

Eric Sunshine June 10, 2019, 7:57 a.m. UTC | #1
On Sun, Jun 9, 2019 at 6:44 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Replace several hard-coded full and partial object IDs with variables or
> computed values.  Create junk data to stuff inside an invalid tree that
> can be either 20 or 32 bytes long.  Compute a binary all-zeros object ID
> instead of hard-coding a 20-byte length.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> @@ -9,6 +9,7 @@ test_description='git fsck random collection of tests
>  test_expect_success setup '
> +       test_oid_init &&
>         git config gc.auto 0 &&
>         git config i18n.commitencoding ISO-8859-1 &&
>         test_commit A fileA one &&
> @@ -16,7 +17,8 @@ test_expect_success setup '
>         git checkout HEAD^0 &&
>         test_commit B fileB two &&
>         git tag -d A B &&
> -       git reflog expire --expire=now --all
> +       git reflog expire --expire=now --all &&
> +       test_oid_init
>  '

Why does this function call test_oid_init() twice?
brian m. carlson June 10, 2019, 10:09 p.m. UTC | #2
On 2019-06-10 at 07:57:18, Eric Sunshine wrote:
> On Sun, Jun 9, 2019 at 6:44 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > Replace several hard-coded full and partial object IDs with variables or
> > computed values.  Create junk data to stuff inside an invalid tree that
> > can be either 20 or 32 bytes long.  Compute a binary all-zeros object ID
> > instead of hard-coding a 20-byte length.
> > [...]
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> > @@ -9,6 +9,7 @@ test_description='git fsck random collection of tests
> >  test_expect_success setup '
> > +       test_oid_init &&
> >         git config gc.auto 0 &&
> >         git config i18n.commitencoding ISO-8859-1 &&
> >         test_commit A fileA one &&
> > @@ -16,7 +17,8 @@ test_expect_success setup '
> >         git checkout HEAD^0 &&
> >         test_commit B fileB two &&
> >         git tag -d A B &&
> > -       git reflog expire --expire=now --all
> > +       git reflog expire --expire=now --all &&
> > +       test_oid_init
> >  '
> 
> Why does this function call test_oid_init() twice?

Probably because this was a squash of a couple different patches. I'll
reroll with that fixed.
Jonathan Tan June 11, 2019, 11:02 p.m. UTC | #3
> @@ -84,7 +86,7 @@ test_expect_success 'branch pointing to non-commit' '
>  test_expect_success 'HEAD link pointing at a funny object' '
>  	test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
>  	mv .git/HEAD .git/SAVED_HEAD &&
> -	echo 0000000000000000000000000000000000000000 >.git/HEAD &&
> +	echo $ZERO_OID >.git/HEAD &&
>  	# avoid corrupt/broken HEAD from interfering with repo discovery
>  	test_must_fail env GIT_DIR=.git git fsck 2>out &&
>  	cat out &&

ZERO_OID doesn't seem redefined to the SHA256 variant when being tested
under SHA256. Maybe you need a test_oid invocation here.

I couldn't verify this, though - do you know if there is a way for me to
run the tests with SHA256 instead of SHA1?

> @@ -417,13 +426,12 @@ test_expect_success 'force fsck to ignore double author' '
>  '
>  
>  _bz='\0'
> -_bz5="$_bz$_bz$_bz$_bz$_bz"
> -_bz20="$_bz5$_bz5$_bz5$_bz5"
> +_bzoid=$(printf $ZERO_OID | sed -e 's/00/\\0/g')

Same comment here.

> @@ -631,10 +639,12 @@ test_expect_success 'fsck --name-objects' '
>  
>  test_expect_success 'alternate objects are correctly blamed' '
>  	test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
> +	path=$(test_oid numeric) &&
> +	path=$(test_oid_to_path "$path") &&

Double assignment to path?
Eric Sunshine June 11, 2019, 11:20 p.m. UTC | #4
On Tue, Jun 11, 2019 at 7:03 PM Jonathan Tan <jonathantanmy@google.com> wrote:
> >  test_expect_success 'alternate objects are correctly blamed' '
> >       test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
> > +     path=$(test_oid numeric) &&
> > +     path=$(test_oid_to_path "$path") &&
>
> Double assignment to path?

I tripped over this, as well, when reading the patch, but if you look
closely, the second assignment is "refining" a value computed in first
assignment. It would have been clearer if written as:

    name=$(test_oid numeric) &&
    path=$(test_oid_to_path "$name") &&

or:

    path=$(test_oid_to_path $(test_oid numeric))
brian m. carlson June 11, 2019, 11:35 p.m. UTC | #5
On 2019-06-11 at 23:02:55, Jonathan Tan wrote:
> > @@ -84,7 +86,7 @@ test_expect_success 'branch pointing to non-commit' '
> >  test_expect_success 'HEAD link pointing at a funny object' '
> >  	test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
> >  	mv .git/HEAD .git/SAVED_HEAD &&
> > -	echo 0000000000000000000000000000000000000000 >.git/HEAD &&
> > +	echo $ZERO_OID >.git/HEAD &&
> >  	# avoid corrupt/broken HEAD from interfering with repo discovery
> >  	test_must_fail env GIT_DIR=.git git fsck 2>out &&
> >  	cat out &&
> 
> ZERO_OID doesn't seem redefined to the SHA256 variant when being tested
> under SHA256. Maybe you need a test_oid invocation here.

That's actually coming in a later series. Eventually, test_oid_init will
be called automatically and ZERO_OID will be set by calling test_oid
with an appropriate value.

> I couldn't verify this, though - do you know if there is a way for me to
> run the tests with SHA256 instead of SHA1?

There isn't any way in Junio's tree, mostly because there are still a
lot of places that need fixing. However, all of those commits are in my
transition-stage-4 branch at https://github.com/bk2204/git.git, and you
can set the environment variable GIT_TEST_DEFAULT_HASH to "sha256" and
it will run the test suite with SHA-256.

That branch has a fully functional SHA-256 Git if you'd like to test it
out, and the same binary can handle SHA-1 and SHA-256. Interoperability
between SHA-1 and SHA-256 repos hasn't been implemented yet, though.

> > @@ -631,10 +639,12 @@ test_expect_success 'fsck --name-objects' '
> >  
> >  test_expect_success 'alternate objects are correctly blamed' '
> >  	test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
> > +	path=$(test_oid numeric) &&
> > +	path=$(test_oid_to_path "$path") &&
> 
> Double assignment to path?

Good point. Will fix.
brian m. carlson June 11, 2019, 11:40 p.m. UTC | #6
On 2019-06-11 at 23:20:31, Eric Sunshine wrote:
> On Tue, Jun 11, 2019 at 7:03 PM Jonathan Tan <jonathantanmy@google.com> wrote:
> > >  test_expect_success 'alternate objects are correctly blamed' '
> > >       test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
> > > +     path=$(test_oid numeric) &&
> > > +     path=$(test_oid_to_path "$path") &&
> >
> > Double assignment to path?
> 
> I tripped over this, as well, when reading the patch, but if you look
> closely, the second assignment is "refining" a value computed in first
> assignment. It would have been clearer if written as:
> 
>     name=$(test_oid numeric) &&
>     path=$(test_oid_to_path "$name") &&
> 
> or:
> 
>     path=$(test_oid_to_path $(test_oid numeric))

I think in general people have discouraged the latter form because it
doesn't fail predictably if the inner command substitution fails. I'll
rename the first variable so that it's less surprising.

Patch
diff mbox series

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 0f268a3664..f66a39580c 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -9,6 +9,7 @@  test_description='git fsck random collection of tests
 . ./test-lib.sh
 
 test_expect_success setup '
+	test_oid_init &&
 	git config gc.auto 0 &&
 	git config i18n.commitencoding ISO-8859-1 &&
 	test_commit A fileA one &&
@@ -16,7 +17,8 @@  test_expect_success setup '
 	git checkout HEAD^0 &&
 	test_commit B fileB two &&
 	git tag -d A B &&
-	git reflog expire --expire=now --all
+	git reflog expire --expire=now --all &&
+	test_oid_init
 '
 
 test_expect_success 'loose objects borrowed from alternate are not missing' '
@@ -54,8 +56,8 @@  test_expect_success 'setup: helpers for corruption tests' '
 
 test_expect_success 'object with bad sha1' '
 	sha=$(echo blob | git hash-object -w --stdin) &&
-	old=$(echo $sha | sed "s+^..+&/+") &&
-	new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+	old=$(test_oid_to_path "$sha") &&
+	new=$(dirname $old)/$(test_oid ff_2) &&
 	sha="$(dirname $new)$(basename $new)" &&
 	mv .git/objects/$old .git/objects/$new &&
 	test_when_finished "remove_object $sha" &&
@@ -84,7 +86,7 @@  test_expect_success 'branch pointing to non-commit' '
 test_expect_success 'HEAD link pointing at a funny object' '
 	test_when_finished "mv .git/SAVED_HEAD .git/HEAD" &&
 	mv .git/HEAD .git/SAVED_HEAD &&
-	echo 0000000000000000000000000000000000000000 >.git/HEAD &&
+	echo $ZERO_OID >.git/HEAD &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail env GIT_DIR=.git git fsck 2>out &&
 	cat out &&
@@ -244,10 +246,16 @@  test_expect_success 'tree object with duplicate entries' '
 '
 
 test_expect_success 'unparseable tree object' '
+	test_oid_cache <<-\EOF &&
+	junk sha1:twenty-bytes-of-junk
+	junk sha256:twenty-bytes-of-junk-twelve-more
+	EOF
+
 	test_when_finished "git update-ref -d refs/heads/wrong" &&
 	test_when_finished "remove_object \$tree_sha1" &&
 	test_when_finished "remove_object \$commit_sha1" &&
-	tree_sha1=$(printf "100644 \0twenty-bytes-of-junk" | git hash-object -t tree --stdin -w --literally) &&
+	junk=$(test_oid junk) &&
+	tree_sha1=$(printf "100644 \0$junk" | git hash-object -t tree --stdin -w --literally) &&
 	commit_sha1=$(git commit-tree $tree_sha1) &&
 	git update-ref refs/heads/wrong $commit_sha1 &&
 	test_must_fail git fsck 2>out &&
@@ -275,8 +283,9 @@  test_expect_success 'tree entry with type mismatch' '
 '
 
 test_expect_success 'tag pointing to nonexistent' '
-	cat >invalid-tag <<-\EOF &&
-	object ffffffffffffffffffffffffffffffffffffffff
+	badoid=$(test_oid deadbeef) &&
+	cat >invalid-tag <<-EOF &&
+	object $badoid
 	type commit
 	tag invalid
 	tagger T A Gger <tagger@example.com> 1234567890 -0000
@@ -386,8 +395,8 @@  test_expect_success 'rev-list --verify-objects' '
 
 test_expect_success 'rev-list --verify-objects with bad sha1' '
 	sha=$(echo blob | git hash-object -w --stdin) &&
-	old=$(echo $sha | sed "s+^..+&/+") &&
-	new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+	old=$(test_oid_to_path $sha) &&
+	new=$(dirname $old)/$(test_oid ff_2) &&
 	sha="$(dirname $new)$(basename $new)" &&
 	mv .git/objects/$old .git/objects/$new &&
 	test_when_finished "remove_object $sha" &&
@@ -402,7 +411,7 @@  test_expect_success 'rev-list --verify-objects with bad sha1' '
 
 	test_might_fail git rev-list --verify-objects refs/heads/bogus >/dev/null 2>out &&
 	cat out &&
-	test_i18ngrep -q "error: hash mismatch 63ffffffffffffffffffffffffffffffffffffff" out
+	test_i18ngrep -q "error: hash mismatch $(dirname $new)$(test_oid ff_2)" out
 '
 
 test_expect_success 'force fsck to ignore double author' '
@@ -417,13 +426,12 @@  test_expect_success 'force fsck to ignore double author' '
 '
 
 _bz='\0'
-_bz5="$_bz$_bz$_bz$_bz$_bz"
-_bz20="$_bz5$_bz5$_bz5$_bz5"
+_bzoid=$(printf $ZERO_OID | sed -e 's/00/\\0/g')
 
 test_expect_success 'fsck notices blob entry pointing to null sha1' '
 	(git init null-blob &&
 	 cd null-blob &&
-	 sha=$(printf "100644 file$_bz$_bz20" |
+	 sha=$(printf "100644 file$_bz$_bzoid" |
 	       git hash-object -w --stdin -t tree) &&
 	  git fsck 2>out &&
 	  cat out &&
@@ -434,7 +442,7 @@  test_expect_success 'fsck notices blob entry pointing to null sha1' '
 test_expect_success 'fsck notices submodule entry pointing to null sha1' '
 	(git init null-commit &&
 	 cd null-commit &&
-	 sha=$(printf "160000 submodule$_bz$_bz20" |
+	 sha=$(printf "160000 submodule$_bz$_bzoid" |
 	       git hash-object -w --stdin -t tree) &&
 	  git fsck 2>out &&
 	  cat out &&
@@ -586,7 +594,7 @@  test_expect_success 'fsck --connectivity-only' '
 		# its type. That lets us see that --connectivity-only is
 		# not actually looking at the contents, but leaves it
 		# free to examine the type if it chooses.
-		empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
+		empty=.git/objects/$(test_oid_to_path $EMPTY_BLOB) &&
 		blob=$(echo unrelated | git hash-object -w --stdin) &&
 		mv -f $(sha1_file $blob) $empty &&
 
@@ -631,10 +639,12 @@  test_expect_success 'fsck --name-objects' '
 
 test_expect_success 'alternate objects are correctly blamed' '
 	test_when_finished "rm -rf alt.git .git/objects/info/alternates" &&
+	path=$(test_oid numeric) &&
+	path=$(test_oid_to_path "$path") &&
 	git init --bare alt.git &&
 	echo "../../alt.git/objects" >.git/objects/info/alternates &&
-	mkdir alt.git/objects/12 &&
-	>alt.git/objects/12/34567890123456789012345678901234567890 &&
+	mkdir alt.git/objects/$(dirname $path) &&
+	>alt.git/objects/$(dirname $path)/$(basename $path) &&
 	test_must_fail git fsck >out 2>&1 &&
 	test_i18ngrep alt.git out
 '