Patchwork D3849: rebase: refactor dryrun implementation

login
register
mail settings
Submitter phabricator
Date June 27, 2018, 7:09 a.m.
Message ID <differential-rev-PHID-DREV-eoabsj3otvdtcfbmr224-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32457/
State Superseded
Headers show

Comments

phabricator - June 27, 2018, 7:09 a.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch refactor dry-run code to make it easy to add additional
  functionality in dryrun. Otherwise we had to add every functionality
  through _origrebase() which does not seem a good implementation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3849

AFFECTED FILES
  hgext/rebase.py

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - June 27, 2018, 7:19 a.m.
khanchi97 added a subscriber: yuja.
khanchi97 added a comment.


  @yuja In this patch rebaseruntime is instantiated two times, one in _dryrunrebase and second in _origrebase, I don't know if it could make any problem although all tests are passing. Maybe _origrebase() also need some refactoring.
  See if this refactoring is good or if this can be improved, then please give some suggestions.
  Thanks :)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3849

To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - June 27, 2018, 1:27 p.m.
> @yuja In this patch rebaseruntime is instantiated two times, one in
> _dryrunrebase and second in _origrebase, I don't know if it could make any
> problem although all tests are passing. Maybe _origrebase() also need some
> refactoring.

Perhaps. I don't know if that really matters, but it's better to not keep
two separate rbsrt objects alive.

Can you reorder this as follows?

 1. extract _dryrunrebase() with no code change
 2. pass in rbsrt to _origrebase() (or new function extracted from
    _origrebase())
 3. replace _origrebase(abort=True) with rbsrt call

> +    def _abortunfinishedrebase(self, backup=False, suppwarns=True):
> +        repo = self.repo
> +        with repo.wlock(), repo.lock():
> +            retcode = self._prepareabortorcontinue(isabort=True)
> +            return retcode

If this is just calling _prepareabortorcontinue(), we wouldn't need a new
function. And I think the lock should be held by the caller.
phabricator - June 27, 2018, 1:27 p.m.
yuja added a comment.


  > @yuja In this patch rebaseruntime is instantiated two times, one in
  >  _dryrunrebase and second in _origrebase, I don't know if it could make any
  >  problem although all tests are passing. Maybe _origrebase() also need some
  >  refactoring.
  
  Perhaps. I don't know if that really matters, but it's better to not keep
  two separate rbsrt objects alive.
  
  Can you reorder this as follows?
  
  1. extract _dryrunrebase() with no code change
  2. pass in rbsrt to _origrebase() (or new function extracted from _origrebase())
  3. replace _origrebase(abort=True) with rbsrt call
  
  > +    def _abortunfinishedrebase(self, backup=False, suppwarns=True):
  >  +        repo = self.repo
  >  +        with repo.wlock(), repo.lock():
  >  +            retcode = self._prepareabortorcontinue(isabort=True)
  >  +            return retcode
  
  If this is just calling _prepareabortorcontinue(), we wouldn't need a new
  function. And I think the lock should be held by the caller.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3849

To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -644,6 +644,12 @@ 
             repo['.'].node() == repo._bookmarks[self.activebookmark]):
                 bookmarks.activate(repo, self.activebookmark)
 
+    def _abortunfinishedrebase(self, backup=False, suppwarns=True):
+        repo = self.repo
+        with repo.wlock(), repo.lock():
+            retcode = self._prepareabortorcontinue(isabort=True)
+            return retcode
+
 @command('rebase',
     [('s', 'source', '',
      _('rebase the specified changeset and descendants'), _('REV')),
@@ -818,19 +824,7 @@ 
         opts[r'dest'] = '_destautoorphanrebase(SRC)'
 
     if dryrun:
-        try:
-            overrides = {('rebase', 'singletransaction'): True}
-            with ui.configoverride(overrides, 'rebase'):
-                _origrebase(ui, repo, inmemory=True, leaveunfinished=True,
-                            **opts)
-        except error.InMemoryMergeConflictsError:
-            ui.status(_('hit a merge conflict\n'))
-            return 1
-        else:
-            ui.status(_('there will be no conflict, you can rebase\n'))
-            return 0
-        finally:
-            _origrebase(ui, repo, abort=True)
+        return _dryrunrebase(ui, repo, **opts)
     elif inmemory:
         try:
             # in-memory merge doesn't support conflicts, so if we hit any, abort
@@ -846,6 +840,24 @@ 
     else:
         return _origrebase(ui, repo, **opts)
 
+def _dryrunrebase(ui, repo, **opts):
+    leaveunfinished = True
+    inmemory = True
+    rbsrt = rebaseruntime(repo, ui, inmemory, opts)
+    try:
+        overrides = {('rebase', 'singletransaction'): True}
+        with ui.configoverride(overrides, 'rebase'):
+            _origrebase(ui, repo, inmemory=True,
+                        leaveunfinished=leaveunfinished, **opts)
+    except error.InMemoryMergeConflictsError:
+        ui.status(_('hit a merge conflict\n'))
+        return 1
+    else:
+        ui.status(_('there will be no conflict, you can rebase\n'))
+        return 0
+    finally:
+        rbsrt._abortunfinishedrebase()
+
 def _origrebase(ui, repo, inmemory=False, leaveunfinished=False, **opts):
     opts = pycompat.byteskwargs(opts)
     rbsrt = rebaseruntime(repo, ui, inmemory, opts)