Patchwork D2933: repository: define interface for local repositories

login
register
mail settings
Submitter phabricator
Date March 22, 2018, 4:10 a.m.
Message ID <differential-rev-PHID-DREV-rlskhx6skqcntkcpzi6u-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29771/
State Superseded
Headers show

Comments

phabricator - March 22, 2018, 4:10 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Per discussions on the mailing list and at the 4.4 and 4.6 sprints,
  we want to start defining interfaces for local repository primitives
  so that we a) have a better idea of what the formal interface for
  various types is b) can more easily introduce alternate implementations
  of various components (e.g. in Rust).
  
  We have previously implemented interfaces that declare the peer and
  wire protocol APIs using the abc module.
  
  This commit introduces a monolithic interface for the localrepository
  class. It uses zope.interface - not abc - for defining and declaring
  the interface.
  
  The newly defined "completelocalrepository" interface is objectively
  horrible. It is based on what is actually in localrepository and
  doesn't represent a reasonable interface definition IMO. There's lots
  of... unwanted garbage in the interface. In other words, it reflects
  the horrible state of the localrepository "god object." But this is
  fine: a goal of this commit is to get the interface defined so that
  we have an interface. Future commits can refactor the interface
  into sub-interfaces, remove unwanted public attributes, etc.
  
  I attempted to define reasonable docstrings for the various interface
  members. But there are so many of them and I didn't know what some are
  used for. So I was lazy in a number of places and didn't write
  docstrings or detailed usage docs.
  
  Also, the members of the interface are defined in the order they are
  declared in localrepo.py. This revealed that the grouping of things
  in localrepo.py is... odd.
  
  The localrepository class now declares that it implements our newly
  defined interface. Unlike abc, zope.interface doesn't check interface
  conformance at type creation time (abc uses __metaclass__ magic to
  validate interface conformance when a type is created - usually at
  module import time). It does provide some functions for validating
  class and object conformance with declared interfaces. We add these
  checks to test-check-interfaces.py. We /could/ validate at run-time.
  But we hold off for now. (I'm a bit scared of doing that because of
  the various ways extensions monkeypatch repo instances.)
  
  After this commit, test-check-interfaces.py will fail if the set of
  public attributes on the localrepository class or instances change
  without corresponding updates to the interface. This is by design.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/localrepo.py
  mercurial/repository.py
  tests/test-check-interfaces.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 29, 2018, 5:04 p.m.
durin42 added inline comments.

INLINE COMMENTS

> repository.py:273
> +
> +class completelocalrepository(zi.Interface):
> +    """Monolithic interface for local repositories.

I'm unclear: are all the methods supposed to be abstract, or are some of them default implementations?

> repository.py:484
> +
> +    def cancopy():
> +        pass

Is this really the right default impl?

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - March 30, 2018, 5:17 p.m.
indygreg marked 2 inline comments as done.
indygreg added inline comments.

INLINE COMMENTS

> durin42 wrote in repository.py:273
> I'm unclear: are all the methods supposed to be abstract, or are some of them default implementations?

In `zope.interface`, the interface declaration is its own type which is completely independent from the implementation. This is different from `abc`, where the implementation inherits the abstract base class.

This means you can't have default implementations in the interface. Also, there is no `self` argument on methods. That's kind of wonky and confuses my IDE. But it does work.

> durin42 wrote in repository.py:484
> Is this really the right default impl?

No. I just didn't feel like adding a docstring because I didn't feel like researching what this is used for :)

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: durin42, mercurial-devel

Patch

diff --git a/tests/test-check-interfaces.py b/tests/test-check-interfaces.py
--- a/tests/test-check-interfaces.py
+++ b/tests/test-check-interfaces.py
@@ -2,16 +2,27 @@ 
 
 from __future__ import absolute_import, print_function
 
+import os
+
+from mercurial.thirdparty.zope import (
+    interface as zi,
+)
+from mercurial.thirdparty.zope.interface import (
+    verify as ziverify,
+)
 from mercurial import (
     bundlerepo,
     httppeer,
     localrepo,
+    repository,
     sshpeer,
     statichttprepo,
     ui as uimod,
     unionrepo,
 )
 
+rootdir = os.path.normpath(os.path.join(os.path.dirname(__file__), '..'))
+
 def checkobject(o):
     """Verify a constructed object conforms to interface rules.
 
@@ -41,6 +52,30 @@ 
         print('public attributes not in abstract interface: %s.%s' % (
             name, attr))
 
+def checkzobject(o):
+    """Verify an object with a zope interface."""
+    ifaces = zi.providedBy(o)
+    if not ifaces:
+        print('%r does not provide any zope interfaces' % o)
+        return
+
+    # Run zope.interface's built-in verification routine. This verifies that
+    # everything that is supposed to be present is present.
+    for iface in ifaces:
+        ziverify.verifyObject(iface, o)
+
+    # Now verify that the object provides no extra public attributes that
+    # aren't declared as part of interfaces.
+    allowed = set()
+    for iface in ifaces:
+        allowed |= set(iface.names(all=True))
+
+    public = {a for a in dir(o) if not a.startswith('_')}
+
+    for attr in sorted(public - allowed):
+        print('public attribute not declared in interfaces: %s.%s' % (
+            o.__class__.__name__, attr))
+
 # Facilitates testing localpeer.
 class dummyrepo(object):
     def __init__(self):
@@ -68,6 +103,8 @@ 
 
 def main():
     ui = uimod.ui()
+    # Needed so we can open a local repo with obsstore without a warning.
+    ui.setconfig('experimental', 'evolution.createmarkers', True)
 
     checkobject(badpeer())
     checkobject(httppeer.httppeer(None, None, None, dummyopener()))
@@ -80,4 +117,9 @@ 
     checkobject(statichttprepo.statichttppeer(dummyrepo()))
     checkobject(unionrepo.unionpeer(dummyrepo()))
 
+    ziverify.verifyClass(repository.completelocalrepository,
+                         localrepo.localrepository)
+    repo = localrepo.localrepository(ui, rootdir)
+    checkzobject(repo)
+
 main()
diff --git a/mercurial/repository.py b/mercurial/repository.py
--- a/mercurial/repository.py
+++ b/mercurial/repository.py
@@ -10,6 +10,9 @@ 
 import abc
 
 from .i18n import _
+from .thirdparty.zope import (
+    interface as zi,
+)
 from . import (
     error,
 )
@@ -266,3 +269,360 @@ 
 
 class legacypeer(peer, _baselegacywirecommands):
     """peer but with support for legacy wire protocol commands."""
+
+class completelocalrepository(zi.Interface):
+    """Monolithic interface for local repositories.
+
+    This currently captures the reality of things - not how things should be.
+    """
+
+    supportedformats = zi.Attribute(
+        """Set of requirements that apply to stream clone.
+
+        This is actually a class attribute and is shared among all instances.
+        """)
+
+    openerreqs = zi.Attribute(
+        """Set of requirements that are passed to the opener.
+
+        This is actually a class attribute and is shared among all instances.
+        """)
+
+    supported = zi.Attribute(
+        """Set of requirements that this repo is capable of opening.""")
+
+    requirements = zi.Attribute(
+        """Set of requirements this repo uses.""")
+
+    filtername = zi.Attribute(
+        """Name of the repoview that is active on this repo.""")
+
+    wvfs = zi.Attribute(
+        """VFS used to access the working directory.""")
+
+    vfs = zi.Attribute(
+        """VFS rooted at the .hg directory.
+
+        Used to access repository data not in the store.
+        """)
+
+    svfs = zi.Attribute(
+        """VFS rooted at the store.
+
+        Used to access repository data in the store. Typically .hg/store.
+        But can point elsewhere if the store is shared.
+        """)
+
+    root = zi.Attribute(
+        """Path to the root of the working directory.""")
+
+    path = zi.Attribute(
+        """Path to the .hg directory.""")
+
+    origroot = zi.Attribute(
+        """The filesystem path that was used to construct the repo.""")
+
+    auditor = zi.Attribute(
+        """A pathauditor for the working directory.
+
+        This checks if a path refers to a nested repository.
+
+        Operates on the filesystem.
+        """)
+
+    nofsauditor = zi.Attribute(
+        """A pathauditor for the working directory.
+
+        This is like ``auditor`` except it doesn't do filesystem checks.
+        """)
+
+    baseui = zi.Attribute(
+        """Original ui instance passed into constructor.""")
+
+    ui = zi.Attribute(
+        """Main ui instance for this instance.""")
+
+    sharedpath = zi.Attribute(
+        """Path to the .hg directory of the repo this repo was shared from.""")
+
+    store = zi.Attribute(
+        """A store instance.""")
+
+    spath = zi.Attribute(
+        """Path to the store.""")
+
+    sjoin = zi.Attribute(
+        """Alias to self.store.join.""")
+
+    cachevfs = zi.Attribute(
+        """A VFS used to access the cache directory.
+
+        Typically .hg/cache.
+        """)
+
+    filteredrevcache = zi.Attribute(
+        """Holds sets of revisions to be filtered.""")
+
+    names = zi.Attribute(
+        """A ``namespaces`` instance.""")
+
+    def close():
+        """Close the handle on this repository."""
+
+    def peer():
+        """Obtain an object conforming to the ``peer`` interface."""
+
+    def unfiltered():
+        """Obtain an unfiltered/raw view of this repo."""
+
+    def filtered(name, visibilityexceptions=None):
+        """Obtain a named view of this repository."""
+
+    obsstore = zi.Attribute(
+        """A store of obsolescence data.""")
+
+    changelog = zi.Attribute(
+        """A handle on the changelog revlog.""")
+
+    manifestlog = zi.Attribute(
+        """A handle on the root manifest revlog.""")
+
+    dirstate = zi.Attribute(
+        """Working directory state.""")
+
+    narrowpats = zi.Attribute(
+        """Matcher patterns for this repository's narrowspec.""")
+
+    def narrowmatch():
+        """Obtain a matcher for the narrowspec."""
+
+    def setnarrowpats(newincludes, newexcludes):
+        """Define the narrowspec for this repository."""
+
+    def __getitem__(changeid):
+        """Try to resolve a changectx."""
+
+    def __contains__(changeid):
+        """Whether a changeset exists."""
+
+    def __nonzero__():
+        """Always returns True."""
+        return True
+
+    __bool__ = __nonzero__
+
+    def __len__():
+        """Returns the number of changesets in the repo."""
+
+    def __iter__():
+        """Iterate over revisions in the changelog."""
+
+    def revs(expr, *args):
+        """Evaluate a revset.
+
+        Emits revisions.
+        """
+
+    def set(expr, *args):
+        """Evaluate a revset.
+
+        Emits changectx instances.
+        """
+
+    def anyrevs(specs, user=False, localalias=None):
+        """Find revisions matching one of the given revsets."""
+
+    def url():
+        """Returns a string representing the location of this repo."""
+
+    def hook(name, throw=False, **args):
+        """Call a hook."""
+
+    def tags():
+        """Return a mapping of tag to node."""
+
+    def tagtype(tagname):
+        """Return the type of a given tag."""
+
+    def tagslist():
+        """Return a list of tags ordered by revision."""
+
+    def nodetags(node):
+        """Return the tags associated with a node."""
+
+    def nodebookmarks(node):
+        """Return the list of bookmarks pointing to the specified node."""
+
+    def branchmap():
+        """Return a mapping of branch to heads in that branch."""
+
+    def revbranchcache():
+        pass
+
+    def branchtip(branchtip, ignoremissing=False):
+        """Return the tip node for a given branch."""
+
+    def lookup(key):
+        """Resolve the node for a revision."""
+
+    def lookupbranch(key, remote=None):
+        """Look up the branch name of the given revision or branch name."""
+
+    def known(nodes):
+        """Determine whether a series of nodes is known.
+
+        Returns a list of bools.
+        """
+
+    def local():
+        """Whether the repository is local."""
+        return True
+
+    def publishing():
+        """Whether the repository is a publishing repository."""
+
+    def cancopy():
+        pass
+
+    def shared():
+        """The type of shared repository or None."""
+
+    def wjoin(f, *insidef):
+        """Calls self.vfs.reljoin(self.root, f, *insidef)"""
+
+    def file(f):
+        """Obtain a filelog for a tracked path."""
+
+    def changectx(changeid):
+        """Obtains a changectx for a revision.
+
+        Identical to __getitem__.
+        """
+
+    def setparents(p1, p2):
+        """Set the parent nodes of the working directory."""
+
+    def filectx(path, changeid=None, fileid=None):
+        """Obtain a filectx for the given file revision."""
+
+    def getcwd():
+        """Obtain the current working directory from the dirstate."""
+
+    def pathto(f, cwd=None):
+        """Obtain the relative path to a file."""
+
+    def adddatafilter(name, fltr):
+        pass
+
+    def wread(filename):
+        """Read a file from wvfs, using data filters."""
+
+    def wwrite(filename, data, flags, backgroundclose=False, **kwargs):
+        """Write data to a file in the wvfs, using data filters."""
+
+    def wwritedata(filename, data):
+        """Resolve data for writing to the wvfs, using data filters."""
+
+    def currenttransaction():
+        """Obtain the current transaction instance or None."""
+
+    def transaction(desc, report=None):
+        """Open a new transaction to write to the repository."""
+
+    def undofiles():
+        """Returns a list of (vfs, path) for files to undo transactions."""
+
+    def recover():
+        """Roll back an interrupted transaction."""
+
+    def rollback(dryrun=False, force=False):
+        """Undo the last transaction.
+
+        DANGEROUS.
+        """
+
+    def updatecaches(tr=None, full=False):
+        """Warm repo caches."""
+
+    def invalidatecaches():
+        """Invalidate cached data due to the repository mutating."""
+
+    def invalidatevolatilesets():
+        pass
+
+    def invalidatedirstate():
+        """Invalidate the dirstate."""
+
+    def invalidate(clearfilecache=False):
+        pass
+
+    def invalidateall():
+        pass
+
+    def lock(wait=True):
+        """Lock the repository store and return a lock instance."""
+
+    def wlock(wait=True):
+        """Lock the non-store parts of the repository."""
+
+    def currentwlock():
+        """Return the wlock if it's held or None."""
+
+    def checkcommitpatterns(wctx, vdirs, match, status, fail):
+        pass
+
+    def commit(text='', user=None, date=None, match=None, force=False,
+               editor=False, extra=None):
+        """Add a new revision to the repository."""
+
+    def commitctx(ctx, error=False):
+        """Commit a commitctx instance to the repository."""
+
+    def destroying():
+        """Inform the repository that nodes are about to be destroyed."""
+
+    def destroyed():
+        """Inform the repository that nodes have been destroyed."""
+
+    def status(node1='.', node2=None, match=None, ignored=False,
+               clean=False, unknown=False, listsubrepos=False):
+        """Convenience method to call repo[x].status()."""
+
+    def addpostdsstatus(ps):
+        pass
+
+    def postdsstatus():
+        pass
+
+    def clearpostdsstatus():
+        pass
+
+    def heads(start=None):
+        """Obtain list of nodes that are DAG heads."""
+
+    def branchheads(branch=None, start=None, closed=False):
+        pass
+
+    def branches(nodes):
+        pass
+
+    def between(pairs):
+        pass
+
+    def checkpush(pushop):
+        pass
+
+    prepushoutgoinghooks = zi.Attribute(
+        """util.hooks instance.""")
+
+    def pushkey(namespace, key, old, new):
+        pass
+
+    def listkeys(namespace):
+        pass
+
+    def debugwireargs(one, two, three=None, four=None, five=None):
+        pass
+
+    def savecommitmessage(text):
+        pass
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -20,6 +20,9 @@ 
     nullid,
     short,
 )
+from .thirdparty.zope import (
+    interface as zi,
+)
 from . import (
     bookmarks,
     branchmap,
@@ -314,6 +317,7 @@ 
 # set to reflect that the extension knows how to handle that requirements.
 featuresetupfuncs = set()
 
+@zi.implementer(repository.completelocalrepository)
 class localrepository(object):
 
     # obsolete experimental requirements: