Patchwork D3849: rebase: refactor dryrun implementation

login
register
mail settings
Submitter phabricator
Date June 27, 2018, 2:26 p.m.
Message ID <ef3851a6dbaaa849cd5848b513461101@localhost.localdomain>
Download mbox | patch
Permalink /patch/32462/
State Not Applicable
Headers show

Comments

phabricator - June 27, 2018, 2:26 p.m.
khanchi97 updated this revision to Diff 9324.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D3849?vs=9321&id=9324

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

AFFECTED FILES
  hgext/rebase.py

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - June 28, 2018, 12:31 p.m.
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)
```
phabricator - June 28, 2018, 12:32 p.m.
yuja added a comment.


  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)

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -818,19 +818,23 @@ 
         opts[r'dest'] = '_destautoorphanrebase(SRC)'
 
     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)
     elif inmemory:
         try:
             # in-memory merge doesn't support conflicts, so if we hit any, abort
@@ -846,9 +850,11 @@ 
     else:
         return _origrebase(ui, repo, **opts)
 
-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)
 
     with repo.wlock(), repo.lock():
         # Validate input and define rebasing points