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.
Details
- Reviewers
- None
- Group Reviewers
hg-reviewers - Commits
- rHG56b2074114b1: rebase: refactor dryrun 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
@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 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?
- extract _dryrunrebase() with no code change
- pass in rbsrt to _origrebase() (or new function extracted from _origrebase())
- 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.
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)