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

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

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
536

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

734

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
537

Typo: nonrzero.

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

doh!

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

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
299

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
299

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 2007

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

replace = {'local': a, 'base': b, 'other': c, 'output': out} replace = {'local': a, 'base': b, 'other': c, 'output': out}
args = util.interpolate(r'\$', replace, args, args = util.interpolate(r'\$', replace, args,
lambda s: util.shellquote(util.localpath(s))) lambda s: util.shellquote(util.localpath(s)))
cmd = toolpath + ' ' + args cmd = toolpath + ' ' + args
if _toolbool(ui, tool, "gui"): if _toolbool(ui, tool, "gui"):
repo.ui.status(_('running merge tool %s for file %s\n') % repo.ui.status(_('running merge tool %s for file %s\n') %
(tool, fcd.path())) (tool, fcd.path()))
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') onerr = getmergeerrorhandler(repo)
r = ui.system(cmd, cwd=repo.root, environ=env, blockedtag='mergetool',
onerr=onerr)
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):
"""
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.
"""
onerr = None
if repo.ui.configbool('merge', 'abortonfailure'):
def onerr(*args):
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…
msg = _('merge aborted due to nonrzero mergetool return code')
mbthomasUnsubmitted
Done

Typo: nonrzero.

mbthomas: Typo: `nonrzero`.
ryanmceAuthorUnsubmitted
Done

doh!

ryanmce: doh!
raise 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.
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
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()
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 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-filemerge-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
$ hg rebase -s 1 -d 2 --tool false
ryanmceAuthorUnsubmitted
Done

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

ryanmce: Or did you mean this? I guess here I can use `:fail`
ryanmceAuthorUnsubmitted
Done

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

ryanmce: Actually I can't because I need an external tool to fail here.
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
abort: merge aborted due to nonrzero mergetool return code
[255]
$ hg rebase --abort
rebase aborted
$ hg rebase -s 1 -d 2 --tool true
rebasing 1:1f28a51c3c9b "c"
merging a
merging b
note: rebase of 1:1f28a51c3c9b created no changes to commit
saved backup bundle to $TESTTMP/repo/.hg/strip-backup/1f28a51c3c9b-5312ccf5-rebase.hg (glob)