Merge lp:~jelmer/wikkid/dulwich into lp:wikkid

[Jelmer Vernooij]

Add a dulwich-based filestore backend to wikkid, which can be used for git repositories.

[lp:~jelmer/wikkid/dulwich]

76. By Jelmer Vernooij on 2013-09-02

Simplify commit, add --format=git option.

77. By Jelmer Vernooij on 2013-09-02

Fix user id.

[Jelmer Vernooij (jelmer)]

ping, anyone?

[Jelmer Vernooij (jelmer)]

ping (-:

[Jelmer Vernooij (jelmer)]

ping :-)

[Gavin Panella (allenap)]

I'm not sure that I'm really qualified to review this, but cosmic
radiation seems to have flipped a bit in Launchpad's database such that
I have been invited to pass judgement.

[1]

+            try:
+                (mode, sha) = tree[el]
+                if not stat.S_ISDIR(mode):
+                    raise FileExists(
+                        "File %s exists and is not a directory" % el)
+            except KeyError:
+                tree = Tree()
+            else:
+                tree = self.store[sha]

I'm guessing that you're not trying to guard against a KeyError in the
`if not stat.S_ISDIR(...` block, so it probably ought to go in the else:
block.

[2]

+                ret.append(
+                    File(self.store, mode, sha, posixpath.join(directory_path, name), commit_id))

In places you use "/" as the separator. Consider using posixpath.sep
for, I don't know, cleanliness?

[3]

Can you add dulwich as a dependency in setup.py?

[4]

There are a lot of new things here without obvious tests, like the
middleware, but given that there's little other development activity I
hardly want to block for that. In other words, you're very naughty for
not testing things, but there's no punishment, and thanks for the
contribution :)

review: Approve

[Jelmer Vernooij (jelmer)]

Thanks for the review, Gavin. Much appreciated! :-)

I'll follow up with some fixes when I get back from vacation.

[lp:~jelmer/wikkid/dulwich]

78. By Jelmer Vernooij on 2014-01-15

Merge trunk.

79. By Jelmer Vernooij on 2014-01-16

Add dulwich dependency.

80. By Jelmer Vernooij on 2014-01-16

Use posixpath.sep rather than /

81. By Jelmer Vernooij on 2014-01-16

Move extra code out of try/except.

[Jelmer Vernooij (jelmer)]

Thanks, I've now fixed the issues you pointed out.

[Tim Penhey (thumper)]

On 16/01/14 13:06, Jelmer Vernooij wrote:
> Thanks, I've now fixed the issues you pointed out.

Now I have to remember what this is all about and how to run the tests :-)

[Jelmer Vernooij (jelmer)]

On Thu, Jan 16, 2014 at 12:18:16AM -0000, Tim Penhey wrote:
> On 16/01/14 13:06, Jelmer Vernooij wrote:
> > Thanks, I've now fixed the issues you pointed out.
>
> Now I have to remember what this is all about and how to run the tests :-)

FWIW I get two failing tests, but those happen under trunk as well. :-)

[Jelmer Vernooij (jelmer)]

ping? :)

[Tim Penhey (thumper)]

Guys, I've been pretty crap at following up, but both Jelmer and Gavin have commit rights to trunk :-)

Trunk is owned by ~wikkid. Jelmer, you are good to merge.

review: Approve

[Jelmer Vernooij (jelmer)]

Thanks, Tim! Merged.

=== modified file 'bin/wikkid-serve'
--- bin/wikkid-serve	2012-05-17 09:32:49 +0000
+++ bin/wikkid-serve	2014-01-16 00:05:37 +0000
@@ -18,8 +18,6 @@
 import optparse
 import sys
 
-from bzrlib.workingtree import WorkingTree
-
 from wikkid import version
 from wikkid.app import WikkidApp
 from wikkid.context import (
@@ -28,8 +26,6 @@
     DEFAULT_PORT,
     ExecutionContext,
     )
-from wikkid.filestore.bzr import FileStore
-from wikkid.user.bzr import LocalBazaarUserMiddleware
 
 
 def setup_logging():
@@ -48,6 +44,8 @@
     usage = "Usage: %prog [options] <wiki-branch>"
     parser = optparse.OptionParser(
         usage=usage, description="Run a Wikkid Wiki server.", version=version)
+    parser.add_option('--format', type='choice', default='bzr',
+        choices=['bzr', 'git'], help=("Default repository format to use."))
     parser.add_option(
         '--host', type='string', default=DEFAULT_HOST,
         help=('The interface to listen on. Defaults to %r' % DEFAULT_HOST))
@@ -82,12 +80,24 @@
     logger = logging.getLogger('wikkid')
     logger.setLevel(logging.INFO)
 
-    working_tree = WorkingTree.open(branch)
-    logger.info('Using: %s', working_tree)
-    filestore = FileStore(working_tree)
+    if options.format == 'bzr':
+        from bzrlib.workingtree import WorkingTree
+        from wikkid.filestore.bzr import FileStore
+        from wikkid.user.bzr import LocalBazaarUserMiddleware
+
+        working_tree = WorkingTree.open(branch)
+        logger.info('Using: %s', working_tree)
+        filestore = FileStore(working_tree)
+    elif options.format == 'git':
+        from wikkid.filestore.git import FileStore
+        from wikkid.user.git import LocalGitUserMiddleware
+        filestore = FileStore.from_path(branch)
 
     app = WikkidApp(filestore=filestore, execution_context=execution_context)
-    app = LocalBazaarUserMiddleware(app, working_tree.branch)
+    if options.format == 'bzr':
+        app = LocalBazaarUserMiddleware(app, working_tree.branch)
+    elif options.format == 'git':
+        app = LocalGitUserMiddleware(app, filestore.repo)
     from wsgiref.simple_server import make_server
     httpd = make_server(options.host, options.port, app)
     logger.info('Serving on http://%s:%s', options.host, options.port)
 
=== modified file 'setup.py'
--- setup.py	2012-05-17 10:28:31 +0000
+++ setup.py	2014-01-16 00:05:37 +0000
@@ -24,6 +24,7 @@
     include_package_data=True,
     install_requires=[
         'docutils',
+        'dulwich',
         'jinja2',
         'pygments',
         'twisted',
 
=== modified file 'wikkid/dispatcher.py'
--- wikkid/dispatcher.py	2010-11-12 09:00:42 +0000
+++ wikkid/dispatcher.py	2014-01-16 00:05:37 +0000
@@ -53,7 +53,7 @@
         # Don't register.
         return
     key = (interface, view_name)
-    assert key not in _VIEW_REGISTRY, "key already registered: %r" % key
+    assert key not in _VIEW_REGISTRY, "key already registered: %r" % (key,)
     _VIEW_REGISTRY[key] = view_class
     if default_view:
         _VIEW_REGISTRY[(interface, None)] = view_class
 
=== modified file 'wikkid/filestore/basefile.py'
--- wikkid/filestore/basefile.py	2010-05-12 10:39:13 +0000
+++ wikkid/filestore/basefile.py	2014-01-16 00:05:37 +0000
@@ -16,9 +16,8 @@
 class BaseFile(object):
     """Provide common fields and methods and properties for files."""
 
-    def __init__(self, path, file_id):
+    def __init__(self, path):
         self.path = path
-        self.file_id = file_id
         self.base_name = urlutils.basename(path)
         self._mimetype = mimetypes.guess_type(self.base_name)[0]
 
 
=== modified file 'wikkid/filestore/bzr.py'
--- wikkid/filestore/bzr.py	2012-06-14 01:14:00 +0000
+++ wikkid/filestore/bzr.py	2014-01-16 00:05:37 +0000
@@ -243,7 +243,8 @@
     implements(IFile)
 
     def __init__(self, filestore, path, file_id):
-        BaseFile.__init__(self, path, file_id)
+        BaseFile.__init__(self, path)
+        self.file_id = file_id
         self.filestore = filestore
         # This isn't entirely necessary.
         self.tree = self.filestore.tree
 
=== added file 'wikkid/filestore/git.py'
--- wikkid/filestore/git.py	1970-01-01 00:00:00 +0000
+++ wikkid/filestore/git.py	2014-01-16 00:05:37 +0000
@@ -0,0 +1,205 @@
+#
+# Copyright (C) 2012 Wikkid Developers.
+#
+# This software is licensed under the GNU Affero General Public License
+# version 3 (see the file LICENSE).
+
+"""A git filestore using Dulwich.
+
+"""
+
+import datetime
+import mimetypes
+
+from dulwich.objects import Blob, Tree, ZERO_SHA
+from dulwich.object_store import tree_lookup_path
+from dulwich.repo import Repo
+from dulwich.walk import Walker
+import posixpath
+import stat
+
+from zope.interface import implements
+
+from wikkid.errors import FileExists, UpdateConflicts
+from wikkid.interface.filestore import FileType, IFile, IFileStore
+
+
+class FileStore(object):
+    """A filestore that just uses an internal map to store data."""
+
+    implements(IFileStore)
+
+    @classmethod
+    def from_path(cls, path):
+        return cls(Repo(path))
+
+    def __init__(self, repo, ref='HEAD'):
+        """Repo is a dulwich repository."""
+        self.repo = repo
+        self.ref = ref
+
+    @property
+    def store(self):
+        return self.repo.object_store
+
+    def _get_root(self, revision=None):
+        if revision is None:
+            try:
+                revision = self.repo.refs[self.ref]
+            except KeyError:
+                revision = ZERO_SHA
+        try:
+            return (revision, self.repo[revision].tree)
+        except KeyError:
+            return None, None
+
+    def get_file(self, path):
+        """Return an object representing the file."""
+        commit_id, root_id = self._get_root()
+        if root_id is None:
+            return None
+        try:
+            (mode, sha) = tree_lookup_path(self.store.__getitem__,
+                root_id, path)
+        except KeyError:
+            return None
+        return File(self.store, mode, sha, path, commit_id)
+
+    def update_file(self, path, content, user, parent_revision,
+                    commit_message=None):
+        """The `user` is updating the file at `path` with `content`."""
+        commit_id, root_id = self._get_root()
+        if root_id is None:
+            root_tree = Tree()
+        else:
+            root_tree = self.store[root_id]
+        # Find all tree objects involved
+        tree = root_tree
+        trees = [root_tree]
+        elements = path.strip(posixpath.sep).split(posixpath.sep)
+        for el in elements[:-1]:
+            try:
+                (mode, sha) = tree[el]
+            except KeyError:
+                tree = Tree()
+            else:
+                if not stat.S_ISDIR(mode):
+                    raise FileExists(
+                        "File %s exists and is not a directory" % el)
+                tree = self.store[sha]
+            trees.append(tree)
+        if elements[-1] in tree:
+            (old_mode, old_sha) = tree[elements[-1]]
+            if stat.S_ISDIR(old_mode):
+                raise FileExists("File %s exists and is a directory" % path)
+            if old_sha != parent_revision and parent_revision is not None:
+                raise UpdateConflicts("File conflict %s != %s" % (old_sha,
+                    parent_revision), old_sha)
+        blob = Blob.from_string(content.encode("utf-8"))
+        child = (stat.S_IFREG | 0644, blob.id)
+        self.store.add_object(blob)
+        assert len(trees) == len(elements)
+        for tree, name in zip(reversed(trees), reversed(elements)):
+            assert name != ""
+            tree[name] = child
+            self.store.add_object(tree)
+            child = (stat.S_IFDIR, tree.id)
+        if commit_message is None:
+            commit_message = ""
+        self.repo.do_commit(ref=self.ref, message=commit_message, author=user,
+            tree=tree.id)
+
+    def list_directory(self, directory_path):
+        """Return a list of File objects for in the directory path.
+
+        If the path doesn't exist, returns None.  If the path exists but is
+        empty, an empty list is returned.  Otherwise a list of File objects in
+        that directory.
+        """
+        if directory_path is None:
+            directory_path = ''
+        else:
+            directory_path = directory_path.strip(posixpath.sep)
+        commit_id, root_id = self._get_root()
+        if directory_path == '':
+            sha = root_id
+            mode = stat.S_IFDIR
+        else:
+            if root_id is None:
+                return None
+            try:
+                (mode, sha) = tree_lookup_path(self.store.__getitem__,
+                    root_id, directory_path)
+            except KeyError:
+                return None
+        if mode is not None and stat.S_ISDIR(mode):
+            ret = []
+            for (name, mode, sha) in self.store[sha].iteritems():
+                ret.append(
+                    File(self.store, mode, sha, posixpath.join(directory_path, name), commit_id))
+            return ret
+        else:
+            return None
+
+
+class File(object):
+    """A Git file object."""
+
+    implements(IFile)
+
+    def __init__(self, store, mode, sha, path, commit_sha):
+        self.store = store
+        self.mode = mode
+        self.sha = sha
+        self.path = path
+        self.commit_sha = commit_sha
+        self.base_name = posixpath.basename(path)
+        self.mimetype = mimetypes.guess_type(self.base_name)[0]
+
+    @property
+    def file_type(self):
+        """Work out the filetype based on the mimetype if possible."""
+        if self._is_directory:
+            return FileType.DIRECTORY
+        else:
+            if self.mimetype is None:
+                binary = self._is_binary
+            else:
+                binary = not self.mimetype.startswith('text/')
+            if binary:
+                return FileType.BINARY_FILE
+            else:
+                return FileType.TEXT_FILE
+
+    def get_content(self):
+        o = self.store[self.sha]
+        if isinstance(o, Blob):
+            return o.data
+        else:
+            return None
+
+    @property
+    def _is_directory(self):
+        return stat.S_ISDIR(self.mode)
+
+    @property
+    def _is_binary(self):
+        return '\0' in self.get_content()
+
+    def _get_last_modified_commit(self):
+        walker = Walker(self.store, include=[self.commit_sha],
+                paths=[self.path])
+        return iter(walker).next().commit
+
+    @property
+    def last_modified_in_revision(self):
+        return self.sha
+
+    @property
+    def last_modified_by(self):
+        return self._get_last_modified_commit().author
+
+    @property
+    def last_modified_date(self):
+        c = self._get_last_modified_commit()
+        return datetime.datetime.utcfromtimestamp(c.author_time)
 
=== modified file 'wikkid/filestore/volatile.py'
--- wikkid/filestore/volatile.py	2010-06-07 08:56:13 +0000
+++ wikkid/filestore/volatile.py	2014-01-16 00:05:37 +0000
@@ -113,7 +113,8 @@
     implements(IFile)
 
     def __init__(self, path, content, file_id, user):
-        BaseFile.__init__(self, path, file_id)
+        BaseFile.__init__(self, path)
+        self.file_id = file_id
         self.content = content
         self.last_modified_in_revision = None
         self.last_modified_by = user
 
=== modified file 'wikkid/interface/filestore.py'
--- wikkid/interface/filestore.py	2010-06-07 08:24:06 +0000
+++ wikkid/interface/filestore.py	2014-01-16 00:05:37 +0000
@@ -70,9 +70,6 @@
 
     base_name = Attribute("The last part of the path.")
 
-    file_id = Attribute(
-        "The unique identifier for the file in the filestore.")
-
     file_type = Attribute("Soon to be a Choice with a lazr.enum.")
 
     mimetype = Attribute(
 
=== modified file 'wikkid/tests/__init__.py'
--- wikkid/tests/__init__.py	2012-06-14 08:40:53 +0000
+++ wikkid/tests/__init__.py	2014-01-16 00:05:37 +0000
@@ -41,6 +41,7 @@
         'volatile_filestore',
         'bzr_filestore',
         'bzr_user',
+        'git_filestore',
         'view_dispatcher',
         'model',
         ]
 
=== modified file 'wikkid/tests/filestore.py'
--- wikkid/tests/filestore.py	2010-11-08 00:00:06 +0000
+++ wikkid/tests/filestore.py	2014-01-16 00:05:37 +0000
@@ -42,6 +42,7 @@
         filestore = self.make_filestore(
             [('README', 'Content'),
              ('lib/', None),
+             ('lib/foo', 'dummy data'),
              ('image.jpg', 'pretend image'),
              ('binary-file', 'a\0binary\0file'),
              ('simple.txt', 'A text file'),
@@ -57,6 +58,7 @@
         filestore = self.make_filestore(
             [('README', 'Content'),
              ('lib/', None),
+             ('lib/data', 'dummy data'),
              ('image.jpg', 'pretend image'),
              ('binary-file', 'a\0binary\0file'),
              ('simple.txt', 'A text file'),
 
=== added file 'wikkid/tests/test_git_filestore.py'
--- wikkid/tests/test_git_filestore.py	1970-01-01 00:00:00 +0000
+++ wikkid/tests/test_git_filestore.py	2014-01-16 00:05:37 +0000
@@ -0,0 +1,55 @@
+#
+# Copyright (C) 2012 Wikkid Developers.
+#
+# This software is licensed under the GNU Affero General Public License
+# version 3 (see the file LICENSE).
+
+"""Tests for the wikkid.filestore.git.FileStore."""
+
+from dulwich.repo import MemoryRepo
+
+from textwrap import dedent
+
+from wikkid.errors import UpdateConflicts
+from wikkid.filestore.git import (
+    FileStore,
+    )
+from wikkid.tests import ProvidesMixin, TestCase
+from wikkid.tests.filestore import TestFileStore
+
+
+class TestGitFileStore(TestCase, ProvidesMixin, TestFileStore):
+    """Tests for the git filestore and files."""
+
+    def make_filestore(self, contents=None):
+        repo = MemoryRepo()
+        fs = FileStore(repo)
+        if contents:
+            for (path, contents) in contents:
+                if contents is None:
+                    # Directory
+                    continue
+                fs.update_file(path, contents,
+                    user="Somebody <[e-mail]>",
+                    parent_revision=None,
+                    commit_message="Added by make_filestore")
+        return fs
+
+    def test_empty(self):
+        # Empty files do not have line endings, but they can be saved
+        # nonetheless.
+        filestore = self.make_filestore(
+            [('test.txt', 'several\nlines\nof\ncontent')])
+        f = filestore.get_file('test.txt')
+        base_rev = f.last_modified_in_revision
+        filestore.update_file(
+            'test.txt', '', 'Test Author <[e-mail]>', base_rev)
+        curr = filestore.get_file('test.txt')
+        self.assertEqual('', curr.get_content())
+
+    def test_listing_directory_empty(self):
+        filestore = self.make_filestore(
+            [('empty/', None),
+            ])
+        listing = filestore.list_directory('empty')
+        self.assertIs(None, listing)
 
=== added file 'wikkid/user/git.py'
--- wikkid/user/git.py	1970-01-01 00:00:00 +0000
+++ wikkid/user/git.py	2014-01-16 00:05:37 +0000
@@ -0,0 +1,74 @@
+#
+# Copyright (C) 2010 Wikkid Developers
+#
+# This software is licensed under the GNU Affero General Public License
+# version 3 (see the file LICENSE).
+
+"""A user factory and user class which uses the git identity from the
+local Git config."""
+
+import email
+import logging
+
+from webob import Request
+from zope.interface import implements
+
+from wikkid.interface.user import IUser, IUserFactory
+from wikkid.user.baseuser import BaseUser
+
+
+def create_git_user_from_author_string(author):
+    name, address = email.Utils.parseaddr(author)
+    if name:
+        display_name = name
+    else:
+        display_name = address
+    return User(address, display_name, author)
+
+
+class LocalGitUserMiddleware(object):
+    """A middleware to inject a user into the environment."""
+
+    def __init__(self, app, repo):
+        self.app = app
+        config = repo.get_config_stack()
+        email = config.get(("user", ), "email")
+        name = config.get(("user", ), "name")
+        self.user = User(email, name, "%s <%s>" % (name, email))
+
+    def __call__(self, environ, start_response):
+        environ['wikkid.user'] = self.user
+        req = Request(environ)
+        resp = req.get_response(self.app)
+        return resp(environ, start_response)
+
+
+class UserFactory(object):
+    """Generate a user from local bazaar config."""
+
+    implements(IUserFactory)
+
+    def __init__(self, branch):
+        """Use the user config from the branch."""
+        config = branch.get_config()
+        self.user = create_git_user_from_author_string(config.username())
+        logger = logging.getLogger('wikkid')
+        logger.info(
+            'Using git identity: "%s", "%s"',
+            self.user.display_name, self.user.email)
+
+    def create(self, request):
+        """Create a User."""
+        return self.user
+
+
+class User(BaseUser):
+    """A user from the local bazaar config."""
+
+    implements(IUser)
+
+    def __init__(self, email, display_name, committer_id):
+        self.email = email
+        self.display_name = display_name
+        self.committer_id = committer_id
+