Re: [PATCH] Fix segfault in merge-recursive

From: Dave O <cxreg@pobox.com>
Date: May 8, 2009, 23:54 UTC
Message-ID: <alpine.DEB.2.00.0905081624230.30999@narbuckle.genericorp.net>
In-Reply-To: <alpine.DEB.1.00.0905090012410.4601@intel-tinevez-2-302>

On Sat, 9 May 2009, Johannes Schindelin wrote:

Show 27 quoted lines

> Hi,
>
> On Fri, 8 May 2009, Dave O wrote:
>
>> On Fri, 8 May 2009, Johannes Schindelin wrote:
>>
>>> When there is no "common" tree (for whatever reason), we must not
>>> throw a segmentation fault.
>>>
>>> Noticed by Dave O.
>>
>> While this patch does prevent a segfault, it totally fails to recognize
>> any conflicts in the merge.  Reverting 36e3b5e produces an ordinary
>> merge conflict with some rename/delete conflicts, and others including
>> content related conflicts.  I'm not sure I wouldn't rather have the
>> segfault than the grossly incorrect automerge.
>>
>> I'll continue debugging the triggering condition to see if I can
>> understand why the index is left dirty, leading to this NULL tree.
>
> One thing I realized while trying to quickly fix the issue for you was
> that the recognized merge base was NULL.  I.e. merge-recursive did _not_
> find a merge base.
>
> but due to too many renames, maybe it did not.
>
> Probably that is the issue.

That's not what I witnessed, although it's possible I missed something:

./git-merge-crash: line 127: 29751 Segmentation fault git merge F count@bokonon:~/git-crash/crash-test$ git merge-base -a F G 8ffd08037781ab7811f9e7983b87a29ea9ea21d9 79ac36c0bd8525e087fdb278bac9cabfa655ba47 count@bokonon:~/git-crash/crash-test$ git merge-base -a 8ffd080 79ac36c 03ca38c681cd9f832fe68d30ea2d8dfa54cbaf75

What I did find, is that the tree is coming back NULL due to the early return in write_tree_from_memory(), which in turn is due to unmerged_cache() returning true. If verbosity is up high enough, that function will indicate that the paths of the unmerged entries are exactly the ones affected in the commit I referenced earlier:

   There are unmerged index entries:
   3 data/moved-99
   3 data/moved-990
   [...]

Interestingly, I was able to remove quite a bit of the script and still induce the crash, including the part where it causes too many renames. It appears that all that's needed is a delete/rename conflict in a recursive call.

The new version is here: http://genericorp.net/~count/git-merge-crash-shorter

Once again, I don't really know what the implications of the index operations that are happening here are, but the update_stages() call in a recursive merge must be doing surprising.

     Dave