D3757: rebase: add dry-run functionality

[indygreg (Gregory Szorc)]

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