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

Ryan McElroy rm at fb.com
Mon Oct 20 10:31:12 CDT 2014

I don't have an intuition on how to reproduce this without obsolescence markers -- in order to make the changelog and filelog manifests have different topologies, the stripping needs to be avoided, which requires obsolescence, right? I'm happy to be wrong here, and I'll give it more thought to see if I can figure out where this would reproduce otherwise.

I'll update the docstring and move the test in the meantime.


-----Original Message-----
From: Pierre-Yves David [mailto:pierre-yves.david at ens-lyon.org] 
Sent: Saturday, October 18, 2014 2:33 PM
To: Matt Mackall; Ryan McElroy
Cc: mercurial-devel at selenic.com
Subject: Re: [PATCH] amend: fix amending rename commit with obsolescence markers

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.


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

More information about the Mercurial-devel mailing list