This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8196

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

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

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG6a34e438461b: commit: clear resolved mergestate even if working copy is clean
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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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.

martinvonz added a comment.Feb 29 2020, 11:15 AM
In D8196#121923, @pulkit wrote:

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
In D8196#121924, @martinvonz wrote:

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.

martinvonz added a commit: rHG6a34e438461b: commit: clear resolved mergestate even if working copy is clean.Mar 4 2020, 5:09 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG6a34e438461b: commit: clear resolved mergestate even if working copy is clean (authored by martinvonz). · Explain Why
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.

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (1 line)
Mrelnotes/next (3 lines)
Mtests/test-update-branches.t (1 line)

Diff 20402

mercurial/localrepo.py

allowemptycommit = ( allowemptycommit = (
wctx.branch() != wctx.p1().branch() wctx.branch() != wctx.p1().branch()
or extra.get(b'close') or extra.get(b'close')
or merge or merge
or cctx.files() or cctx.files()
or self.ui.configbool(b'ui', b'allowemptycommit') or self.ui.configbool(b'ui', b'allowemptycommit')
) )
if not allowemptycommit: if not allowemptycommit:
ms.reset()
pulkitUnsubmitted
Not Done

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

pulkit: Lets add a `ui.debug()` here about clearing of mergestate.
return None return None
if merge and cctx.deleted(): if merge and cctx.deleted():
raise error.Abort(_(b"cannot commit merge with missing files")) raise error.Abort(_(b"cannot commit merge with missing files"))
if editor: if editor:
cctx._text = editor(self, cctx, subs) cctx._text = editor(self, cctx, subs)
edited = text != cctx._text edited = text != cctx._text

relnotes/next

parent during a conflict. parent during a conflict.
* `hg recover` does not verify the validity of the whole repository * `hg recover` does not verify the validity of the whole repository
anymore. You can pass `--verify` or call `hg verify` if necessary. anymore. You can pass `--verify` or call `hg verify` if necessary.
* `hg debugmergestate` output format changed. Let us know if that is * `hg debugmergestate` output format changed. Let us know if that is
causing you problems and we'll roll it back. causing you problems and we'll roll it back.
* Resolved merge conflicts are now cleared by `hg commit` even if the
working copy has no changes.
== Internal API Changes == == Internal API Changes ==
* The deprecated `ui.progress()` has now been deleted. Please use * The deprecated `ui.progress()` has now been deleted. Please use
`ui.makeprogress()` instead. `ui.makeprogress()` instead.
* `hg.merge()` has lost its `abort` argument. Please call * `hg.merge()` has lost its `abort` argument. Please call
`hg.abortmerge()` directly instead. `hg.abortmerge()` directly instead.
* `hg.merge()` has lost its `mergeforce` argument. It should have * `hg.merge()` has lost its `mergeforce` argument. It should have
only ever been called with the same value as the `force` argument. only ever been called with the same value as the `force` argument.
* The `*others` argument of `cmdutil.check_incompatible_arguments()` * The `*others` argument of `cmdutil.check_incompatible_arguments()`
changed from being varargs argument to being a single collection. changed from being varargs argument to being a single collection.

tests/test-update-branches.t

$ hg resolve -m a $ hg resolve -m a
(no more unresolved files) (no more unresolved files)
$ hg resolve -l $ hg resolve -l
R a R a
$ hg ci -m empty $ hg ci -m empty
nothing changed nothing changed
[1] [1]
$ hg resolve -l $ hg resolve -l
R a
Change/delete conflict is not allowed Change/delete conflict is not allowed
$ hg up -qC 3 $ hg up -qC 3
$ hg rm foo $ hg rm foo
$ hg up -q 4 $ hg up -q 4
Uses default value of "linear" when value is misspelled Uses default value of "linear" when value is misspelled
$ echo 'update.check = linyar' >> .hg/hgrc $ echo 'update.check = linyar' >> .hg/hgrc