Patchwork [3,of,8,v8] rebase: make collapsing use explicit logic to decide on the rev to reuse

login
register
mail settings
Submitter Kostia Balytskyi
Date July 1, 2016, 12:30 p.m.
Message ID <AM3PR06MB147827D46D11AA6555B1193D3250@AM3PR06MB147.eurprd06.prod.outlook.com>
Download mbox | patch
Permalink /patch/15705/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Kostia Balytskyi - July 1, 2016, 12:30 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1467374993 -7200
#      Fri Jul 01 14:09:53 2016 +0200
# Node ID 9b17aa69af8c86f2d1124a967b4289c1d2bbda1b
# Parent  e81fd23ec13aaad3fc0035994bc8a5904d23b72a
rebase: make collapsing use explicit logic to decide on the rev to reuse

This code:

    for rev in sortedstate:
        ...
    ...
    newnode = concludenode(repo, rev, p1, rbsrt.external,
                           commitmsg=commitmsg,
                           extrafn=extrafn, editor=editor,
                           keepbranches=rbsrt.keepbranchesf,
                           date=rbsrt.date)

uses 'rev' variable in 'concludenode' function invocation. It is not
explicitly assigned before, but its value comes as last value or 'rev' in
a for loop, e.g. last element in a 'sortedstate'. IMO this a bad style and it
also makes it hard to refactor the function, so it is better to explicitly
define the value passed to 'concludenode'.
Yuya Nishihara - July 3, 2016, 9:24 a.m.
On Fri, 1 Jul 2016 12:30:00 +0000, Kostiantyn Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1467374993 -7200
> #      Fri Jul 01 14:09:53 2016 +0200
> # Node ID 9b17aa69af8c86f2d1124a967b4289c1d2bbda1b
> # Parent  e81fd23ec13aaad3fc0035994bc8a5904d23b72a
> rebase: make collapsing use explicit logic to decide on the rev to reuse
> 
> This code:
> 
>     for rev in sortedstate:
>         ...
>     ...
>     newnode = concludenode(repo, rev, p1, rbsrt.external,
>                            commitmsg=commitmsg,
>                            extrafn=extrafn, editor=editor,
>                            keepbranches=rbsrt.keepbranchesf,
>                            date=rbsrt.date)
> 
> uses 'rev' variable in 'concludenode' function invocation. It is not
> explicitly assigned before, but its value comes as last value or 'rev' in
> a for loop, e.g. last element in a 'sortedstate'. IMO this a bad style and it
> also makes it hard to refactor the function, so it is better to explicitly
> define the value passed to 'concludenode'.
> 
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -499,10 +499,10 @@ def rebase(ui, repo, **opts):
>  
>          extrafn = _makeextrafn(rbsrt.extrafns)
>  
> -        sortedstate = sorted(rbsrt.state)
> -        total = len(sortedstate)
> +        rbsrt.sortedstate = sorted(rbsrt.state)
> +        total = len(rbsrt.sortedstate)
>          pos = 0
> -        for rev in sortedstate:
> +        for rev in rbsrt.sortedstate:

It's confusing that sortedstate is the list of sorted revisions.

> +            revtoreuse = rbsrt.sortedstate[-1]

Instead, it can be simply written as max(self.state).

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -499,10 +499,10 @@  def rebase(ui, repo, **opts):
 
         extrafn = _makeextrafn(rbsrt.extrafns)
 
-        sortedstate = sorted(rbsrt.state)
-        total = len(sortedstate)
+        rbsrt.sortedstate = sorted(rbsrt.state)
+        total = len(rbsrt.sortedstate)
         pos = 0
-        for rev in sortedstate:
+        for rev in rbsrt.sortedstate:
             ctx = repo[rev]
             desc = '%d:%s "%s"' % (ctx.rev(), ctx,
                                    ctx.description().split('\n', 1)[0])
@@ -599,7 +599,8 @@  def rebase(ui, repo, **opts):
                         commitmsg += '\n* %s' % repo[rebased].description()
                 editopt = True
             editor = cmdutil.getcommiteditor(edit=editopt, editform=editform)
-            newnode = concludenode(repo, rev, p1, rbsrt.external,
+            revtoreuse = rbsrt.sortedstate[-1]
+            newnode = concludenode(repo, revtoreuse, p1, rbsrt.external,
                                    commitmsg=commitmsg,
                                    extrafn=extrafn, editor=editor,
                                    keepbranches=rbsrt.keepbranchesf,