threads / rfc / 2933

[RFC] t5500 considered dangerous

Subject: [RFC] t5500 considered dangerous

## tl;dr

5 messages between Dec 26, 2005 and Dec 27, 2005.

replies: 4people: 2as markdown or json

Johannes Schindelin· Dec 26, 2005, 23:35 UTC · lore

'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` &&
Junio C Hamano· Dec 27, 2005, 02:10 UTC · re: Johannes Schindelin · lore

Re: [RFC] t5300 considered dangerous

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 :;
Johannes Schindelin· Dec 27, 2005, 13:57 UTC · re: Junio C Hamano · lore

Re: [RFC] t5300 considered dangerous

Hi,
On Mon, 26 Dec 2005, Junio C Hamano wrote:
Show 19 quoted lines
> 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.
Show 10 quoted lines
> 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.

Show 6 quoted lines
>       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

Johannes Schindelin· Dec 27, 2005, 14:08 UTC · re: Johannes Schindelin · lore

Re: [RFC] t5300 considered dangerous

Hi,
On Tue, 27 Dec 2005, Johannes Schindelin wrote:
Show 15 quoted lines
> 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

← back to recent threads