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

Proposed by Jelmer Vernooij on 2013-09-02
Status: Merged
Merged at revision: 74
Proposed branch: lp:~jelmer/wikkid/dulwich
Merge into: lp:wikkid
Diff against target: 522 lines (+362/-16)
12 files modified
bin/wikkid-serve (+18/-8)
setup.py (+1/-0)
wikkid/dispatcher.py (+1/-1)
wikkid/filestore/basefile.py (+1/-2)
wikkid/filestore/bzr.py (+2/-1)
wikkid/filestore/git.py (+205/-0)
wikkid/filestore/volatile.py (+2/-1)
wikkid/interface/filestore.py (+0/-3)
wikkid/tests/__init__.py (+1/-0)
wikkid/tests/filestore.py (+2/-0)
wikkid/tests/test_git_filestore.py (+55/-0)
wikkid/user/git.py (+74/-0)
To merge this branch: bzr merge lp:~jelmer/wikkid/dulwich
Reviewer Review Type Date Requested Status
Tim Penhey 2014-01-16 Approve on 2014-04-07
Gavin Panella 2013-09-02 Approve on 2013-12-18
Review via email: mp+183373@code.launchpad.net

Description of the change

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

To post a comment you must log in.
lp:~jelmer/wikkid/dulwich updated on 2013-09-02
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) wrote :

ping, anyone?

Jelmer Vernooij (jelmer) wrote :

ping (-:

Jelmer Vernooij (jelmer) wrote :

ping :-)

Gavin Panella (allenap) wrote :

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) wrote :

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

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

lp:~jelmer/wikkid/dulwich updated on 2014-01-16
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) wrote :

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

Tim Penhey (thumper) 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 :-)

Jelmer Vernooij (jelmer) wrote :

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) wrote :

ping? :)

Tim Penhey (thumper) wrote :

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) wrote :

Thanks, Tim! Merged.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/wikkid-serve'
2--- bin/wikkid-serve 2012-05-17 09:32:49 +0000
3+++ bin/wikkid-serve 2014-01-16 00:05:37 +0000
4@@ -18,8 +18,6 @@
5 import optparse
6 import sys
7
8-from bzrlib.workingtree import WorkingTree
9-
10 from wikkid import version
11 from wikkid.app import WikkidApp
12 from wikkid.context import (
13@@ -28,8 +26,6 @@
14 DEFAULT_PORT,
15 ExecutionContext,
16 )
17-from wikkid.filestore.bzr import FileStore
18-from wikkid.user.bzr import LocalBazaarUserMiddleware
19
20
21 def setup_logging():
22@@ -48,6 +44,8 @@
23 usage = "Usage: %prog [options] <wiki-branch>"
24 parser = optparse.OptionParser(
25 usage=usage, description="Run a Wikkid Wiki server.", version=version)
26+ parser.add_option('--format', type='choice', default='bzr',
27+ choices=['bzr', 'git'], help=("Default repository format to use."))
28 parser.add_option(
29 '--host', type='string', default=DEFAULT_HOST,
30 help=('The interface to listen on. Defaults to %r' % DEFAULT_HOST))
31@@ -82,12 +80,24 @@
32 logger = logging.getLogger('wikkid')
33 logger.setLevel(logging.INFO)
34
35- working_tree = WorkingTree.open(branch)
36- logger.info('Using: %s', working_tree)
37- filestore = FileStore(working_tree)
38+ if options.format == 'bzr':
39+ from bzrlib.workingtree import WorkingTree
40+ from wikkid.filestore.bzr import FileStore
41+ from wikkid.user.bzr import LocalBazaarUserMiddleware
42+
43+ working_tree = WorkingTree.open(branch)
44+ logger.info('Using: %s', working_tree)
45+ filestore = FileStore(working_tree)
46+ elif options.format == 'git':
47+ from wikkid.filestore.git import FileStore
48+ from wikkid.user.git import LocalGitUserMiddleware
49+ filestore = FileStore.from_path(branch)
50
51 app = WikkidApp(filestore=filestore, execution_context=execution_context)
52- app = LocalBazaarUserMiddleware(app, working_tree.branch)
53+ if options.format == 'bzr':
54+ app = LocalBazaarUserMiddleware(app, working_tree.branch)
55+ elif options.format == 'git':
56+ app = LocalGitUserMiddleware(app, filestore.repo)
57 from wsgiref.simple_server import make_server
58 httpd = make_server(options.host, options.port, app)
59 logger.info('Serving on http://%s:%s', options.host, options.port)
60
61=== modified file 'setup.py'
62--- setup.py 2012-05-17 10:28:31 +0000
63+++ setup.py 2014-01-16 00:05:37 +0000
64@@ -24,6 +24,7 @@
65 include_package_data=True,
66 install_requires=[
67 'docutils',
68+ 'dulwich',
69 'jinja2',
70 'pygments',
71 'twisted',
72
73=== modified file 'wikkid/dispatcher.py'
74--- wikkid/dispatcher.py 2010-11-12 09:00:42 +0000
75+++ wikkid/dispatcher.py 2014-01-16 00:05:37 +0000
76@@ -53,7 +53,7 @@
77 # Don't register.
78 return
79 key = (interface, view_name)
80- assert key not in _VIEW_REGISTRY, "key already registered: %r" % key
81+ assert key not in _VIEW_REGISTRY, "key already registered: %r" % (key,)
82 _VIEW_REGISTRY[key] = view_class
83 if default_view:
84 _VIEW_REGISTRY[(interface, None)] = view_class
85
86=== modified file 'wikkid/filestore/basefile.py'
87--- wikkid/filestore/basefile.py 2010-05-12 10:39:13 +0000
88+++ wikkid/filestore/basefile.py 2014-01-16 00:05:37 +0000
89@@ -16,9 +16,8 @@
90 class BaseFile(object):
91 """Provide common fields and methods and properties for files."""
92
93- def __init__(self, path, file_id):
94+ def __init__(self, path):
95 self.path = path
96- self.file_id = file_id
97 self.base_name = urlutils.basename(path)
98 self._mimetype = mimetypes.guess_type(self.base_name)[0]
99
100
101=== modified file 'wikkid/filestore/bzr.py'
102--- wikkid/filestore/bzr.py 2012-06-14 01:14:00 +0000
103+++ wikkid/filestore/bzr.py 2014-01-16 00:05:37 +0000
104@@ -243,7 +243,8 @@
105 implements(IFile)
106
107 def __init__(self, filestore, path, file_id):
108- BaseFile.__init__(self, path, file_id)
109+ BaseFile.__init__(self, path)
110+ self.file_id = file_id
111 self.filestore = filestore
112 # This isn't entirely necessary.
113 self.tree = self.filestore.tree
114
115=== added file 'wikkid/filestore/git.py'
116--- wikkid/filestore/git.py 1970-01-01 00:00:00 +0000
117+++ wikkid/filestore/git.py 2014-01-16 00:05:37 +0000
118@@ -0,0 +1,205 @@
119+#
120+# Copyright (C) 2012 Wikkid Developers.
121+#
122+# This software is licensed under the GNU Affero General Public License
123+# version 3 (see the file LICENSE).
124+
125+"""A git filestore using Dulwich.
126+
127+"""
128+
129+import datetime
130+import mimetypes
131+
132+from dulwich.objects import Blob, Tree, ZERO_SHA
133+from dulwich.object_store import tree_lookup_path
134+from dulwich.repo import Repo
135+from dulwich.walk import Walker
136+import posixpath
137+import stat
138+
139+from zope.interface import implements
140+
141+from wikkid.errors import FileExists, UpdateConflicts
142+from wikkid.interface.filestore import FileType, IFile, IFileStore
143+
144+
145+class FileStore(object):
146+ """A filestore that just uses an internal map to store data."""
147+
148+ implements(IFileStore)
149+
150+ @classmethod
151+ def from_path(cls, path):
152+ return cls(Repo(path))
153+
154+ def __init__(self, repo, ref='HEAD'):
155+ """Repo is a dulwich repository."""
156+ self.repo = repo
157+ self.ref = ref
158+
159+ @property
160+ def store(self):
161+ return self.repo.object_store
162+
163+ def _get_root(self, revision=None):
164+ if revision is None:
165+ try:
166+ revision = self.repo.refs[self.ref]
167+ except KeyError:
168+ revision = ZERO_SHA
169+ try:
170+ return (revision, self.repo[revision].tree)
171+ except KeyError:
172+ return None, None
173+
174+ def get_file(self, path):
175+ """Return an object representing the file."""
176+ commit_id, root_id = self._get_root()
177+ if root_id is None:
178+ return None
179+ try:
180+ (mode, sha) = tree_lookup_path(self.store.__getitem__,
181+ root_id, path)
182+ except KeyError:
183+ return None
184+ return File(self.store, mode, sha, path, commit_id)
185+
186+ def update_file(self, path, content, user, parent_revision,
187+ commit_message=None):
188+ """The `user` is updating the file at `path` with `content`."""
189+ commit_id, root_id = self._get_root()
190+ if root_id is None:
191+ root_tree = Tree()
192+ else:
193+ root_tree = self.store[root_id]
194+ # Find all tree objects involved
195+ tree = root_tree
196+ trees = [root_tree]
197+ elements = path.strip(posixpath.sep).split(posixpath.sep)
198+ for el in elements[:-1]:
199+ try:
200+ (mode, sha) = tree[el]
201+ except KeyError:
202+ tree = Tree()
203+ else:
204+ if not stat.S_ISDIR(mode):
205+ raise FileExists(
206+ "File %s exists and is not a directory" % el)
207+ tree = self.store[sha]
208+ trees.append(tree)
209+ if elements[-1] in tree:
210+ (old_mode, old_sha) = tree[elements[-1]]
211+ if stat.S_ISDIR(old_mode):
212+ raise FileExists("File %s exists and is a directory" % path)
213+ if old_sha != parent_revision and parent_revision is not None:
214+ raise UpdateConflicts("File conflict %s != %s" % (old_sha,
215+ parent_revision), old_sha)
216+ blob = Blob.from_string(content.encode("utf-8"))
217+ child = (stat.S_IFREG | 0644, blob.id)
218+ self.store.add_object(blob)
219+ assert len(trees) == len(elements)
220+ for tree, name in zip(reversed(trees), reversed(elements)):
221+ assert name != ""
222+ tree[name] = child
223+ self.store.add_object(tree)
224+ child = (stat.S_IFDIR, tree.id)
225+ if commit_message is None:
226+ commit_message = ""
227+ self.repo.do_commit(ref=self.ref, message=commit_message, author=user,
228+ tree=tree.id)
229+
230+ def list_directory(self, directory_path):
231+ """Return a list of File objects for in the directory path.
232+
233+ If the path doesn't exist, returns None. If the path exists but is
234+ empty, an empty list is returned. Otherwise a list of File objects in
235+ that directory.
236+ """
237+ if directory_path is None:
238+ directory_path = ''
239+ else:
240+ directory_path = directory_path.strip(posixpath.sep)
241+ commit_id, root_id = self._get_root()
242+ if directory_path == '':
243+ sha = root_id
244+ mode = stat.S_IFDIR
245+ else:
246+ if root_id is None:
247+ return None
248+ try:
249+ (mode, sha) = tree_lookup_path(self.store.__getitem__,
250+ root_id, directory_path)
251+ except KeyError:
252+ return None
253+ if mode is not None and stat.S_ISDIR(mode):
254+ ret = []
255+ for (name, mode, sha) in self.store[sha].iteritems():
256+ ret.append(
257+ File(self.store, mode, sha, posixpath.join(directory_path, name), commit_id))
258+ return ret
259+ else:
260+ return None
261+
262+
263+class File(object):
264+ """A Git file object."""
265+
266+ implements(IFile)
267+
268+ def __init__(self, store, mode, sha, path, commit_sha):
269+ self.store = store
270+ self.mode = mode
271+ self.sha = sha
272+ self.path = path
273+ self.commit_sha = commit_sha
274+ self.base_name = posixpath.basename(path)
275+ self.mimetype = mimetypes.guess_type(self.base_name)[0]
276+
277+ @property
278+ def file_type(self):
279+ """Work out the filetype based on the mimetype if possible."""
280+ if self._is_directory:
281+ return FileType.DIRECTORY
282+ else:
283+ if self.mimetype is None:
284+ binary = self._is_binary
285+ else:
286+ binary = not self.mimetype.startswith('text/')
287+ if binary:
288+ return FileType.BINARY_FILE
289+ else:
290+ return FileType.TEXT_FILE
291+
292+ def get_content(self):
293+ o = self.store[self.sha]
294+ if isinstance(o, Blob):
295+ return o.data
296+ else:
297+ return None
298+
299+ @property
300+ def _is_directory(self):
301+ return stat.S_ISDIR(self.mode)
302+
303+ @property
304+ def _is_binary(self):
305+ return '\0' in self.get_content()
306+
307+ def _get_last_modified_commit(self):
308+ walker = Walker(self.store, include=[self.commit_sha],
309+ paths=[self.path])
310+ return iter(walker).next().commit
311+
312+ @property
313+ def last_modified_in_revision(self):
314+ return self.sha
315+
316+ @property
317+ def last_modified_by(self):
318+ return self._get_last_modified_commit().author
319+
320+ @property
321+ def last_modified_date(self):
322+ c = self._get_last_modified_commit()
323+ return datetime.datetime.utcfromtimestamp(c.author_time)
324
325=== modified file 'wikkid/filestore/volatile.py'
326--- wikkid/filestore/volatile.py 2010-06-07 08:56:13 +0000
327+++ wikkid/filestore/volatile.py 2014-01-16 00:05:37 +0000
328@@ -113,7 +113,8 @@
329 implements(IFile)
330
331 def __init__(self, path, content, file_id, user):
332- BaseFile.__init__(self, path, file_id)
333+ BaseFile.__init__(self, path)
334+ self.file_id = file_id
335 self.content = content
336 self.last_modified_in_revision = None
337 self.last_modified_by = user
338
339=== modified file 'wikkid/interface/filestore.py'
340--- wikkid/interface/filestore.py 2010-06-07 08:24:06 +0000
341+++ wikkid/interface/filestore.py 2014-01-16 00:05:37 +0000
342@@ -70,9 +70,6 @@
343
344 base_name = Attribute("The last part of the path.")
345
346- file_id = Attribute(
347- "The unique identifier for the file in the filestore.")
348-
349 file_type = Attribute("Soon to be a Choice with a lazr.enum.")
350
351 mimetype = Attribute(
352
353=== modified file 'wikkid/tests/__init__.py'
354--- wikkid/tests/__init__.py 2012-06-14 08:40:53 +0000
355+++ wikkid/tests/__init__.py 2014-01-16 00:05:37 +0000
356@@ -41,6 +41,7 @@
357 'volatile_filestore',
358 'bzr_filestore',
359 'bzr_user',
360+ 'git_filestore',
361 'view_dispatcher',
362 'model',
363 ]
364
365=== modified file 'wikkid/tests/filestore.py'
366--- wikkid/tests/filestore.py 2010-11-08 00:00:06 +0000
367+++ wikkid/tests/filestore.py 2014-01-16 00:05:37 +0000
368@@ -42,6 +42,7 @@
369 filestore = self.make_filestore(
370 [('README', 'Content'),
371 ('lib/', None),
372+ ('lib/foo', 'dummy data'),
373 ('image.jpg', 'pretend image'),
374 ('binary-file', 'a\0binary\0file'),
375 ('simple.txt', 'A text file'),
376@@ -57,6 +58,7 @@
377 filestore = self.make_filestore(
378 [('README', 'Content'),
379 ('lib/', None),
380+ ('lib/data', 'dummy data'),
381 ('image.jpg', 'pretend image'),
382 ('binary-file', 'a\0binary\0file'),
383 ('simple.txt', 'A text file'),
384
385=== added file 'wikkid/tests/test_git_filestore.py'
386--- wikkid/tests/test_git_filestore.py 1970-01-01 00:00:00 +0000
387+++ wikkid/tests/test_git_filestore.py 2014-01-16 00:05:37 +0000
388@@ -0,0 +1,55 @@
389+#
390+# Copyright (C) 2012 Wikkid Developers.
391+#
392+# This software is licensed under the GNU Affero General Public License
393+# version 3 (see the file LICENSE).
394+
395+"""Tests for the wikkid.filestore.git.FileStore."""
396+
397+from dulwich.repo import MemoryRepo
398+
399+from textwrap import dedent
400+
401+from wikkid.errors import UpdateConflicts
402+from wikkid.filestore.git import (
403+ FileStore,
404+ )
405+from wikkid.tests import ProvidesMixin, TestCase
406+from wikkid.tests.filestore import TestFileStore
407+
408+
409+class TestGitFileStore(TestCase, ProvidesMixin, TestFileStore):
410+ """Tests for the git filestore and files."""
411+
412+ def make_filestore(self, contents=None):
413+ repo = MemoryRepo()
414+ fs = FileStore(repo)
415+ if contents:
416+ for (path, contents) in contents:
417+ if contents is None:
418+ # Directory
419+ continue
420+ fs.update_file(path, contents,
421+ user="Somebody <test@example.com>",
422+ parent_revision=None,
423+ commit_message="Added by make_filestore")
424+ return fs
425+
426+ def test_empty(self):
427+ # Empty files do not have line endings, but they can be saved
428+ # nonetheless.
429+ filestore = self.make_filestore(
430+ [('test.txt', 'several\nlines\nof\ncontent')])
431+ f = filestore.get_file('test.txt')
432+ base_rev = f.last_modified_in_revision
433+ filestore.update_file(
434+ 'test.txt', '', 'Test Author <test@example.com>', base_rev)
435+ curr = filestore.get_file('test.txt')
436+ self.assertEqual('', curr.get_content())
437+
438+ def test_listing_directory_empty(self):
439+ filestore = self.make_filestore(
440+ [('empty/', None),
441+ ])
442+ listing = filestore.list_directory('empty')
443+ self.assertIs(None, listing)
444
445=== added file 'wikkid/user/git.py'
446--- wikkid/user/git.py 1970-01-01 00:00:00 +0000
447+++ wikkid/user/git.py 2014-01-16 00:05:37 +0000
448@@ -0,0 +1,74 @@
449+#
450+# Copyright (C) 2010 Wikkid Developers
451+#
452+# This software is licensed under the GNU Affero General Public License
453+# version 3 (see the file LICENSE).
454+
455+"""A user factory and user class which uses the git identity from the
456+local Git config."""
457+
458+import email
459+import logging
460+
461+from webob import Request
462+from zope.interface import implements
463+
464+from wikkid.interface.user import IUser, IUserFactory
465+from wikkid.user.baseuser import BaseUser
466+
467+
468+def create_git_user_from_author_string(author):
469+ name, address = email.Utils.parseaddr(author)
470+ if name:
471+ display_name = name
472+ else:
473+ display_name = address
474+ return User(address, display_name, author)
475+
476+
477+class LocalGitUserMiddleware(object):
478+ """A middleware to inject a user into the environment."""
479+
480+ def __init__(self, app, repo):
481+ self.app = app
482+ config = repo.get_config_stack()
483+ email = config.get(("user", ), "email")
484+ name = config.get(("user", ), "name")
485+ self.user = User(email, name, "%s <%s>" % (name, email))
486+
487+ def __call__(self, environ, start_response):
488+ environ['wikkid.user'] = self.user
489+ req = Request(environ)
490+ resp = req.get_response(self.app)
491+ return resp(environ, start_response)
492+
493+
494+class UserFactory(object):
495+ """Generate a user from local bazaar config."""
496+
497+ implements(IUserFactory)
498+
499+ def __init__(self, branch):
500+ """Use the user config from the branch."""
501+ config = branch.get_config()
502+ self.user = create_git_user_from_author_string(config.username())
503+ logger = logging.getLogger('wikkid')
504+ logger.info(
505+ 'Using git identity: "%s", "%s"',
506+ self.user.display_name, self.user.email)
507+
508+ def create(self, request):
509+ """Create a User."""
510+ return self.user
511+
512+
513+class User(BaseUser):
514+ """A user from the local bazaar config."""
515+
516+ implements(IUser)
517+
518+ def __init__(self, email, display_name, committer_id):
519+ self.email = email
520+ self.display_name = display_name
521+ self.committer_id = committer_id
522+

Subscribers

People subscribed via source and target branches