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

login
register
mail settings
Submitter Mads Kiilerich
Date Oct. 15, 2015, 9:46 p.m.
Message ID <8c7010f12530b69c86d2.1444945616@localhost.localdomain>
Download mbox | patch
Permalink /patch/11124/
State Deferred
Headers show

Comments

Mads Kiilerich - Oct. 15, 2015, 9:46 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@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.
Durham Goode - Oct. 16, 2015, 9:11 p.m.
On 10/15/15 2:46 PM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@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?
Durham Goode - Oct. 16, 2015, 9:14 p.m.
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@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?
Mads Kiilerich - Oct. 22, 2015, 10:16 a.m.
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@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
Durham Goode - Oct. 22, 2015, 5:15 p.m.
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@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)

Patch

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