Odd behavior with git and cairo-devel repo

Odd behavior with git and cairo-devel repo

[Art Haas]

Hi.

I've been seeing some odd errors with git when it is dealing with
the Cairo graphics library repo. The errors include git failing
a 'fsck-objects' with a 'Floating point exception' error message
and 'git prune' mangling the repo.

A fresh cloning of the repo is needed to see the FPE error. The
repo is about 13M in size, btw.

$ git clone git://git.cairographics.org/git/cairo cairo
[ ... git clones the repo without problem ... ]
$ cd cairo
$ git fsck-objects
Floating point exception
$

The strace of this shows that things bomb after the '.git/index' file is
read. Here's the tail end of the strace output ...

close(3)                                = 0
open(".git/index", O_RDONLY|O_LARGEFILE) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=51472, ...}) = 0
mmap2(NULL, 51472, PROT_READ|PROT_WRITE, MAP_PRIVATE, 3, 0) = 0xb7f4b000
close(3)                                = 0
--- SIGFPE (Floating point exception) @ 0 (0) ---
+++ killed by SIGFPE +++

Now, if the repo is added to and you 'git pull' to update your copy,
the 'git fsck-objects' command completes without error.

As for the problem where 'git prune' mangles the local copy, it would
happen after a 'git fsck-objects' run succeeds without incident.
Then 'git prune' would run, seemingly without problems, and I would
try and repack my repo with 'git repack -a -d'. Here, things go
boom with SHA1 errors, and subsequent 'git fsck-objects' runs
produce errors.

My local 'git' build is current with the 'master' branch. I've observed
these problems with the Cairo repo on a machine running Debian unstable,
Fedora Core 4, and Fedora Rawhide. The Debian machine uses git built
by the current gcc-4.1 package, FC4 uses its current gcc-4.0 package,
and rawhide uses its current gcc-4.1 package. All these machines are
kept up-to-date with the respective current packages versions for
that distro.

Can anyone out in git-land seeing problems with the Cairo
git repo, or able to duplicate these problems?

Thanks in advance,

Art Haas
-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: Odd behavior with git and cairo-devel repo

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On 20:00, Art Haas wrote:

> $ git clone git://git.cairographics.org/git/cairo cairo
> [ ... git clones the repo without problem ... ]
> $ cd cairo
> $ git fsck-objects
> Floating point exception

This is due to refs_hash_size being zero in mark_reachable().
Both "git fsck-objects --full" and "git repack -a -d" seem to work
fine with the patch below (tested by cloning your repo).

---
Currently, we don't check refs_hash_size size and happily call
lookup_object_refs() even if refs_hash_size is zero which leads to
a division by zero in hash_obj().

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
diff --git a/object-refs.c b/object-refs.c
index 8afa227..a7d49c6 100644
--- a/object-refs.c
+++ b/object-refs.c
@@ -127,6 +127,9 @@ void mark_reachable(struct object *obj, 
 
 	if (!track_object_refs)
 		die("cannot do reachability with object refs turned off");
+	/* nothing to lookup */
+	if (!refs_hash_size)
+		return;
 	/* If we've been here already, don't bother */
 	if (obj->flags & mask)
 		return;
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Odd behavior with git and cairo-devel repo

[Junio C Hamano]

Andre Noll <maan@systemlinux.org> writes:

> On 20:00, Art Haas wrote:
>
>> $ git clone git://git.cairographics.org/git/cairo cairo
>> [ ... git clones the repo without problem ... ]
>> $ cd cairo
>> $ git fsck-objects
>> Floating point exception
>
> This is due to refs_hash_size being zero in mark_reachable().
> Both "git fsck-objects --full" and "git repack -a -d" seem to work
> fine with the patch below (tested by cloning your repo).

Good catch.  Thanks both!

Re: Odd behavior with git and cairo-devel repo

[Art Haas]

On Wed, Jun 21, 2006 at 04:46:05AM +0200, Andre Noll wrote:
> On 20:00, Art Haas wrote:
> 
> > $ git clone git://git.cairographics.org/git/cairo cairo
> > [ ... git clones the repo without problem ... ]
> > $ cd cairo
> > $ git fsck-objects
> > Floating point exception
> 
> This is due to refs_hash_size being zero in mark_reachable().
> Both "git fsck-objects --full" and "git repack -a -d" seem to work
> fine with the patch below (tested by cloning your repo).
> 
> [ ... snip ... ]

Hi.

I see this patch has made it into git now, and it fixes the problem
described above. Thanks!

However, I'm still seeing the problem where 'git prune' leaves the
repo in an invalid state. In the case above, running 'git prune' on a
freshly checked-out repo works without problems; when the repo has a
number of unpacked objects, however, things go bad. On the FC4 machine
I have, I updated my git repo, rebuilt, and installed the build with
the patch above, then updated my cairo repo. The 'git pull' retrieved
a handful of objects, and 'git fsck-objects' ran without problem.
Then 'git prune' was run, seemingly without problem, and when I tried
'git repack -a -d' things went boom. A subsequent 'git fsck-object'
run indicated the repo was missing tree and commit objects.

Is anyone else seeing a similar problem with 'git prune'?

Art Haas
-- 
Man once surrendering his reason, has no remaining guard against absurdities
the most monstrous, and like a ship without rudder, is the sport of every wind.

-Thomas Jefferson to James Smith, 1822

Re: Odd behavior with git and cairo-devel repo

[Andre Noll]

On 07:06, Art Haas wrote:
> I see this patch has made it into git now, and it fixes the problem
> described above. Thanks!
> 
> However, I'm still seeing the problem where 'git prune' leaves the
> repo in an invalid state. In the case above, running 'git prune' on a
> freshly checked-out repo works without problems; when the repo has a
> number of unpacked objects, however, things go bad. On the FC4 machine
> I have, I updated my git repo, rebuilt, and installed the build with
> the patch above, then updated my cairo repo. The 'git pull' retrieved
> a handful of objects, and 'git fsck-objects' ran without problem.
> Then 'git prune' was run, seemingly without problem, and when I tried
> 'git repack -a -d' things went boom. A subsequent 'git fsck-object'
> run indicated the repo was missing tree and commit objects.
> 
> Is anyone else seeing a similar problem with 'git prune'?

Yeah. This is due to a thinko in grow_refs_hash() which was introduced in

	commit 3e4339e6f96e8c4f38a9c6607b98d3e96a2ed783
	Author: Linus Torvalds <torvalds@osdl.org>
	Date:   Sun Jun 18 11:45:02 2006 -0700

	    Remove "refs" field from "struct object"

Fix below.
---
Fix grow_refs_hash()

As the hash values depend on the hash size, they have to be
recalulated when growing the hash. It's possible (though unlikely)
that there are duplicates even with the new, larger hash size, so
make grow_refs_hash() check for duplicates.

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
diff --git a/object-refs.c b/object-refs.c
index 8afa227..fa1d3c1 100644
--- a/object-refs.c
+++ b/object-refs.c
@@ -25,6 +25,8 @@ static void grow_refs_hash(void)
 		if (!ref)
 			continue;
 		j = hash_obj(ref->base, new_hash_size);
+		while (new_hash[j])
+			j = (j + 1) % new_hash_size;
 		new_hash[j] = ref;
 	}
 	free(refs_hash);
-- 
The only person who always got his work done by Friday was Robinson Crusoe

Re: Odd behavior with git and cairo-devel repo

[Linus Torvalds]

On Wed, 21 Jun 2006, Andre Noll wrote:
>
> Fix grow_refs_hash()

Ouch. 

Actually, the alternate patch is the one I had intended to do but for some 
reasons didn't.

This one also removes two more lines than it adds, but it's obviously a 
bigger patch.

Junio, depending on which one you prefer, add the appropriate

	Acked-by: Linus Torvalds <torvalds@osdl.org>

(for Andre's one) or

	Signed-off-by: Linus Torvalds <torvalds@osdl.org>

if you take this one.

		Linus
---
 object-refs.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/object-refs.c b/object-refs.c
index a7d49c6..b1b8065 100644
--- a/object-refs.c
+++ b/object-refs.c
@@ -12,6 +12,18 @@ static unsigned int hash_obj(struct obje
 	return hash % n;
 }
 
+static void insert_ref_hash(struct object_refs *ref, struct object_refs **hash, unsigned int size)
+{
+	int j = hash_obj(ref->base, size);
+
+	while (hash[j]) {
+		j++;
+		if (j >= size)
+			j = 0;
+	}
+	hash[j] = ref;
+}
+
 static void grow_refs_hash(void)
 {
 	int i;
@@ -20,30 +32,16 @@ static void grow_refs_hash(void)
 
 	new_hash = calloc(new_hash_size, sizeof(struct object_refs *));
 	for (i = 0; i < refs_hash_size; i++) {
-		int j;
 		struct object_refs *ref = refs_hash[i];
 		if (!ref)
 			continue;
-		j = hash_obj(ref->base, new_hash_size);
-		new_hash[j] = ref;
+		insert_ref_hash(ref, new_hash, new_hash_size);
 	}
 	free(refs_hash);
 	refs_hash = new_hash;
 	refs_hash_size = new_hash_size;
 }
 
-static void insert_ref_hash(struct object_refs *ref)
-{
-	int j = hash_obj(ref->base, refs_hash_size);
-
-	while (refs_hash[j]) {
-		j++;
-		if (j >= refs_hash_size)
-			j = 0;
-	}
-	refs_hash[j] = ref;
-}
-
 static void add_object_refs(struct object *obj, struct object_refs *ref)
 {
 	int nr = nr_object_refs + 1;
@@ -51,7 +49,7 @@ static void add_object_refs(struct objec
 	if (nr > refs_hash_size * 2 / 3)
 		grow_refs_hash();
 	ref->base = obj;
-	insert_ref_hash(ref);
+	insert_ref_hash(ref, refs_hash, refs_hash_size);
 	nr_object_refs = nr;
 }
 

Re: Odd behavior with git and cairo-devel repo

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Ouch. 

Ouch indeed ;-).

> Actually, the alternate patch is the one I had intended to do but for some 
> reasons didn't.
>
> This one also removes two more lines than it adds, but it's obviously a 
> bigger patch.

I'll take this, since I think keeping the hash population
(collision maintenance) code in one place like your version does
seems cleaner for me to read three months down the road.

Again, thanks Andre, Art and Linus.