Patchwork D3757: rebase: add dry-run functionality

login
register
mail settings
Submitter phabricator
Date June 16, 2018, 1:47 p.m.
Message ID <differential-rev-PHID-DREV-wjdnuiqd5q4thukn7w4m-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32197/
State Superseded
Headers show

Comments

phabricator - June 16, 2018, 1:47 p.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  For now, it just notify that if we will hit a conflict or not, but
  we can improve this like making it a --confirm flag or by showing the
  graph that would result when we will run the command without --dry-run

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-inmemory.t

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - June 16, 2018, 8:57 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  Overall I think this is a great feature! The patch as is needs a bit of UI work.
  
  Others may have different opinions from mine. So you may want to wait for another core developer to weigh in on things.

INLINE COMMENTS

> rebase.py:831-833
> +            ui.status(_('Hit a merge conflict\n'))
> +        else:
> +            ui.status(_('There will be no conflict, you can rebase\n'))

Nit: please use lower case letters to being the message so we're consistent with the rest of Mercurial.

> test-rebase-inmemory.t:210-215
> +  $ hg rebase -s 2 -d 6 -n
> +  rebasing 2:177f92b77385 "c"
> +  rebasing 3:055a42cdd887 "d"
> +  rebasing 4:e860deea161a "e"
> +  There will be no conflict, you can rebase
> +  rebase aborted

This is potentially follow-up territory, but I think there is room to improve the output here.

I think it would be better to print a message before beginning the dry-run rebase so the output is clear that no permanent actions are being taken. e.g. `(starting dry-run rebase; repository will not be changed)`.

Similarly, let's tweak the success message a bit. How about `dry-run rebase completed successfully; run without -n/--dry-run to perform this rebase`. And I don't think we need to print the `rebase aborted` message in this case.

> test-rebase-inmemory.t:280-288
> +  $ hg rebase -s 2 -d 7 -n
> +  rebasing 2:177f92b77385 "c"
> +  rebasing 3:055a42cdd887 "d"
> +  rebasing 4:e860deea161a "e"
> +  merging e
> +  transaction abort!
> +  rollback completed

Again, more nit picks around the messaging.

Again, I think this should print a message that we are starting a dry-run rebase.

The `transaction abort!` and `rollback completed` messages are a bit concerning to me as a user because a dry-run isn't supposed to alert the repository. But the presence of these messages implies it is being altered. Come to think of it, the repository may actually be altered during dry-run rebases. Is it? But if it is, why don't we see these messages for the successful dry-run case?

I also think it would be helpful if the abort message clearly stated the changeset that triggered the failure and the file(s) the merge conflict was in. Yes, you can infer it from the output. But summaries are nice. Especially since we have been known to change the `rebasing` messages and tweaks could render the output difficult to parse for the failure.

As a follow-up feature, it would be pretty cool if adding `--verbose` would print the diff of the file section that has the merge conflict. That may require a refactoring of the merge code though. This is clearly a follow-up feature and shouldn't be included in this patch.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
Yuya Nishihara - June 17, 2018, 4:34 a.m.
Just nitpicking. The feature generally looks good to me.

> @@ -798,6 +797,15 @@
>  
>      """
>      inmemory = ui.configbool('rebase', 'experimental.inmemory')
> +    dryrun = opts.get(r'dry_run')
> +    if dryrun:
> +        if (opts.get(r'abort')):
> +                raise error.Abort(_('cannot specify both --dry-run '
> +                                    'and --abort'))
> +        if (opts.get(r'continue')):
> +                raise error.Abort(_('cannot specify both --dry-run '
> +                                    'and --continue'))
           ^^^^^^^^
excessive indent and unnecessary parens.

> -    if inmemory:
> +    if dryrun:
>          try:
> -            # in-memory merge doesn't support conflicts, so if we hit any, abort
> -            # and re-run as an on-disk merge.
>              overrides = {('rebase', 'singletransaction'): True}
>              with ui.configoverride(overrides, 'rebase'):
> -                return _origrebase(ui, repo, inmemory=inmemory, **opts)
> +                _origrebase(ui, repo, inmemory=True, dryrun=dryrun, **opts)
                                                        ^^^^^^^^^^^^^

Perhaps this flag shouldn't be called a `dryrun` because it would leave
a rebase session unless we do abort.

>          except error.InMemoryMergeConflictsError:
> -            ui.warn(_('hit merge conflicts; re-running rebase without in-memory'
> -                      ' merge\n'))
> +            ui.status(_('Hit a merge conflict\n'))
> +        else:
> +            ui.status(_('There will be no conflict, you can rebase\n'))
> +        finally:
>              _origrebase(ui, repo, **{r'abort': True})
                                     ^^^^^^^^^^^^^^^^^^^

It can be written as just `abort=True`.

> -            return _origrebase(ui, repo, inmemory=False, **opts)
> +
>      else:
> -        return _origrebase(ui, repo, **opts)
> +        if inmemory:

I slightly prefer `elif inmemory` than nesting one more depth.
phabricator - June 17, 2018, 4:35 a.m.
yuja added a comment.


  Just nitpicking. The feature generally looks good to me.
  
  > @@ -798,6 +797,15 @@
  > 
  >   """
  >   inmemory = ui.configbool('rebase', 'experimental.inmemory')
  > 
  > +    dryrun = opts.get(r'dry_run')
  >  +    if dryrun:
  >  +        if (opts.get(r'abort')):
  >  +                raise error.Abort(_('cannot specify both --dry-run '
  >  +                                    'and --abort'))
  >  +        if (opts.get(r'continue')):
  >  +                raise error.Abort(_('cannot specify both --dry-run '
  >  +                                    'and --continue'))
  
    ^^^^^^^^
  
  excessive indent and unnecessary parens.
  
  > - if inmemory: +    if dryrun: try:
  > - # in-memory merge doesn't support conflicts, so if we hit any, abort
  > - # and re-run as an on-disk merge. overrides = {('rebase', 'singletransaction'): True} with ui.configoverride(overrides, 'rebase'):
  > - return _origrebase(ui, repo, inmemory=inmemory, **opts) +                _origrebase(ui, repo, inmemory=True, dryrun=dryrun, **opts)
  
    ^^^^^^^^^^^^^
  
  Perhaps this flag shouldn't be called a `dryrun` because it would leave
  a rebase session unless we do abort.
  
  >   except error.InMemoryMergeConflictsError:
  > 
  > - ui.warn(_('hit merge conflicts; re-running rebase without in-memory'
  > - ' merge\n')) +            ui.status(_('Hit a merge conflict\n')) +        else: +            ui.status(_('There will be no conflict, you can rebase\n')) +        finally: _origrebase(ui, repo, **{r'abort': True})
  
    ^^^^^^^^^^^^^^^^^^^
  
  It can be written as just `abort=True`.
  
  > - return _origrebase(ui, repo, inmemory=False, **opts) + else:
  > - return _origrebase(ui, repo, **opts) +        if inmemory:
  
  I slightly prefer `elif inmemory` than nesting one more depth.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
phabricator - June 17, 2018, 7:07 a.m.
khanchi97 added a comment.


  @yuja I have the made the requested changes. See if I have made the correct
  changes.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
phabricator - June 17, 2018, 7:17 a.m.
khanchi97 added a comment.


  @indygreg  @yuja thanks for your reviews :)  
  I will send some patches to improve --dry-run for rebase as greg suggested.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
phabricator - June 17, 2018, 9:05 a.m.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D3757#58993, @yuja wrote:
  
  > > +    dryrun = opts.get(r'dry_run')
  > >  +    if dryrun:
  > >  +        if opts.get(r'abort'):
  > >  +                raise error.Abort(_('cannot specify both --dry-run and --abort'))
  > >  +        if opts.get(r'continue'):
  > >  +                raise error.Abort(_('cannot specify both --dry-run and --continue'))
  >
  > Please remove the excessive 4 spaces before the "raise".
  
  
  Oh, sorry.
  
  > 
  > 
  >> +    if dryrun:
  >>  +        try:
  >>  +            overrides = {('rebase', 'singletransaction'): True}
  >>  +            with ui.configoverride(overrides, 'rebase'):
  >>  +                _origrebase(ui, repo, inmemory=True, dryrun=dryrun, **opts)
  > 
  > I meant the argument name `dryrun=` is misleading because it actually does
  >  rebase so `inmemory=True` and `_origrebase(ui, repo, abort=True)` are required.
  >  I think it's something like `leaveunfinished=`.
  
  Ah, right. I got it.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
phabricator - June 22, 2018, 12:44 p.m.
khanchi97 added a comment.


  I was also thinking to add --confirm option. It will show the same output as dry-run but at the end, will ask the user to continue. So that user don't have to write that command again. What do you say?

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
phabricator - June 22, 2018, 12:45 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D3757#59775, @khanchi97 wrote:
  
  > I was also thinking to add --confirm option. It will show the same output as dry-run but at the end, will ask the user to continue. So that user don't have to write that command again. What do you say?
  
  
  That will be great! Just do it.

REPOSITORY
  rHG Mercurial

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

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

Patch

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
@@ -155,4 +155,169 @@ 
   |/
   o  0: b173517d0057 'a'
   
+Test dry-run rebasing
+  $ hg init skrepo
+  $ cd skrepo
+  $ echo a>a
+  $ hg ci -Aqma
+  $ echo b>b
+  $ hg ci -Aqmb
+  $ echo c>c
+  $ hg ci -Aqmc
+  $ echo d>d
+  $ hg ci -Aqmd
+  $ echo e>e
+  $ hg ci -Aqme
 
+  $ hg up 1 -q
+  $ echo f>f
+  $ hg ci -Amf
+  adding f
+  created new head
+  $ echo g>g
+  $ hg ci -Aqmg
+  $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n"
+  @  6:baf10c5166d4 test
+  |  g
+  |
+  o  5:6343ca3eff20 test
+  |  f
+  |
+  | o  4:e860deea161a test
+  | |  e
+  | |
+  | o  3:055a42cdd887 test
+  | |  d
+  | |
+  | o  2:177f92b77385 test
+  |/   c
+  |
+  o  1:d2ae7f538514 test
+  |  b
+  |
+  o  0:cb9a9f314b8b test
+     a
+  
+Make sure it throws error while passing --continue or --abort with --dry-run
+  $ hg rebase -s 2 -d 6 -n --continue
+  abort: cannot specify both --dry-run and --continue
+  [255]
+  $ hg rebase -s 2 -d 6 -n --abort
+  abort: cannot specify both --dry-run and --abort
+  [255]
+
+Check dryrun gives correct results when there is no conflict in rebasing
+  $ hg rebase -s 2 -d 6 -n
+  rebasing 2:177f92b77385 "c"
+  rebasing 3:055a42cdd887 "d"
+  rebasing 4:e860deea161a "e"
+  There will be no conflict, you can rebase
+  rebase aborted
+
+  $ hg diff
+  $ hg status
+
+  $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n"
+  @  6:baf10c5166d4 test
+  |  g
+  |
+  o  5:6343ca3eff20 test
+  |  f
+  |
+  | o  4:e860deea161a test
+  | |  e
+  | |
+  | o  3:055a42cdd887 test
+  | |  d
+  | |
+  | o  2:177f92b77385 test
+  |/   c
+  |
+  o  1:d2ae7f538514 test
+  |  b
+  |
+  o  0:cb9a9f314b8b test
+     a
+  
+Check dryrun working with --collapse when there is no conflict
+  $ hg rebase -s 2 -d 6 -n --collapse
+  rebasing 2:177f92b77385 "c"
+  rebasing 3:055a42cdd887 "d"
+  rebasing 4:e860deea161a "e"
+  There will be no conflict, you can rebase
+  rebase aborted
+
+Check dryrun gives correct results when there is conflict in rebasing
+Make a conflict:
+  $ hg up 6 -q
+  $ echo conflict>e
+  $ hg ci -Aqm "conflict with e"
+  $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n"
+  @  7:d2c195b28050 test
+  |  conflict with e
+  |
+  o  6:baf10c5166d4 test
+  |  g
+  |
+  o  5:6343ca3eff20 test
+  |  f
+  |
+  | o  4:e860deea161a test
+  | |  e
+  | |
+  | o  3:055a42cdd887 test
+  | |  d
+  | |
+  | o  2:177f92b77385 test
+  |/   c
+  |
+  o  1:d2ae7f538514 test
+  |  b
+  |
+  o  0:cb9a9f314b8b test
+     a
+  
+  $ hg rebase -s 2 -d 7 -n
+  rebasing 2:177f92b77385 "c"
+  rebasing 3:055a42cdd887 "d"
+  rebasing 4:e860deea161a "e"
+  merging e
+  transaction abort!
+  rollback completed
+  Hit a merge conflict
+  rebase aborted
+  $ hg diff
+  $ hg status
+  $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n"
+  @  7:d2c195b28050 test
+  |  conflict with e
+  |
+  o  6:baf10c5166d4 test
+  |  g
+  |
+  o  5:6343ca3eff20 test
+  |  f
+  |
+  | o  4:e860deea161a test
+  | |  e
+  | |
+  | o  3:055a42cdd887 test
+  | |  d
+  | |
+  | o  2:177f92b77385 test
+  |/   c
+  |
+  o  1:d2ae7f538514 test
+  |  b
+  |
+  o  0:cb9a9f314b8b test
+     a
+  
+Check dryrun working with --collapse when there is conflicts
+  $ hg rebase -s 2 -d 7 -n --collapse
+  rebasing 2:177f92b77385 "c"
+  rebasing 3:055a42cdd887 "d"
+  rebasing 4:e860deea161a "e"
+  merging e
+  Hit a merge conflict
+  rebase aborted
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -673,8 +673,7 @@ 
     ('a', 'abort', False, _('abort an interrupted rebase')),
     ('', 'auto-orphans', '', _('automatically rebase orphan revisions '
                                'in the specified revset (EXPERIMENTAL)')),
-     ] +
-    cmdutil.formatteropts,
+     ] + cmdutil.dryrunopts + cmdutil.formatteropts,
     _('[-s REV | -b REV] [-d REV] [OPTION]'))
 def rebase(ui, repo, **opts):
     """move changeset (and descendants) to a different branch
@@ -798,6 +797,15 @@ 
 
     """
     inmemory = ui.configbool('rebase', 'experimental.inmemory')
+    dryrun = opts.get(r'dry_run')
+    if dryrun:
+        if (opts.get(r'abort')):
+                raise error.Abort(_('cannot specify both --dry-run '
+                                    'and --abort'))
+        if (opts.get(r'continue')):
+                raise error.Abort(_('cannot specify both --dry-run '
+                                    'and --continue'))
+
     if (opts.get(r'continue') or opts.get(r'abort') or
         repo.currenttransaction() is not None):
         # in-memory rebase is not compatible with resuming rebases.
@@ -814,22 +822,35 @@ 
         opts[r'rev'] = [revsetlang.formatspec('%ld and orphan()', userrevs)]
         opts[r'dest'] = '_destautoorphanrebase(SRC)'
 
-    if inmemory:
+    if dryrun:
         try:
-            # in-memory merge doesn't support conflicts, so if we hit any, abort
-            # and re-run as an on-disk merge.
             overrides = {('rebase', 'singletransaction'): True}
             with ui.configoverride(overrides, 'rebase'):
-                return _origrebase(ui, repo, inmemory=inmemory, **opts)
+                _origrebase(ui, repo, inmemory=True, dryrun=dryrun, **opts)
         except error.InMemoryMergeConflictsError:
-            ui.warn(_('hit merge conflicts; re-running rebase without in-memory'
-                      ' merge\n'))
+            ui.status(_('Hit a merge conflict\n'))
+        else:
+            ui.status(_('There will be no conflict, you can rebase\n'))
+        finally:
             _origrebase(ui, repo, **{r'abort': True})
-            return _origrebase(ui, repo, inmemory=False, **opts)
+
     else:
-        return _origrebase(ui, repo, **opts)
+        if inmemory:
+            try:
+                # in-memory merge doesn't support conflicts, so if we hit any, abort
+                # and re-run as an on-disk merge.
+                overrides = {('rebase', 'singletransaction'): True}
+                with ui.configoverride(overrides, 'rebase'):
+                    return _origrebase(ui, repo, inmemory=inmemory, **opts)
+            except error.InMemoryMergeConflictsError:
+                ui.warn(_('hit merge conflicts; re-running rebase without in-memory'
+                          ' merge\n'))
+                _origrebase(ui, repo, **{r'abort': True})
+                return _origrebase(ui, repo, inmemory=False, **opts)
+        else:
+            return _origrebase(ui, repo, **opts)
 
-def _origrebase(ui, repo, inmemory=False, **opts):
+def _origrebase(ui, repo, inmemory=False, dryrun=None, **opts):
     opts = pycompat.byteskwargs(opts)
     rbsrt = rebaseruntime(repo, ui, inmemory, opts)
 
@@ -902,7 +923,8 @@ 
                 dsguard = dirstateguard.dirstateguard(repo, 'rebase')
             with util.acceptintervention(dsguard):
                 rbsrt._performrebase(tr)
-                rbsrt._finishrebase()
+                if not dryrun:
+                    rbsrt._finishrebase()
 
 def _definedestmap(ui, repo, inmemory, destf=None, srcf=None, basef=None,
                    revf=None, destspace=None):