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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
1990–1992

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
2010

maybe these IOErrors should be ProgrammingError?

2032–2037

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)
2038

a little unnecessary :-)

2067

can reduce some repetition by extracting something like

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

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

2088

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

2096

you're not setting date here, which seems inconsistent

2097

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)

2106

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

2129

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)

2148–2152

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
2030

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

2033–2038

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

2041

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

2056

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

2102

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

2102–2114
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
2009

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
2009

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

2148–2152

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 (188 lines)

Diff 1744

mercurial/context.py

# Only a case insensitive filesystem needs magic to translate user input # Only a case insensitive filesystem needs magic to translate user input
# to actual case in the filesystem. # to actual case in the filesystem.
icasefs = not util.fscasesensitive(r.root) icasefs = not util.fscasesensitive(r.root)
return matchmod.match(r.root, r.getcwd(), pats, include, exclude, return matchmod.match(r.root, r.getcwd(), pats, include, exclude,
default, auditor=r.auditor, ctx=self, default, auditor=r.auditor, ctx=self,
listsubrepos=listsubrepos, badfn=badfn, listsubrepos=listsubrepos, badfn=badfn,
icasefs=icasefs) icasefs=icasefs)
def flushall(self):
pass # For overlayworkingfilectx compatibility.
def _filtersuspectsymlink(self, files): def _filtersuspectsymlink(self, files):
if not files or self._repo.dirstate._checklink: if not files or self._repo.dirstate._checklink:
return files return files
# Symlink placeholders may get non-symlink-like contents # Symlink placeholders may get non-symlink-like contents
# via user error or dereferencing by NFS or Samba servers, # via user error or dereferencing by NFS or Samba servers,
# so we filter out any placeholders that don't look like a # so we filter out any placeholders that don't look like a
# symlink # symlink
""" """
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, `flags` must be non-None 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._wrappedctx[path].data()
else:
raise error.ProgrammingError("No such file or directory: %s" %
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...
self._path)
martinvonzUnsubmitted
Done

maybe these IOErrors should be ProgrammingError?

martinvonz: maybe these IOErrors should be ProgrammingError?
else:
return self._wrappedctx[path].data()
def filedate(self, path):
if self.isdirty(path):
return self._cache[path]['date']
else:
return self._wrappedctx[path].date()
def flags(self, path):
if self.isdirty(path):
if self._cache[path]['exists']:
return self._cache[path]['flags']
else:
raise error.ProgrammingError("No such file or directory: %s" %
self._path)
else:
return self._wrappedctx[path].flags()
def write(self, path, data, flags=''):
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
if data is None:
raise error.ProgrammingError("data must be non-None")
self._markdirty(path, exists=True, data=data, date=util.makedate(),
flags=flags)
def setflags(self, path, l, x):
self._markdirty(path, exists=True, date=util.makedate(),
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…
flags=(l and 'l' or '') + (x and 'x' or ''))
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 remove(self, path):
self._markdirty(path, exists=False)
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…
def exists(self, path):
"""exists behaves like `lexists`, but needs to follow symlinks and
return False if they are broken.
"""
if self.isdirty(path):
# If this path exists and is a symlink, "follow" it by calling
# exists on the destination path.
if (self._cache[path]['exists'] and
'l' in self._cache[path]['flags']):
return self.exists(self._cache[path]['data'].strip())
else:
return self._cache[path]['exists']
return self._wrappedctx[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 lexists(self, path):
"""lexists returns True if the path exists"""
if self.isdirty(path):
return self._cache[path]['exists']
return self._wrappedctx[path].lexists()
def size(self, path):
if self.isdirty(path):
if self._cache[path]['exists']:
return len(self._cache[path]['data'])
else:
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…
raise error.ProgrammingError("No such file or directory: %s" %
self._path)
return self._wrappedctx[path].size()
def flushall(self):
for path in self._writeorder:
entry = self._cache[path]
if entry['exists']:
self._wrappedctx[path].clearunknown()
if entry['data'] is not None:
if entry['flags'] is None:
raise error.ProgrammingError('data set but not flags')
self._wrappedctx[path].write(
entry['data'],
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
entry['flags'])
else:
self._wrappedctx[path].setflags(
'l' in entry['flags'],
'x' in entry['flags'])
else:
self._wrappedctx[path].remove(path)
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._clean()
def isdirty(self, path):
return path in self._cache
def _clean(self):
self._cache = {}
self._writeorder = []
martinvonzUnsubmitted
Done

you're not setting date here, which seems inconsistent

martinvonz: you're not setting date here, which seems inconsistent
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…
def _markdirty(self, path, exists, data=None, date=None, flags=''):
if path not in self._cache:
self._writeorder.append(path)
self._cache[path] = {
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…
'exists': exists,
'data': data,
'date': date,
'flags': flags,
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…
}
def filectx(self, path, filelog=None):
return overlayworkingfilectx(self._repo, path, parent=self,
filelog=filelog)
class overlayworkingfilectx(workingfilectx):
"""Wrap a ``workingfilectx`` but intercepts all writes into an in-memory
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] = {…
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)
self._repo = repo
self._parent = parent
self._path = path
def ctx(self):
return self._parent
def data(self):
return self._parent.data(self._path)
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 date(self):
return self._parent.filedate(self._path)
def exists(self):
return self.lexists()
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
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 flags(self):
return self._parent.flags(self._path)
def setflags(self, islink, isexec):
return self._parent.setflags(self._path, islink, isexec)
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,