This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1527

context: add an abstract base class for filectx
AbandonedPublic

Authored by phillco on Nov 27 2017, 10:35 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Summary

basectx is very well and good, but we have two other filectx-like classes
that do not inherit from it (arbitraryfilectx and absentfilectx`), and
cannot, because basectx is still rather tied to representing a file in a
changectx.

Let's add some structure to these classes with a mininum set of abstract
methods enforced by the ABC module.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Nov 27 2017, 10:35 PM
phillco added a reviewer: martinvonz.Nov 27 2017, 10:36 PM

Note that we can't actually make absentfilectx based off this class today because of the context -> merge imprt cycle.

martinvonz added a comment.Dec 1 2017, 2:43 PM
In D1527#25742, @phillco wrote:

Note that we can't actually make absentfilectx based off this class today because of the context -> merge imprt cycle.

What exactly is that cycle? Can we break one of the links with a local import?

mercurial/context.py
761

For a separate patch: I find it weird to refer to these undefined fields (_repo and _path). Can we pass these into the constructor (which is currently not defined) and assign them there?

2625

Can we put them in abstractfilectx if they seems generic enough? We can still created optimized versions in subclasses.

2652

Maybe in a separate patch, you could make this just "return self.data()"?

phillco updated this revision to Diff 4175.Dec 7 2017, 4:22 PM
phillco added a comment.Dec 7 2017, 4:41 PM

What exactly is that cycle? Can we break one of the links with a local import?

It's the same filectx -> ctx -> fileset -> merge -> filemerge cycle that frustrated earlier. We can't circumvent it this time (I think) because the import is in the module, not a function.

phillco planned changes to this revision.Dec 11 2017, 3:28 PM
phillco abandoned this revision.Jan 21 2018, 11:22 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (67 lines)
Mmercurial/filemerge.py (12 lines)

Diff 4175

mercurial/context.py

# context.py - changeset and file context objects for mercurial # context.py - changeset and file context objects for mercurial
# #
# Copyright 2006, 2007 Matt Mackall <mpm@selenic.com> # Copyright 2006, 2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import abc
import errno import errno
import filecmp import filecmp
import os import os
import re import re
import stat import stat
from .i18n import _ from .i18n import _
from .node import ( from .node import (
match.bad(fn, _('no such file in rev %s') % self) match.bad(fn, _('no such file in rev %s') % self)
m = matchmod.badmatch(match, bad) m = matchmod.badmatch(match, bad)
return self._manifest.walk(m) return self._manifest.walk(m)
def matches(self, match): def matches(self, match):
return self.walk(match) return self.walk(match)
class basefilectx(object): class abstractfilectx(object):
"""Defines the mininum required methods for filectxs"""
__metaclass__ = abc.ABCMeta
@abc.abstractmethod
def data(self): pass
@abc.abstractmethod
def cmp(self, other): pass
@abc.abstractmethod
def changectx(self): pass
@abc.abstractmethod
def flags(self): pass
@abc.abstractmethod
def path(self): pass
@abc.abstractmethod
def size(self): pass
@abc.abstractmethod
def isbinary(self): pass
@abc.abstractmethod
def islink(self): pass
@abc.abstractmethod
def isexec(self): pass
@abc.abstractmethod
def isabsent(self): pass
@abc.abstractmethod
def write(self, data, flags): pass
@abc.abstractmethod
def setflags(self, l, x): pass
class basefilectx(abstractfilectx):
"""A filecontext object represents the common logic for its children: """A filecontext object represents the common logic for its children:
filectx: read-only access to a filerevision that is already present filectx: read-only access to a filerevision that is already present
in the repo, in the repo,
workingfilectx: a filecontext that represents files from the working workingfilectx: a filecontext that represents files from the working
directory, directory,
memfilectx: a filecontext that represents files in-memory, memfilectx: a filecontext that represents files in-memory,
overlayfilectx: duplicate another filecontext with some fields overridden. overlayfilectx: duplicate another filecontext with some fields overridden.
""" """
@propertycache @propertycache
def _filelog(self): def _filelog(self):
return self._repo.file(self._path) return self._repo.file(self._path)
martinvonzUnsubmitted
Not Done

For a separate patch: I find it weird to refer to these undefined fields (_repo and _path). Can we pass these into the constructor (which is currently not defined) and assign them there?

martinvonz: For a separate patch: I find it weird to refer to these undefined fields (_repo and _path). Can…
@propertycache @propertycache
def _changeid(self): def _changeid(self):
if r'_changeid' in self.__dict__: if r'_changeid' in self.__dict__:
return self._changeid return self._changeid
elif r'_changectx' in self.__dict__: elif r'_changectx' in self.__dict__:
return self._changectx.rev() return self._changectx.rev()
elif r'_descendantrev' in self.__dict__: elif r'_descendantrev' in self.__dict__:
return renamed return renamed
def children(self): def children(self):
# hard for renames # hard for renames
c = self._filelog.children(self._filenode) c = self._filelog.children(self._filenode)
return [filectx(self._repo, self._path, fileid=x, return [filectx(self._repo, self._path, fileid=x,
filelog=self._filelog) for x in c] filelog=self._filelog) for x in c]
def setflags(self, l, x):
raise error.ProgrammingError("filectx is immutable")
def write(self, data, flags):
raise error.ProgrammingError("filectx is immutable")
class committablectx(basectx): class committablectx(basectx):
"""A committablectx object provides common functionality for a context that """A committablectx object provides common functionality for a context that
wants the ability to commit, e.g. workingctx or memctx.""" wants the ability to commit, e.g. workingctx or memctx."""
def __init__(self, repo, text="", user=None, date=None, extra=None, def __init__(self, repo, text="", user=None, date=None, extra=None,
changes=None): changes=None):
self._repo = repo self._repo = repo
self._rev = None self._rev = None
self._node = None self._node = None
"""wraps unlink for a repo's working directory""" """wraps unlink for a repo's working directory"""
# need to figure out what to do here # need to figure out what to do here
del self._changectx[self._path] del self._changectx[self._path]
def write(self, data, flags): def write(self, data, flags):
"""wraps repo.wwrite""" """wraps repo.wwrite"""
self._data = data self._data = data
def setflags(self, l, x):
raise NotImplementedError
class overlayfilectx(committablefilectx): class overlayfilectx(committablefilectx):
"""Like memfilectx but take an original filectx and optional parameters to """Like memfilectx but take an original filectx and optional parameters to
override parts of it. This is useful when fctx.data() is expensive (i.e. override parts of it. This is useful when fctx.data() is expensive (i.e.
flag processor is expensive) and raw data, flags, and filenode could be flag processor is expensive) and raw data, flags, and filenode could be
reused (ex. rebase or mode-only amend a REVIDX_EXTSTORED file). reused (ex. rebase or mode-only amend a REVIDX_EXTSTORED file).
""" """
def __init__(self, originalfctx, datafunc=None, path=None, flags=None, def __init__(self, originalfctx, datafunc=None, path=None, flags=None,
added.append(f) added.append(f)
elif f in self: elif f in self:
modified.append(f) modified.append(f)
else: else:
removed.append(f) removed.append(f)
return scmutil.status(modified, added, removed, [], [], [], []) return scmutil.status(modified, added, removed, [], [], [], [])
class arbitraryfilectx(object): class arbitraryfilectx(abstractfilectx):
"""Allows you to use filectx-like functions on a file in an arbitrary """Allows you to use filectx-like functions on a file in an arbitrary
location on disk, possibly not in the working directory. location on disk, possibly not in the working directory.
""" """
def __init__(self, path, repo=None): def __init__(self, path, repo=None):
# Repo is optional because contrib/simplemerge uses this class. # Repo is optional because contrib/simplemerge uses this class.
self._repo = repo self._repo = repo
self._path = path self._path = path
def cmp(self, fctx): def cmp(self, fctx):
# filecmp follows symlinks whereas `cmp` should not, so skip the fast # filecmp follows symlinks whereas `cmp` should not, so skip the fast
# path if either side is a symlink. # path if either side is a symlink.
symlinks = ('l' in self.flags() or 'l' in fctx.flags()) symlinks = ('l' in self.flags() or 'l' in fctx.flags())
if not symlinks and isinstance(fctx, workingfilectx) and self._repo: if not symlinks and isinstance(fctx, workingfilectx) and self._repo:
# Add a fast-path for merge if both sides are disk-backed. # Add a fast-path for merge if both sides are disk-backed.
# Note that filecmp uses the opposite return values (True if same) # Note that filecmp uses the opposite return values (True if same)
# from our cmp functions (True if different). # from our cmp functions (True if different).
return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path())) return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))
return self.data() != fctx.data() return self.data() != fctx.data()
def changectx(self):
raise error.ProgrammingError("an arbitraryfilectx cannot have a "
"changectx")
def isabsent(self):
return False
# TODO deduplicate these from ``basefilectx``
martinvonzUnsubmitted
Not Done

Can we put them in abstractfilectx if they seems generic enough? We can still created optimized versions in subclasses.

martinvonz: Can we put them in abstractfilectx if they seems generic enough? We can still created optimized…
def isbinary(self):
try:
return util.binary(self.data())
except IOError:
return False
def isexec(self):
return 'x' in self.flags()
def islink(self):
return 'l' in self.flags()
def size(self):
return os.stat(self._path).st_size
def path(self): def path(self):
return self._path return self._path
def flags(self): def flags(self):
return '' return ''
def setflags(self, l, x):
raise NotImplementedError
def data(self): def data(self):
return util.readfile(self._path) return util.readfile(self._path)
def decodeddata(self): def decodeddata(self):
with open(self._path, "rb") as f: with open(self._path, "rb") as f:
martinvonzUnsubmitted
Not Done

Maybe in a separate patch, you could make this just "return self.data()"?

martinvonz: Maybe in a separate patch, you could make this just "return self.data()"?
return f.read() return f.read()
def remove(self): def remove(self):
util.unlink(self._path) util.unlink(self._path)
def write(self, data, flags): def write(self, data, flags):
assert not flags assert not flags
with open(self._path, "w") as f: with open(self._path, "w") as f:
f.write(data) f.write(data)

mercurial/filemerge.py

return self._ctx return self._ctx
def isbinary(self): def isbinary(self):
return False return False
def isabsent(self): def isabsent(self):
return True return True
def isexec(self):
return False
def islink(self):
return False
def setflags(self, l, x):
raise error.ProgrammingError("absentfilectx is immutable")
def write(self, data, flags):
raise error.ProgrammingError("absentfilectx is immutable")
def _findtool(ui, tool): def _findtool(ui, tool):
if tool in internals: if tool in internals:
return tool return tool
return findexternaltool(ui, tool) return findexternaltool(ui, tool)
def findexternaltool(ui, tool): def findexternaltool(ui, tool):
for kn in ("regkey", "regkeyalt"): for kn in ("regkey", "regkeyalt"):
k = _toolstr(ui, tool, kn) k = _toolstr(ui, tool, kn)