Re: SEGV in git-merge recursive:

Hi,

On Thu, 29 Mar 2007, Linus Torvalds wrote:

On Thu, 29 Mar 2007, Linus Torvalds wrote:

quoted

It's not the initial commit. It's a criss-cross merge, and it's a 
virtual commit created by a previous level of merging.

Apply this patch to see it blow up much earlier, when that bogus 
commit with a NULL tree is created.

(I didn't debug *why* that happens, but maybe this gets somebody 
further)

Well, it happens because "git_write_tree()" returns NULL. Which in turn 
is because "unmerged_index()" returns true.

merge_trees() tries to clean up the unmerged index, but apparently 
doesn't do good enough of a job, so git_write_tree() is called with 
entries still unmerged..

Actually, this is not the complete truth.

This particular case has a conflicting rename/rename in an _intermediate_ 
commit. This _cannot_ be resolved automatically, not even by putting 
conflict markers into the appropriate files (*1*).

IMHO, there is actually no way merge_trees() can fix the conflicts enough 
to write a tree.

So, the only way I see to avoid that SEGV is to something like this:

diff --git a/merge-recursive.c b/merge-recursive.c
index ece2238..cbc39e9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -1135,8 +1135,13 @@ static int merge_trees(struct tree *head,
 	else
 		clean = 1;
 
-	if (index_only)
+	if (index_only) {
 		*result = git_write_tree();
+		if (!*result) {
+			flush_output();
+			die ("cannot continue merging.");
+		}
+	}
 
 	return clean;
 }

NOTE: I will not make the error again _not_ to point out that this is 
_just_ a hint at what a proper patch would look like.

For example, a proper patch would include a test case, _and_ would print a 
proper hint about GIT_MERGE_VERBOSITY (otherwise, you will only get a 
fatal error "cannot continue merging", without any hint about what went 
wrong).

Ciao,
Dscho

*1* I played with the idea to do a threeway merge of the conflicting files 
(src->dst1,dst2, using src as common version), but I am not quite sure if 
it is worth the confusion it seeds.

Besides, there is another type of rename/rename conflict, which _cannot_ 
be solved in that manner: (src1,src2->dst). And for this case, we have to 
have a sane way out anyway.