This is an archive of the discontinued Mercurial Phabricator instance.

rebase: refactor dryrun implementation
ClosedPublic

Authored by khanchi97 on Jun 27 2018, 3:09 AM.

Details

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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

khanchi97 created this revision.Jun 27 2018, 3:09 AM
khanchi97 added a subscriber: yuja.Jun 27 2018, 3:19 AM

@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 :)

yuja added a comment.Jun 27 2018, 9:27 AM

@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.

khanchi97 updated this revision to Diff 9324.Jun 27 2018, 10:25 AM
This revision was automatically updated to reflect the committed changes.
yuja added a comment.Jun 28 2018, 8:32 AM

Queued, thanks.

I found a couple of nits. Can you send a follow up?

if dryrun:

+ 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=True,
  • **opts)

+ _origrebase(ui, repo, inmemory=True, rbsrt=rbsrt,
+ 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:
  • _origrebase(ui, repo, abort=True)

+ with repo.wlock(), repo.lock():
+ rbsrt._prepareabortorcontinue(isabort=True)

We need a lock covering the whole dryrun process. Otherwise, another hg could
interrupt the dryrun before aborting.

Since we'll need one more indented block, I think it's time to extract dryrun
to a function.

-def _origrebase(ui, repo, inmemory=False, leaveunfinished=False, opts):
+def _origrebase(ui, repo, inmemory=False, leaveunfinished=False, rbsrt=None,
+
opts):

opts = pycompat.byteskwargs(opts)
  • rbsrt = rebaseruntime(repo, ui, inmemory, opts)

+ if not rbsrt:
+ rbsrt = rebaseruntime(repo, ui, inmemory, opts)

I think it's slightly better to split function than adding optional rbsrt
argument.

with repo.wlock(), repo.lock():
    # Validate input and define rebasing points

Perhaps we can extract the post-lock block to new function, and the dryrun
function will call it in place of _origrebase():

rbsrt = rebaseruntime(repo, ui, inmemory, opts)
 with repo.wlock(), repo.lock():
     try:
         do in-memory rebase
     finally:
         rbsrt._prepareabortorcontinue(isabort=True)