Patchwork [3,of,4,v2] rebase: avoid redundant repo[rev].rev() - just keep working in rev space

login
register
mail settings
Submitter Mads Kiilerich
Date Dec. 2, 2014, 4:13 a.m.
Message ID <63e92a66482bb5af8b9b.1417493628@localhost.localdomain>
Download mbox | patch
Permalink /patch/6939/
State Accepted
Commit ffef6d503ab2b6c732ae6c786066b6ac9f6e858d
Headers show

Comments

Mads Kiilerich - Dec. 2, 2014, 4:13 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1417493579 -3600
#      Tue Dec 02 05:12:59 2014 +0100
# Node ID 63e92a66482bb5af8b9bdd95dfb0fff500eef582
# Parent  9a8d248290a4e634a6bdc0a1d07514fe8e971ad9
rebase: avoid redundant repo[rev].rev() - just keep working in rev space
Matt Harbison - Dec. 5, 2014, 4:39 a.m.
On Mon, 01 Dec 2014 23:13:48 -0500, Mads Kiilerich <mads@kiilerich.com>  
wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1417493579 -3600
> #      Tue Dec 02 05:12:59 2014 +0100
> # Node ID 63e92a66482bb5af8b9bdd95dfb0fff500eef582
> # Parent  9a8d248290a4e634a6bdc0a1d07514fe8e971ad9
> rebase: avoid redundant repo[rev].rev() - just keep working in rev space
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -540,20 +540,20 @@ def rebasenode(repo, rev, p1, state, col
>      'Rebase a single revision'
>      # Merge phase
>      # Update to target and merge it with local
> -    if repo['.'].rev() != repo[p1].rev():
> -        repo.ui.debug(" update to %d:%s\n" % (repo[p1].rev(), repo[p1]))
> +    if repo['.'].rev() != p1:
> +        repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))

This crashed when running evolve 8d28bb4fc127, but the Windows test suite  
runs clean.  Maybe it needs to be "%s:%s", but I don't have time to chase  
down if p1 is always a string right now, and didn't want to throw out a  
patch that guesses.  There are a couple of debug lines below this change  
too.

$ ../hg evolve
move:[23950] addremove: restore the relative path printing when files are  
named
atop:[23951] match: introduce uipath() to properly style a file path
** Unknown exception encountered with possibly-broken third-party  
extension evolve
** which supports versions 3.2 of Mercurial.
** Please disable evolve and try your action again.
** If that fixes the bug please report it to http://bz.selenic.com/
** Python 2.7.2 (default, Jun 12 2011, 15:08:59) [MSC v.1500 32 bit  
(Intel)]
** Mercurial Distributed SCM (version 3.2.2+44-c237499a7fba+20141205)
** Extensions loaded: eol, convert, graphlog, patchbomb, progress,  
extdiff, strip, mq, evolve, rebase
Traceback (most recent call last):
   File "../hg", line 43, in <module>
     mercurial.dispatch.run()
...
   File "c:\Users\Matt\Projects\hg\mercurial\util.py", line 679, in check
     return func(*args, **kwargs)
   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1225, in  
evolve
     progresscb=progresscb)
   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1251, in  
_evolveany
     return _solveunstable(ui, repo, tro, dryrunopt, confirmopt, progresscb)
   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1355, in  
_solveunstable
     relocate(repo, orig, target, keepbranch)
   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 811, in  
relocate
     orig.p1().node())
   File "c:\Users\Matt\Projects\hg\hgext\rebase.py", line 547, in rebasenode
     repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
TypeError: %d format: a number is required, not str

--Matt
Mads Kiilerich - Dec. 5, 2014, 3:16 p.m.
On 12/05/2014 05:39 AM, Matt Harbison wrote:
>
> On Mon, 01 Dec 2014 23:13:48 -0500, Mads Kiilerich 
> <mads@kiilerich.com> wrote:
>
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1417493579 -3600
>> #      Tue Dec 02 05:12:59 2014 +0100
>> # Node ID 63e92a66482bb5af8b9bdd95dfb0fff500eef582
>> # Parent  9a8d248290a4e634a6bdc0a1d07514fe8e971ad9
>> rebase: avoid redundant repo[rev].rev() - just keep working in rev space
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -540,20 +540,20 @@ def rebasenode(repo, rev, p1, state, col
>>      'Rebase a single revision'
>>      # Merge phase
>>      # Update to target and merge it with local
>> -    if repo['.'].rev() != repo[p1].rev():
>> -        repo.ui.debug(" update to %d:%s\n" % (repo[p1].rev(), 
>> repo[p1]))
>> +    if repo['.'].rev() != p1:
>> +        repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
>
> This crashed when running evolve 8d28bb4fc127, but the Windows test 
> suite runs clean.  Maybe it needs to be "%s:%s", but I don't have time 
> to chase down if p1 is always a string right now, and didn't want to 
> throw out a patch that guesses.  There are a couple of debug lines 
> below this change too.
>
> $ ../hg evolve
> move:[23950] addremove: restore the relative path printing when files 
> are named
> atop:[23951] match: introduce uipath() to properly style a file path
> ** Unknown exception encountered with possibly-broken third-party 
> extension evolve
> ** which supports versions 3.2 of Mercurial.
> ** Please disable evolve and try your action again.
> ** If that fixes the bug please report it to http://bz.selenic.com/
> ** Python 2.7.2 (default, Jun 12 2011, 15:08:59) [MSC v.1500 32 bit 
> (Intel)]
> ** Mercurial Distributed SCM (version 3.2.2+44-c237499a7fba+20141205)
> ** Extensions loaded: eol, convert, graphlog, patchbomb, progress, 
> extdiff, strip, mq, evolve, rebase
> Traceback (most recent call last):
>   File "../hg", line 43, in <module>
>     mercurial.dispatch.run()
> ...
>   File "c:\Users\Matt\Projects\hg\mercurial\util.py", line 679, in check
>     return func(*args, **kwargs)
>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1225, 
> in evolve
>     progresscb=progresscb)
>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1251, 
> in _evolveany
>     return _solveunstable(ui, repo, tro, dryrunopt, confirmopt, 
> progresscb)
>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1355, 
> in _solveunstable
>     relocate(repo, orig, target, keepbranch)
>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 811, 
> in relocate
>     orig.p1().node())
>   File "c:\Users\Matt\Projects\hg\hgext\rebase.py", line 547, in 
> rebasenode
>     repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
> TypeError: %d format: a number is required, not str

So evolve calls rebasenode directly, passing nodeids where Mercurial 
itself only passes revs. Fortunately it is caught early - 
http://selenic.com/hg/rev/ffef6d503ab2 removed the unused support for 
nodeids.

Calling rebasenode directly hints that evolve duplicates some parts of 
rebase internals - that seems unfortunate. If rebasenode has to stay a 
stable API then it should be documented as such ... but I doubt that is 
how it is.

/Mads
Pierre-Yves David - Dec. 5, 2014, 9:04 p.m.
On 12/05/2014 07:16 AM, Mads Kiilerich wrote:
> On 12/05/2014 05:39 AM, Matt Harbison wrote:
>>
>> On Mon, 01 Dec 2014 23:13:48 -0500, Mads Kiilerich
>> <mads@kiilerich.com> wrote:
>>
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1417493579 -3600
>>> #      Tue Dec 02 05:12:59 2014 +0100
>>> # Node ID 63e92a66482bb5af8b9bdd95dfb0fff500eef582
>>> # Parent  9a8d248290a4e634a6bdc0a1d07514fe8e971ad9
>>> rebase: avoid redundant repo[rev].rev() - just keep working in rev space
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -540,20 +540,20 @@ def rebasenode(repo, rev, p1, state, col
>>>      'Rebase a single revision'
>>>      # Merge phase
>>>      # Update to target and merge it with local
>>> -    if repo['.'].rev() != repo[p1].rev():
>>> -        repo.ui.debug(" update to %d:%s\n" % (repo[p1].rev(),
>>> repo[p1]))
>>> +    if repo['.'].rev() != p1:
>>> +        repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
>>
>> This crashed when running evolve 8d28bb4fc127, but the Windows test
>> suite runs clean.  Maybe it needs to be "%s:%s", but I don't have time
>> to chase down if p1 is always a string right now, and didn't want to
>> throw out a patch that guesses.  There are a couple of debug lines
>> below this change too.
>>
>> $ ../hg evolve
>> move:[23950] addremove: restore the relative path printing when files
>> are named
>> atop:[23951] match: introduce uipath() to properly style a file path
>> ** Unknown exception encountered with possibly-broken third-party
>> extension evolve
>> ** which supports versions 3.2 of Mercurial.
>> ** Please disable evolve and try your action again.
>> ** If that fixes the bug please report it to http://bz.selenic.com/
>> ** Python 2.7.2 (default, Jun 12 2011, 15:08:59) [MSC v.1500 32 bit
>> (Intel)]
>> ** Mercurial Distributed SCM (version 3.2.2+44-c237499a7fba+20141205)
>> ** Extensions loaded: eol, convert, graphlog, patchbomb, progress,
>> extdiff, strip, mq, evolve, rebase
>> Traceback (most recent call last):
>>   File "../hg", line 43, in <module>
>>     mercurial.dispatch.run()
>> ...
>>   File "c:\Users\Matt\Projects\hg\mercurial\util.py", line 679, in check
>>     return func(*args, **kwargs)
>>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1225,
>> in evolve
>>     progresscb=progresscb)
>>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1251,
>> in _evolveany
>>     return _solveunstable(ui, repo, tro, dryrunopt, confirmopt,
>> progresscb)
>>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 1355,
>> in _solveunstable
>>     relocate(repo, orig, target, keepbranch)
>>   File "C:/Users/Matt/Projects/hg-evolve/hgext/evolve.py", line 811,
>> in relocate
>>     orig.p1().node())
>>   File "c:\Users\Matt\Projects\hg\hgext\rebase.py", line 547, in
>> rebasenode
>>     repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
>> TypeError: %d format: a number is required, not str
>
> So evolve calls rebasenode directly, passing nodeids where Mercurial
> itself only passes revs. Fortunately it is caught early -
> http://selenic.com/hg/rev/ffef6d503ab2 removed the unused support for
> nodeids.

Yes, I need to migrate it to the new API matt made for 3.2 but I did not 
come to it yet

>
> Calling rebasenode directly hints that evolve duplicates some parts of
> rebase internals - that seems unfortunate. If rebasenode has to stay a
> stable API then it should be documented as such ... but I doubt that is
> how it is.

Feel free to break evolve.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -389,7 +389,7 @@  def rebase(ui, repo, **opts):
                     finally:
                         ui.setconfig('ui', 'forcemerge', '', 'rebase')
                 if not collapsef:
-                    merging = repo[p2].rev() != nullrev
+                    merging = p2 != nullrev
                     editform = cmdutil.mergeeditform(merging, 'rebase')
                     editor = cmdutil.getcommiteditor(editform=editform, **opts)
                     newnode = concludenode(repo, rev, p1, p2, extrafn=extrafn,
@@ -540,20 +540,20 @@  def rebasenode(repo, rev, p1, state, col
     'Rebase a single revision'
     # Merge phase
     # Update to target and merge it with local
-    if repo['.'].rev() != repo[p1].rev():
-        repo.ui.debug(" update to %d:%s\n" % (repo[p1].rev(), repo[p1]))
+    if repo['.'].rev() != p1:
+        repo.ui.debug(" update to %d:%s\n" % (p1, repo[p1]))
         merge.update(repo, p1, False, True, False)
     else:
         repo.ui.debug(" already in target\n")
     repo.dirstate.write()
-    repo.ui.debug(" merge against %d:%s\n" % (repo[rev].rev(), repo[rev]))
-    if repo[rev].rev() == repo[min(state)].rev():
+    repo.ui.debug(" merge against %d:%s\n" % (rev, repo[rev]))
+    if rev == min(state):
         # Case (1) initial changeset of a non-detaching rebase.
         # Let the merge mechanism find the base itself.
         base = None
     elif not repo[rev].p2():
         # Case (2) detaching the node with a single parent, use this parent
-        base = repo[rev].p1().node()
+        base = repo[rev].p1().rev()
     else:
         # In case of merge, we need to pick the right parent as merge base.
         #
@@ -580,8 +580,8 @@  def rebasenode(repo, rev, p1, state, col
         # Which does not represent anything sensible and creates a lot of
         # conflicts.
         for p in repo[rev].parents():
-            if state.get(p.rev()) == repo[p1].rev():
-                base = p.node()
+            if state.get(p.rev()) == p1:
+                base = p.rev()
                 break
         else: # fallback when base not found
             base = None
@@ -590,7 +590,7 @@  def rebasenode(repo, rev, p1, state, col
             raise AssertionError('no base found to rebase on '
                                  '(rebasenode called wrong)')
     if base is not None:
-        repo.ui.debug("   detach base %d:%s\n" % (repo[base].rev(), repo[base]))
+        repo.ui.debug("   detach base %d:%s\n" % (base, repo[base]))
     # When collapsing in-place, the parent is the common ancestor, we
     # have to allow merging with it.
     stats = merge.update(repo, rev, True, True, False, base, collapse,
@@ -832,7 +832,7 @@  def abort(repo, originalwd, target, stat
     if cleanup:
         # Update away from the rebase if necessary
         if inrebase(repo, originalwd, state):
-            merge.update(repo, repo[originalwd].rev(), False, True, False)
+            merge.update(repo, originalwd, False, True, False)
 
         # Strip from the first rebased revision
         rebased = filter(lambda x: x > -1 and x != target, state.values())