Page MenuHomePhabricator

resolve: also detect new :mergediff conflict markers
ClosedPublic

Authored by martinvonz on Jan 19 2021, 6:19 PM.

Details

Summary

The conflict markers created by :mergediff were not detected as
conflicts, which affects both commands.resolve.mark-check and
mergetools.<tool>.check. This patch fixes that.

The new regex it uses for finding conflict markers is less restrictive
because it :mergediff doesn't follow the <<<<<<< and >>>>>>>
lines by a space (and a description). Hopefully lines like that don't
give too many false positives. We can add back the space and make
:mergediff add trailing spaces if it turns out to be a
problem. OTOH, there will always be some false positives and we have
ways of overriding the checks already.

This patch can go onto the default or stable branch, depending on how
much we care about an experimental feature.

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

martinvonz created this revision.Jan 19 2021, 6:19 PM
spectral added inline comments.
mercurial/filemerge.py
1199

Will mergediff always have a -{7} .* and \+{7} .*? i.e. could/should we keep it as:

^(<<<<<<< .*|=======|>>>>>>> .*)$

And just add the two new cases? We don't need to match every line, just any line.

^(<<<<<<< .*|------- .*|\+\+\+\+\+\+\+ .*|=======|>>>>>>> .*)$

If it's not guaranteed there will be a -{7} .* or a +{7} .*, this won't work, obviously.

martinvonz added inline comments.Jan 19 2021, 6:50 PM
mercurial/filemerge.py
1199

It's not uncommon for me to partially resolve a conflict and end up leaving just the >>>>>>>, for example. We have detected that so far, but maybe we've only done so by accident. I guess the fact that the expression wasn't just <<<<<<< suggests that it was trying to catch partial markers. What do you think?

Actually, mergediff will have a message on the ======= line, so I should at least add .* at the end of that expression to allow for that if we care about partial markers.

To answer your question: yes, mergediff will always have exactly one -------, one +++++++, and one ======= line (in Mercurial).

martinvonz updated this revision to Diff 25162.Jan 19 2021, 6:56 PM
spectral accepted this revision.Jan 19 2021, 6:57 PM
spectral added inline comments.
mercurial/filemerge.py
1199

Partially resolving a conflict is a good point, there's been more than one time where I've missed deleting one of the markers.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.