This is an archive of the discontinued Mercurial Phabricator instance.

Differential D789

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?

ryanmce added a comment.Sep 22 2017, 8:32 AM

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
ryanmce added a comment.Sep 26 2017, 9:45 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

Revision Contents
Changeset List

Diff 2333

mercurial/configitems.py

default=None, default=None,
) )
coreconfigitem('http_proxy', 'user', coreconfigitem('http_proxy', 'user',
default=None, default=None,
) )
coreconfigitem('merge', 'followcopies', coreconfigitem('merge', 'followcopies',
default=True, default=True,
) )
coreconfigitem('merge', 'abortonfailure',
markandUnsubmitted
Done

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

markand: Please follow the naming style as defined in the [UI Guidelines](https://www.mercurial-scm.
ryanmceAuthorUnsubmitted
Not Done

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: Thanks! I didn't know about this new naming guideline. I definitely like it better than the old…
default=False,
)
coreconfigitem('pager', 'ignore', coreconfigitem('pager', 'ignore',
default=list, default=list,
) )
coreconfigitem('patch', 'eol', coreconfigitem('patch', 'eol',
default='strict', default='strict',
) )
coreconfigitem('patch', 'fuzz', coreconfigitem('patch', 'fuzz',
default=2, default=2,

mercurial/filemerge.py

repo.ui.debug('launching merge tool: %s\n' % cmd) repo.ui.debug('launching merge tool: %s\n' % cmd)
r = ui.system(cmd, cwd=repo.root, environ=env, blockedtag='mergetool') r = ui.system(cmd, cwd=repo.root, environ=env, blockedtag='mergetool')
repo.ui.debug('merge tool returned: %s\n' % r) repo.ui.debug('merge tool returned: %s\n' % r)
return True, r, False return True, r, False
finally: finally:
util.unlink(b) util.unlink(b)
util.unlink(c) util.unlink(c)
def getmergeerrorhandler(repo):
onerr = None
if repo.ui.configbool('merge', 'abortonfailure'):
def onerr(*args):
msg = _('merge aborted due to nonzero mergetool return code')
return error.Abort(msg)
return onerr
def _formatconflictmarker(repo, ctx, template, label, pad): def _formatconflictmarker(repo, ctx, template, label, pad):
"""Applies the given template to the ctx, prefixed by the label. """Applies the given template to the ctx, prefixed by the label.
yujaUnsubmitted
Done

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

yuja: Nit: onerr() is a function to return an exception object (e.g. an exception type), not a…
Pad is the minimum width of the label prefix, so that multiple markers Pad is the minimum width of the label prefix, so that multiple markers
mbthomasUnsubmitted
Done

Typo: nonrzero.

mbthomas: Typo: `nonrzero`.
ryanmceAuthorUnsubmitted
Done

doh!

ryanmce: doh!
can have aligned templated parts. can have aligned templated parts.
""" """
if ctx.node() is None: if ctx.node() is None:
ctx = ctx.p1() ctx = ctx.p1()
props = templatekw.keywords.copy() props = templatekw.keywords.copy()
props['templ'] = template props['templ'] = template
props['ctx'] = ctx props['ctx'] = ctx
needcheck, r, deleted = func(repo, mynode, orig, fcd, fco, fca, needcheck, r, deleted = func(repo, mynode, orig, fcd, fco, fca,
toolconf, files, labels=labels) toolconf, files, labels=labels)
if needcheck: if needcheck:
r = _check(repo, r, ui, tool, fcd, files) r = _check(repo, r, ui, tool, fcd, files)
if r: if r:
if repo.ui.configbool('merge', 'abortonfailure'):
msg = _('merge aborted due to merge tool failure')
raise error.Abort(msg)
if onfailure: if onfailure:
ui.warn(onfailure % fd) ui.warn(onfailure % fd)
yujaUnsubmitted
Done

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.

yuja: Perhaps it's better to abort here, not in _xmerge(). Several merge tools do not return non…
ryanmceAuthorUnsubmitted
Done

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: I looked through the code and you're right -- this is a better place. This also makes this…
return True, r, deleted return True, r, deleted
finally: finally:
if not r and back is not None: if not r and back is not None:
util.unlink(back) util.unlink(back)
def _check(repo, r, ui, tool, fcd, files): def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path() fd = fcd.path()

mercurial/help/config.txt

``.orig``. (default: ``abort``) ``.orig``. (default: ``abort``)
``checkunknown`` ``checkunknown``
Controls behavior when an unknown file that isn't ignored has the same name Controls behavior when an unknown file that isn't ignored has the same name
as a tracked file in the changeset being merged or updated to, and has as a tracked file in the changeset being merged or updated to, and has
different contents. Similar to ``merge.checkignored``, except for files that different contents. Similar to ``merge.checkignored``, except for files that
are not ignored. (default: ``abort``) are not ignored. (default: ``abort``)
``abortonfailure``
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.
(default: ``False``)
``merge-patterns`` ``merge-patterns``
------------------ ------------------
This section specifies merge tools to associate with particular file This section specifies merge tools to associate with particular file
patterns. Tools matched here will take precedence over the default patterns. Tools matched here will take precedence over the default
merge tool. Patterns are globs by default, rooted at the repository merge tool. Patterns are globs by default, rooted at the repository
root. root.

tests/test-merge-tool-abort.t

  • This file was added.
$ cat >> $HGRCPATH <<EOF
> [extensions]
> rebase=
> [phases]
> publish=False
> [merge]
> abortonfailure=True
> EOF
$ hg init repo
$ cd repo
$ echo a > a
$ echo b > b
$ hg commit -qAm ab
$ echo c >> a
$ echo c >> b
$ hg commit -qAm c
$ hg up -q ".^"
$ echo d >> a
$ echo d >> b
$ hg commit -qAm d
Check that failing external tool aborts the merge
$ hg rebase -s 1 -d 2 --tool false
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
abort: merge aborted due to merge tool failure
[255]
$ hg rebase --abort
rebase aborted
Check that successful tool with failed post-check aborts the merge
$ hg rebase -s 1 -d 2 --tool true --config merge-tools.true.check=changed
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
output file a appears unchanged
was merge successful (yn)? n
abort: merge aborted due to merge tool failure
[255]
$ hg rebase --abort
rebase aborted
$ hg rebase -s 1 -d 2 --tool true --config merge-tools.true.check=conflicts --config merge-tools.true.premerge=keep
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
abort: merge aborted due to merge tool failure
[255]
$ hg rebase --abort
rebase aborted
$ hg rebase -s 1 -d 2 --tool true --config merge-tools.true.check=prompt
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
was merge of 'a' successful (yn)? n
abort: merge aborted due to merge tool failure
[255]
$ hg rebase --abort
rebase aborted
Check that successful tool otherwise allows the merge to continue
$ hg rebase -s 1 -d 2 --tool echo --keep --config merge-tools.echo.premerge=keep
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
$TESTTMP/repo/a *a~base* *a~other* (glob)
$TESTTMP/repo/b *b~base* *b~other* (glob)