This is an archive of the discontinued Mercurial Phabricator instance.

filemerge: introduce functions to halt merge flow
ClosedPublic

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

Details

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

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
1297

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

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
1297

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
534

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

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
534

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
This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.
mercurial/filemerge.py
755–756

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
755–756

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)

775–776

See here

783–785

And here

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

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

Fixing unclear prompts in freeze is fine with me.

In D932#20158, @durin42 wrote:

Fixing unclear prompts in freeze is fine with me.

+1