[PATCH] backout: handle file moves correctly (issue1932)

[Pierre-Yves David]

On 01/08/2015 03:26 PM, Mateusz Kwapich wrote:
>
> On 1/8/15, 3:06 PM, "Matt Mackall" <mpm at selenic.com> wrote:
>
>> On Thu, 2015-01-08 at 13:39 -0800, Mateusz Kwapich wrote:
>>> # HG changeset patch
>>> # User Mateusz Kwapich <mitrandir at fb.com>
>>> # Date 1420752870 28800
>>> #      Thu Jan 08 13:34:30 2015 -0800
>>> # Node ID b140af105987b9db7a296d24584a557bdc67fc6e
>>> # Parent  7ad155e13f0f51df8e986a0ec4e58ac9a0ccedbb
>>> backout: handle file moves correctly (issue1932)
>>
>>> The merge logic (ab)used by backout
>>
>> It's actually not abuse. It's very important core functionality.
>>
>>>   wasn't detecting file moves when the
>>> parameter which should be common ancestor of merge heads was in fact
>>> child of
>>> one of them.
>>
>> In the abstract, merge is simply the operation that given three state
>> vectors A, X, Y, creates a new state M = A + (X - A) + (Y - A) (where +
>> is "patch" and - is "diff"). It doesn't matter what the relationships
>> between the three inputs are. Don't get too distracted that we call 'A'
>> the 'ancestor' in the code, it's simply more convenient to reason about
>> in terms of a normal merge.
>>
>> However, various parts of the merge and copy code were indeed originally
>> written with the assumption that a would be an ancestor of x and y and
>> made various assumptions about revlog ordering, which
>> has implications for every single thing that calls merge in a
>> non-vanilla fashion: backout, rebase, graft, evolve, histedit, etc..
>>
>> Most of that is fixed. The primary offender at this point is
>> mergecopies(). I started down the path to replacing that by writing
>> pathcopies() a few years back, which doesn't care about direction in
>> history and deals with most of our non-merge copy-detection needs.
>>
>> But finishing the job requires rewriting mergecopies() based on
>> pathcopies() or otherwise making it direction-agnostic... which is what
>> I've told everyone who's asked about merge+copy bugs for the past few
>> years.
>>
>> Since you only mention "backout" in your subject and only hit a very
>> specific special case, I suspect you're missing most of the related
>> problems here and are just making things that much more confusing for
>> whoever comes along to do the larger fix.
>>
>> --
>> Mathematics is the supreme nostalgia of our time.
>
> Hello Matt,
>
> Thanks for detailed info about merge logic. I see that mergecopies() needs
> a direction agnostic rewrite. This patch came from the need for working
> backouting moves and copies. It¹s definitely more hotfix than a permanent
> solution and yes: it¹s not making the mergecopies() more readable. But I
> think I works good in this particular case and it¹s fixing a backout
> behavior to match users expectations. So I think before somebody rewrites
> mergecopies() in near future (maybe me? :-) ) it will be good to have
> backout working correctly.

Matt have a good point, this copy logic is broken for age and someone 
needs to eventually fix it once and for all. There is a lot of other bug 
affecting people right now related to this (graft, histedit, rebase, 
etc). As Mateusz already spent time analyzing the issue and 
understanding the code, Mateusz is a good candidate for building a fix.

However, If Mateusz want ammo to be lazy, he could point that this issue 
with rename is probably a regression from 3.0.


-- 
Pierre-Yves David