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.
Details
- Reviewers
ryanmce - Group Reviewers
Restricted Project - Commits
- rFBHGX886a701c9350: treedirstate: dirstatemap.identity should be a property
Diff Detail
- Repository
- rFBHGX Facebook Mercurial Extensions
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
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.
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.