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.

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
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 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.

mbolin abandoned this revision.Dec 7 2017, 4:16 PM

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...