This is an archive of the discontinued Mercurial Phabricator instance.

Differential D932

filemerge: introduce functions to halt merge flow
ClosedPublic

Authored by ryanmce on Oct 4 2017, 9:58 AM.

Details

Reviewers
krbullock
mbthomas
durin42
Group Reviewers
hg-reviewers
Commits
rHGed91846c29cf: filemerge: introduce functions to halt merge flow
rHGa1625035db16: filemerge: introduce functions to halt merge flow
Summary

Depends on D931.

This patch introduces functions and a config option that will allow a user
to halt the merge if there are failures during a file merge. These functions
will be used in the next patch.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

ryanmce created this revision.Oct 4 2017, 9:58 AM
pulkit added a subscriber: pulkit.Oct 4 2017, 10:05 AM
pulkit added inline comments.
mercurial/help/config.txt
1248

typo, s/half/halt

krbullock requested changes to this revision.Oct 4 2017, 3:08 PM
krbullock added a subscriber: krbullock.

I was initially confused by the description of this change, because the user can already halt (by configuration) after the first failed file merge. But this introduces a way for them to be prompted whether to halt or continue, right? Also:

These functions will be used in the next patch.

What's the "next patch"? I'm not seeing it in Phabricator.

This revision now requires changes to proceed.Oct 4 2017, 3:08 PM
ryanmce added a comment.Oct 5 2017, 5:23 AM

I was initially confused by the description of this change, because the user can already halt (by configuration) after the first failed file merge.

As far as I know, this is simply not true. I know of no way in mercurial, before this series, to halt a merge after the first failed file merge. If it's already there, I'm happy to be informed of it and drop this series.

But this introduces a way for them to be prompted whether to halt or continue, right?

It introduces the first way that I'm aware of to halt a merge after a failed file merge. It also gives an option to prompt which also didn't exist before.

Also: What's the "next patch"? I'm not seeing it in Phabricator.

I had my internet connection at the airport interrupted and was only now able to finish sending the series. Sorry about that.

mercurial/help/config.txt
1248

Will fix.

ryanmce updated this revision to Diff 2457.Oct 5 2017, 5:28 AM
ryanmce marked 2 inline comments as done.Oct 5 2017, 5:29 AM
mbthomas accepted this revision.Oct 6 2017, 5:13 AM
ryanmce added inline comments.Oct 6 2017, 6:39 AM
mercurial/configitems.py
308

this could be on-failure, per the comment on D789

ryanmce marked an inline comment as done.Oct 6 2017, 9:20 AM
ryanmce updated this revision to Diff 2501.Oct 6 2017, 9:51 AM
quark set the repository for this revision to rHG Mercurial.Oct 7 2017, 1:14 PM
durin42 added a subscriber: durin42.Oct 14 2017, 12:34 AM

I've landed this stack through D931, will hold this one until at least Monday in case anyone objects on the style change.

mercurial/configitems.py
308

I'm following up on the list (with @ryanmce cc'ed) about the policy change - I dug up the email thread and it kind of trailed off without any kind of clear consensus that we should change.

FWIW, I like this style, and endorse it, I just want to make sure nobody objects before we lock it in. :)

durin42 accepted this revision.Oct 16 2017, 9:39 PM
Closed by commit rHGa1625035db16: filemerge: introduce functions to halt merge flow (authored by ryanmce). · Explain WhyOct 16 2017, 9:47 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Oct 19 2017, 2:17 AM
martinvonz added inline comments.
mercurial/filemerge.py
731–732

nit: Is the default used only with non-interactive ui? If it also works as a default that lets the user just press enter to choose Yes, I'd expect the "yn" to be "Yn"

ryanmce added inline comments.Oct 19 2017, 4:32 PM
mercurial/filemerge.py
731–732

I followed the pattern seen elsewhere in this file.

If you just hit enter, you do get the default, though.

I can follow-up with a patch to fix all of these unclear prompts if there are no objections... (after the freeze of course)

751–752

See here

759–761

And here

krbullock added a subscriber: yuja.Oct 19 2017, 5:22 PM
krbullock added inline comments.
mercurial/filemerge.py
731–732

I think fixes to unclear prompts are acceptable during the freeze. @martinvonz @durin42 @yuja what do y'all think?

durin42 added a comment.Oct 19 2017, 5:25 PM

Fixing unclear prompts in freeze is fine with me.

martinvonz added a comment.Oct 19 2017, 5:49 PM
In D932#20158, @durin42 wrote:

Fixing unclear prompts in freeze is fine with me.

+1

Revision Contents
Changeset List

Diff 2501

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', 'on-failure',
ryanmceAuthorUnsubmitted
Done

this could be on-failure, per the comment on D789

ryanmce: this could be `on-failure`, per the comment on D789
durin42Unsubmitted
Not Done

I'm following up on the list (with @ryanmce cc'ed) about the policy change - I dug up the email thread and it kind of trailed off without any kind of clear consensus that we should change.

FWIW, I like this style, and endorse it, I just want to make sure nobody objects before we lock it in. :)

durin42: I'm following up on the list (with @ryanmce cc'ed) about the policy change - I dug up the email…
default='continue',
)
coreconfigitem('merge', 'preferancestor', coreconfigitem('merge', 'preferancestor',
default=lambda: ['*'], default=lambda: ['*'],
) )
coreconfigitem('pager', 'ignore', coreconfigitem('pager', 'ignore',
default=list, default=list,
) )
coreconfigitem('patch', 'eol', coreconfigitem('patch', 'eol',
default='strict', default='strict',

mercurial/filemerge.py

if onfailure: if onfailure:
ui.warn(onfailure % fd) ui.warn(onfailure % fd)
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 _haltmerge():
msg = _('merge halted after failed merge (see hg resolve)')
raise error.InterventionRequired(msg)
def _onfilemergefailure(ui):
action = ui.config('merge', 'on-failure')
if action == 'prompt':
msg = _('continue merge operation (yn)?' '$$ &Yes $$ &No')
if ui.promptchoice(msg, 0) == 1:
martinvonzUnsubmitted
Not Done

nit: Is the default used only with non-interactive ui? If it also works as a default that lets the user just press enter to choose Yes, I'd expect the "yn" to be "Yn"

martinvonz: nit: Is the default used only with non-interactive ui? If it also works as a default that lets…
ryanmceAuthorUnsubmitted
Not Done

I followed the pattern seen elsewhere in this file.

If you just hit enter, you do get the default, though.

I can follow-up with a patch to fix all of these unclear prompts if there are no objections... (after the freeze of course)

ryanmce: I followed the pattern seen elsewhere in this file. If you just hit enter, you do get the…
krbullockUnsubmitted
Not Done

I think fixes to unclear prompts are acceptable during the freeze. @martinvonz @durin42 @yuja what do y'all think?

krbullock: I think fixes to unclear prompts are acceptable during the freeze. @martinvonz @durin42 @yuja…
_haltmerge()
if action == 'halt':
_haltmerge()
# default action is 'continue', in which case we neither prompt nor halt
def _check(repo, r, ui, tool, fcd, files): def _check(repo, r, ui, tool, fcd, files):
fd = fcd.path() fd = fcd.path()
unused, unused, unused, back = files unused, unused, unused, back = files
if not r and (_toolbool(ui, tool, "checkconflicts") or if not r and (_toolbool(ui, tool, "checkconflicts") or
'conflicts' in _toollist(ui, tool, "check")): 'conflicts' in _toollist(ui, tool, "check")):
if re.search("^(<<<<<<< .*|=======|>>>>>>> .*)$", fcd.data(), if re.search("^(<<<<<<< .*|=======|>>>>>>> .*)$", fcd.data(),
re.MULTILINE): re.MULTILINE):
r = 1 r = 1
checked = False checked = False
if 'prompt' in _toollist(ui, tool, "check"): if 'prompt' in _toollist(ui, tool, "check"):
checked = True checked = True
if ui.promptchoice(_("was merge of '%s' successful (yn)?" if ui.promptchoice(_("was merge of '%s' successful (yn)?"
"$$ &Yes $$ &No") % fd, 1): "$$ &Yes $$ &No") % fd, 1):
ryanmceAuthorUnsubmitted
Not Done

See here

ryanmce: See here
r = 1 r = 1
if not r and not checked and (_toolbool(ui, tool, "checkchanged") or if not r and not checked and (_toolbool(ui, tool, "checkchanged") or
'changed' in 'changed' in
_toollist(ui, tool, "check")): _toollist(ui, tool, "check")):
if back is not None and filecmp.cmp(_workingpath(repo, fcd), back): if back is not None and filecmp.cmp(_workingpath(repo, fcd), back):
if ui.promptchoice(_(" output file %s appears unchanged\n" if ui.promptchoice(_(" output file %s appears unchanged\n"
"was merge successful (yn)?" "was merge successful (yn)?"
"$$ &Yes $$ &No") % fd, 1): "$$ &Yes $$ &No") % fd, 1):
ryanmceAuthorUnsubmitted
Not Done

And here

ryanmce: And here
r = 1 r = 1
if back is not None and _toolbool(ui, tool, "fixeol"): if back is not None and _toolbool(ui, tool, "fixeol"):
_matcheol(_workingpath(repo, fcd), back) _matcheol(_workingpath(repo, fcd), back)
return r return r
def _workingpath(repo, ctx): def _workingpath(repo, ctx):

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``)
``on-failure``
When set to ``continue`` (the default), the merge process attempts to
merge all unresolved files using the merge chosen tool, regardless of
whether previous file merge attempts during the process succeeded or not.
Setting this to ``prompt`` will prompt after any merge failure continue
or halt the merge process. Setting this to ``halt`` will automatically
halt the merge process on any merge tool failure. The merge process
pulkitUnsubmitted
Done

typo, s/half/halt

pulkit: typo, s/half/halt
ryanmceAuthorUnsubmitted
Done

Will fix.

ryanmce: Will fix.
can be restarted by using the ``resolve`` command. When a merge is
halted, the repository is left in a normal ``unresolved`` merge state.
(default: ``continue``)
``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.