This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8728

overlayworkingctx: rename misleadingly named `isempty()` method
ClosedPublic

Authored by mjacob on Jul 10 2020, 7:16 PM.

Download Raw Diff

Details

Reviewers

martinvonz

Group Reviewers

hg-reviewers

Commits

rHG83f75f1efdcc: overlayworkingctx: rename misleadingly named `isempty()` method

Summary

This method is only about whether there are file changes, not about whether the
commit will be empty or not.

One user of the method was incorrectly assuming the latter meaning, leading to
the bug for which a test case was added in D8727. I’ve added a FIXME to the
code.

The original motivation for the rename was that I want to add
committablectx.isempty(), that properly checks if a commit will be empty,
using the exact same logic as in repo.commit(), and I wanted to avoid a name
clash.

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

mjacob created this revision.Jul 10 2020, 7:16 PM

Herald added a reviewer: martinvonz. · View Herald TranscriptJul 10 2020, 7:16 PM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

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

mjacob added a child revision: D8729: commit: factor out empty commit check to `basectx.isempty()`.Jul 10 2020, 7:16 PM

mjacob updated this revision to Diff 21862.Jul 11 2020, 5:11 PM

martinvonz added inline comments.Jul 13 2020, 8:57 AM

mercurial/context.py
2519–2529	I feel like it would be better to find better names for these three methods together since they're about the same concept. Maybe simply renaming this one to `isclean()` matches well? The only issue I see is that Mercurial usually skips the "is" prefix and "clean" is both a verb and an adjective, so it's not obvious at first why there's both `clean()` and `isclean()` (people familiar with Mercurial naming might think they're the same). Probably still okay?

mjacob added inline comments.Jul 13 2020, 11:19 AM

mercurial/context.py
2519–2529	To be honest, I didn’t think too hard about this name (at long as it’s more correct as the old name), as the method will become unused and removed a few patches later. If you still think we should rename it for the in-between steps, I’d be fine renaming it.

martinvonz added inline comments.Jul 13 2020, 4:47 PM

mercurial/context.py
2519–2529	No, that's fine. I had just not realized that it was going away in a later patch.

Manuel Jacob <me@manueljacob.de> added a commit: rHG83f75f1efdcc: overlayworkingctx: rename misleadingly named `isempty()` method.Jul 14 2020, 12:52 AM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG83f75f1efdcc: overlayworkingctx: rename misleadingly named `isempty()` method (authored by Manuel Jacob <me@manueljacob.de>). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/rebase.py (6 lines)
M			mercurial/context.py (2 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	21839		Jul 10 2020, 7:15 PM	★	★
Diff 2	21862		Jul 11 2020, 5:11 PM	★	★
Diff 3	21880	rHG83f75f1efdcca90550c11e1a6caa5ec254c6d92f	Jul 10 2020, 6:53 PM	★	★

Status	Author	Revision
Closed	mjacob	D8733 overlayworkingctx: remove unused `nofilechanges()` and `_compact()` methods
Closed	mjacob	D8732 rebase: correctly check for empty commit in in-memory mode
Closed	mjacob	D8731 memctx: make `parents()` return list of one element if it’s not a merge
Closed	mjacob	D8729 commit: factor out empty commit check to `basectx.isempty()`
Closed	mjacob	D8728 overlayworkingctx: rename misleadingly named `isempty()` method
Closed	mjacob	D8727 rebase: add test case that shows that inmemory rebase does not preserve merges
Closed	mjacob	D8725 rebase: remove now unnecessary logic to allow empty commit when branch changes
Closed	mjacob	D8724 commit: check branch change on to-be-committed changeset

Diff 21880

hgext/rebase.py

	)			)
	% (max(destancestors), b', '.join(b"%d" % p for p in sorted(parents)))			% (max(destancestors), b', '.join(b"%d" % p for p in sorted(parents)))
	)			)


	def commitmemorynode(repo, wctx, editor, extra, user, date, commitmsg):			def commitmemorynode(repo, wctx, editor, extra, user, date, commitmsg):
	'''Commit the memory changes with parents p1 and p2.			'''Commit the memory changes with parents p1 and p2.
	Return node of committed revision.'''			Return node of committed revision.'''
	# Replicates the empty check in ``repo.commit``.			# FIXME: make empty commit check consistent with ``repo.commit``
	if wctx.isempty() and not repo.ui.configbool(b'ui', b'allowemptycommit'):			if wctx.nofilechanges() and not repo.ui.configbool(
				b'ui', b'allowemptycommit'
				):
	return None			return None

	# By convention, ``extra['branch']`` (set by extrafn) clobbers			# By convention, ``extra['branch']`` (set by extrafn) clobbers
	# ``branch`` (used when passing ``--keepbranches``).			# ``branch`` (used when passing ``--keepbranches``).
	branch = None			branch = None
	if b'branch' in extra:			if b'branch' in extra:
	branch = extra[b'branch']			branch = extra[b'branch']

mercurial/context.py

	return self.tomemctx(			return self.tomemctx(
	text=precursor.description(),			text=precursor.description(),
	branch=precursor.branch(),			branch=precursor.branch(),
	extra=extra,			extra=extra,
	date=precursor.date(),			date=precursor.date(),
	user=precursor.user(),			user=precursor.user(),
	)			)

	def isdirty(self, path):			def isdirty(self, path):
	return path in self._cache			return path in self._cache

	def isempty(self):			def nofilechanges(self):
	# We need to discard any keys that are actually clean before the empty			# We need to discard any keys that are actually clean before the empty
	# commit check.			# commit check.
	self._compact()			self._compact()
	return len(self._cache) == 0			return len(self._cache) == 0

	def clean(self):			def clean(self):
	self._cache = {}			self._cache = {}
				martinvonzUnsubmitted Not Done I feel like it would be better to find better names for these three methods together since they're about the same concept. Maybe simply renaming this one to `isclean()` matches well? The only issue I see is that Mercurial usually skips the "is" prefix and "clean" is both a verb and an adjective, so it's not obvious at first why there's both `clean()` and `isclean()` (people familiar with Mercurial naming might think they're the same). Probably still okay? martinvonz: I feel like it would be better to find better names for these three methods together since…
				mjacobAuthorUnsubmitted Done To be honest, I didn’t think too hard about this name (at long as it’s more correct as the old name), as the method will become unused and removed a few patches later. If you still think we should rename it for the in-between steps, I’d be fine renaming it. mjacob: To be honest, I didn’t think too hard about this name (at long as it’s more correct as the old…
				martinvonzUnsubmitted Not Done No, that's fine. I had just not realized that it was going away in a later patch. martinvonz: No, that's fine. I had just not realized that it was going away in a later patch.

	def _compact(self):			def _compact(self):
	"""Removes keys from the cache that are actually clean, by comparing			"""Removes keys from the cache that are actually clean, by comparing
	them with the underlying context.			them with the underlying context.

	This can occur during the merge process, e.g. by passing --tool :local			This can occur during the merge process, e.g. by passing --tool :local
	to resolve a conflict.			to resolve a conflict.
	"""			"""