Patchwork D3827: rebase: no need to store backup during dry-run while aborting

login
register
mail settings
Submitter phabricator
Date June 23, 2018, 12:48 p.m.
Message ID <b884462784fb8946e1a241ef06c62246@localhost.localdomain>
Download mbox | patch
Permalink /patch/32392/
State Not Applicable
Headers show

Comments

phabricator - June 23, 2018, 12:48 p.m.
khanchi97 updated this revision to Diff 9262.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D3827?vs=9260&id=9262

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-inmemory.t

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - June 24, 2018, 3:04 a.m.
> -    def _prepareabortorcontinue(self, isabort):
> +    def _prepareabortorcontinue(self, isabort, opts={}):

Mutable default value should be avoided. It can be `backup=True` since we
aren't processing command-level thingy here.

And please make sure tests pass before sending out patches.

> @@ -828,7 +829,8 @@
>          else:
>              ui.status(_('there will be no conflict, you can rebase\n'))
>          finally:
> -            _origrebase(ui, repo, abort=True)
> +            opts = {'abort':True, 'no_backup':True}
> +            _origrebase(ui, repo, **opts)

Maybe we can refactor the dryrun handling in a way we don't have to pass around
the no_backup flag. IIUC, `_origrebase()` is a high-level function which has
many parameter/state checks, but what we want is to cancel the in-memory
session we've made just before. No user error should occur.

> @@ -1588,7 +1590,10 @@
>  
>              # Strip from the first rebased revision
>              if rebased:
> -                repair.strip(repo.ui, repo, strippoints)
> +                if nobackup:
> +                    repair.strip(repo.ui, repo, strippoints, backup=False)
> +                else:
> +                    repair.strip(repo.ui, repo, strippoints)

This can be written as `backup=not nobackup` or `backup=backup`.
phabricator - June 24, 2018, 3:12 a.m.
yuja added a comment.


  > - def _prepareabortorcontinue(self, isabort): +    def _prepareabortorcontinue(self, isabort, opts={}):
  
  Mutable default value should be avoided. It can be `backup=True` since we
  aren't processing command-level thingy here.
  
  And please make sure tests pass before sending out patches.
  
  > @@ -828,7 +829,8 @@
  > 
  >   else:
  >       ui.status(_('there will be no conflict, you can rebase\n'))
  >   finally:
  > 
  > - _origrebase(ui, repo, abort=True) +            opts = {'abort':True, 'no_backup':True} +            _origrebase(ui, repo, **opts)
  
  Maybe we can refactor the dryrun handling in a way we don't have to pass around
  the no_backup flag. IIUC, `_origrebase()` is a high-level function which has
  many parameter/state checks, but what we want is to cancel the in-memory
  session we've made just before. No user error should occur.
  
  > @@ -1588,7 +1590,10 @@
  > 
  >   1. Strip from the first rebased revision if rebased:
  > - repair.strip(repo.ui, repo, strippoints) +                if nobackup: +                    repair.strip(repo.ui, repo, strippoints, backup=False) +                else: +                    repair.strip(repo.ui, repo, strippoints)
  
  This can be written as `backup=not nobackup` or `backup=backup`.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-rebase-inmemory.t b/tests/test-rebase-inmemory.t
--- a/tests/test-rebase-inmemory.t
+++ b/tests/test-rebase-inmemory.t
@@ -212,7 +212,6 @@ 
   rebasing 3:055a42cdd887 "d"
   rebasing 4:e860deea161a "e"
   there will be no conflict, you can rebase
-  saved backup bundle to $TESTTMP/repo1/repo2/skrepo/.hg/strip-backup/c83b1da5b1ae-f1e0beb9-backup.hg
   rebase aborted
 
   $ hg diff
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -325,7 +325,7 @@ 
         skippedset.update(obsoleteextinctsuccessors)
         _checkobsrebase(self.repo, self.ui, obsoleteset, skippedset)
 
-    def _prepareabortorcontinue(self, isabort):
+    def _prepareabortorcontinue(self, isabort, opts={}):
         try:
             self.restorestatus()
             self.collapsemsg = restorecollapsemsg(self.repo, isabort)
@@ -341,8 +341,9 @@ 
                 hint = _('use "hg rebase --abort" to clear broken state')
                 raise error.Abort(msg, hint=hint)
         if isabort:
-            return abort(self.repo, self.originalwd, self.destmap,
-                         self.state, activebookmark=self.activebookmark)
+            nobackup = opts.get(r'no_backup')
+            return abort(self.repo, self.originalwd, self.destmap, self.state,
+                         activebookmark=self.activebookmark, nobackup=nobackup)
 
     def _preparenewrebase(self, destmap):
         if not destmap:
@@ -828,7 +829,8 @@ 
         else:
             ui.status(_('there will be no conflict, you can rebase\n'))
         finally:
-            _origrebase(ui, repo, abort=True)
+            opts = {'abort':True, 'no_backup':True}
+            _origrebase(ui, repo, **opts)
     elif inmemory:
         try:
             # in-memory merge doesn't support conflicts, so if we hit any, abort
@@ -889,7 +891,7 @@ 
                 ms = mergemod.mergestate.read(repo)
                 mergeutil.checkunresolved(ms)
 
-            retcode = rbsrt._prepareabortorcontinue(abortf)
+            retcode = rbsrt._prepareabortorcontinue(abortf, opts=opts)
             if retcode is not None:
                 return retcode
         else:
@@ -1543,7 +1545,7 @@ 
 
     return False
 
-def abort(repo, originalwd, destmap, state, activebookmark=None):
+def abort(repo, originalwd, destmap, state, activebookmark=None, nobackup=False):
     '''Restore the repository to its original state.  Additional args:
 
     activebookmark: the name of the bookmark that should be active after the
@@ -1588,7 +1590,10 @@ 
 
             # Strip from the first rebased revision
             if rebased:
-                repair.strip(repo.ui, repo, strippoints)
+                if nobackup:
+                    repair.strip(repo.ui, repo, strippoints, backup=False)
+                else:
+                    repair.strip(repo.ui, repo, strippoints)
 
         if activebookmark and activebookmark in repo._bookmarks:
             bookmarks.activate(repo, activebookmark)