This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3827

rebase: no need to store backup in case of dryrun
ClosedPublic

Authored by khanchi97 on Jun 22 2018, 9:21 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGc892a30bafb9: rebase: no need to store backup in case of dryrun
Summary

While aborting an unfinished rebase in case of dryrun
there is no need to store backup for those rebased csets.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

khanchi97 created this revision.Jun 22 2018, 9:21 AM
khanchi97 updated this revision to Diff 9260.Jun 22 2018, 1:10 PM
khanchi97 updated this revision to Diff 9262.Jun 23 2018, 8:48 AM
yuja added a subscriber: yuja.Jun 23 2018, 11:12 PM
  • 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.

khanchi97 updated this revision to Diff 9265.Jun 24 2018, 5:30 AM
yuja added a comment.Jun 25 2018, 8:14 AM
  • 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.

khanchi97 added a comment.Jun 25 2018, 8:48 AM
In 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?

khanchi97 added a comment.Jun 25 2018, 8:53 AM

If we pass that indicator flag for dryrun then we don't have pass that
leaveunfinished flag too.

yuja added a comment.Jun 25 2018, 10:18 AM
> > - 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.

khanchi97 added a comment.Jun 25 2018, 12:06 PM
In 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 :)

khanchi97 edited the summary of this revision. (Show Details)Jun 28 2018, 4:10 PM
khanchi97 retitled this revision from rebase: no need to store backup during dry-run while aborting to rebase: no need to store backup in case of dryrun.
khanchi97 added a child revision: D3857: rebase: suppress warning thrown when aborting rebase in case of dryrun.
khanchi97 updated this revision to Diff 9345.
khanchi97 updated this revision to Diff 9357.Jun 29 2018, 2:09 PM
Closed by commit rHGc892a30bafb9: rebase: no need to store backup in case of dryrun (authored by khanchi97). · Explain WhyJun 29 2018, 9:47 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (18 lines)
Mtests/test-rebase-inmemory.t (1 line)

Diff 9260

hgext/rebase.py

self.obsoletewithoutsuccessorindestination, self.obsoletewithoutsuccessorindestination,
obsoleteextinctsuccessors) = _computeobsoletenotrebased( obsoleteextinctsuccessors) = _computeobsoletenotrebased(
self.repo, obsoleteset, destmap) self.repo, obsoleteset, destmap)
skippedset = set(self.obsoletenotrebased) skippedset = set(self.obsoletenotrebased)
skippedset.update(self.obsoletewithoutsuccessorindestination) skippedset.update(self.obsoletewithoutsuccessorindestination)
skippedset.update(obsoleteextinctsuccessors) skippedset.update(obsoleteextinctsuccessors)
_checkobsrebase(self.repo, self.ui, obsoleteset, skippedset) _checkobsrebase(self.repo, self.ui, obsoleteset, skippedset)
def _prepareabortorcontinue(self, isabort): def _prepareabortorcontinue(self, isabort, nobackup=False):
try: try:
self.restorestatus() self.restorestatus()
self.collapsemsg = restorecollapsemsg(self.repo, isabort) self.collapsemsg = restorecollapsemsg(self.repo, isabort)
except error.RepoLookupError: except error.RepoLookupError:
if isabort: if isabort:
clearstatus(self.repo) clearstatus(self.repo)
clearcollapsemsg(self.repo) clearcollapsemsg(self.repo)
self.repo.ui.warn(_('rebase aborted (no revision is removed,' self.repo.ui.warn(_('rebase aborted (no revision is removed,'
' only broken state is cleared)\n')) ' only broken state is cleared)\n'))
return 0 return 0
else: else:
msg = _('cannot continue inconsistent rebase') msg = _('cannot continue inconsistent rebase')
hint = _('use "hg rebase --abort" to clear broken state') hint = _('use "hg rebase --abort" to clear broken state')
raise error.Abort(msg, hint=hint) raise error.Abort(msg, hint=hint)
if isabort: if isabort:
return abort(self.repo, self.originalwd, self.destmap, return abort(self.repo, self.originalwd, self.destmap, self.state,
self.state, activebookmark=self.activebookmark) activebookmark=self.activebookmark, nobackup=nobackup)
def _preparenewrebase(self, destmap): def _preparenewrebase(self, destmap):
if not destmap: if not destmap:
return _nothingtorebase() return _nothingtorebase()
rebaseset = destmap.keys() rebaseset = destmap.keys()
allowunstable = obsolete.isenabled(self.repo, obsolete.allowunstableopt) allowunstable = obsolete.isenabled(self.repo, obsolete.allowunstableopt)
if (not (self.keepf or allowunstable) if (not (self.keepf or allowunstable)
with ui.configoverride(overrides, 'rebase'): with ui.configoverride(overrides, 'rebase'):
_origrebase(ui, repo, inmemory=True, leaveunfinished=True, _origrebase(ui, repo, inmemory=True, leaveunfinished=True,
**opts) **opts)
except error.InMemoryMergeConflictsError: except error.InMemoryMergeConflictsError:
ui.status(_('hit a merge conflict\n')) ui.status(_('hit a merge conflict\n'))
else: else:
ui.status(_('there will be no conflict, you can rebase\n')) ui.status(_('there will be no conflict, you can rebase\n'))
finally: finally:
_origrebase(ui, repo, abort=True) _origrebase(ui, repo, abort=True, nobackup=True)
elif inmemory: elif inmemory:
try: try:
# in-memory merge doesn't support conflicts, so if we hit any, abort # in-memory merge doesn't support conflicts, so if we hit any, abort
# and re-run as an on-disk merge. # and re-run as an on-disk merge.
overrides = {('rebase', 'singletransaction'): True} overrides = {('rebase', 'singletransaction'): True}
with ui.configoverride(overrides, 'rebase'): with ui.configoverride(overrides, 'rebase'):
return _origrebase(ui, repo, inmemory=inmemory, **opts) return _origrebase(ui, repo, inmemory=inmemory, **opts)
except error.InMemoryMergeConflictsError: except error.InMemoryMergeConflictsError:
srcf = opts.get('source', None) srcf = opts.get('source', None)
basef = opts.get('base', None) basef = opts.get('base', None)
revf = opts.get('rev', []) revf = opts.get('rev', [])
# search default destination in this space # search default destination in this space
# used in the 'hg pull --rebase' case, see issue 5214. # used in the 'hg pull --rebase' case, see issue 5214.
destspace = opts.get('_destspace') destspace = opts.get('_destspace')
contf = opts.get('continue') contf = opts.get('continue')
abortf = opts.get('abort') abortf = opts.get('abort')
nobackup = opts.get('nobackup')
if opts.get('interactive'): if opts.get('interactive'):
try: try:
if extensions.find('histedit'): if extensions.find('histedit'):
enablehistedit = '' enablehistedit = ''
except KeyError: except KeyError:
enablehistedit = " --config extensions.histedit=" enablehistedit = " --config extensions.histedit="
help = "hg%s help -e histedit" % enablehistedit help = "hg%s help -e histedit" % enablehistedit
msg = _("interactive history editing is supported by the " msg = _("interactive history editing is supported by the "
raise error.Abort( raise error.Abort(
_('abort and continue do not allow specifying revisions')) _('abort and continue do not allow specifying revisions'))
if abortf and opts.get('tool', False): if abortf and opts.get('tool', False):
ui.warn(_('tool option will be ignored\n')) ui.warn(_('tool option will be ignored\n'))
if contf: if contf:
ms = mergemod.mergestate.read(repo) ms = mergemod.mergestate.read(repo)
mergeutil.checkunresolved(ms) mergeutil.checkunresolved(ms)
retcode = rbsrt._prepareabortorcontinue(abortf) retcode = rbsrt._prepareabortorcontinue(abortf, nobackup=nobackup)
if retcode is not None: if retcode is not None:
return retcode return retcode
else: else:
destmap = _definedestmap(ui, repo, inmemory, destf, srcf, basef, destmap = _definedestmap(ui, repo, inmemory, destf, srcf, basef,
revf, destspace=destspace) revf, destspace=destspace)
retcode = rbsrt._preparenewrebase(destmap) retcode = rbsrt._preparenewrebase(destmap)
if retcode is not None: if retcode is not None:
return retcode return retcode
# We should be standing on the first as-of-yet unrebased commit. # We should be standing on the first as-of-yet unrebased commit.
firstunrebased = min([old for old, new in state.iteritems() firstunrebased = min([old for old, new in state.iteritems()
if new == nullrev]) if new == nullrev])
if firstunrebased in parents: if firstunrebased in parents:
return True return True
return False 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: '''Restore the repository to its original state. Additional args:
activebookmark: the name of the bookmark that should be active after the activebookmark: the name of the bookmark that should be active after the
restore''' restore'''
try: try:
# If the first commits in the rebased set get skipped during the rebase, # If the first commits in the rebased set get skipped during the rebase,
# their values within the state mapping will be the dest rev id. The # their values within the state mapping will be the dest rev id. The
shouldupdate = repo['.'].rev() in updateifonnodes shouldupdate = repo['.'].rev() in updateifonnodes
# Update away from the rebase if necessary # Update away from the rebase if necessary
if shouldupdate or needupdate(repo, state): if shouldupdate or needupdate(repo, state):
mergemod.update(repo, originalwd, False, True) mergemod.update(repo, originalwd, False, True)
# Strip from the first rebased revision # Strip from the first rebased revision
if rebased: if rebased:
if nobackup:
repair.strip(repo.ui, repo, strippoints, backup=False)
else:
repair.strip(repo.ui, repo, strippoints) repair.strip(repo.ui, repo, strippoints)
if activebookmark and activebookmark in repo._bookmarks: if activebookmark and activebookmark in repo._bookmarks:
bookmarks.activate(repo, activebookmark) bookmarks.activate(repo, activebookmark)
finally: finally:
clearstatus(repo) clearstatus(repo)
clearcollapsemsg(repo) clearcollapsemsg(repo)
repo.ui.warn(_('rebase aborted\n')) repo.ui.warn(_('rebase aborted\n'))

tests/test-rebase-inmemory.t

[255] [255]
Check dryrun gives correct results when there is no conflict in rebasing Check dryrun gives correct results when there is no conflict in rebasing
$ hg rebase -s 2 -d 6 -n $ hg rebase -s 2 -d 6 -n
rebasing 2:177f92b77385 "c" rebasing 2:177f92b77385 "c"
rebasing 3:055a42cdd887 "d" rebasing 3:055a42cdd887 "d"
rebasing 4:e860deea161a "e" rebasing 4:e860deea161a "e"
there will be no conflict, you can rebase 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 rebase aborted
$ hg diff $ hg diff
$ hg status $ hg status
$ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n"
@ 6:baf10c5166d4 test @ 6:baf10c5166d4 test
| g | g