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.

Download Raw Diff

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

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptFeb 28 2020, 2:57 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

martinvonz added a child revision: D8197: cleanup: remove redundant clearing of mergestate in rebase and shelve.Feb 28 2020, 2:57 PM

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.

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?

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.

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

			Path	Packages
M			mercurial/localrepo.py (1 line)
M			relnotes/next (3 lines)
M			tests/test-update-branches.t (1 line)

Status	Author	Revision
Closed	martinvonz	D8197 cleanup: remove redundant clearing of mergestate in rebase and shelve
Closed	martinvonz	D8196 commit: clear resolved mergestate even if working copy is clean
Closed	martinvonz	D8195 commit: error out on unresolved files even if commit would be empty
Closed	martinvonz	D8194 tests: add test of committing with conflicts but no changes in wdir

Diff 20489

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()` now takes a `ctx` instead of the previous `repo` and			* `hg.merge()` now takes a `ctx` instead of the previous `repo` and
	`node` arguments.			`node` arguments.

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

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	20402		Feb 28 2020, 2:57 PM	★	★
Diff 2	20489	rHG6a34e438461bd1dd8b07c56ca8af30bbf5a7772c	Feb 28 2020, 2:32 PM	★	★