This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2933

repository: define interface for local repositories
ClosedPublic

Authored by indygreg on Mar 22 2018, 12:09 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG0dfb5672f015: repository: define interface for local repositories
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 22 2018, 12:09 AM
durin42 added a subscriber: durin42.Mar 29 2018, 1:04 PM
durin42 added inline comments.
mercurial/repository.py
273

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

484

Is this really the right default impl?

indygreg marked 2 inline comments as done.Mar 30 2018, 1:17 PM
indygreg added inline comments.
mercurial/repository.py
273

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.

484

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

indygreg marked 2 inline comments as done.Mar 30 2018, 2:13 PM
Closed by commit rHG0dfb5672f015: repository: define interface for local repositories (authored by indygreg). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 7246

mercurial/localrepo.py

import weakref import weakref
from .i18n import _ from .i18n import _
from .node import ( from .node import (
hex, hex,
nullid, nullid,
short, short,
) )
from .thirdparty.zope import (
interface as zi,
)
from . import ( from . import (
bookmarks, bookmarks,
branchmap, branchmap,
bundle2, bundle2,
changegroup, changegroup,
changelog, changelog,
color, color,
context, context,
# the ability to load repositories with custom requirements. Only # the ability to load repositories with custom requirements. Only
# functions defined in loaded extensions are called. # functions defined in loaded extensions are called.
# #
# The function receives a set of requirement strings that the repository # The function receives a set of requirement strings that the repository
# is capable of opening. Functions will typically add elements to the # is capable of opening. Functions will typically add elements to the
# set to reflect that the extension knows how to handle that requirements. # set to reflect that the extension knows how to handle that requirements.
featuresetupfuncs = set() featuresetupfuncs = set()
@zi.implementer(repository.completelocalrepository)
class localrepository(object): class localrepository(object):
# obsolete experimental requirements: # obsolete experimental requirements:
# - manifestv2: An experimental new manifest format that allowed # - manifestv2: An experimental new manifest format that allowed
# for stem compression of long paths. Experiment ended up not # for stem compression of long paths. Experiment ended up not
# being successful (repository sizes went up due to worse delta # being successful (repository sizes went up due to worse delta
# chains), and the code was deleted in 4.6. # chains), and the code was deleted in 4.6.
supportedformats = { supportedformats = {

mercurial/repository.py

# repository.py - Interfaces and base classes for repositories and peers. # repository.py - Interfaces and base classes for repositories and peers.
# #
# Copyright 2017 Gregory Szorc <gregory.szorc@gmail.com> # Copyright 2017 Gregory Szorc <gregory.szorc@gmail.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 abc
from .i18n import _ from .i18n import _
from .thirdparty.zope import (
interface as zi,
)
from . import ( from . import (
error, error,
) )
class _basepeer(object): class _basepeer(object):
"""Represents a "connection" to a repository. """Represents a "connection" to a repository.
This is the base interface for representing a connection to a repository. This is the base interface for representing a connection to a repository.
return return
raise error.CapabilityError( raise error.CapabilityError(
_('cannot %s; remote repository does not support the %r ' _('cannot %s; remote repository does not support the %r '
'capability') % (purpose, name)) 'capability') % (purpose, name))
class legacypeer(peer, _baselegacywirecommands): class legacypeer(peer, _baselegacywirecommands):
"""peer but with support for legacy wire protocol commands.""" """peer but with support for legacy wire protocol commands."""
class completelocalrepository(zi.Interface):
durin42Unsubmitted
Done

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

durin42: I'm unclear: are all the methods supposed to be abstract, or are some of them default…
indygregAuthorUnsubmitted
Not Done

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.

indygreg: In `zope.interface`, the interface declaration is its own type which is completely independent…
"""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():
durin42Unsubmitted
Done

Is this really the right default impl?

durin42: Is this really the right default impl?
indygregAuthorUnsubmitted
Not Done

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

indygreg: No. I just didn't feel like adding a docstring because I didn't feel like researching what this…
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

tests/test-check-interfaces.py

# Test that certain objects conform to well-defined interfaces. # Test that certain objects conform to well-defined interfaces.
from __future__ import absolute_import, print_function 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 ( from mercurial import (
bundlerepo, bundlerepo,
httppeer, httppeer,
localrepo, localrepo,
repository,
sshpeer, sshpeer,
statichttprepo, statichttprepo,
ui as uimod, ui as uimod,
unionrepo, unionrepo,
) )
rootdir = os.path.normpath(os.path.join(os.path.dirname(__file__), '..'))
def checkobject(o): def checkobject(o):
"""Verify a constructed object conforms to interface rules. """Verify a constructed object conforms to interface rules.
An object must have __abstractmethods__ defined. An object must have __abstractmethods__ defined.
All "public" attributes of the object (attributes not prefixed with All "public" attributes of the object (attributes not prefixed with
an underscore) must be in __abstractmethods__ or appear on a base class an underscore) must be in __abstractmethods__ or appear on a base class
with __abstractmethods__. with __abstractmethods__.
return return
public = {a for a in dir(o) if not a.startswith('_')} public = {a for a in dir(o) if not a.startswith('_')}
for attr in sorted(public - allowed): for attr in sorted(public - allowed):
print('public attributes not in abstract interface: %s.%s' % ( print('public attributes not in abstract interface: %s.%s' % (
name, attr)) 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. # Facilitates testing localpeer.
class dummyrepo(object): class dummyrepo(object):
def __init__(self): def __init__(self):
self.ui = uimod.ui() self.ui = uimod.ui()
def filtered(self, name): def filtered(self, name):
pass pass
def _restrictcapabilities(self, caps): def _restrictcapabilities(self, caps):
pass pass
pass pass
class dummypipe(object): class dummypipe(object):
def close(self): def close(self):
pass pass
def main(): def main():
ui = uimod.ui() 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(badpeer())
checkobject(httppeer.httppeer(None, None, None, dummyopener())) checkobject(httppeer.httppeer(None, None, None, dummyopener()))
checkobject(localrepo.localpeer(dummyrepo())) checkobject(localrepo.localpeer(dummyrepo()))
checkobject(sshpeer.sshv1peer(ui, 'ssh://localhost/foo', None, dummypipe(), checkobject(sshpeer.sshv1peer(ui, 'ssh://localhost/foo', None, dummypipe(),
dummypipe(), None, None)) dummypipe(), None, None))
checkobject(sshpeer.sshv2peer(ui, 'ssh://localhost/foo', None, dummypipe(), checkobject(sshpeer.sshv2peer(ui, 'ssh://localhost/foo', None, dummypipe(),
dummypipe(), None, None)) dummypipe(), None, None))
checkobject(bundlerepo.bundlepeer(dummyrepo())) checkobject(bundlerepo.bundlepeer(dummyrepo()))
checkobject(statichttprepo.statichttppeer(dummyrepo())) checkobject(statichttprepo.statichttppeer(dummyrepo()))
checkobject(unionrepo.unionpeer(dummyrepo())) checkobject(unionrepo.unionpeer(dummyrepo()))
ziverify.verifyClass(repository.completelocalrepository,
localrepo.localrepository)
repo = localrepo.localrepository(ui, rootdir)
checkzobject(repo)
main() main()