launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'turnip/api/store.py'
> --- turnip/api/store.py	2015-02-26 21:34:02 +0000
> +++ turnip/api/store.py	2015-03-05 19:34:14 +0000
> @@ -3,29 +3,63 @@
>  import os
>  import shutil
>  
> -import pygit2
> -
> -
> -class Store(object):
> -    """Provides methods for manipulating repos on disk with pygit2."""
> -
> -    @staticmethod
> -    def init(repo, is_bare=True):
> -        """Initialise a git repo with pygit2."""
> -        if os.path.exists(repo):
> -            raise Exception("Repository '%s' already exists" % repo)
> -        try:
> -            repo_path = pygit2.init_repository(repo, is_bare)
> -        except pygit2.GitError:
> -            print('Unable to create repository in {}.'.format(repo))
> -            raise
> -        return repo_path
> -
> -    @staticmethod
> -    def delete(repo):
> -        """Permanently delete a git repository from repo store."""
> -        try:
> -            shutil.rmtree(repo)
> -        except (IOError, OSError):
> -            print('Unable to delete repository from {}.'.format(repo))
> -            raise
> +from pygit2 import (
> +    GIT_OBJ_BLOB,
> +    GIT_OBJ_COMMIT,
> +    GIT_OBJ_TREE,
> +    GIT_OBJ_TAG,
> +    init_repository,
> +    Repository,
> +)
> +
> +
> +REF_TYPE_NAME = {GIT_OBJ_COMMIT: 'commit',
> +                 GIT_OBJ_TREE: 'tree',
> +                 GIT_OBJ_BLOB: 'blob',
> +                 GIT_OBJ_TAG: 'tag'}
> +
> +
> +def format_refs(ref, git_object):
> +    return {
> +        ref: {
> +            "object": {'sha1': git_object.oid.hex,
> +                       'type': REF_TYPE_NAME[git_object.type]}
> +        }
> +    }
> +
> +
> +def init_repo(repo, is_bare=True):
> +    """Initialise a git repository."""
> +    if os.path.exists(repo):
> +        raise Exception("Repository '%s' already exists" % repo)
> +    repo_path = init_repository(repo, is_bare)
> +    return repo_path
> +
> +
> +def open_repo(repo_path):
> +    """Open an existing git repository."""
> +    repo = Repository(repo_path)
> +    return repo
> +
> +
> +def delete_repo(repo):
> +    """Permanently delete a git repository from repo store."""
> +    shutil.rmtree(repo)
> +
> +
> +def get_refs(repo_path):
> +    """Return all refs for a git repository."""
> +    repo = open_repo(repo_path)
> +    refs = {}
> +    for ref in repo.listall_references():
> +        git_object = repo.lookup_reference(ref).get_object()
> +        refs.update(format_refs(ref, git_object))
> +        return refs

Could you make sure that this omits refs that aren't representable in UTF-8?  I've looked at filtering those out on the Launchpad side, but it's very much easier if we can require a valid UTF-8 JSON stream and reject the entire response if it's invalid, rather than playing guessing games with the encoding mid-stream.

> +
> +
> +def get_ref(repo_path, ref):
> +    """Return a specific ref for a git repository."""
> +    repo = open_repo(repo_path)
> +    git_object = repo.lookup_reference(ref).peel()
> +    ref_obj = format_refs(ref, git_object)
> +    return ref_obj
> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-02-25 07:28:44 +0000
> +++ turnip/api/tests/test_api.py	2015-03-05 19:34:14 +0000
> @@ -15,6 +15,7 @@
>  from webtest import TestApp
>  
>  from turnip import api
> +from turnip.api.tests import test_helpers
>  
>  
>  class ApiTestCase(TestCase):
> @@ -26,9 +27,19 @@
>          self.app = TestApp(api.main({}))
>          self.repo_path = str(uuid.uuid1())
>          self.repo_store = os.path.join(repo_store, self.repo_path)
> -
> -    def test_repo_post(self):
> -        resp = self.app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +        self.commit = {'ref': 'refs/heads/master', 'message': 'test commit.'}
> +        self.tag = {'name': 'test-tag', 'message': 'tag message'}
> +
> +    def get_ref(self, ref):
> +        test_helpers.init_repo(self.repo_store,
> +                               commits=[self.commit], tags=[self.tag])
> +        resp = self.app.get('/repo/{}/{}'.format(
> +            self.repo_path, ref))
> +        return json.loads(resp.json_body)
> +
> +    def test_repo_create(self):
> +        resp = self.app.post('/repo', json.dumps(
> +            {'repo_path': self.repo_path}))
>          self.assertEqual(resp.status_code, 200)
>  
>      def test_repo_delete(self):
> @@ -37,6 +48,30 @@
>          self.assertEqual(resp.status_code, 200)
>          self.assertFalse(os.path.exists(self.repo_store))
>  
> +    def test_repo_get_refs(self):
> +        """Ensure expected ref objects are returned and shas match."""
> +        ref = self.commit.get('ref')
> +        repo = test_helpers.init_repo(self.repo_store,
> +                                      commits=[self.commit], tags=[self.tag])
> +        resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
> +        body = json.loads(resp.json_body)
> +        self.assertTrue(ref in body)
> +        self.assertTrue('refs/tags/{}'.format(self.tag.get('name') in body))
> +
> +        oid = repo.head.get_object().oid.hex  # git object sha
> +        resp_sha = body[ref]['object'].get('sha1')
> +        self.assertEqual(oid, resp_sha)
> +
> +    def test_repo_get_ref(self):
> +        ref = 'refs/heads/master'
> +        resp = self.get_ref(ref)
> +        self.assertTrue(ref in resp)
> +
> +    def test_repo_get_tag(self):
> +        tag = 'refs/tags/test-tag'
> +        resp = self.get_ref(tag)
> +        self.assertTrue(tag in resp)
> +
>  
>  if __name__ == '__main__':
>      unittest.main()
> 
> === added file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/tests/test_helpers.py	2015-03-05 19:34:14 +0000
> @@ -0,0 +1,39 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +from pygit2 import (
> +    init_repository,
> +    GIT_OBJ_COMMIT,
> +    Signature,
> +    )
> +
> +AUTHOR = Signature('Test Author', 'author@xxxxxxx')
> +COMMITTER = Signature('Test Commiter', 'committer@xxxxxxx')
> +
> +
> +def create_commits(repo, commits, parents=[]):
> +    tree = repo.TreeBuilder().write()
> +    for commit in commits:
> +        commit = repo.create_commit(
> +            commit['ref'],
> +            AUTHOR, COMMITTER, commit['message'],
> +            tree,
> +            parents
> +        )
> +    return repo
> +
> +
> +def create_tags(repo, tags):
> +    oid = repo.head.get_object().oid
> +    for tag in tags:
> +        tag = repo.create_tag(
> +            tag['name'], oid, GIT_OBJ_COMMIT, COMMITTER, tag['message'])
> +    return repo
> +
> +
> +def init_repo(repo_path, commits=None, tags=None):
> +    repo = init_repository(repo_path, True)
> +    if commits:
> +        repo = create_commits(repo, commits)
> +    if tags:
> +        repo = create_tags(repo, tags)
> +    return repo
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-03-04 08:39:15 +0000
> +++ turnip/api/views.py	2015-03-05 19:34:14 +0000
> @@ -1,16 +1,30 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
>  
> +import json
>  import os
>  
>  from cornice.resource import resource
>  from cornice.util import extract_json_data
> +from pygit2 import GitError
>  import pyramid.httpexceptions as exc
>  
>  from turnip.config import TurnipConfig
> -from turnip.api.store import Store
> -
> -
> -@resource(collection_path='repo', path='/repo/{name}')
> +from turnip.api import store
> +
> +
> +def repo_path(func):
> +    """Decorator builds repo path from request name and repo_store."""
> +    def func_wrapper(self):
> +        name = self.request.matchdict['name']
> +        if not name:
> +            self.request.errors.add('body', 'name', 'repo name is missing')
> +            return
> +        self.repo = os.path.join(self.repo_store, name)
> +        return func(self)

I like this decorator approach, but I think I would find it less surprising if you passed the constructed repository path as an argument to the wrapped function rather than assigning it to self.repo.  Feel free to ignore this if you disagree, though.

> +    return func_wrapper
> +
> +
> +@resource(collection_path='/repo', path='/repo/{name}')
>  class RepoAPI(object):
>      """Provides HTTP API for repository actions."""
>  
> @@ -28,18 +42,47 @@
>              return
>          repo = os.path.join(self.repo_store, repo_path)
>          try:
> +<<<<<<< TREE
>              Store.init(repo)
>          except Exception:
> +=======
> +            store.init_repo(repo, is_bare)
> +        except GitError:
> +>>>>>>> MERGE-SOURCE

You'll need to fix this recently-gained conflict, of course.

>              return exc.HTTPConflict()  # 409
>  
> +    @repo_path
>      def delete(self):
>          """Delete an existing git repository."""
> -        name = self.request.matchdict['name']
> -        if not name:
> -            self.request.errors.add('body', 'name', 'repo name is missing')
> -            return
> -        repo = os.path.join(self.repo_store, name)
> -        try:
> -            Store.delete(repo)
> -        except Exception:
> -            return exc.HTTPNotFound()  # 404
> +        try:
> +            store.delete_repo(self.repo)
> +        except (IOError, OSError):
> +            return exc.HTTPNotFound()  # 404
> +
> +
> +@resource(collection_path='/repo/{name}/refs',
> +          path='/repo/{name}/refs/{ref:.*}')
> +class RefAPI(object):
> +    """Provides HTTP API for git references."""
> +
> +    def __init__(self, request):
> +        config = TurnipConfig()
> +        self.request = request
> +        self.repo_store = config.get('repo_store')
> +
> +    @repo_path
> +    def collection_get(self):
> +        try:
> +            refs = store.get_refs(self.repo)
> +        except GitError:
> +            return exc.HTTPNotFound()  # 404
> +        return json.dumps(refs)

Maybe ensure_ascii=False here and in the get method below, at least if cornice is happy with these methods returning unicode?  (Probably wants a specific unit test with UTF-8 data and another with non-ASCII but non-UTF-8 data.)

> +
> +    @repo_path
> +    def get(self):
> +        ref = 'refs/' + self.request.matchdict['ref']
> +        try:
> +            ref = store.get_ref(self.repo, ref)
> +        except GitError:
> +            return exc.HTTPNotFound()
> +        return json.dumps(ref)
> 


-- 
https://code.launchpad.net/~blr/turnip/api-refs/+merge/251394
Your team Launchpad code reviewers is subscribed to branch lp:turnip.