[RFC] t5500 considered dangerous

[RFC] t5500 considered dangerous

[Johannes Schindelin]

'corrupt a pack and see if verify catches' is bound to fail sometimes for 
non-obvious reasons:

The test case corrupts a pack, and then also the index of that pack, by 
writing "0" at certain offsets, and then tests if git-verify-pack fails.

However, said pack contains a commit, which is variable by virtue of 
storing two dates. Therefore, the deflated objects are no longer 
deterministic, and neither the pack. What happens is that in roughly every 
256th run, there *is* a 0 at one of these offsets, and the test fails.

This patch makes the test deterministic by skipping the commit.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	I just hunted down a bug which showed up very rarely, and it did 
	not make the hunting easier.

 t/t5300-pack-object.sh |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index df6549c..d6b6697 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -20,12 +20,11 @@ test_expect_success \
      done &&
      cat c >d && echo foo >>d && git-update-index --add d &&
      tree=`git-write-tree` &&
-     commit=`git-commit-tree $tree </dev/null` && {
+     {
 	 echo $tree &&
-	 echo $commit &&
 	 git-ls-tree $tree | sed -e "s/.* \\([0-9a-f]*\\)	.*/\\1/"
      } >obj-list && {
-	 git-diff-tree --root -p $commit &&
+	 git-diff-tree --root -p $tree &&
 	 while read object
 	 do
 	    t=`git-cat-file -t $object` &&
@@ -99,7 +98,7 @@ test_expect_success \
      GIT_OBJECT_DIRECTORY=.git2/objects &&
      export GIT_OBJECT_DIRECTORY &&
      cp test-1-${packname_1}.pack test-1-${packname_1}.idx .git2/objects/pack && {
-	 git-diff-tree --root -p $commit &&
+	 git-diff-tree --root -p $tree &&
 	 while read object
 	 do
 	    t=`git-cat-file -t $object` &&
@@ -114,7 +113,7 @@ test_expect_success \
      export GIT_OBJECT_DIRECTORY &&
      rm -f .git2/objects/pack/test-?.idx &&
      cp test-2-${packname_2}.pack test-2-${packname_2}.idx .git2/objects/pack && {
-	 git-diff-tree --root -p $commit &&
+	 git-diff-tree --root -p $tree &&
 	 while read object
 	 do
 	    t=`git-cat-file -t $object` &&

Re: [RFC] t5500 considered dangerous

[Johannes Schindelin]

Of course, it is t5300. Sorry for the noise.

Re: [RFC] t5300 considered dangerous

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The test case corrupts a pack, and then also the index of that pack, by 
> writing "0" at certain offsets, and then tests if git-verify-pack fails.

These certain offsets are:

 . byte    2 in pack
   This corrupts PACK_SIGNATURE ("A" in "PACK").

 . byte    7 in pack
   This corrupts PACK_VERSION ("\002" in "\0\0\0\002").

 . byte   12 in pack
   This corrupts the type/size byte of whatever the first object
   happens to be (the type/size byte has non-zero upper nibble).

 . byte 1200 in idx that is currently 1208 bytes long.
   This tries to corrupt the checksum of the index file itself,

You are right about the last one; this location has 1/256 chance
of validly being 0, and making the set of objects to be packed
constant is one way to fix it.  More appropriate way would be to
measure how long the .idx file is, and corrupt the last 20 bytes;
I was too lazy.

-- >8 --

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 7dfb1ab..dd719bb 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -163,8 +163,10 @@ test_expect_success \
      else :;
      fi &&
 
+     l=`wc -c <test-3.idx` &&
+     l=`expr "$l" - 20` &&
      cp test-1-${packname_1}.pack test-3.pack &&
-     dd if=/dev/zero of=test-3.idx count=1 bs=1 conv=notrunc seek=1200 &&
+     dd if=/dev/zero of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&
      if git-verify-pack test-3.pack
      then false
      else :;

Re: [RFC] t5300 considered dangerous

[Johannes Schindelin]

Hi,

On Mon, 26 Dec 2005, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The test case corrupts a pack, and then also the index of that pack, by 
> > writing "0" at certain offsets, and then tests if git-verify-pack fails.
> 
> These certain offsets are:
> 
>  . byte    2 in pack
>    This corrupts PACK_SIGNATURE ("A" in "PACK").
> 
>  . byte    7 in pack
>    This corrupts PACK_VERSION ("\002" in "\0\0\0\002").
> 
>  . byte   12 in pack
>    This corrupts the type/size byte of whatever the first object
>    happens to be (the type/size byte has non-zero upper nibble).
> 
>  . byte 1200 in idx that is currently 1208 bytes long.
>    This tries to corrupt the checksum of the index file itself,

Thank you for clarifying this.

> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 7dfb1ab..dd719bb 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -163,8 +163,10 @@ test_expect_success \
>       else :;
>       fi &&
>  
> +     l=`wc -c <test-3.idx` &&
> +     l=`expr "$l" - 20` &&

This needs to be

> +     l=`expr $l - 20` &&

since there are textutils out there (notably version 2.1) where "wc -c" 
prints a leading space.

>       cp test-1-${packname_1}.pack test-3.pack &&
> -     dd if=/dev/zero of=test-3.idx count=1 bs=1 conv=notrunc seek=1200 &&
> +     dd if=/dev/zero of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&
>       if git-verify-pack test-3.pack
>       then false
>       else :;

When the commit is not skipped, I fail to see why this should not fail in 
one out of 256 cases: the input is (partially) pseudo-random. The last 20 
bytes are the SHA-1 which should inherit this pseudo-randomness. So, the 
first byte should be pseudo-random, too.

Something I missed?

Ciao,
Dscho

Re: [RFC] t5300 considered dangerous

[Johannes Schindelin]

Hi,

On Tue, 27 Dec 2005, Johannes Schindelin wrote:

> On Mon, 26 Dec 2005, Junio C Hamano wrote:
> 
> >       cp test-1-${packname_1}.pack test-3.pack &&
> > -     dd if=/dev/zero of=test-3.idx count=1 bs=1 conv=notrunc seek=1200 &&
> > +     dd if=/dev/zero of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&
> >       if git-verify-pack test-3.pack
> >       then false
> >       else :;
> 
> When the commit is not skipped, I fail to see why this should not fail in 
> one out of 256 cases: the input is (partially) pseudo-random. The last 20 
> bytes are the SHA-1 which should inherit this pseudo-randomness. So, the 
> first byte should be pseudo-random, too.
> 
> Something I missed?

Yes, I missed the "count=20".

Sorry,
Dscho