Re: regression in 92392b4

[Pierre Habouzit <madcoder@debian.org>]

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

On Wed, Jul 23, 2008 at 10:49:04AM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 23 Jul 2008, Björn Steinbrink wrote:
> 
> > On 2008.07.23 01:17:45 +0200, Pierre Habouzit wrote:
> > >   Hi, here is a manual painful down-secting (opposed to a bisect ;P) I
> > > did, since git in next cannot fetch on a regular basis for me. The
> > > culprit seems to be commit  92392b4:
> > > 
> > >     ┌─(1:11)──<~/dev/scm/git 92392b4....>──
> > >     └[artemis] git fetch
> > >     remote: Counting objects: 461, done.
> > >     remote: Compressing objects: 100% (141/141), done.
> > >     remote: Total 263 (delta 227), reused 155 (delta 121)
> > >     Receiving objects: 100% (263/263), 95.55 KiB, done.
> > >     fatal: Out of memory, malloc failed
> > >     fatal: index-pack failed
> > >     [2]    16674 abort (core dumped)  git fetch
> > > 
> > >     ┌─(1:12)──<~/dev/scm/git 92392b4....>──
> > >     └[artemis] git checkout -m HEAD~1; make git-index-pack
> > >     Previous HEAD position was 92392b4... index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
> > >     HEAD is now at 03993e1... index-pack: Track the object_entry that creates each base_data
> > >     GIT_VERSION = 1.5.6.3.3.g03993
> > > 	CC index-pack.o
> > > 	LINK git-index-pack
> > > 
> > >     ┌─(1:12)──<~/dev/scm/git 03993e1....>──
> > >     └[artemis] git fetch
> > >     remote: Counting objects: 461, done.
> > >     remote: Compressing objects: 100% (141/141), done.
> > >     remote: Total 263 (delta 227), reused 155 (delta 121)
> > >     Receiving objects: 100% (263/263), 95.55 KiB, done.
> > >     Resolving deltas: 100% (227/227), completed with 153 local objects.
> > >     From git://git.kernel.org/pub/scm/git/git
> > >        5ba2c22..0868a30  html       -> origin/html
> > >        2857e17..abeeabe  man        -> origin/man
> > >        93310a4..95f8ebb  master     -> origin/master
> > >        559998f..e8bf351  next       -> origin/next
> > > 
> > > You can see the commit sha's in the prompt. 03993e1 is fine, 92392b4 is
> > > broken, I've absolutely no clue about what happens.
> > > 
> > > All I can say is that at some point in get_data_from_pack, obj[1].idx
> > > points to something that is *not* a sha so it's probably corrupted.
> > > (from index-pack.c).
> > 
> > Here's how to reproduce:
> 
> Funny.  That does not reproduce the bug here at all.
> 
> But then, it is unsurprising, since both Pierre and me did something 
> similar yesterday, fetching _just_ the pre-fetch refs into a freshly 
> initted Git repository, and then fetching from kernel.org.
> 
> Tested on x86_64.

I can reproduce on x86_64 here. And I think I get the problem, and IMHO
the pruning thing is flawed. We need more than *one* base to be kept at
a time, pruning is too aggressive, and we still keep pointers to
actually pruned data.

This patch makes the issue non reproducible for me. The rationale is
that get_base_data will already prune and is called as often, and in
safer places.

=================================
diff --git a/index-pack.c b/index-pack.c
index ac20a46..5440e43 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -245,7 +245,6 @@ static void link_base_data(struct base_data *base, struct base_data *c)
 	c->base = base;
 	c->child = NULL;
 	base_cache_used += c->size;
-	prune_base_data(c);
 }
 
 static void unlink_base_data(struct base_data *c)
=================================

*But* I'm absolutely not sure it's enough. This should be written using
reference counting instead of the "retain" hack, and prune should not
free() anything that isn't with a 0 reference counter. The current code
is brittle, it makes my head explode when I try to understand if the
get_base_data() we temporarily keep pointers too may be harmed or not.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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