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-03-11 01:52:35 +0000
> +++ turnip/api/store.py	2015-03-12 19:46:39 +0000
> @@ -1,9 +1,11 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
>  
> +import itertools
>  import os
>  import shutil
>  
>  from pygit2 import (
> +    GitError,
>      GIT_OBJ_BLOB,
>      GIT_OBJ_COMMIT,
>      GIT_OBJ_TREE,
> @@ -13,7 +15,6 @@
>      )
>  
>  
> -
>  REF_TYPE_NAME = {
>      GIT_OBJ_COMMIT: 'commit',
>      GIT_OBJ_TREE: 'tree',
> @@ -33,6 +34,28 @@
>          }
>  
>  
> +def format_commit(git_object):

Worth an assertion that git_object.type is what we expect?

> +    parents = []
> +    for parent in git_object.parent_ids:
> +        parents.append(str(parent))

If parent's an Oid, this should probably be parent.hex for consistency. This could also be a list comprehension.

> +    return {
> +        'sha1': git_object.oid.hex,
> +        'message': git_object.message,
> +        'author': {
> +            'name': git_object.author.name,
> +            'email': git_object.author.email,
> +            'time': git_object.author.time
> +            },
> +        'committer': {
> +            'name': git_object.committer.name,
> +            'email': git_object.committer.email,
> +            'time': git_object.committer.time
> +            },

It's not too bad right now, but it may possibly be useful to have a format_signature to dedupe this and other similar places.

> +        'parents': parents,
> +        'tree': git_object.tree.hex

git_object.tree_id.hex to avoid inflating the tree unnecessarily.

> +        }
> +
> +
>  def init_repo(repo, is_bare=True):
>      """Initialise a git repository."""
>      if os.path.exists(repo):
> @@ -75,3 +98,42 @@
>      git_object = repo.lookup_reference(ref).peel()
>      ref_obj = format_ref(ref, git_object)
>      return ref_obj
> +
> +
> +def get_log(repo_path, start=None, limit=None, stop=None):
> +    """Return a commit collection from HEAD or optionally a start oid.
> +
> +    :param start: sha1 or branch to start listing commits from.
> +    :param limit: limit number of commits to return.
> +    :param stop: ignore a commit (and its ancestors).
> +    """
> +    repo = open_repo(repo_path)
> +    if not start:
> +        start = repo.head.target  # walk from HEAD
> +    walker = repo.walk(start)
> +    if stop:
> +        walker.hide(stop)  # filter stop sha1 and its ancestors
> +    if limit:
> +        walker = itertools.islice(walker, int(limit))
> +    commits = []
> +    for commit in walker:
> +        commits.append(format_commit(commit))
> +    return commits

This could just return an iterator, rather than implementing the limit itself. And at that point it's not clear that get_log is useful rather than just using a direct repo.walk call.

> +
> +
> +def get_commit(repo_path, commit_oid):
> +    """Return a single commit object from an oid."""
> +    repo = open_repo(repo_path)
> +    git_object = repo.get(commit_oid)
> +    if git_object.type != GIT_OBJ_COMMIT:
> +        raise GitError

Can you raise a more specific error, including the object ID and the fact that it wasn't a commit?

> +    commit = format_commit(git_object)
> +    return commit
> +
> +
> +def get_commits(repo_path, commit_oids):
> +    """Return a collection of commit objects from a list of oids."""
> +    commit_objects = []
> +    for commit in commit_oids:
> +        commit_objects.append(get_commit(repo_path, commit))
> +    return commit_objects

This could just be a list comprehension. I'd also open the repo just once, to avoid expensive recreation of state each time.

> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-03-11 23:15:14 +0000
> +++ turnip/api/tests/test_api.py	2015-03-12 19:46:39 +0000
> @@ -125,6 +125,71 @@
>          resp = self.get_ref(tag)
>          self.assertTrue(tag in resp)
>  
> +    def test_repo_get_commit(self):
> +        factory = RepoFactory(self.repo_store)
> +        message = 'Computers make me angry.'
> +        commit_oid = factory.add_commit(message, 'foobar.txt')
> +
> +        resp = self.app.get('/repo/{}/commits/{}'.format(
> +            self.repo_path, commit_oid.hex))
> +        commit_resp = resp.json
> +        self.assertEqual(commit_oid.hex, commit_resp['sha1'])
> +        self.assertEqual(message, commit_resp['message'])

We need to verify that the author/committer/etc. are rendered correctly.

> +
> +    def test_repo_get_commit_collection(self):
> +        """Ensure commits can be returned in bulk."""
> +        factory = RepoFactory(self.repo_store, num_commits=10)
> +        factory.build()
> +        bulk_commits = {'commits': [c.hex for c in factory.commits[0::2]]}
> +
> +        resp = self.app.post_json('/repo/{}/commits'.format(
> +            self.repo_path), bulk_commits)
> +        self.assertEqual(len(resp.json), 5)
> +        self.assertEqual(bulk_commits['commits'][0], resp.json[0]['sha1'])
> +
> +    def test_repo_get_log(self):
> +        factory = RepoFactory(self.repo_store, num_commits=4)
> +        factory.build()
> +        commits_from = factory.commits[2].hex
> +        resp = self.app.get('/repo/{}/log/{}'.format(
> +            self.repo_path, commits_from))
> +        self.assertEqual(len(resp.json), 3)
> +
> +    def test_repo_get_unicode_log(self):
> +        factory = RepoFactory(self.repo_store)
> +        message = u'나는 김치 사랑'.encode('utf-8')
> +        message2 = u'(╯°□°)╯︵ ┻━┻'.encode('utf-8')
> +        oid = factory.add_commit(message, '자장면/짜장면.py')
> +        oid2 = factory.add_commit(message2, '엄마야!.js', [oid])
> +
> +        resp = self.app.get('/repo/{}/log/{}'.format(
> +            self.repo_path, oid2))
> +        self.assertEqual(resp.json[0]['message'],
> +                         message2.decode('utf-8', 'replace'))
> +        self.assertEqual(resp.json[1]['message'],
> +                         message.decode('utf-8', 'replace'))

What happens in the case of invalid UTF-8?

> +
> +    def test_repo_get_log_with_limit(self):
> +        """Ensure the commit log can filtered by limit."""
> +        factory = RepoFactory(self.repo_store, num_commits=10)
> +        repo = factory.build()
> +        head = repo.head.target
> +        resp = self.app.get('/repo/{}/log/{}?limit=5'.format(
> +            self.repo_path, head))
> +        self.assertEqual(len(resp.json), 5)
> +
> +    def test_repo_get_log_with_stop(self):
> +        """Ensure the commit log can be filtered by a stop commit."""
> +        factory = RepoFactory(self.repo_store, num_commits=10)
> +        repo = factory.build()
> +        stop_commit = factory.commits[4]
> +        excluded_commit = factory.commits[5]
> +        head = repo.head.target
> +        resp = self.app.get('/repo/{}/log/{}?stop={}'.format(
> +            self.repo_path, head, stop_commit))
> +        self.assertEqual(len(resp.json), 5)
> +        self.assertNotIn(resp.json, excluded_commit)
> +
>  
>  if __name__ == '__main__':
>      unittest.main()
> 
> === modified file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	2015-03-10 03:22:33 +0000
> +++ turnip/api/tests/test_helpers.py	2015-03-12 19:46:39 +0000
> @@ -18,6 +18,7 @@
>      def __init__(self, repo_path=None, num_commits=None, num_tags=None):
>          self.author = Signature('Test Author', 'author@xxxxxxx')
>          self.committer = Signature('Test Commiter', 'committer@xxxxxxx')
> +        self.commits = []
>          self.num_commits = num_commits
>          self.num_tags = num_tags
>          self.repo_path = repo_path
> @@ -38,10 +39,8 @@
>          blob_entry = IndexEntry(file_path, blob_oid, GIT_FILEMODE_BLOB)
>          repo.index.add(blob_entry)
>          tree_id = repo.index.write_tree()
> -        oid = repo.create_commit(ref,
> -                                 self.author,
> -                                 self.committer,
> -                                 'commit', tree_id, parents)
> +        oid = repo.create_commit(ref, self.author, self.committer,
> +                                 blob_content, tree_id, parents)
>          return oid
>  
>      def add_tag(self, tag_name, tag_message, oid):
> @@ -69,6 +68,7 @@
>              self.stage(test_file)
>  
>              commit_oid = self.add_commit(blob_content, test_file, parents)
> +            self.commits.append(commit_oid)
>              parents = [commit_oid]
>  
>      def generate_tags(self, num_tags):
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-03-11 23:15:14 +0000
> +++ turnip/api/views.py	2015-03-12 19:46:39 +0000
> @@ -27,14 +27,19 @@
>      return repo_path_decorator
>  
>  
> +class BaseAPI(object):
> +    def __init__(self):
> +        config = TurnipConfig()
> +        self.repo_store = config.get('repo_store')
> +
> +
>  @resource(collection_path='/repo', path='/repo/{name}')
> -class RepoAPI(object):
> +class RepoAPI(BaseAPI):
>      """Provides HTTP API for repository actions."""
>  
>      def __init__(self, request):
> -        config = TurnipConfig()
> +        super(RepoAPI, self).__init__()
>          self.request = request
> -        self.repo_store = config.get('repo_store')
>  
>      def collection_post(self):
>          """Initialise a new git repository."""
> @@ -60,13 +65,12 @@
>  
>  @resource(collection_path='/repo/{name}/refs',
>            path='/repo/{name}/refs/{ref:.*}')
> -class RefAPI(object):
> +class RefAPI(BaseAPI):
>      """Provides HTTP API for git references."""
>  
>      def __init__(self, request):
> -        config = TurnipConfig()
> +        super(RefAPI, self).__init__()
>          self.request = request
> -        self.repo_store = config.get('repo_store')
>  
>      @repo_path
>      def collection_get(self, repo_path):
> @@ -82,3 +86,54 @@
>              return store.get_ref(repo_path, ref)
>          except (KeyError, GitError):
>              return exc.HTTPNotFound()
> +
> +
> +@resource(collection_path='/repo/{name}/commits',
> +          path='/repo/{name}/commits/{sha1}')
> +class CommitAPI(BaseAPI):
> +    """Provides HTTP API for git commits."""
> +
> +    def __init__(self, request):
> +        super(CommitAPI, self).__init__()
> +        self.request = request
> +
> +    @repo_path
> +    def get(self, repo_path):
> +        commit_sha1 = self.request.matchdict['sha1']
> +        try:
> +            commit = store.get_commit(repo_path, commit_sha1)
> +        except GitError:
> +            return exc.HTTPNotFound()
> +        return commit
> +
> +    @repo_path
> +    def collection_post(self, repo_path):
> +        """Get commits in bulk."""
> +        commits = extract_json_data(self.request).get('commits')
> +        try:
> +            commits = store.get_commits(repo_path, commits)
> +        except GitError:
> +            return exc.HTTPNotFound()
> +        return commits
> +
> +
> +@resource(path='/repo/{name}/log/{sha1}')
> +class LogAPI(BaseAPI):
> +    """Provides HTTP API for git logs."""
> +
> +    def __init__(self, request):
> +        super(LogAPI, self).__init__()
> +        self.request = request
> +
> +    @repo_path
> +    def get(self, repo_path):
> +        """Get log by sha1, filtered by limit and stop."""
> +        sha1 = self.request.matchdict['sha1']
> +        limit = self.request.params.get('limit')
> +        stop = self.request.params.get('stop')
> +
> +        try:
> +            log = store.get_log(repo_path, sha1, limit, stop)
> +        except GitError:
> +            return exc.HTTPNotFound()
> +        return log
> 


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