D3827: rebase: no need to store backup during dry-run while aborting
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3827: rebase: no need to store backup during dry-run while aborting
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D3827: rebase: no need to store backup during dry-run while aborting
> > > - 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D3827: rebase: no need to store backup during dry-run while aborting
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) < phabrica...@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 > ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3827: rebase: no need to store backup during dry-run while aborting
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
D3827: rebase: no need to store backup during dry-run while aborting
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. 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
Re: D3827: rebase: no need to store backup during dry-run while aborting
> -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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3827: rebase: no need to store backup during dry-run while aborting
khanchi97 updated this revision to Diff 9265.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D3827?vs=9262=9265
REVISION DETAIL
https://phab.mercurial-scm.org/D3827
AFFECTED FILES
hgext/rebase.py
tests/test-rebase-inmemory.t
CHANGE DETAILS
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, backup=True):
try:
self.restorestatus()
self.collapsemsg = restorecollapsemsg(self.repo, isabort)
@@ -341,8 +341,8 @@
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)
+return abort(self.repo, self.originalwd, self.destmap, self.state,
+ activebookmark=self.activebookmark, backup=backup)
def _preparenewrebase(self, destmap):
if not destmap:
@@ -828,7 +828,7 @@
else:
ui.status(_('there will be no conflict, you can rebase\n'))
finally:
-_origrebase(ui, repo, abort=True)
+_origrebase(ui, repo, inmemory=True, abort=True)
elif inmemory:
try:
# in-memory merge doesn't support conflicts, so if we hit any,
abort
@@ -889,7 +889,9 @@
ms = mergemod.mergestate.read(repo)
mergeutil.checkunresolved(ms)
-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)
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, backup=True):
'''Restore the repository to its original state. Additional args:
activebookmark: the name of the bookmark that should be active after the
@@ -1588,7 +1590,7 @@
# Strip from the first rebased revision
if rebased:
-repair.strip(repo.ui, repo, strippoints)
+repair.strip(repo.ui, repo, strippoints, backup=backup)
if activebookmark and activebookmark in repo._bookmarks:
bookmarks.activate(repo, activebookmark)
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
D3827: rebase: no need to store backup during dry-run while aborting
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: D3827: rebase: no need to store backup during dry-run while aborting
> -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`.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3827: rebase: no need to store backup during dry-run while aborting
khanchi97 updated this revision to Diff 9262.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D3827?vs=9260=9262
REVISION DETAIL
https://phab.mercurial-scm.org/D3827
AFFECTED FILES
hgext/rebase.py
tests/test-rebase-inmemory.t
CHANGE DETAILS
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)
To: khanchi97, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3827: rebase: no need to store backup during dry-run while aborting
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
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)
To: khanchi97, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
