Page MenuHomePhabricator

merge: add option to abort merge process on failure
AbandonedPublic

Authored by ryanmce on Sep 22 2017, 7:28 AM.

Details

Reviewers
quark
yuja
mbthomas
Group Reviewers
hg-reviewers
Summary

Traditionally, the merge process attempts to merge all unresolved files
using the merge tool of choice, regardless of whether previous file merge
attempts succeeded or failed. Setting the merge.abortonfailure option to
True changes this behavior so that a failed merge will immediately abort
the merge operation, leaving the user in a normal unresolved merge state.

This is particularly useful when there may be many files to be merged and the
user just wants to abort the process by exiting their editor with an error.

Test Plan

Added a new test

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

ryanmce created this revision.Sep 22 2017, 7:28 AM
quark added a subscriber: quark.Sep 22 2017, 8:31 AM

Is this the same as --tool internal:fail?

No, it's an option that let's me :cq out of vim and stop additional windows from popping up, AFTER the merge process has already started.

tests/test-filemerge-abort.t
22 ↗(On Diff #2007)

Or did you mean this? I guess here I can use :fail

quark accepted this revision.Sep 22 2017, 8:33 AM

Answer myself - this is different and useful.

ryanmce marked an inline comment as done.Sep 22 2017, 8:55 AM
ryanmce added inline comments.
tests/test-filemerge-abort.t
22 ↗(On Diff #2007)

Actually I can't because I need an external tool to fail here.

ryanmce marked 2 inline comments as done.Sep 22 2017, 8:55 AM
yuja requested changes to this revision.Sep 26 2017, 9:39 AM
yuja added a subscriber: yuja.

Can you rename or fold the test so we can do ./run-tests.py test-merge-*?

mercurial/filemerge.py
534

Nit: onerr() is a function to return an exception object (e.g. an exception type), not a function to raise an exception.

728

Perhaps it's better to abort here, not in _xmerge().

Several merge tools do not return non-zero status on error, so
we have extra _check() to detect merge error.

This revision now requires changes to proceed.Sep 26 2017, 9:39 AM

Thanks for the feedback @yuja! I'll work on a second version.

mbthomas requested changes to this revision.Sep 26 2017, 9:59 AM
mbthomas added a subscriber: mbthomas.
mbthomas added inline comments.
mercurial/filemerge.py
535

Typo: nonrzero.

ryanmce added inline comments.Sep 26 2017, 10:41 AM
mercurial/filemerge.py
535

doh!

ryanmce marked 4 inline comments as done.Oct 2 2017, 10:48 AM
ryanmce added inline comments.
mercurial/filemerge.py
728

I looked through the code and you're right -- this is a better place. This also makes this change smaller overall. Thanks for the excellent suggestion!

ryanmce marked an inline comment as done.Oct 2 2017, 11:13 AM
ryanmce updated this revision to Diff 2332.

Followed yuja's advice and moved the abort later.

This has many advantages as shown in the updated test, which shows this abort
also helps with merge tool post-checks, which are also tested.

It also gives me ideas for the prompts that we can improve to allow this to
be an option at runtime rather than only a config-time option as it is now.

ryanmce updated this revision to Diff 2333.Oct 2 2017, 11:47 AM

fix check-code -- whoops

ryanmce marked an inline comment as done.Oct 2 2017, 11:47 AM
ryanmce planned changes to this revision.Oct 2 2017, 12:45 PM

This doesn't actually lead to merge conflicts in the repo -- the error.Abort is "too strong". I need deeper surgery here.

ryanmce abandoned this revision.Oct 6 2017, 5:03 AM

Replaced by series ending at D953

markand added a subscriber: markand.Oct 6 2017, 6:26 AM
markand added inline comments.
mercurial/configitems.py
302

Please follow the naming style as defined in the UI Guidelines. Therefore it should be abort-on-failure.

ryanmce marked an inline comment as done.Oct 6 2017, 6:38 AM
ryanmce added inline comments.
mercurial/configitems.py
302

Thanks! I didn't know about this new naming guideline. I definitely like it better than the old allthewordssmashedtogether guideline. Will update the current series.

ryanmce marked an inline comment as done.Oct 9 2017, 8:36 AM