This is an archive of the discontinued Mercurial Phabricator instance.

dirstate: change all writes to dirstatemap._map to go through one method
AbandonedPublic

Authored by mbolin on Nov 9 2017, 8:32 PM.

Download Raw Diff

Details

Reviewers

mbthomas
durham

Group Reviewers

hg-reviewers

Summary

This separates some concerns that were introduced in
https://phab.mercurial-scm.org/D1341.

In particular, this makes it easier for eden_dirstate to provide its own
implementation, which incidentally, does not use dirstatetuple.

Test Plan

Ran this against a complementary change in Eden and verified that all of Eden's
integration tests pass.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

mbolin created this revision.Nov 9 2017, 8:32 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 9 2017, 8:32 PM

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

mbolin retitled this revision from dirstate: change all writes to dirstatemap._map to go through one method. to dirstate: change all writes to dirstatemap._map to go through one method.Nov 9 2017, 8:48 PM

I'm not really happy with this. Why can't eden be replacing the dirstatemap entirely, and be intercepting things at that layer?

durin42 mentioned this in D1347: dirstate: make map implementation overridable.Nov 13 2017, 5:36 PM

@durin42 The issue is that dirstatemap is doing so much "stuff" beyond just storage that Eden prefers to subclass it rather than copy/paste all of the business logic.

In D1354#22991, @mbolin wrote:

@durin42 The issue is that dirstatemap is doing so much "stuff" beyond just storage that Eden prefers to subclass it rather than copy/paste all of the business logic.

That's an argument for further splitting dirstatemap. That said, does D1347 resolve the specific concern that motivated this change?

mbthomas updated this revision to Diff 3522.Nov 15 2017, 4:08 AM

Is this still relevant? Nobody's answered my question.

D1347 did not address the issue because we still want to subclass it in Eden. If you're curious, you can see exactly what we're doing in https://github.com/facebookexperimental/eden-hg/blob/master/eden/hg/eden/eden_dirstate_map.py.

Regardless, we'll find another way, so I'll abandon this.

If you want, I'd be happy to make the dirstatetuple factory a class-level attribute on the dirstatemap - I'm not quite following what the extra logic you need is, but I'd very much like to make Eden easier, you just need to help me understand the constraints better: I can't read your mind.

@durin42 Here's the implementation of this method we are using in Eden:

def _insert_tuple(self, filename, state, mode, size, mtime):  # override
    if size != MERGE_STATE_BOTH_PARENTS and size != MERGE_STATE_OTHER_PARENT:
        merge_state = MERGE_STATE_NOT_APPLICABLE
    else:
        merge_state = size

    self._map[filename] = (state, mode, merge_state)

As you can see, we want to do some conditional logic before doing the insert into self._map (and we end up dropping one of the tuple fields), so it seemed like the most straightforward thing for Eden to do is to intercept the writes. It's true that we could also redefine dirstatetuple(), but I think that's more confusing because there are places where we need to convert between Eden's internal tuples stored in eden_dirstate_map and the dirstatetuple type that Mercurial expects everywhere else, so I'm disinclined to conflate the two things.

In D1354#28409, @mbolin wrote:
@durin42 Here's the implementation of this method we are using in Eden:
def _insert_tuple(self, filename, state, mode, size, mtime):  # override
    if size != MERGE_STATE_BOTH_PARENTS and size != MERGE_STATE_OTHER_PARENT:
        merge_state = MERGE_STATE_NOT_APPLICABLE
    else:
        merge_state = size
    self._map[filename] = (state, mode, merge_state)
As you can see, we want to do some conditional logic before doing the insert into self._map (and we end up dropping one of the tuple fields), so it seemed like the most straightforward thing for Eden to do is to intercept the writes. It's true that we could also redefine dirstatetuple(), but I think that's more confusing because there are places where we need to convert between Eden's internal tuples stored in eden_dirstate_map and the dirstatetuple type that Mercurial expects everywhere else, so I'm disinclined to conflate the two things.

Ahhhh. That was extremely not obvious from your commit message. If it'd still help, I'd be happy to land this with a clearer log message and a comment on _insert_tuple that it's been extracted to ease the implementation of hg on FUSE filesystems and other similar use cases (is that a good summary?) so that it makes more sense to readers...

Revision Contents
Changeset List

			Path	Packages
M			mercurial/dirstate.py (9 lines)

Diff 3522

mercurial/dirstate.py

	self._map			self._map

	def addfile(self, f, oldstate, state, mode, size, mtime):			def addfile(self, f, oldstate, state, mode, size, mtime):
	"""Add a tracked file to the dirstate."""			"""Add a tracked file to the dirstate."""
	if oldstate in "?r" and "_dirs" in self.__dict__:			if oldstate in "?r" and "_dirs" in self.__dict__:
	self._dirs.addpath(f)			self._dirs.addpath(f)
	if oldstate == "?" and "_alldirs" in self.__dict__:			if oldstate == "?" and "_alldirs" in self.__dict__:
	self._alldirs.addpath(f)			self._alldirs.addpath(f)
	self._map[f] = dirstatetuple(state, mode, size, mtime)			self._insert_tuple(f, state, mode, size, mtime)
	if state != 'n' or mtime == -1:			if state != 'n' or mtime == -1:
	self.nonnormalset.add(f)			self.nonnormalset.add(f)
	if size == -2:			if size == -2:
	self.otherparentset.add(f)			self.otherparentset.add(f)

	def removefile(self, f, oldstate, size):			def removefile(self, f, oldstate, size):
	"""			"""
	Mark a file as removed in the dirstate.			Mark a file as removed in the dirstate.

	The `size` parameter is used to store sentinel values that indicate			The `size` parameter is used to store sentinel values that indicate
	the file's previous state. In the future, we should refactor this			the file's previous state. In the future, we should refactor this
	to be more explicit about what that state is.			to be more explicit about what that state is.
	"""			"""
	if oldstate not in "?r" and "_dirs" in self.__dict__:			if oldstate not in "?r" and "_dirs" in self.__dict__:
	self._dirs.delpath(f)			self._dirs.delpath(f)
	if oldstate == "?" and "_alldirs" in self.__dict__:			if oldstate == "?" and "_alldirs" in self.__dict__:
	self._alldirs.addpath(f)			self._alldirs.addpath(f)
	if "filefoldmap" in self.__dict__:			if "filefoldmap" in self.__dict__:
	normed = util.normcase(f)			normed = util.normcase(f)
	self.filefoldmap.pop(normed, None)			self.filefoldmap.pop(normed, None)
	self._map[f] = dirstatetuple('r', 0, size, 0)			self._insert_tuple(f, 'r', 0, size, 0)
	self.nonnormalset.add(f)			self.nonnormalset.add(f)

	def dropfile(self, f, oldstate):			def dropfile(self, f, oldstate):
	"""			"""
	Remove a file from the dirstate. Returns True if the file was			Remove a file from the dirstate. Returns True if the file was
	previously recorded.			previously recorded.
	"""			"""
	exists = self._map.pop(f, None) is not None			exists = self._map.pop(f, None) is not None
	if exists:			if exists:
	if oldstate != "r" and "_dirs" in self.__dict__:			if oldstate != "r" and "_dirs" in self.__dict__:
	self._dirs.delpath(f)			self._dirs.delpath(f)
	if "_alldirs" in self.__dict__:			if "_alldirs" in self.__dict__:
	self._alldirs.delpath(f)			self._alldirs.delpath(f)
	if "filefoldmap" in self.__dict__:			if "filefoldmap" in self.__dict__:
	normed = util.normcase(f)			normed = util.normcase(f)
	self.filefoldmap.pop(normed, None)			self.filefoldmap.pop(normed, None)
	self.nonnormalset.discard(f)			self.nonnormalset.discard(f)
	return exists			return exists

	def clearambiguoustimes(self, files, now):			def clearambiguoustimes(self, files, now):
	for f in files:			for f in files:
	e = self.get(f)			e = self.get(f)
	if e is not None and e[0] == 'n' and e[3] == now:			if e is not None and e[0] == 'n' and e[3] == now:
	self._map[f] = dirstatetuple(e[0], e[1], e[2], -1)			self._insert_tuple(f, e[0], e[1], e[2], -1)
	self.nonnormalset.add(f)			self.nonnormalset.add(f)

				def _insert_tuple(self, f, state, mode, size, mtime):
				self._map[f] = dirstatetuple(state, mode, size, mtime)

	def nonnormalentries(self):			def nonnormalentries(self):
	'''Compute the nonnormal dirstate entries from the dmap'''			'''Compute the nonnormal dirstate entries from the dmap'''
	try:			try:
	return parsers.nonnormalotherparententries(self._map)			return parsers.nonnormalotherparententries(self._map)
	except AttributeError:			except AttributeError:
	nonnorm = set()			nonnorm = set()
	otherparent = set()			otherparent = set()
	for fname, e in self._map.iteritems():			for fname, e in self._map.iteritems():

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	3383		Nov 9 2017, 8:32 PM	★	★
Diff 2	3522		Nov 15 2017, 4:08 AM	★	★