( )⚙ D1340 dirstate: add explicit methods for modifying dirstate

This is an archive of the discontinued Mercurial Phabricator instance.

dirstate: add explicit methods for modifying dirstate
ClosedPublic

Authored by mbthomas on Nov 8 2017, 12:31 PM.

Details

Summary

Instead of assigning dirstatetuple objects to entries in the dirstate, move
responsibility for creating tuples into the dirstatemap.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbthomas created this revision.Nov 8 2017, 12:31 PM
mbolin added a subscriber: mbolin.Nov 9 2017, 8:00 PM
mbolin added inline comments.
mercurial/dirstate.py
1314

To avoid doing two lookups in self._map:

try:
    self._map.pop(f)
    return True
except KeyError:
    return False
durin42 requested changes to this revision.Nov 10 2017, 5:06 PM
durin42 added a subscriber: durin42.
durin42 added inline comments.
mercurial/dirstate.py
130

Why is ~all the interesting content of this docstring removed? Where is the meaning of the dirstate map documented other than this docstring?

1196–1235

Per my comment above, let's insert a patch before this one that documents in more detail the API that dirstatemap is supposed to be providing, probably in a docstring on this class. Sound reasonable?

(Then the above docstring I complained about could point to this docstring.)

This revision now requires changes to proceed.Nov 10 2017, 5:06 PM
mbthomas added inline comments.Nov 11 2017, 7:59 AM
mercurial/dirstate.py
1196–1235

Agreed. I removed the docstring above because it became a lie (we no longer implement __setitem__ or __delitem__, so it's not really a dict any more). I will add a docstring here on Monday.

mbthomas added inline comments.Nov 11 2017, 8:04 AM
mercurial/dirstate.py
1314

Good idea. I think return self._map.pop(f, None) is not None would avoid two lookups and also the exception. This function's going to be getting more complex in later diffs, so I'd like to avoid an exception handler if possible.

mbthomas updated this revision to Diff 3437.Nov 13 2017, 7:18 AM
durin42 accepted this revision.Nov 13 2017, 5:42 PM
This revision is now accepted and ready to land.Nov 13 2017, 5:42 PM
mbthomas updated this revision to Diff 3514.Nov 15 2017, 4:08 AM
durin42 accepted this revision.Nov 17 2017, 5:22 PM
This revision was automatically updated to reflect the committed changes.