launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



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-02 07:08:53 +0000
> @@ -3,7 +3,24 @@
>  import os
>  import shutil
>  
> -import pygit2
> +from pygit2 import (
> +    GitError,
> +    GIT_OBJ_BLOB,
> +    GIT_OBJ_COMMIT,
> +    GIT_OBJ_TREE,
> +    GIT_OBJ_TAG,
> +    init_repository,
> +    Repository,
> +    )
> +
> +
> +def get_ref_type_name(ref_type_code):
> +    """Returns human readable ref type from ref type int."""
> +    types = {GIT_OBJ_COMMIT: 'commit',
> +             GIT_OBJ_TREE: 'tree',
> +             GIT_OBJ_BLOB: 'blob',
> +             GIT_OBJ_TAG: 'tag'}
> +    return types.get(ref_type_code)

Does this need to be a method rather than just a dict constant?

>  
>  
>  class Store(object):
> @@ -11,21 +28,54 @@
>  
>      @staticmethod
>      def init(repo, is_bare=True):
> -        """Initialise a git repo with pygit2."""
> +        """Initialise a git repository."""
>          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))
> +            repo_path = init_repository(repo, is_bare)
> +        except GitError:
>              raise

An except block containing just a raise is redundant.

>          return repo_path
>  
>      @staticmethod
> +    def open_repo(repo_path):
> +        """Open an existing git repository."""
> +        try:
> +            repo = Repository(repo_path)
> +        except GitError:
> +            raise

Likewise.

> +        return repo
> +
> +    @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
> +            raise
> +
> +    @staticmethod
> +    def get_refs(repo_path):
> +        """Return all refs for a git repository."""
> +        repo = Store.open_repo(repo_path)
> +        refs = {}
> +        for ref in repo.listall_references():
> +            git_object = repo.lookup_reference(ref).get_object()
> +            refs[ref] = {
> +                "object": {'sha': git_object.oid.hex,
> +                           'type': get_ref_type_name(git_object.type)}
> +            }

I'd use peel() rather than get_object(), so we get the commit pointed to by an annotated tag instead of the annotated tag itself. We may later want to include the annotated tag details too, and they could go in a sibling of the "object" dict.

I'd also say "sha1" rather than "sha".

> +        return refs
> +
> +    @staticmethod
> +    def get_ref(repo_path, ref):
> +        """Return a specific ref for a git repository."""
> +        repo = Store.open_repo(repo_path)
> +        try:
> +            git_object = repo.lookup_reference(ref).get_object()
> +        except GitError:
> +            raise

Same here.

> +        ref = {"ref": ref,
> +               "object": {'sha': git_object.oid.hex,
> +                          'type': get_ref_type_name(git_object.type)}}

This ref -> dict formatting code is duplicated with get_refs. Can you factor it out?

> +        return ref
> 
> === 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-02 07:08:53 +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,31 @@
>          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('sha')
> +        self.assertEqual(oid, resp_sha)
> +
> +    def test_repo_get_ref(self):
> +        ref = 'refs/heads/master'
> +        resp = self.get_ref(ref)
> +        self.assertEqual(ref, resp['ref'])
> +
> +    def test_repo_get_tag(self):
> +        tag = 'refs/tags/test-tag'
> +        resp = self.get_ref(tag)
> +        self.assertEqual(tag, resp['ref'])
> +
>  
>  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-02 07:08:53 +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-02-26 21:34:02 +0000
> +++ turnip/api/views.py	2015-03-02 07:08:53 +0000
> @@ -1,5 +1,6 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
>  
> +import json
>  import os
>  
>  from cornice.resource import resource
> @@ -10,7 +11,19 @@
>  from turnip.api.store import Store
>  
>  
> -@resource(collection_path='repo', path='/repo/{name}')
> +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)
> +    return func_wrapper
> +
> +
> +@resource(collection_path='/repo', path='/repo/{name}')
>  class RepoAPI(object):
>      """Provides HTTP API for repository actions."""
>  
> @@ -33,14 +46,38 @@
>          except Exception:
>              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(self.repo)
> +        except Exception:
> +            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 Exception:
> +            return exc.HTTPNotFound()  # 404

404ing on Exception is a bit too general.

> +        return json.dumps(refs)
> +
> +    @repo_path
> +    def get(self):
> +        ref = 'refs/' + self.request.matchdict['ref']
> +        try:
> +            ref = Store.get_ref(self.repo, ref)
> +        except Exception:
> +            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.