( )⚙ D1680 treedirstate: dirstatemap.identity should be a property

This is an archive of the discontinued Mercurial Phabricator instance.

treedirstate: dirstatemap.identity should be a property

Authored by mbthomas on Dec 13 2017, 12:25 PM.


Group Reviewers
Restricted Project
rFBHGX886a701c9350: treedirstate: dirstatemap.identity should be a property

The dirstatemap identity attribute should be a property, as the dirstate
accesses it this way. Currently it is using the bound method as if it were the
identity, which it is not.

Diff Detail

rFBHGX Facebook Mercurial Extensions
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Dec 13 2017, 12:25 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 13 2017, 12:25 PM
ryanmce requested changes to this revision.Dec 13 2017, 12:52 PM
ryanmce added a subscriber: ryanmce.

Can you add a test that demonstrates what this fixes?

This revision now requires changes to proceed.Dec 13 2017, 12:52 PM

I'm not actually sure what problem it would cause, I just know it's wrong. I think the only place we actually use identity is in deciding when to update the dirstate after a status call that discovers the dirstate isn't right (e.g. normallookup files can now be marked normal). Normally status doesn't have the lock, so it optimistically grabs the lock, but only does the update if the identity matches. I don't really have a good idea what the function would do when it's using the bound method rather than the real identity. Suggestions for a test for this race?

I don't think it's possible to reliably provoke the bad behaviour. What's happening here is we are using the bound identity method of the dirstatemap instance as the identity of the dirstate. This nearly always changes when the dirstate is recreated (which happens when the dirstate file changes on disk by virtue of dirstate being a filecache property and by localrepo.invalidatedirstate. The only time this will actually cause problems is if the new dirstatemap object ends up with the same address as the old one. In that case we will think the dirstate hasn't changed (as bound methods are compared using the combination of object address and method address), and potentially update using an old dirstate instance.

quark added a subscriber: quark.EditedDec 14 2017, 11:47 AM

Maybe write a small Python function and test repo.dirstate._map.identity is repo.dirstate._map.identity ?

That said, I also found it difficult to write a "proper" test here. The fix is clear. I think it's okay to not be blocked by a test.

mbthomas requested review of this revision.Dec 15 2017, 11:44 AM
ryanmce accepted this revision.Dec 15 2017, 11:45 AM

I've been convinced by all the text above that is is okay!

This revision is now accepted and ready to land.Dec 15 2017, 11:45 AM
This revision was automatically updated to reflect the committed changes.