[PATCH 2 of 2] simplemerge: Display "base" in case of conflict

[Mads Kiilerich]

See also http://markmail.org/message/kfuhkf4677k4rxkv .


Pierre-Yves David wrote, On 10/02/2012 07:32 PM:
> On Thu, Sep 27, 2012 at 01:49:43PM +0200, Mads Kiilerich wrote:
>> On 09/27/2012 12:06 PM, pierre-yves.david at logilab.fr wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>>> # Date 1348740176 -7200
>>> # Node ID ce8ea3bbea8d818067750c06ebcf89f4c7471ddf
>>> # Parent  ac1131f2927400cb0e031d6929cfa3ce07cf1a27
>>> simplemerge: Display "base" in case of conflict
>>>
>>> This changeset enables a feature already present in `simplemerge;py`: Displaying
>>> common-base content during merges. For this purpose we have to disable an unused
>>> function of simple merge.
>> It would probably be better to do this part first in a separate change.
> Right
>
>> But IIRC simplemerge.py has a lot of cruft that only is used by
>> contrib/simplemerge. It would perhaps be better to drop that from
>> contrib remove the cruft.
> Missing word here. Do you suggest to just drop the feature completly ?

Kind of.

If I was touching simplemerge.py again then I would consider dropping 
contrib/simplemerge and clean it up first.

The maintenance overhead for simplemerge is however low as long as 
nobody touches the code and no need for more maintenance is introduced, 
so I would not unconditionally suggest giving it the knife.

>
>>> Adding the base revision to merge conflict help a lot to understand the source
>>> of a conflict. Being able to compare each side of the merge with the original
>>> code allows to understand the changes introduced by each branches to create a
>>> valid merge.
>>>
>>> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
>>> --- a/mercurial/filemerge.py
>>> +++ b/mercurial/filemerge.py
>>> @@ -208,12 +208,13 @@ def _imerge(repo, mynode, orig, fcd, fco
>>>       if r:
>>>           a, b, c, back = files
>>>           ui = repo.ui
>>>           label = ['local: %s' % fcd.changectx(),
>>> -                 'other: %s' % fco.changectx()]
>>> -        r = simplemerge.simplemerge(ui, a, b, c, label=label)
>>> +                 'other: %s' % fco.changectx(),
>>> +                 'base: %s'  % fca.changectx()]
>>> +        r = simplemerge.simplemerge(ui, a, b, c, label=label, no_minimal=True)
>> no_minimal=True is a big and unacceptable change.
> no_minimal=False only enables simple merge to detect "manually solved
> conflict". See the `reprocess_merge_regions` function in
> `mercurial.simplemerge`. it's doc is:
>
>      """Where there are conflict regions, remove the agreed lines.
>
>      Lines where both A and B have made the same changes are
>      eliminated.
>      """
>
> This is **never** useful here as the unchanged file from A and B are extracted
> from mercurial before any call to simple merge.

I don't get your reasoning about A and B, but it is **sometimes** 
essential to get a 'minimal' diff.

Consider

   <<<<<<< local: ddc480535864+
   2
   9
   4
   1
   3
   2
   1
   6
   9
   5
   ======= base: e47bb24f5f69
   =======
   2
   9
   4
   1
   3
   2
   3
   6
   9
   5
   >>>>>>> other: dd5a0173b17f

vs

   2
   9
   4
   1
   3
   2
   <<<<<<< local
   1
   =======
   3
   >>>>>>> other
   6
   9
   5


>
>> It will cause a
>> different and confusingly complex markup in the case where almost
>> the same change has been done in both branches. (I suggest extending
>> test-conflict.t to cover that case.)
> Are you arguing about adding the base ? It' more often useful to me that
> detrimental. Several user have complained that "you do not know the base while
> merging with marker. I think it's a net win.

As an option, yes. I missed it too when I switched from Perforce. But I 
don't think it should be mandatory and I don't think it should be default.

>
>> I suggest introducing a [merge-tools] internal:merge.showbase
>> boolean setting for enabling this new behaviour.

/Mads