( )⚙ D752 dirstate: create new dirstatemap class

This is an archive of the discontinued Mercurial Phabricator instance.

dirstate: create new dirstatemap class
ClosedPublic

Authored by durham on Sep 20 2017, 8:03 PM.

Details

Summary

This is part of a larger refactor to move the dirstate storage logic to a
separate class, so it's easier to rewrite the dirstate storage layer without
having to rewrite all the algorithms as well.

Step one it to create the class, and replace dirstate._map with it. The
abstraction bleeds through in a few places where the main dirstate class has to
access self._map._map, but those will be cleaned up in future patches.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durham created this revision.Sep 20 2017, 8:03 PM

This series is the first 8 commits of a 20 patch series. At the end all the storage for the dirstate goes through the dirstatemap class.

indygreg accepted this revision.Sep 24 2017, 11:07 AM
indygreg added a subscriber: indygreg.

Looks good. Nits on API can be addressed as a follow-up, if needed.

mercurial/dirstate.py
1345

Do we want this inherit from collections.MutableMapping?

A good reason to say "no" is KISS, especially if we'll be having multiple backends.

1349–1350

Given that iteritems() no longer exists in Python 3, should we change this to items() and standardize on the modern convention?

This revision is now accepted and ready to land.Sep 24 2017, 11:07 AM

In the spirit of full disclosure, I reviewed this series after a long-haul flight. I'm a little jet lagged and am not an expert in dirstate. This series seems like a pretty straightforward refactor to me, which is why I felt comfortable giving review. But if there be dragons in this code, an extra set of eyeballs may be warranted.

durham added inline comments.Sep 26 2017, 6:40 AM
mercurial/dirstate.py
1345

For now I'd say no, until we've done at least one other backend implementation and have seen that MutableMapping is still appropriate. In particular, we might not want to allow iteration without a matcher, to discourage O(working copy) operations.

1349–1350

Do we do that anywhere else in the code base already? My initial tendency is to just follow existing patterns, especially during a refactor, to minimize churn.

This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.
mercurial/dirstate.py
1345

I was wondering if all this delegation would be noticeably costly, but I couldn't think of a case where it is (no significant change in "hg files -0 | xargs -0 touch; time ~/hg/hg st", for example)