[PATCH 1 of 2] convert: temporarily disable hg p2 optimization

[Durham Goode]

On 10/22/15 3:16 AM, Mads Kiilerich wrote:
> On 10/16/2015 11:14 PM, Durham Goode wrote:
>>
>>
>> On 10/16/15 2:11 PM, Durham Goode wrote:
>>>
>>>
>>> On 10/15/15 2:46 PM, Mads Kiilerich wrote:
>>>> # HG changeset patch
>>>> # User Mads Kiilerich <madski at unity3d.com>
>>>> # Date 1444945384 -7200
>>>> #      Thu Oct 15 23:43:04 2015 +0200
>>>> # Node ID 8c7010f12530b69c86d2bcdd70098ced582ece49
>>>> # Parent  07db7e95c464537aeb2dd7aba39de0813eaffd04
>>>> convert: temporarily disable hg p2 optimization
>>>>
>>>> Disabling the optimization gives a test change. That proves that 
>>>> the optimized
>>>> code path is wrong. That seems to have been introduced in 
>>>> a75d24539aba.
>>>>
>>>> diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
>>>> --- a/hgext/convert/hg.py
>>>> +++ b/hgext/convert/hg.py
>>>> @@ -221,7 +221,7 @@ class mercurial_sink(converter_sink):
>>>>           files = dict(files)
>>>>             def getfilectx(repo, memctx, f):
>>>> -            if p2ctx and f in p2files and f not in copies:
>>>> +            if p2ctx and f in p2files and f not in copies and False:
>>>>                   self.ui.debug('reusing %s from p2\n' % f)
>>>>                   try:
>>>>                       return p2ctx[f]
>>>> diff --git a/tests/test-convert-filemap.t 
>>>> b/tests/test-convert-filemap.t
>>>> --- a/tests/test-convert-filemap.t
>>>> +++ b/tests/test-convert-filemap.t
>>>> @@ -723,11 +723,11 @@ test converting merges into a repo that
>>>>     $ hg -R merge-test2 manifest -r tip
>>>>     converted/a
>>>>     converted/b
>>>> -  x
>>>>     $ hg -R merge-test2 log -G -T '{shortest(node)} {desc}\n{files 
>>>> % "- {file}\n"}\n'
>>>> -  o    6eaa merge a & b
>>>> +  o    0390 merge a & b
>>>>     |\   - converted/a
>>>>     | |  - toberemoved
>>>> +  | |  - x
>>>>     | |
>>>>     | o  2995 add b
>>>>     | |  - converted/b
>>> I don't understand what you're saying here.
>>>
>>> In the test, it takes merge-test1 (which adds file 'x' in rev 1), 
>>> and it calls convert in such a way that it imports the contents of 
>>> merge-test2 on top of the 'x' commit.  That means 'x' should not be 
>>> affected by the convert, and that it should appear in the tip 
>>> manifest, as it currently does.
>>>
>>> Your change makes the convert delete 'x', when 'x' isn't related to 
>>> the incoming changes in any way.  So am I missing something?
>> I got my repo-names reversed.  Here's the same text with the correct 
>> repo names:
>>
>> In the test, it takes merge-test1 (which edits files 'a' and 'b') and 
>> merge-test2 (which adds 'x'), and it calls convert in such a way that 
>> it imports the contents of merge-test1 on top of the 'x' commit.  
>> That means 'x' should not be affected by the convert, and that it 
>> should appear in the tip manifest, as it currently does.
>>
>> Your change makes the convert delete 'x', when 'x' isn't related to 
>> the incoming changes in any way.  So am I missing something?
>
> First, do you agree that the p2clean part just is an optimization? Or 
> should we intentionally put different semantics in that?
>
> /Mads
>
p2clean may be an optimization, but p2clean is no longer used inside 
getfilectx(). We now use p2files, which is a union of p2clean and the 
files that exist in the target p2 that don't exist anywhere in the source.

p2files is necessary because it is the only way target-p2 specific data 
can make it into the commit (because the way convert works is to take 
target-p1 and apply the incoming changes on top of it, which would lose 
changes specific to target-p2)