Re: git died

[Junio C Hamano <junkio@cox.net>]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, 26 Apr 2007, Christian wrote:
>
>> Julian Phillips wrote:
>> 
>> Loaded symbols for /lib/tls/libnss_dns.so.2
>> #0  decode_tree_entry (desc=0xbff5110c, buf=0x0, size=454) at tree-walk.c:10
>> 10              while ((c = *str++) != ' ') {
>> (gdb) bt
>> #0  decode_tree_entry (desc=0xbff5110c, buf=0x0, size=454) at tree-walk.c:10
>> #1  0x0804b9a8 in mark_tree_uninteresting (tree=0x80d0240) at revision.c:65
>> #2  0x080534cd in main (argc=2, argv=Cannot access memory at address 0x1ca
>> ) at http-push.c:1998
>
> It looks like some strange corruption in http-push.
> 
> You seem to have a NULL "buf" pointer (which certainly explains a 
> SIGSEGV!), but mark_tree_uninteresting will have done
>
> 	if (parse_tree(tree) < 0)
> 		die("bad tree %s", sha1_to_hex(obj->sha1));
>
> 	init_tree_desc(&desc, tree->buffer, tree->size);
> 	while (tree_entry(&desc, &entry)) {
> 		..
>
>
> and tree_entry() does
>
> 	*entry = desc->entry;
> 	update_tree_entry(desc);
>
> and none of that sets "entry->buf" to NULL unless "size" was also zero.

This has been bugging me for quite some time by now.

The call to parse_tree is coming from mark_edge_parents_uninteresting(),
which in turn is called from mark_edges_uninteresting(), both in
http-push.c

However, I think these two functions are cut&paste from old rev-list.c,
which we refactored into list-objects.c some time ago (list-objects.c
has almost identical looking functions).

I admit that I do not care much about http-push, but I looked at
these two functions in list-objects.c; it is not clear why we do
not get the same error while running rev-list --objects-edge
(aka "thin pack transfer").

mark_edges_uninteresting calls mark_tree_uninteresting() on the
tree associated with uninteresting edge commits.  I am
reasonably sure that these commits have always been parsed when
prepare_revision_walk() returns.  However, it also calls
mark_edge_parents_uninteresting(), trying to make the parent
commit and its tree uninteresting.  I do not see how we are
guaranteeing that the parents are parsed (and their trees are
available to us).

While I think your analysis above is correct, which means that
the breakage Julian is seeing (which does not have NULL tree
object) has nothing to do with what I observed above, I'd sleep
better if we had something like this patch:


 http-push.c    |    2 ++
 list-objects.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/http-push.c b/http-push.c
index e3f7675..156f42e 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1995,6 +1995,8 @@ static void mark_edge_parents_uninteresting(struct commit *commit)
 		struct commit *parent = parents->item;
 		if (!(parent->object.flags & UNINTERESTING))
 			continue;
+		if (!parent->object.parsed)
+			parse_object(parent->object.sha1);
 		mark_tree_uninteresting(parent->tree);
 	}
 }
diff --git a/list-objects.c b/list-objects.c
index 310f8d3..c2e74f1 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -109,6 +109,8 @@ static void mark_edge_parents_uninteresting(struct commit *commit,
 		struct commit *parent = parents->item;
 		if (!(parent->object.flags & UNINTERESTING))
 			continue;
+		if (!parent->object.parsed)
+			parse_object(parent->object.sha1);
 		mark_tree_uninteresting(parent->tree);
 		if (revs->edge_hint && !(parent->object.flags & SHOWN)) {
 			parent->object.flags |= SHOWN;