Patchwork D3764: rebase: improve output of --dry-run

login
register
mail settings
Submitter phabricator
Date June 17, 2018, 11:48 a.m.
Message ID <differential-rev-PHID-DREV-ba2o3ls5dlyvvmtgjivl-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32216/
State Superseded
Headers show

Comments

phabricator - June 17, 2018, 11:48 a.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

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

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

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - June 18, 2018, 8:20 p.m.
pulkit added a comment.


  Thinking out loud here: I see that `--dry-run` for rebase will be very useful for automation. How about returning 1 when there are conflicts and returning 0 when there are no conflicts? Also maybe we should add formatter support to the dry-run output for each command so that automation can read the JSON output and tell user what can happen.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 18, 2018, 11:03 p.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D3764#59425, @pulkit wrote:
  
  > Thinking out loud here: I see that `--dry-run` for rebase will be very useful for automation. How about returning 1 when there are conflicts and returning 0 when there are no conflicts? Also maybe we should add formatter support to the dry-run output for each command so that automation can read the JSON output and tell user what can happen.
  
  
  I approve of both of these feature suggestions! However, they should be implemented in standalone changesets and not incorporated into this one.

REPOSITORY
  rHG Mercurial

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

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


  okay @pulkit , let me confirm if IIUC your points.
  
  1. First thing is we would have a function which will accept a `return_code`(0 or 1) and `output_data` (what can happen without --dry-run)  from any command (which has dry-run functionality). And will give output to the user according to the `return_code`. Following is the rough code for this, correct me if I am wrong at any point.
  
    def dryrunformatter(retcode, **outputdata):
        ui.status(_("starting dry-run; repository will not be changed"))
    
        # here show the outputdata accordingly
    
        if retcode == 0:
            ui.status(_("dry-run completed successfully; run without --dry-run/-n to perform this action"))
        else:
            ui.status(_("hit conflicts!"))
  
  
  
  2. If above explanation is right, talking about "additional functionality in dryrun" like --verbose mode in rebase which @indygreg suggested. May be we can add this type of functionality too in dryrunformatter? What do you say?

REPOSITORY
  rHG Mercurial

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

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


  @khanchi97 you should have created a new differential so that we don't loose your earlier patch titled: 'rebase: improve output of --dry-run' which is yet under review.

REPOSITORY
  rHG Mercurial

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

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


  In https://phab.mercurial-scm.org/D3764#59443, @indygreg wrote:
  
  > In https://phab.mercurial-scm.org/D3764#59425, @pulkit wrote:
  >
  > > Thinking out loud here: I see that `--dry-run` for rebase will be very useful for automation. How about returning 1 when there are conflicts and returning 0 when there are no conflicts? Also maybe we should add formatter support to the dry-run output for each command so that automation can read the JSON output and tell user what can happen.
  >
  >
  > I approve of both of these feature suggestions! However, they should be implemented in standalone changesets and not incorporated into this one.
  
  
  I agree.

REPOSITORY
  rHG Mercurial

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

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


  In https://phab.mercurial-scm.org/D3764#59797, @pulkit wrote:
  
  > @khanchi97 you should have created a new differential so that we don't loose your earlier patch titled: 'rebase: improve output of --dry-run' which is yet under review.
  
  
  Oh sorry, it was by mistake.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: indygreg, pulkit, 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
@@ -208,11 +208,11 @@ 
 
 Check dryrun gives correct results when there is no conflict in rebasing
   $ hg rebase -s 2 -d 6 -n
+  starting dry-run rebase; repository will not be changed
   rebasing 2:177f92b77385 "c"
   rebasing 3:055a42cdd887 "d"
   rebasing 4:e860deea161a "e"
-  there will be no conflict, you can rebase
-  rebase aborted
+  dry-run rebase completed successfully; run without -n/--dry-run to perform this rebase
 
   $ hg diff
   $ hg status
@@ -241,11 +241,11 @@ 
   
 Check dryrun working with --collapse when there is no conflict
   $ hg rebase -s 2 -d 6 -n --collapse
+  starting dry-run rebase; repository will not be changed
   rebasing 2:177f92b77385 "c"
   rebasing 3:055a42cdd887 "d"
   rebasing 4:e860deea161a "e"
-  there will be no conflict, you can rebase
-  rebase aborted
+  dry-run rebase completed successfully; run without -n/--dry-run to perform this rebase
 
 Check dryrun gives correct results when there is conflict in rebasing
 Make a conflict:
@@ -278,14 +278,14 @@ 
      a
   
   $ hg rebase -s 2 -d 7 -n
+  starting dry-run rebase; repository will not be changed
   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"
@@ -315,9 +315,9 @@ 
   
 Check dryrun working with --collapse when there is conflicts
   $ hg rebase -s 2 -d 7 -n --collapse
+  starting dry-run rebase; repository will not be changed
   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
@@ -325,7 +325,7 @@ 
         skippedset.update(obsoleteextinctsuccessors)
         _checkobsrebase(self.repo, self.ui, obsoleteset, skippedset)
 
-    def _prepareabortorcontinue(self, isabort):
+    def _prepareabortorcontinue(self, isabort, **opts):
         try:
             self.restorestatus()
             self.collapsemsg = restorecollapsemsg(self.repo, isabort)
@@ -341,8 +341,10 @@ 
                 hint = _('use "hg rebase --abort" to clear broken state')
                 raise error.Abort(msg, hint=hint)
         if isabort:
-            return abort(self.repo, self.originalwd, self.destmap,
-                         self.state, activebookmark=self.activebookmark)
+            suppwarning = opts.get(r'dry_run')
+            return abort(self.repo, self.originalwd, self.destmap, self.state,
+                         activebookmark=self.activebookmark,
+                         suppwarning=suppwarning)
 
     def _preparenewrebase(self, destmap):
         if not destmap:
@@ -821,16 +823,19 @@ 
         opts[r'dest'] = '_destautoorphanrebase(SRC)'
 
     if dryrun:
+        ui.status(_('starting dry-run rebase; repository will not be changed\n'))
         try:
             overrides = {('rebase', 'singletransaction'): True}
             with ui.configoverride(overrides, 'rebase'):
                 _origrebase(ui, repo, inmemory=True, leaveunfinished=True, **opts)
         except error.InMemoryMergeConflictsError:
             ui.status(_('hit a merge conflict\n'))
         else:
-            ui.status(_('there will be no conflict, you can rebase\n'))
+            ui.status(_('dry-run rebase completed successfully; run without '
+                        '-n/--dry-run to perform this rebase\n'))
         finally:
-            _origrebase(ui, repo, abort=True)
+            opts = {'abort':True, 'dry_run':True}
+            _origrebase(ui, repo, **opts)
     elif inmemory:
         try:
             # in-memory merge doesn't support conflicts, so if we hit any, abort
@@ -891,7 +896,7 @@ 
                 ms = mergemod.mergestate.read(repo)
                 mergeutil.checkunresolved(ms)
 
-            retcode = rbsrt._prepareabortorcontinue(abortf)
+            retcode = rbsrt._prepareabortorcontinue(abortf, **opts)
             if retcode is not None:
                 return retcode
         else:
@@ -1545,7 +1550,8 @@ 
 
     return False
 
-def abort(repo, originalwd, destmap, state, activebookmark=None):
+def abort(repo, originalwd, destmap, state, activebookmark=None,
+          suppwarning=None):
     '''Restore the repository to its original state.  Additional args:
 
     activebookmark: the name of the bookmark that should be active after the
@@ -1599,7 +1605,8 @@ 
     finally:
         clearstatus(repo)
         clearcollapsemsg(repo)
-        repo.ui.warn(_('rebase aborted\n'))
+        if not suppwarning:
+            repo.ui.warn(_('rebase aborted\n'))
     return 0
 
 def sortsource(destmap):