This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3757

rebase: add dry-run functionality
ClosedPublic

Authored by khanchi97 on Jun 16 2018, 9:47 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGf4f1fb1cbfb4: rebase: add dry-run functionality
Summary

For now, it gives stats about rebase would be successful or hit a
conflict. Remaining work is to improve the output and adding verbose mode
where will show the diff of conflicting files if we hit any.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

khanchi97 created this revision.Jun 16 2018, 9:47 AM
indygreg requested changes to this revision.Jun 16 2018, 4:57 PM
indygreg added a subscriber: indygreg.

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.

hgext/rebase.py
831–833

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

tests/test-rebase-inmemory.t
210–215

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.

280–288

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.

This revision now requires changes to proceed.Jun 16 2018, 4:57 PM
yuja added a subscriber: yuja.Jun 17 2018, 12:35 AM

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.

khanchi97 edited the summary of this revision. (Show Details)Jun 17 2018, 2:53 AM
khanchi97 updated this revision to Diff 9122.
khanchi97 added a comment.Jun 17 2018, 3:07 AM

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

khanchi97 added a comment.Jun 17 2018, 3:17 AM

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

yuja added a comment.Jun 17 2018, 4:46 AM

+ 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".

+ 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=.

khanchi97 added a comment.Jun 17 2018, 5:05 AM
In 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.

khanchi97 updated this revision to Diff 9124.Jun 17 2018, 5:22 AM
yuja added a comment.Jun 18 2018, 8:19 AM

Queued, thanks.

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

I did s/None/False/ for readability.

Closed by commit rHGf4f1fb1cbfb4: rebase: add dry-run functionality (authored by khanchi97). · Explain WhyJun 18 2018, 8:33 AM
This revision was automatically updated to reflect the committed changes.
khanchi97 added a comment.Jun 22 2018, 8:44 AM

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?

pulkit added a subscriber: pulkit.Jun 22 2018, 8:45 AM
In 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.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (46 lines)
Mtests/test-rebase-inmemory.t (165 lines)

Diff 9109

hgext/rebase.py

('', 'keepbranches', False, _('keep original branch names')), ('', 'keepbranches', False, _('keep original branch names')),
('D', 'detach', False, _('(DEPRECATED)')), ('D', 'detach', False, _('(DEPRECATED)')),
('i', 'interactive', False, _('(DEPRECATED)')), ('i', 'interactive', False, _('(DEPRECATED)')),
('t', 'tool', '', _('specify merge tool')), ('t', 'tool', '', _('specify merge tool')),
('c', 'continue', False, _('continue an interrupted rebase')), ('c', 'continue', False, _('continue an interrupted rebase')),
('a', 'abort', False, _('abort an interrupted rebase')), ('a', 'abort', False, _('abort an interrupted rebase')),
('', 'auto-orphans', '', _('automatically rebase orphan revisions ' ('', 'auto-orphans', '', _('automatically rebase orphan revisions '
'in the specified revset (EXPERIMENTAL)')), 'in the specified revset (EXPERIMENTAL)')),
] + ] + cmdutil.dryrunopts + cmdutil.formatteropts,
cmdutil.formatteropts,
_('[-s REV | -b REV] [-d REV] [OPTION]')) _('[-s REV | -b REV] [-d REV] [OPTION]'))
def rebase(ui, repo, **opts): def rebase(ui, repo, **opts):
"""move changeset (and descendants) to a different branch """move changeset (and descendants) to a different branch
Rebase uses repeated merging to graft changesets from one part of Rebase uses repeated merging to graft changesets from one part of
history (the source) onto another (the destination). This can be history (the source) onto another (the destination). This can be
useful for linearizing *local* changes relative to a master useful for linearizing *local* changes relative to a master
development tree. development tree.
Return Values: Return Values:
Returns 0 on success, 1 if nothing to rebase or there are Returns 0 on success, 1 if nothing to rebase or there are
unresolved conflicts. unresolved conflicts.
""" """
inmemory = ui.configbool('rebase', 'experimental.inmemory') 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 if (opts.get(r'continue') or opts.get(r'abort') or
repo.currenttransaction() is not None): repo.currenttransaction() is not None):
# in-memory rebase is not compatible with resuming rebases. # in-memory rebase is not compatible with resuming rebases.
# (Or if it is run within a transaction, since the restart logic can # (Or if it is run within a transaction, since the restart logic can
# fail the entire transaction.) # fail the entire transaction.)
inmemory = False inmemory = False
if opts.get(r'auto_orphans'): if opts.get(r'auto_orphans'):
for key in opts: for key in opts:
if key != r'auto_orphans' and opts.get(key): if key != r'auto_orphans' and opts.get(key):
raise error.Abort(_('--auto-orphans is incompatible with %s') % raise error.Abort(_('--auto-orphans is incompatible with %s') %
('--' + pycompat.bytestr(key))) ('--' + pycompat.bytestr(key)))
userrevs = list(repo.revs(opts.get(r'auto_orphans'))) userrevs = list(repo.revs(opts.get(r'auto_orphans')))
opts[r'rev'] = [revsetlang.formatspec('%ld and orphan()', userrevs)] opts[r'rev'] = [revsetlang.formatspec('%ld and orphan()', userrevs)]
opts[r'dest'] = '_destautoorphanrebase(SRC)' opts[r'dest'] = '_destautoorphanrebase(SRC)'
if dryrun:
try:
overrides = {('rebase', 'singletransaction'): True}
with ui.configoverride(overrides, 'rebase'):
_origrebase(ui, repo, inmemory=True, dryrun=dryrun, **opts)
except error.InMemoryMergeConflictsError:
ui.status(_('Hit a merge conflict\n'))
else:
ui.status(_('There will be no conflict, you can rebase\n'))
indygregUnsubmitted
Not Done

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

indygreg: Nit: please use lower case letters to being the message so we're consistent with the rest of…
finally:
_origrebase(ui, repo, **{r'abort': True})
else:
if inmemory: if inmemory:
try: try:
# in-memory merge doesn't support conflicts, so if we hit any, abort # in-memory merge doesn't support conflicts, so if we hit any, abort
# and re-run as an on-disk merge. # and re-run as an on-disk merge.
overrides = {('rebase', 'singletransaction'): True} overrides = {('rebase', 'singletransaction'): True}
with ui.configoverride(overrides, 'rebase'): with ui.configoverride(overrides, 'rebase'):
return _origrebase(ui, repo, inmemory=inmemory, **opts) return _origrebase(ui, repo, inmemory=inmemory, **opts)
except error.InMemoryMergeConflictsError: except error.InMemoryMergeConflictsError:
ui.warn(_('hit merge conflicts; re-running rebase without in-memory' ui.warn(_('hit merge conflicts; re-running rebase without in-memory'
' merge\n')) ' merge\n'))
_origrebase(ui, repo, **{r'abort': True}) _origrebase(ui, repo, **{r'abort': True})
return _origrebase(ui, repo, inmemory=False, **opts) return _origrebase(ui, repo, inmemory=False, **opts)
else: else:
return _origrebase(ui, repo, **opts) 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) opts = pycompat.byteskwargs(opts)
rbsrt = rebaseruntime(repo, ui, inmemory, opts) rbsrt = rebaseruntime(repo, ui, inmemory, opts)
with repo.wlock(), repo.lock(): with repo.wlock(), repo.lock():
# Validate input and define rebasing points # Validate input and define rebasing points
destf = opts.get('dest', None) destf = opts.get('dest', None)
srcf = opts.get('source', None) srcf = opts.get('source', None)
basef = opts.get('base', None) basef = opts.get('base', None)
with util.acceptintervention(tr): with util.acceptintervention(tr):
# Same logic for the dirstate guard, except we don't create one when # Same logic for the dirstate guard, except we don't create one when
# rebasing in-memory (it's not needed). # rebasing in-memory (it's not needed).
dsguard = None dsguard = None
if singletr and not inmemory: if singletr and not inmemory:
dsguard = dirstateguard.dirstateguard(repo, 'rebase') dsguard = dirstateguard.dirstateguard(repo, 'rebase')
with util.acceptintervention(dsguard): with util.acceptintervention(dsguard):
rbsrt._performrebase(tr) rbsrt._performrebase(tr)
if not dryrun:
rbsrt._finishrebase() rbsrt._finishrebase()
def _definedestmap(ui, repo, inmemory, destf=None, srcf=None, basef=None, def _definedestmap(ui, repo, inmemory, destf=None, srcf=None, basef=None,
revf=None, destspace=None): revf=None, destspace=None):
"""use revisions argument to define destmap {srcrev: destrev}""" """use revisions argument to define destmap {srcrev: destrev}"""
if revf is None: if revf is None:
revf = [] revf = []
# destspace is here to work around issues with `hg pull --rebase` see # destspace is here to work around issues with `hg pull --rebase` see

tests/test-rebase-inmemory.t

@ 3: 844a7de3e617 'c' @ 3: 844a7de3e617 'c'
| |
| o 2: 09c044d2cb43 'd' | o 2: 09c044d2cb43 'd'
| | | |
| o 1: fc055c3b4d33 'b' | o 1: fc055c3b4d33 'b'
|/ |/
o 0: b173517d0057 'a' 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
indygregUnsubmitted
Not Done

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.

indygreg: This is potentially follow-up territory, but I think there is room to improve the output here.
$ 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
indygregUnsubmitted
Not Done

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.

indygreg: Again, more nit picks around the messaging. Again, I think this should print a message that we…
$ 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