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

login
register
mail settings
Submitter phabricator
Date June 22, 2018, 1:21 p.m.
Message ID <differential-rev-PHID-DREV-djjfpqflkr3lztdml7sc-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32380/
State Superseded
Headers show

Comments

phabricator - June 22, 2018, 1:21 p.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When we are in dry-run mode in rebasing, there is no need to store backup
  when we abort rebase to make wrdir as it was before.

REPOSITORY
  rHG Mercurial

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
phabricator - June 25, 2018, 12:48 p.m.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D3827#59873, @yuja wrote:
  
  > > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If in-memory, means aborting during dry-run, no need to backup +            backup = not rbsrt.inmemory +            retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)
  >
  > This seems confusing and is probably wrong since we wouldn't overwrite
  >  `inmemory` to `False` if in-memory rebase were resumable.
  >
  > I think explicit `backup` flag is less bad.
  
  
  @yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for `backup` and `suppwarns` (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel
sushil khanchi - June 25, 2018, 12:53 p.m.
If we pass that indicator flag for dryrun then we don't have pass that
`leaveunfinished` flag too.

On Mon, Jun 25, 2018 at 6:18 PM, khanchi97 (Sushil khanchi) <
phabricator@mercurial-scm.org> wrote:

> khanchi97 added a comment.
>
>
>   In https://phab.mercurial-scm.org/D3827#59873, @yuja wrote:
>
>   > > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If
> in-memory, means aborting during dry-run, no need to backup +
> backup = not rbsrt.inmemory +            retcode =
> rbsrt._prepareabortorcontinue(abortf, backup=backup)
>   >
>   > This seems confusing and is probably wrong since we wouldn't overwrite
>   >  `inmemory` to `False` if in-memory rebase were resumable.
>   >
>   > I think explicit `backup` flag is less bad.
>
>
>   @yuja Can I pass a indicator type flag to _origrebase() just to indicate
> that we are in dryrun and then I can use that flag to set values for
> `backup` and `suppwarns` (suppress warning when aborting rebase). I think
> it would be better than explicitly passing flags for each case. What do you
> say?
>
> REPOSITORY
>   rHG Mercurial
>
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D3827
>
> To: khanchi97, #hg-reviewers
> Cc: yuja, mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - June 25, 2018, 2:18 p.m.
>   > > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If in-memory, means aborting during dry-run, no need to backup +            backup = not rbsrt.inmemory +            retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)
>   >
>   > This seems confusing and is probably wrong since we wouldn't overwrite
>   >  `inmemory` to `False` if in-memory rebase were resumable.
>   >
>   > I think explicit `backup` flag is less bad.
>   
>   
>   @yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for `backup` and `suppwarns` (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?

Sounds good.

A cleaner (but not simple) approach would be to stop calling `_origrebase()`
twice, and instead refactor `_origrebase()` and/or `rebaseruntime` to support
dry-run operation.
phabricator - June 25, 2018, 2:18 p.m.
yuja added a comment.


  >   > > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If in-memory, means aborting during dry-run, no need to backup +            backup = not rbsrt.inmemory +            retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)
  >   >
  >   > This seems confusing and is probably wrong since we wouldn't overwrite
  >   >  `inmemory` to `False` if in-memory rebase were resumable.
  >   >
  >   > I think explicit `backup` flag is less bad.
  >   
  >   
  >   @yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for `backup` and `suppwarns` (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?
  
  Sounds good.
  
  A cleaner (but not simple) approach would be to stop calling `_origrebase()`
  twice, and instead refactor `_origrebase()` and/or `rebaseruntime` to support
  dry-run operation.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 25, 2018, 4:06 p.m.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D3827#59878, @yuja wrote:
  
  > >   > > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If in-memory, means aborting during dry-run, no need to backup +            backup = not rbsrt.inmemory +            retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)
  > >   >
  > >   > This seems confusing and is probably wrong since we wouldn't overwrite
  > >   >  `inmemory` to `False` if in-memory rebase were resumable.
  > >   >
  > >   > I think explicit `backup` flag is less bad.
  > >   
  > >   
  > >   @yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for `backup` and `suppwarns` (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?
  >
  > Sounds good.
  >
  > A cleaner (but not simple) approach would be to stop calling `_origrebase()`
  >  twice, and instead refactor `_origrebase()` and/or `rebaseruntime` to support
  >  dry-run operation.
  
  
  Sounds interesting. Let me try this.
  Thanks for review :)

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('nobackup')
+            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,7 @@ 
         else:
             ui.status(_('there will be no conflict, you can rebase\n'))
         finally:
-            _origrebase(ui, repo, abort=True)
+            _origrebase(ui, repo, abort=True, nobackup=True)
     elif inmemory:
         try:
             # in-memory merge doesn't support conflicts, so if we hit any, abort
@@ -859,6 +860,7 @@ 
         destspace = opts.get('_destspace')
         contf = opts.get('continue')
         abortf = opts.get('abort')
+        nobackup = opts.get('nobackup')
         if opts.get('interactive'):
             try:
                 if extensions.find('histedit'):
@@ -889,7 +891,7 @@ 
                 ms = mergemod.mergestate.read(repo)
                 mergeutil.checkunresolved(ms)
 
-            retcode = rbsrt._prepareabortorcontinue(abortf)
+            retcode = rbsrt._prepareabortorcontinue(abortf, nobackup=nobackup)
             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)