This is an archive of the discontinued Mercurial Phabricator instance.

Differential D616

context: add overlayworkingcontext and overlayworkingfilectx
ClosedPublic

Authored by phillco on Sep 3 2017, 3:41 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGf698bb31bdfb: context: add overlayworkingcontext and overlayworkingfilectx
Summary

These two classes will be used extensively in the first in-memory merge
milestone.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Sep 3 2017, 3:41 PM
martinvonz added a subscriber: martinvonz.Sep 7 2017, 1:46 PM
martinvonz added inline comments.
mercurial/context.py
1987–1989

It sounds like a shorter version of

If `exists` is True, either `data` or `flags` must be non-None (either both, or just `flags`)

is

If `exists` is True, `flags` must be non-None
2007

maybe these IOErrors should be ProgrammingError?

2029–2034

looks like this can be both simpler and more efficient if written with _markdirty() like this:

def _markdirty(self, path, flags=None, exists=True, data=None, date=None)
2035

a little unnecessary :-)

2064

can reduce some repetition by extracting something like

entry = self._cache[path]
if entry['exists'] is True:
    data = entry['data']
    flags = entry['flags']
...
2078

doesn't seem like this can happen. i'd drop this and the "is True/False" in the conditions above

2085

should this be "private"? seems kind of risky to call it outside of flushall()

2093

you're not setting date here, which seems inconsistent

2094

i think no flags are usually expressed as empty string, not None, so i think we should at least make sure the return value of flags() follows that (and the simplest way of doing that is probably to change this line)

2103

This can also be written "self._wrappedctx[path]", which seems similarly short and clear, so I'm not sure this method buys us much

2126

Why is it safe to call lexists() here? I don't know how exists() is used, so it's not clear to me. Maybe add a comment (if it actually is safe)

2145–2149

Unrelated to this patch, but this interface seems pretty uglily asymmetric (flags() returns a string of character flags, setflags() take two boolean flags). And that's why you're forced to make the flag handling in your setflags() above ugly.

quark added a subscriber: quark.Sep 8 2017, 2:18 PM

This is where I guess it might be implemented differently. Storing fctx in overlayworkingctx._cache may be possible. It allows us to have some fast path for LFS use-case (ex. skip reading .data() but reuse .rawdata()). It could also simplify read methods by proxying them to getattr(_underlyingctx(path), methodname).

That said, I'm fine with this being committed as is. I'll follow up later once we really need the LFS fast path.

phillco marked 10 inline comments as done.Sep 10 2017, 11:54 PM
phillco updated this revision to Diff 1699.
phillco updated this revision to Diff 1705.Sep 11 2017, 1:22 AM
phillco updated this revision to Diff 1706.Sep 11 2017, 1:25 AM
phillco updated this revision to Diff 1707.
phillco updated this revision to Diff 1708.Sep 11 2017, 1:39 AM
martinvonz added inline comments.Sep 11 2017, 1:55 PM
mercurial/context.py
2027

use flags='' here too and you can drop the check below

2030–2035

forgot to update this section to use _markdirty(path, data=data, flags=flags, date=util.makedate())

2038

drop "exists=True" since it's the default (or remove the default value from the method if you prefer)

2053

I've heard of symlinks that have a trailing newline. I wonder if we need to strip that here.

2099

nit: move "exists" before "flags" since it always makes sense and it determines whether flags+data+date make sense. would be nice to match the order in the dict below

2099–2111
if path not in self._cache:
    self._writeorder.append(path)
self._cache[path] = {
        'exists': exists,
        'data': data,
        'date': date,
        'flags': flags,
}
phillco marked 6 inline comments as done.Sep 11 2017, 4:02 PM
phillco updated this revision to Diff 1723.
martinvonz added inline comments.Sep 11 2017, 5:39 PM
mercurial/context.py
2006

test-check-pyflakes.t fails with

+  mercurial/context.py:2009: undefined name 'ProgrammingError'
+  mercurial/context.py:2025: undefined name 'ProgrammingError'
+  mercurial/context.py:2068: undefined name 'ProgrammingError'

I'll qualify it in flight

phillco added inline comments.Sep 11 2017, 6:14 PM
mercurial/context.py
2006

Ah, thanks. Didn't have pyflakes installed on my test box, as it turned out...

2145–2149

Yes, it is. Possibly worth a future refactor.

Closed by commit rHGf698bb31bdfb: context: add overlayworkingcontext and overlayworkingfilectx (authored by phillco). · Explain WhySep 11 2017, 7:41 PM
This revision was automatically updated to reflect the committed changes.
durin42 added a subscriber: durin42.Sep 12 2017, 11:10 AM

This is already pushed, but please consider breaking up context.py into a package in the near future - it's far too large to comprehend, and I think it'd help organization if we had each of the various file{ctx} types (mem, working, overlay) in their own file and then re-exported in mercurial/context/__init__.py (so no other code would have to change, and external-to-hg clients of memctx (of which there are many) could be ignorant of the move).

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (180 lines)

Diff 1592

mercurial/context.py

""" """
wvfs = self._repo.wvfs wvfs = self._repo.wvfs
if wvfs.isdir(self._path) and not wvfs.islink(self._path): if wvfs.isdir(self._path) and not wvfs.islink(self._path):
wvfs.removedirs(self._path) wvfs.removedirs(self._path)
def setflags(self, l, x): def setflags(self, l, x):
self._repo.wvfs.setflags(self._path, l, x) self._repo.wvfs.setflags(self._path, l, x)
class overlayworkingctx(workingctx):
"""Wraps another mutable context with a write-back cache that can be flushed
at a later time.
self._cache[path] maps to a dict with keys: {
'exists': bool?
'date': date?
'data': str?
'flags': str?
}
If `exists` is True, either `data` or `flags` must be non-None (either both,
or just `flags`), and 'date' is non-None. If it is `False`, the file was
deleted.
martinvonzUnsubmitted
Done

It sounds like a shorter version of

If `exists` is True, either `data` or `flags` must be non-None (either both, or just `flags`)

is

If `exists` is True, `flags` must be non-None
martinvonz: It sounds like a shorter version of If `exists` is True, either `data` or `flags` must be…
"""
def __init__(self, repo, wrappedctx):
super(overlayworkingctx, self).__init__(repo)
self._repo = repo
self._wrappedctx = wrappedctx
self.clean()
def data(self, path):
if self.isdirty(path):
if self._cache[path]['exists']:
if self._cache[path]['data']:
return self._cache[path]['data']
else:
# Must fallback here, too, because we only set flags.
return self._underlyingctx(path).data()
else:
martinvonzUnsubmitted
Not Done

test-check-pyflakes.t fails with

+  mercurial/context.py:2009: undefined name 'ProgrammingError'
+  mercurial/context.py:2025: undefined name 'ProgrammingError'
+  mercurial/context.py:2068: undefined name 'ProgrammingError'

I'll qualify it in flight

martinvonz: test-check-pyflakes.t fails with + mercurial/context.py:2009: undefined name…
phillcoAuthorUnsubmitted
Not Done

Ah, thanks. Didn't have pyflakes installed on my test box, as it turned out...

phillco: Ah, thanks. Didn't have pyflakes installed on my test box, as it turned out...
raise IOError("No such file or directory: %s" % self._path)
martinvonzUnsubmitted
Done

maybe these IOErrors should be ProgrammingError?

martinvonz: maybe these IOErrors should be ProgrammingError?
else:
return self._underlyingctx(path).data()
def filedate(self, path):
if self.isdirty(path):
return self._cache[path]['date']
else:
return self._underlyingctx(path).date()
def flags(self, path):
if self.isdirty(path):
if self._cache[path]['exists']:
return self._cache[path]['flags']
else:
raise IOError("No such file or directory: %s" % self._path)
else:
return self._underlyingctx(path).flags()
def write(self, path, data, flags=None):
if data is None:
martinvonzUnsubmitted
Done

use flags='' here too and you can drop the check below

martinvonz: use flags='' here too and you can drop the check below
raise error.ProgrammingError("data must be non-None")
self._markdirty(path)
self._cache[path]['exists'] = True
self._cache[path]['data'] = data
self._cache[path]['date'] = util.makedate()
if flags is not None:
self._cache[path]['flags'] = flags
martinvonzUnsubmitted
Done

looks like this can be both simpler and more efficient if written with _markdirty() like this:

def _markdirty(self, path, flags=None, exists=True, data=None, date=None)
martinvonz: looks like this can be both simpler and more efficient if written with _markdirty() like this…
pass
martinvonzUnsubmitted
Done

a little unnecessary :-)

martinvonz: a little unnecessary :-)
martinvonzUnsubmitted
Done

forgot to update this section to use _markdirty(path, data=data, flags=flags, date=util.makedate())

martinvonz: forgot to update this section to use _markdirty(path, data=data, flags=flags, date=util.
def setflags(self, path, l, x):
self._markdirty(path)
martinvonzUnsubmitted
Done

drop "exists=True" since it's the default (or remove the default value from the method if you prefer)

martinvonz: drop "exists=True" since it's the default (or remove the default value from the method if you…
self._cache[path]['exists'] = True
self._cache[path]['date'] = util.makedate()
self._cache[path]['flags'] = (l and 'l' or '') + (x and 'x' or '')
def remove(self, path):
self._markdirty(path)
self._cache[path]['exists'] = False
self._cache[path]['data'] = None
self._cache[path]['flags'] = None
def exists(self, path):
if self.isdirty(path):
return self._cache[path]['exists']
return self._underlyingctx(path).exists()
martinvonzUnsubmitted
Done

I've heard of symlinks that have a trailing newline. I wonder if we need to strip that here.

martinvonz: I've heard of symlinks that have a trailing newline. I wonder if we need to strip that here.
def size(self, path):
if self.isdirty(path):
if self._cache[path]['exists']:
return len(self._cache[path]['data'])
else:
raise IOError("No such file or directory: %s" % self._path)
return self._underlyingctx(path).size()
def flushall(self):
for path in self._writeorder:
if self._cache[path]['exists'] is True:
martinvonzUnsubmitted
Done

can reduce some repetition by extracting something like

entry = self._cache[path]
if entry['exists'] is True:
    data = entry['data']
    flags = entry['flags']
...
martinvonz: can reduce some repetition by extracting something like entry = self._cache[path] if entry…
self._underlyingctx(path).clearunknown()
if self._cache[path]['data'] is not None:
if self._cache[path]['flags'] is None:
raise error.ProgrammingError('data set but not flags')
self._underlyingctx(path).write(
self._cache[path]['data'],
self._cache[path]['flags'])
else:
self._underlyingctx(path).setflags(
'l' in self._cache[path]['flags'],
'x' in self._cache[path]['flags'])
elif self._cache[path]['exists'] is False:
self._underlyingctx(path).remove(path)
else:
martinvonzUnsubmitted
Done

doesn't seem like this can happen. i'd drop this and the "is True/False" in the conditions above

martinvonz: doesn't seem like this can happen. i'd drop this and the "is True/False" in the conditions above
continue
self.clean()
def isdirty(self, path):
return path in self._cache
def clean(self):
martinvonzUnsubmitted
Done

should this be "private"? seems kind of risky to call it outside of flushall()

martinvonz: should this be "private"? seems kind of risky to call it outside of flushall()
self._cache = {}
self._writeorder = []
def _markdirty(self, path):
if path not in self._cache:
self._cache[path] = {
'exists': None,
'data': None,
martinvonzUnsubmitted
Done

you're not setting date here, which seems inconsistent

martinvonz: you're not setting date here, which seems inconsistent
'flags': None,
martinvonzUnsubmitted
Not Done

i think no flags are usually expressed as empty string, not None, so i think we should at least make sure the return value of flags() follows that (and the simplest way of doing that is probably to change this line)

martinvonz: i think no flags are usually expressed as empty string, not None, so i think we should at least…
}
self._writeorder.append(path)
def filectx(self, path, filelog=None):
return overlayworkingfilectx(self._repo, path, parent=self,
martinvonzUnsubmitted
Done

nit: move "exists" before "flags" since it always makes sense and it determines whether flags+data+date make sense. would be nice to match the order in the dict below

martinvonz: nit: move "exists" before "flags" since it always makes sense and it determines whether…
filelog=filelog)
def _underlyingctx(self, path):
return self._wrappedctx.filectx(path)
martinvonzUnsubmitted
Done

This can also be written "self._wrappedctx[path]", which seems similarly short and clear, so I'm not sure this method buys us much

martinvonz: This can also be written "self._wrappedctx[path]", which seems similarly short and clear, so…
class overlayworkingfilectx(workingfilectx):
"""Wrap a ``workingfilectx`` but intercepts all writes into an in-memory
cache, which can be flushed through later by calling ``flush()``."""
def __init__(self, repo, path, filelog=None, parent=None):
super(overlayworkingfilectx, self).__init__(repo, path, filelog,
parent)
martinvonzUnsubmitted
Done
if path not in self._cache:
    self._writeorder.append(path)
self._cache[path] = {
        'exists': exists,
        'data': data,
        'date': date,
        'flags': flags,
}
martinvonz: if path not in self._cache: self._writeorder.append(path) self._cache[path] = {…
self._repo = repo
self._parent = parent
self._path = path
def ctx(self):
return self._parent
def data(self):
return self._parent.data(self._path)
def date(self):
return self._parent.filedate(self._path)
def exists(self):
return self.lexists()
martinvonzUnsubmitted
Not Done

Why is it safe to call lexists() here? I don't know how exists() is used, so it's not clear to me. Maybe add a comment (if it actually is safe)

martinvonz: Why is it safe to call lexists() here? I don't know how exists() is used, so it's not clear to…
def lexists(self):
return self._parent.exists(self._path)
def renamed(self):
# Copies are currently tracked in the dirstate as before. Straight copy
# from workingfilectx.
rp = self._repo.dirstate.copied(self._path)
if not rp:
return None
return rp, self._changectx._parents[0]._manifest.get(rp, nullid)
def size(self):
return self._parent.size(self._path)
def audit(self):
pass
def flags(self):
return self._parent.flags(self._path)
def setflags(self, islink, isexec):
return self._parent.setflags(self._path, islink, isexec)
martinvonzUnsubmitted
Done

Unrelated to this patch, but this interface seems pretty uglily asymmetric (flags() returns a string of character flags, setflags() take two boolean flags). And that's why you're forced to make the flag handling in your setflags() above ugly.

martinvonz: Unrelated to this patch, but this interface seems pretty uglily asymmetric (flags() returns a…
phillcoAuthorUnsubmitted
Not Done

Yes, it is. Possibly worth a future refactor.

phillco: Yes, it is. Possibly worth a future refactor.
def write(self, data, flags, backgroundclose=False):
return self._parent.write(self._path, data, flags)
def remove(self, ignoremissing=False):
return self._parent.remove(self._path)
class workingcommitctx(workingctx): class workingcommitctx(workingctx):
"""A workingcommitctx object makes access to data related to """A workingcommitctx object makes access to data related to
the revision being committed convenient. the revision being committed convenient.
This hides changes in the working directory, if they aren't This hides changes in the working directory, if they aren't
committed in this context. committed in this context.
""" """
def __init__(self, repo, changes, def __init__(self, repo, changes,