Page MenuHomePhabricator

commit: clear resolved mergestate even if working copy is clean
ClosedPublic

Authored by martinvonz on Feb 28 2020, 2:57 PM.

Details

Summary

If the mergestate has resolved conflicts and a commit is successfully
created (either because there are changes in the working copy or
because ui.allowemptycommit=yes), we will also clear the merge
state. However, if the working copy is clean (and
ui.allowemptycommit=no), we leave the mergestate there. The user may
notice it in hg resolve -l output (but not in hg status -v
output). It's not clear how the user should clear it, but probably via
hg co -C .. It's also quite likely that they won't even notice it
and it will get cleared by a later hg commit (of unrelated
changes).

This patch makes it so that hg commit also clears resolved merge
conflicts even if the command doesn't end up writing a commit because
the working copy was empty. That's probably a little weird (commands
that abort should generally avoid changing the repo), but it still
seems mostly harmless, and it reduces the risk of more bugs like
https://bz.mercurial-scm.org/show_bug.cgi?id=5494. I just ran into a
version of that bug in the Evolve extension and that's what triggered
this series.

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.Feb 28 2020, 2:57 PM
pulkit added a subscriber: pulkit.Feb 29 2020, 10:34 AM

To me, having a hg commit which aborts, doing something seems a bit weird. For the use case of rebase, shelve etc., a function argument or internal config can be better.

To me, having a hg commit which aborts, doing something seems a bit weird.

Yes, I agree, as I said in the commit message.

For the use case of rebase, shelve etc., a function argument or internal config can be better.

I considered that but I also felt that it's weird that we just leave the state there otherwise until the user creates an unrelated commit or hg co -C. If we care about the state, we should be clear that we do (e.g. by registering it as an unfinished state that the user has to clear before they do anything else).

I'm fine with still adding the flag to make hg commit behave differently from internal calls. Still want me to do that or want me to register merge state as an unfinished state?

pulkit added a comment.Mar 2 2020, 7:43 AM

I'm fine with still adding the flag to make hg commit behave differently from internal calls. Still want me to do that or want me to register merge state as an unfinished state?

I thought we already register merge state as an unfinished state but now I see why we don't. Adding a flag sounds good to me.

marmoute accepted this revision.Mar 4 2020, 2:56 PM
marmoute added a subscriber: marmoute.

The case is a bit strange, but the proposed change seems a good way out.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Mar 5 2020, 3:29 AM
mercurial/localrepo.py
2959

Lets add a ui.debug() here about clearing of mergestate.