[PATCH] amend: fix amending rename commit with obsolescence markers

[Pierre-Yves David]

On 10/18/2014 01:49 PM, Matt Mackall wrote:
> On Fri, 2014-10-17 at 19:38 -0700, Ryan McElroy wrote:
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy at fb.com>
>> # Date 1413466506 25200
>> #      Thu Oct 16 06:35:06 2014 -0700
>> # Node ID 90fe3166419ec665f31cdc657a8ebe753d7f168d
>> # Parent  48c0b101a9de1fdbd638daa858da845cd05a6be7
>> amend: fix amending rename commit with obsolescence markers
>>
>> This addresses the bug described in issue4405: when obsolescence markers are
>> enabled, amending a commit with a file move can lead to the copy information
>> being lost.
>
> ..but it's purely a DAG topology issue, as the copies code is completely
> unaware of such markers, right? Which means it can affect anywhere this
> sort of topology can occur: graft, rebase, histedit, shelve, etc.
>
> (It'd be better to take obsolescence (and our horrible horrible amend
> with temporary commits strategy) out of the picture entirely, as it just
> makes things more confusing.)

The obssmarker version is making this issue appears because of its lack 
of stripping. You raise an interesting point here that once we get rid 
of this (horrible horrible) temporary commit, this test will be be a 
valid test for this issue.

+1 for recreating the test without obsolescence involved.

>> +    # @  4 mv b c
>> +    # |
>> +    # | x  3 temporary amend commit for 60e352014901
>> +    # | |
>> +    # | x  2 change message
>> +    # |/
>> +    # | x  1 mv a b
>> +    # |/
>> +    # o  0 add a
>> +    #
>> +    # During the second amend (step 4), limit is calculated to be 3,
> ...
>> +    # In this case, a is rev 4 and b is rev 0;
> ...
>> +    return min(limit, a, b)
>
> This doesn't make sense. See the docstring:
>
>      """Find the earliest revision that's an ancestor of a or b but not both,
>      None if no such revision exists.
>      """

The wrong thing here is the docstring. We could summarize the docstring 
of this function as: "return the right value to bound tracecopy lookup"

> 3 is not an ancestor of a(=4) or b(=0) n your picture, so 3 shouldn't
> happen. And min(limit, a, b) is going to be 0, which doesn't comply with
> the "not both" clause.

As far as I understand this ancestors based logic is made necessary by 
case described in this test case.

http://selenic.com/hg/file/19f5273c9f3e/tests/test-mv-cp-st-diff.t#l1572

This ancestors based logic fails short (pun intended) in the case 
described by Ryan (when the filelog graph and changeset graph have 
different topology).

This mean call get us to we properly search down to the base (param "a") 
in all case and ensure we search beyond that when necessary 
(test-mv-cp-st-diff case).


To conclude I believe this change is correct and that the docstring need 
updating.

-- 
Pierre-Yves David