threads / discuss / 4595

Odd behavior with git and cairo-devel repo

Subject: Odd behavior with git and cairo-devel repo

## tl;dr

7 messages between Jun 21, 2006 and Jun 21, 2006.

replies: 6people: 4as markdown or json

Art Haas· Jun 21, 2006, 01:00 UTC · lore
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
Andre Noll· Jun 21, 2006, 02:46 UTC · re: Art Haas · lore

Re: Odd behavior with git and cairo-devel repo

On 20:00, Art Haas wrote:
Show 5 quoted lines
> $ 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
Junio C Hamano· Jun 21, 2006, 11:01 UTC · re: Andre Noll · lore

Re: Odd behavior with git and cairo-devel repo

Andre Noll <maan@systemlinux.org> writes:
Show 11 quoted lines
> 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!
Art Haas· Jun 21, 2006, 12:06 UTC · re: Andre Noll · lore

Re: Odd behavior with git and cairo-devel repo

On Wed, Jun 21, 2006 at 04:46:05AM +0200, Andre Noll wrote:
Show 13 quoted lines
> 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
Andre Noll· Jun 21, 2006, 17:46 UTC · re: Art Haas · lore

Re: Odd behavior with git and cairo-devel repo

On 07:06, Art Haas wrote:
Show 15 quoted lines
> 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
Linus Torvalds· Jun 21, 2006, 18:01 UTC · re: Andre Noll · lore

Re: Odd behavior with git and cairo-devel repo

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;
 }
Junio C Hamano· Jun 21, 2006, 19:43 UTC · re: Linus Torvalds · lore

Re: Odd behavior with git and cairo-devel repo

Linus Torvalds <torvalds@osdl.org> writes:
> Ouch. 
Ouch indeed ;-).
Show 5 quoted lines
> 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.

← back to recent threads