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-04-02 17:29:58 +0000
> +++ turnip/api/store.py	2015-04-14 18:38:40 +0000
> @@ -117,6 +117,37 @@
>      return ref_obj
>  
>  
> +def get_common_ancestor_diff(repo_path, sha1_target, sha1_source,
> +                             context_lines=3):
> +    """Get diff of common ancestor and source diff.
> +
> +    :param sha1_target: target sha1 for merge base.
> +    :param sha1_source: source sha1 for merge base.
> +    :param context_lines: num unchanged lines that define a hunk boundary.
> +    """
> +    repo = open_repo(repo_path)
> +    common_ancestor = repo.merge_base(sha1_target, sha1_source)
> +    return get_diff(repo_path, common_ancestor, sha1_source)

You need to pass context_lines through.

> +
> +
> +def get_diff(repo_path, sha1_from, sha1_to, context_lines=3):
> +    """Get patch and associated commits of two sha1s.
> +
> +    :param sha1_from: diff from sha1.
> +    :param sha1_to: diff to sha1.
> +    :param context_lines: num unchanged lines that define a hunk boundary.
> +    """
> +    repo = open_repo(repo_path)
> +    shas = [sha1_from, sha1_to]
> +    commits = [get_commit(repo_path, sha, repo) for sha in shas]
> +    diff = {
> +        'commits': commits,
> +        'patch': repo.diff(commits[0]['sha1'], commits[1]['sha1'],
> +                           False, 0, context_lines).patch
> +        }
> +    return diff
> +
> +
>  def get_log(repo_path, start=None, limit=None, stop=None):
>      """Return a commit collection from HEAD or optionally a start oid.
>  
> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-04-01 09:45:24 +0000
> +++ turnip/api/tests/test_api.py	2015-04-14 18:38:40 +0000
> @@ -107,7 +107,7 @@
>  
>          resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
>          refs = resp.json
> -        self.assertEqual(1, len(refs.keys()))
> +        self.assertEqual(0, len(refs.keys()))
>  
>      def test_allow_unicode_refs(self):
>          """Ensure unicode refs are included in ref collection."""
> @@ -119,7 +119,7 @@
>  
>          resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
>          refs = resp.json
> -        self.assertEqual(2, len(refs.keys()))
> +        self.assertEqual(1, len(refs.keys()))
>  
>      def test_repo_get_ref(self):
>          RepoFactory(self.repo_store, num_commits=1).build()
> @@ -160,6 +160,84 @@
>          resp = self.get_ref(tag)
>          self.assertTrue(tag in resp)
>  
> +    def test_repo_compare_commits(self):
> +        """Ensure expected changes exist in diff patch."""
> +        repo = RepoFactory(self.repo_store)
> +        c1_oid = repo.add_commit('foo', 'foobar.txt')
> +        c2_oid = repo.add_commit('bar', 'foobar.txt', parents=[c1_oid])
> +
> +        path = '/repo/{}/compare/{}..{}'.format(self.repo_path, c1_oid, c2_oid)
> +        resp = self.app.get(path)
> +        self.assertIn('-foo', resp.body)
> +        self.assertIn('+bar', resp.body)
> +
> +    def test_repo_diff_commits(self):
> +        """Ensure expected commits objects are returned in diff."""
> +        repo = RepoFactory(self.repo_store)
> +        c1_oid = repo.add_commit('foo', 'foobar.txt')
> +        c2_oid = repo.add_commit('bar', 'foobar.txt', parents=[c1_oid])
> +
> +        path = '/repo/{}/compare/{}..{}'.format(self.repo_path, c1_oid, c2_oid)
> +        resp = self.app.get(path)
> +        self.assertIn(c1_oid.hex, resp.json['commits'][0]['sha1'])
> +        self.assertIn(c2_oid.hex, resp.json['commits'][1]['sha1'])
> +
> +    def test_repo_diff_unicode_commits(self):
> +        """Ensure expected utf-8 commits objects are returned in diff."""
> +        factory = RepoFactory(self.repo_store)
> +        message = u'屋漏偏逢连夜雨'.encode('utf-8')
> +        message2 = u'说曹操，曹操到'.encode('utf-8')
> +        oid = factory.add_commit(message, 'foo.py')
> +        oid2 = factory.add_commit(message2, 'bar.py', [oid])
> +
> +        resp = self.app.get('/repo/{}/compare/{}..{}'.format(
> +            self.repo_path, oid, oid2))
> +        self.assertEqual(resp.json['commits'][0]['message'],
> +                         message.decode('utf-8'))
> +        self.assertEqual(resp.json['commits'][1]['message'],
> +                         message2.decode('utf-8'))
> +
> +    def test_repo_diff_non_unicode_commits(self):
> +        """Ensure non utf-8 chars are handled but stripped from diff."""
> +        factory = RepoFactory(self.repo_store)
> +        message = 'not particularly sensible latin-1: \xe9\xe9\xe9.'
> +        oid = factory.add_commit(message, 'foo.py')
> +        oid2 = factory.add_commit('a sensible commit message', 'foo.py', [oid])
> +
> +        resp = self.app.get('/repo/{}/compare/{}..{}'.format(
> +            self.repo_path, oid, oid2))
> +        self.assertEqual(resp.json['commits'][0]['message'],
> +                         message.decode('utf-8', 'replace'))
> +
> +    def test_repo_get_diff_nonexistent_sha1(self):
> +        """get_diff on a non-existent sha1 returns HTTP 404."""
> +        RepoFactory(self.repo_store).build()
> +        resp = self.app.get('/repo/{}/compare/1..2'.format(
> +            self.repo_path), expect_errors=True)
> +        self.assertEqual(404, resp.status_code)
> +
> +    def test_repo_get_diff_invalid_separator(self):
> +        """get_diff with an invalid separator (not ../...) returns HTTP 404."""
> +        RepoFactory(self.repo_store).build()
> +        resp = self.app.get('/repo/{}/compare/1++2'.format(
> +            self.repo_path), expect_errors=True)
> +        self.assertEqual(400, resp.status_code)
> +
> +    def test_repo_common_ancestor_diff(self):
> +        """Ensure expected changes exist in diff patch."""
> +        repo = RepoFactory(self.repo_store)
> +        c1 = repo.add_commit('foo', 'foobar.txt')
> +        c2_right = repo.add_commit('bar', 'foobar.txt', parents=[c1])
> +        c3_right = repo.add_commit('baz', 'foobar.txt', parents=[c2_right])
> +        c2_left = repo.add_commit('qux', 'foobar.txt', parents=[c1])
> +        c3_left = repo.add_commit('corge', 'foobar.txt', parents=[c2_left])
> +
> +        resp = self.app.get('/repo/{}/compare/{}...{}'.format(
> +            self.repo_path, c3_left, c3_right))
> +        self.assertIn('-foo', resp.json_body['patch'])
> +        self.assertIn('+baz', resp.json_body['patch'])
> +        self.assertNotIn('+corge', resp.json_body['patch'])
> +
>      def test_repo_get_commit(self):
>          factory = RepoFactory(self.repo_store)
>          message = 'Computers make me angry.'
> 
> === modified file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	2015-03-31 15:14:36 +0000
> +++ turnip/api/tests/test_helpers.py	2015-04-14 18:38:40 +0000
> @@ -2,6 +2,7 @@
>  
>  import itertools
>  import os
> +import uuid
>  
>  from pygit2 import (
>      init_repository,
> @@ -27,10 +28,13 @@
>  class RepoFactory():
>      """Builds a git repository in a user defined state."""
>  
> -    def __init__(self, repo_path=None, num_commits=None, num_tags=None):
> +    def __init__(self, repo_path=None, num_commits=None,
> +                 num_branches=None, num_tags=None):
>          self.author = Signature('Test Author', 'author@xxxxxxx')
> +        self.branches = []
>          self.committer = Signature('Test Commiter', 'committer@xxxxxxx')
>          self.commits = []
> +        self.num_branches = num_branches
>          self.num_commits = num_commits
>          self.num_tags = num_tags
>          self.repo_path = repo_path
> @@ -43,7 +47,7 @@
>          return list(self.repo.walk(last.id, GIT_SORT_TIME))
>  
>      def add_commit(self, blob_content, file_path, parents=[],
> -                   ref='refs/heads/master', author=None, committer=None):
> +                   ref=None, author=None, committer=None):
>          """Create a commit from blob_content and file_path."""
>          repo = self.repo
>          if not author:
> @@ -58,6 +62,12 @@
>                                   blob_content, tree_id, parents)
>          return oid
>  
> +    def add_branch(self, name, oid):
> +        commit = self.repo.get(oid)
> +        branch = self.repo.create_branch('branch-{}'.format(name), commit)
> +        self.branches.append(branch)
> +        return branch
> +
>      def add_tag(self, tag_name, tag_message, oid):
>          """Create a tag from tag_name and oid."""
>          repo = self.repo
> @@ -75,20 +85,24 @@
>          repo.index.write()
>          return repo.index.write_tree()
>  
> -    def generate_commits(self, num_commits):
> +    def generate_commits(self, num_commits, parents=[]):
>          """Generate n number of commits."""
> -        parents = []
>          for i in xrange(num_commits):
> -            blob_content = b'commit {}'.format(i)
> +            blob_content = b'commit {} - {}'.format(i, uuid.uuid1())
>              test_file = 'test.txt'
>              with open(os.path.join(self.repo_path, test_file), 'w') as f:
>                  f.write(blob_content)
> -
>              self.stage(test_file)
> -
>              commit_oid = self.add_commit(blob_content, test_file, parents)
>              self.commits.append(commit_oid)
>              parents = [commit_oid]
> +            if i == num_commits - 1:
> +                ref = 'refs/heads/master'
> +                try:
> +                    self.repo.lookup_reference(ref)
> +                except KeyError:
> +                    self.repo.create_reference(ref, commit_oid)
> +                self.repo.set_head(commit_oid)
>  
>      def generate_tags(self, num_tags):
>          """Generate n number of tags."""
> @@ -97,6 +111,17 @@
>          for i in xrange(num_tags):
>              self.add_tag('tag{}'.format(i), 'tag message {}'.format(i), oid)
>  
> +    def generate_branches(self, num_branches, num_commits):
> +        """Generate n number of branches with n commits."""
> +        repo = self.repo
> +        parents = []
> +        for i in xrange(num_branches):
> +            self.generate_commits(num_commits, parents)
> +            oid = repo.revparse_single('HEAD')
> +            branch = repo.create_branch('branch-{}'.format(i), oid)
> +            self.branches.append(branch)
> +            parents.append(self.commits[0])
> +
>      def nonexistent_oid(self):
>          """Return an arbitrary OID that does not exist in this repo."""
>          for oid_chars in itertools.product('0123456789abcdef', repeat=40):
> @@ -110,7 +135,9 @@
>  
>      def build(self):
>          """Return a repo, optionally with generated commits and tags."""
> -        if self.num_commits:
> +        if self.num_branches:
> +            self.generate_branches(self.num_branches, self.num_commits)
> +        if not self.num_branches and self.num_commits:
>              self.generate_commits(self.num_commits)
>          if self.num_tags:
>              self.generate_tags(self.num_tags)
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-03-29 23:57:03 +0000
> +++ turnip/api/views.py	2015-04-14 18:38:40 +0000
> @@ -1,6 +1,7 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
>  
>  import os
> +import re
>  
>  from cornice.resource import resource
>  from cornice.util import extract_json_data
> @@ -110,6 +111,34 @@
>              return exc.HTTPNotFound()
>  
>  
> +@resource(path='/repo/{name}/compare/{commits}')
> +class DiffAPI(BaseAPI):
> +    """Provides HTTP API for rev-rev 'double' and 'tripple dot' diff."""

tripple!

I'd also quickly describe the difference between the two diff types.

> +
> +    def __init__(self, request):
> +        super(DiffAPI, self).__init__()
> +        self.request = request
> +
> +    @repo_path
> +    def get(self, repo_path):
> +        """Returns diff of two commits."""
> +        commits = re.split('(\.{2,3})', self.request.matchdict['commits'])
> +        context_lines = int(self.request.params.get('context_lines', 3))
> +        if not len(commits) == 3:
> +            return exc.HTTPBadRequest()
> +        try:
> +            diff_type = commits[1]
> +            args = (repo_path, commits[0], commits[2], context_lines)
> +            if diff_type == '..':
> +                patch = store.get_diff(*args)
> +            elif diff_type == '...':
> +                patch = store.get_common_ancestor_diff(*args)
> +        except (ValueError, GitError):
> +            # invalid pygit2 sha1's return ValueError: 1: Ambiguous lookup
> +            return exc.HTTPNotFound()
> +        return patch
> +
> +
>  @resource(collection_path='/repo/{name}/commits',
>            path='/repo/{name}/commits/{sha1}')
>  class CommitAPI(BaseAPI):
> 


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