Re: SEGV in git-merge recursive:

From

Johannes Schindelin <johannes.schindelin@gmx.de>

Date

Mar 30, 2007, 21:00 UTC

Message-ID

<Pine.LNX.4.63.0703302239050.4045@wbgn013.biozentrum.uni-wuerzburg.de>

In-Reply-To

<Pine.LNX.4.64.0703291237240.6730@woody.linux-foundation.org>

Hi,

On Thu, 29 Mar 2007, Linus Torvalds wrote:

> On Thu, 29 Mar 2007, Linus Torvalds wrote:
> 
> > 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.

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.