This is an archive of the discontinued Mercurial Phabricator instance.

Differential D758

dirstate: move parent reading to the dirstatemap class
ClosedPublic

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

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG0c3e3810cdb6: dirstate: move parent reading to the dirstatemap class
rHG949276a8cd76: dirstate: move parent reading to the dirstatemap class
Summary

As part of moving dirstate storage logic to a separate class, let's move the
function that reads the parents from the file. This will allow extensions to
write dirstate's that store the parents in other ways.

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

durham created this revision.Sep 20 2017, 8:03 PM
indygreg accepted this revision.Sep 24 2017, 11:25 AM
indygreg added a subscriber: indygreg.

Lots of comments. But nothing that should warrant blocking.

mercurial/dirstate.py
1376

Nit: this should ideally be in a context manager or try..finally block. But it existed this way before, so not worth blocking over.

1381

Another pre-existing poor error message because it isn't actionable :/

1382–1384

Idea for follow-up: a vfs layer that operates on pending files. Maybe we could pass pending=True somewhere? That way we can use vfs.tryread() instead of this low-level code. Not sure if we have enough consumers to warrant that though.

This revision is now accepted and ready to land.Sep 24 2017, 11:25 AM
durham updated this revision to Diff 2082.Sep 26 2017, 6:59 AM
durham added inline comments.Sep 26 2017, 7:00 AM
mercurial/dirstate.py
1376

Left this alone for now to avoid code churn during the refactor.

Closed by commit rHG949276a8cd76: dirstate: move parent reading to the dirstatemap class (authored by durham). · Explain WhySep 29 2017, 2:43 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/dirstate.py (28 lines)

Diff 2161

mercurial/dirstate.py

return self._opener.read("branch").strip() or "default" return self._opener.read("branch").strip() or "default"
except IOError as inst: except IOError as inst:
if inst.errno != errno.ENOENT: if inst.errno != errno.ENOENT:
raise raise
return "default" return "default"
@propertycache @propertycache
def _pl(self): def _pl(self):
try: return self._map.parents()
fp = self._map._opendirstatefile()
st = fp.read(40)
fp.close()
l = len(st)
if l == 40:
return st[:20], st[20:40]
elif l > 0 and l < 40:
raise error.Abort(_('working directory state appears damaged!'))
except IOError as err:
if err.errno != errno.ENOENT:
raise
return [nullid, nullid]
@propertycache @propertycache
def _dirs(self): def _dirs(self):
return self._map.dirs() return self._map.dirs()
def dirs(self): def dirs(self):
return self._dirs return self._dirs
fp, mode = txnutil.trypending(self._root, self._opener, self._filename) fp, mode = txnutil.trypending(self._root, self._opener, self._filename)
if self._pendingmode is not None and self._pendingmode != mode: if self._pendingmode is not None and self._pendingmode != mode:
fp.close() fp.close()
raise error.Abort(_('working directory state may be ' raise error.Abort(_('working directory state may be '
'changed parallelly')) 'changed parallelly'))
self._pendingmode = mode self._pendingmode = mode
return fp return fp
def parents(self):
try:
fp = self._opendirstatefile()
st = fp.read(40)
fp.close()
indygregUnsubmitted
Not Done

Nit: this should ideally be in a context manager or try..finally block. But it existed this way before, so not worth blocking over.

indygreg: Nit: this should ideally be in a context manager or try..finally block. But it existed this way…
durhamAuthorUnsubmitted
Not Done

Left this alone for now to avoid code churn during the refactor.

durham: Left this alone for now to avoid code churn during the refactor.
l = len(st)
if l == 40:
return st[:20], st[20:40]
elif l > 0 and l < 40:
raise error.Abort(_('working directory state appears damaged!'))
indygregUnsubmitted
Not Done

Another pre-existing poor error message because it isn't actionable :/

indygreg: Another pre-existing poor error message because it isn't actionable :/
except IOError as err:
if err.errno != errno.ENOENT:
raise
indygregUnsubmitted
Not Done

Idea for follow-up: a vfs layer that operates on pending files. Maybe we could pass pending=True somewhere? That way we can use vfs.tryread() instead of this low-level code. Not sure if we have enough consumers to warrant that though.

indygreg: Idea for follow-up: a vfs layer that operates on pending files. Maybe we could pass…
return [nullid, nullid]