Patchwork [6,of,7,v6] rebase: move local variables 'contf', 'abortf' and 'destspace' to the RR

login
register
mail settings
Submitter Kostia Balytskyi
Date June 13, 2016, 10:03 p.m.
Message ID <0387ce5764968036c615.1465855431@ikostia-mbp>
Download mbox | patch
Permalink /patch/15492/
State Changes Requested
Headers show

Comments

Kostia Balytskyi - June 13, 2016, 10:03 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1465854334 -3600
#      Mon Jun 13 22:45:34 2016 +0100
# Node ID 0387ce5764968036c615b38f575e45fc9c0ae71d
# Parent  ad8e1710d3337924368905bdb1601c0bbc150e15
rebase: move local variables 'contf', 'abortf' and 'destspace' to the RR

The variables this commit touches are command line options of the rebase
operation. Two of them ('contf' and 'abortf') are responsible for 'abort'
and 'continue' phases of rebase lifecycle, while 'destspace' is used to
search for default rebase destination. Commit moves these variables to
the previously introduced rebaseruntime class.
Yuya Nishihara - June 17, 2016, 2:02 p.m.
On Thu, 16 Jun 2016 15:45:25 +0000, Kostia Balytskyi wrote:
> My goal is to move most of the current logic of rebase to the RR class. This will achieve a couple of goals:
> 
> - reduce the amount of existing (v1, v2, v3, v4 …, v100) = function(u1, u2, … u100) since every variable is accessible via a self container
> 
> - allow further breakdown of logic into functionally independent components without introducing more (…) = func(…) things
> 
> - (in future) possible abstraction of some components of rebase from the others (I don’t have particular examples here)

They all sound good to me.

> In order to move to this state of rebase code, I am trying to move to the class as much things as possible. If we want to separate command-related logic from actual-rebase logic, we can always do it later. So I think that even ‘abort’ and ‘continue’ should be moved to the RR class.

Hmm, I don't see much point to mix the command handling with the rebase logic,
which seem to be roughly separated now.

What I have in mind is something like this. Perhaps the actual code wouldn't
be that simple, but the current code looks similar to this pattern.

    if contf or abortf:
        rbsrt = load_from_state_file()
        if abortf:
            rbsrt.abort()
    else:
        rbsrt = build_from_command_options()
    rbsrt.run()
Kostia Balytskyi - June 17, 2016, 2:23 p.m.
On 6/17/16, 3:02 PM, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

>On Thu, 16 Jun 2016 15:45:25 +0000, Kostia Balytskyi wrote:

>> My goal is to move most of the current logic of rebase to the RR class. This will achieve a couple of goals:

>> 

>> - reduce the amount of existing (v1, v2, v3, v4 …, v100) = function(u1, u2, … u100) since every variable is accessible via a self container

>> 

>> - allow further breakdown of logic into functionally independent components without introducing more (…) = func(…) things

>> 

>> - (in future) possible abstraction of some components of rebase from the others (I don’t have particular examples here)

>

>They all sound good to me.

>

>> In order to move to this state of rebase code, I am trying to move to the class as much things as possible. If we want to separate command-related logic from actual-rebase logic, we can always do it later. So I think that even ‘abort’ and ‘continue’ should be moved to the RR class.

>

>Hmm, I don't see much point to mix the command handling with the rebase logic,

>which seem to be roughly separated now.

>

>What I have in mind is something like this. Perhaps the actual code wouldn't

>be that simple, but the current code looks similar to this pattern.

>

>    if contf or abortf:

>        rbsrt = load_from_state_file()

>        if abortf:

>            rbsrt.abort()

>    else:

>        rbsrt = build_from_command_options()

>    rbsrt.run()


Ok, so our understandings only differ in whether contf and abortf should be enclosed in the same container as everything else. I argue that they should because it’s more convenient to move all the opt validation into one place. You seem to also thing that rebase-logic and rebase-command need to be separated. This is possible, but let’s first lead it to the state where refactoring is not such a pain (e.g. class IMO).

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -139,6 +139,12 @@  class rebaseruntime(object):
         self.basef = opts.get('base', None)
         self.revf = opts.get('rev', [])
 
+        # search default destination in this space
+        # used in the 'hg pull --rebase' case, see issue 5214.
+        self.destspace = opts.get('_destspace')
+        self.contf = opts.get('continue')
+        self.abortf = opts.get('abort')
+
 @command('rebase',
     [('s', 'source', '',
      _('rebase the specified changeset and descendants'), _('REV')),
@@ -256,11 +262,6 @@  def rebase(ui, repo, **opts):
         wlock = repo.wlock()
         lock = repo.lock()
 
-        # search default destination in this space
-        # used in the 'hg pull --rebase' case, see issue 5214.
-        destspace = opts.get('_destspace')
-        contf = opts.get('continue')
-        abortf = opts.get('abort')
         collapsef = opts.get('collapse', False)
         collapsemsg = cmdutil.logmessage(ui, opts)
         date = opts.get('date', None)
@@ -289,8 +290,8 @@  def rebase(ui, repo, **opts):
             raise error.Abort(
                 _('message can only be specified with collapse'))
 
-        if contf or abortf:
-            if contf and abortf:
+        if rbsrt.contf or rbsrt.abortf:
+            if rbsrt.contf and rbsrt.abortf:
                 raise error.Abort(_('cannot use both abort and continue'))
             if collapsef:
                 raise error.Abort(
@@ -298,7 +299,7 @@  def rebase(ui, repo, **opts):
             if rbsrt.srcf or rbsrt.basef or rbsrt.destf:
                 raise error.Abort(
                     _('abort and continue do not allow specifying revisions'))
-            if abortf and opts.get('tool', False):
+            if rbsrt.abortf and opts.get('tool', False):
                 ui.warn(_('tool option will be ignored\n'))
 
             try:
@@ -307,7 +308,7 @@  def rebase(ui, repo, **opts):
                  rbsrt.external, rbsrt.activebookmark) = restorestatus(repo)
                 collapsemsg = restorecollapsemsg(repo)
             except error.RepoLookupError:
-                if abortf:
+                if rbsrt.abortf:
                     clearstatus(repo)
                     clearcollapsemsg(repo)
                     repo.ui.warn(_('rebase aborted (no revision is removed,'
@@ -317,7 +318,7 @@  def rebase(ui, repo, **opts):
                     msg = _('cannot continue inconsistent rebase')
                     hint = _('use "hg rebase --abort" to clear broken state')
                     raise error.Abort(msg, hint=hint)
-            if abortf:
+            if rbsrt.abortf:
                 return abort(repo, rbsrt.originalwd, rbsrt.target, 
                              rbsrt.state,
                              activebookmark=rbsrt.activebookmark)
@@ -336,7 +337,7 @@  def rebase(ui, repo, **opts):
         else:
             dest, rebaseset = _definesets(ui, repo, rbsrt.destf, rbsrt.srcf,
                                           rbsrt.basef, rbsrt.revf,
-                                          destspace=destspace)
+                                          destspace=rbsrt.destspace)
             if dest is None:
                 return _nothingtorebase()