Re: [PATCH] Fix segfault in merge-recursive

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 9, 2009, 07:48 UTC
Message-ID: <alpine.DEB.1.00.0905090947120.27348@pacific.mpi-cbg.de>
In-Reply-To: <7vocu3p3pr.fsf@alter.siamese.dyndns.org>

Hi,

On Fri, 8 May 2009, Junio C Hamano wrote:

Show 9 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When there is no "common" tree (for whatever reason), we must not
> > throw a segmentation fault.
> 
> You described why the old code was wrong (i.e. "init_tree_desc_from_tree 
> is called with common == NULL"), but there is no mention why the new 
> code is correct.  For the purpose of satisfying the above statement, you 
> could have just exit(0) as well ;-)

Yes, I should have marked this patch as "please test if the result is what you think it should be", because I was running out of time and therefore could not properly think things through.

Show 13 quoted lines

> > +	else {
> > +		opts.merge = 0;
> > +		opts.head_idx = 1;
> > +		init_tree_desc_from_tree(t+0, head);
> > +		init_tree_desc_from_tree(t+1, merge);
> > +		rc = unpack_trees(2, t, &opts);
> > +	}
> 
> This looks more like a half of branch-switch from HEAD to MERGE, not a
> merge between HEAD and MERGE as two equal histories.  Shouldn't it be
> doing a three-way tree merge using an empty tree object as the common
> ancestor instead, just like merge_recursive.c::merge_recursive() itself
> does?

But of course!

Thanks, Dscho