Patchwork D1527: context: add an abstract base class for filectx

login
register
mail settings
Submitter phabricator
Date Nov. 28, 2017, 3:36 a.m.
Message ID <differential-rev-PHID-DREV-tsn7vk7x5v77x44mtldy-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25779/
State Superseded
Headers show

Comments

phabricator - Nov. 28, 2017, 3:36 a.m.
phillco created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1527

AFFECTED FILES
  mercurial/context.py
  mercurial/filemerge.py

CHANGE DETAILS




To: phillco, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 28, 2017, 3:37 a.m.
phillco added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1527

To: phillco, #hg-reviewers, martinvonz
Cc: mercurial-devel
phabricator - Dec. 1, 2017, 7:44 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/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?

INLINE COMMENTS

> context.py:761
>      def _filelog(self):
>          return self._repo.file(self._path)
>  

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?

> context.py:2646
> +
> +    # TODO deduplicate these from ``basefilectx``
> +    def isbinary(self):

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

> context.py:2673
>      def decodeddata(self):
>          with open(self._path, "rb") as f:
>              return f.read()

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1527

To: phillco, #hg-reviewers, martinvonz
Cc: mercurial-devel
phabricator - Dec. 7, 2017, 9:41 p.m.
phillco added a comment.


  > 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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1527

To: phillco, #hg-reviewers, martinvonz
Cc: mercurial-devel

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -104,6 +104,18 @@ 
     def isabsent(self):
         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):
     if tool in internals:
         return tool
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -7,6 +7,7 @@ 
 
 from __future__ import absolute_import
 
+import abc
 import errno
 import filecmp
 import os
@@ -717,7 +718,36 @@ 
     def matches(self, 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:
     filectx: read-only access to a filerevision that is already present
              in the repo,
@@ -1302,6 +1332,12 @@ 
         return [filectx(self._repo, self._path, fileid=x,
                         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):
     """A committablectx object provides common functionality for a context that
     wants the ability to commit, e.g. workingctx or memctx."""
@@ -2408,6 +2444,9 @@ 
         """wraps repo.wwrite"""
         self._data = data
 
+    def setflags(self, l, x):
+        raise NotImplementedError
+
 class overlayfilectx(committablefilectx):
     """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.
@@ -2577,7 +2616,7 @@ 
 
         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
     location on disk, possibly not in the working directory.
     """
@@ -2597,12 +2636,36 @@ 
             return not filecmp.cmp(self.path(), self._repo.wjoin(fctx.path()))
         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``
+    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):
         return self._path
 
     def flags(self):
         return ''
 
+    def setflags(self, l, x):
+        raise NotImplementedError
+
     def data(self):
         return util.readfile(self._path)