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

[Mads Kiilerich]

On 12/05/2014 05:39 AM, Matt Harbison wrote:
>
> On Mon, 01 Dec 2014 23:13:48 -0500, Mads Kiilerich 
> <mads at kiilerich.com> wrote:
>
>> # HG changeset patch
>> # User Mads Kiilerich <madski at 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