Page MenuHomePhabricator

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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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

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
302

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
302

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
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)

752–753

See here

760–762

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?

Fixing unclear prompts in freeze is fine with me.

In D932#20158, @durin42 wrote:

Fixing unclear prompts in freeze is fine with me.

+1