This is an archive of the discontinued Mercurial Phabricator instance.

rebase: add dry-run functionality
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
844–845

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.

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

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

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.

This revision was automatically updated to reflect the committed changes.

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

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.