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-16 21:47:08 +0000
> +++ turnip/api/store.py	2015-04-22 05:15:51 +0000
> @@ -63,6 +63,10 @@
>          }
>  
>  
> +def is_bare(repo_path):
> +    return not os.path.exists(os.path.join(repo_path, '.git'))
> +
> +
>  def is_valid_new_path(path):
>      """Verify repo path is new, or raise Exception."""
>      if os.path.exists(path):
> @@ -70,7 +74,25 @@
>      return True
>  
>  
> -def init_repo(repo_path, clone_from=None, is_bare=True):
> +def alternates_path(repo_path):
> +    """Git object alternates path.
> +    See http://git-scm.com/docs/gitrepository-layout
> +    """
> +    return os.path.join(repo_path, 'objects', 'info', 'alternates')
> +
> +
> +def write_alternates(repo_path, alternate_repo_paths):
> +    with open(alternates_path(repo_path), "w") as f:
> +        for path in alternate_repo_paths:
> +            if is_bare(path):
> +                objects_path = os.path.join(path, 'objects')
> +            else:
> +                objects_path = os.path.join(path, '.git', 'objects')
> +            f.write("{}\n".format(objects_path))
> +
> +
> +def init_repo(repo_path, clone_from=None, alternate_repo_paths=None,
> +              is_bare=True):
>      """Initialise a new git repository or clone from existing."""
>      assert is_valid_new_path(repo_path)
>      if clone_from:
> @@ -79,6 +101,8 @@
>          repo = clone_repository(clone_from_url, repo_path, is_bare)
>      else:
>          repo = init_repository(repo_path, is_bare)
> +    if alternate_repo_paths:
> +        write_alternates(repo_path, alternate_repo_paths)
>      return repo.path
>  
>  
> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-04-16 21:47:08 +0000
> +++ turnip/api/tests/test_api.py	2015-04-22 05:15:51 +0000
> @@ -72,6 +72,22 @@
>          self.assertEqual(200, resp.status_code)
>          self.assertIn(new_repo_path, resp.json['repo_url'])
>  
> +    def test_cross_repo_diff(self):
> +        """Diff can be requested across 2 repositories."""
> +        repo = RepoFactory(self.repo_store)
> +        c1 = repo.add_commit('foo', 'foobar.txt')
> +        c2 = repo.add_commit('bar', 'foobar.txt', parents=[c1])
> +
> +        repo2_name = uuid.uuid4().hex
> +        repo2 = RepoFactory(
> +            os.path.join(self.repo_root, repo2_name), clone_from=repo)
> +        c3 = repo2.add_commit('baz', 'foobar.txt')
> +
> +        resp = self.app.get('/repo/{}:{}/diff/{}:{}'.format(
> +            self.repo_path, repo2_name, c2, c3))
> +        self.assertIn('-bar', resp.body)
> +        self.assertIn('+baz', resp.body)

This isn't a totally useful test, since both c2 and c3 exist in repo2. You need to create a parent, clone, then create a child in both repos and diff them in an ephemeral one.

> +
>      def test_repo_delete(self):
>          self.app.post_json('/repo', {'repo_path': self.repo_path})
>          resp = self.app.delete('/repo/{}'.format(self.repo_path))
> @@ -242,10 +258,13 @@
>          """Ensure expected changes exist in diff patch."""
>          repo = RepoFactory(self.repo_store)
>          c1 = repo.add_commit('foo\nbar\nbaz\n', 'blah.txt')
> -        c2_right = repo.add_commit('quux\nbar\nbaz\n', 'blah.txt', parents=[c1])
> -        c3_right = repo.add_commit('quux\nbar\nbaz\n', 'blah.txt', parents=[c2_right])
> +        c2_right = repo.add_commit('quux\nbar\nbaz\n', 'blah.txt',
> +                                   parents=[c1])
> +        c3_right = repo.add_commit('quux\nbar\nbaz\n', 'blah.txt',
> +                                   parents=[c2_right])
>          c2_left = repo.add_commit('foo\nbar\nbar\n', 'blah.txt', parents=[c1])
> -        c3_left = repo.add_commit('foo\nbar\nbar\n', 'blah.txt', parents=[c2_left])
> +        c3_left = repo.add_commit('foo\nbar\nbar\n', 'blah.txt',
> +                                  parents=[c2_left])
>  
>          resp = self.app.get('/repo/{}/compare-merge/{}/{}'.format(
>              self.repo_path, c3_right, c3_left))
> 
> === modified file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	2015-04-14 15:12:31 +0000
> +++ turnip/api/tests/test_helpers.py	2015-04-22 05:15:51 +0000
> @@ -2,9 +2,12 @@
>  
>  import itertools
>  import os
> +import urllib
> +import urlparse
>  import uuid
>  
>  from pygit2 import (
> +    clone_repository,
>      init_repository,
>      GIT_FILEMODE_BLOB,
>      GIT_OBJ_COMMIT,
> @@ -29,7 +32,7 @@
>      """Builds a git repository in a user defined state."""
>  
>      def __init__(self, repo_path=None, num_commits=None,
> -                 num_branches=None, num_tags=None):
> +                 num_branches=None, num_tags=None, clone_from=None):
>          self.author = Signature('Test Author', 'author@xxxxxxx')
>          self.branches = []
>          self.committer = Signature('Test Commiter', 'committer@xxxxxxx')
> @@ -38,7 +41,10 @@
>          self.num_commits = num_commits
>          self.num_tags = num_tags
>          self.repo_path = repo_path
> -        self.repo = self.init_repo()
> +        if clone_from:
> +            self.repo = self.clone_repo(clone_from)
> +        else:
> +            self.repo = self.init_repo()
>  
>      @property
>      def commits(self):
> @@ -133,6 +139,12 @@
>      def init_repo(self):
>          return init_repository(self.repo_path)
>  
> +    def clone_repo(self, repo_factory):
> +        """Return a pygit2 repo object cloned from an existing factory repo."""
> +        clone_from_url = urlparse.urljoin(
> +            'file:', urllib.pathname2url(repo_factory.repo.path))
> +        return clone_repository(clone_from_url, self.repo_path)
> +
>      def build(self):
>          """Return a repo, optionally with generated commits and tags."""
>          if self.num_branches:
> 
> === added file 'turnip/api/tests/test_store.py'
> --- turnip/api/tests/test_store.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/tests/test_store.py	2015-04-22 05:15:51 +0000
> @@ -0,0 +1,57 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +# -*- coding: utf-8 -*-
> +from __future__ import print_function
> +
> +import os
> +import unittest
> +import uuid
> +
> +from fixtures import (
> +    EnvironmentVariable,
> +    TempDir,
> +    )
> +from testtools import TestCase
> +
> +from turnip.api.store import (
> +    alternates_path,
> +    init_repo
> +    )
> +from turnip.api.tests.test_helpers import (
> +    open_repo,
> +    RepoFactory,
> +    )
> +
> +
> +class StoreTestCase(TestCase):
> +
> +    def setUp(self):
> +        super(StoreTestCase, self).setUp()
> +        self.repo_store = self.useFixture(TempDir()).path
> +        self.useFixture(EnvironmentVariable("REPO_STORE", self.repo_store))
> +        self.repo_path = os.path.join(self.repo_store, uuid.uuid1().hex)
> +
> +    def test_repo_with_alternates(self):
> +        """Ensure objects path is defined correctly in repo alternates."""
> +        factory = RepoFactory(self.repo_path)
> +        new_repo_path = os.path.join(self.repo_store, uuid.uuid1().hex)
> +        repo_path_with_alt = init_repo(
> +            new_repo_path, alternate_repo_paths=[factory.repo.path])
> +        objects_path = '{}\n'.format(
> +            os.path.join(factory.repo.path, 'objects'))
> +        with open(alternates_path(repo_path_with_alt), 'r') as alts:
> +            alts_content = alts.read()
> +            self.assertEquals(objects_path, alts_content)
> +
> +    def test_repo_alternates_objects_shared(self):
> +        """Ensure objects are shared from alternate repo."""
> +        factory = RepoFactory(self.repo_path)
> +        commit_oid = factory.add_commit('foo', 'foobar.txt')
> +        new_repo_path = os.path.join(self.repo_store, uuid.uuid4().hex)
> +        repo_path_with_alt = init_repo(
> +            new_repo_path, alternate_repo_paths=[factory.repo.path])
> +        repo_with_alt = open_repo(repo_path_with_alt)
> +        self.assertEqual(commit_oid.hex, repo_with_alt.get(commit_oid).hex)
> +
> +
> +if __name__ == '__main__':
> +    unittest.main()
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-04-16 21:47:08 +0000
> +++ turnip/api/views.py	2015-04-22 05:15:51 +0000
> @@ -2,6 +2,7 @@
>  
>  import os
>  import re
> +import uuid
>  
>  from cornice.resource import resource
>  from cornice.util import extract_json_data
> @@ -71,7 +72,7 @@
>              repo_clone = None
>  
>          try:
> -            new_repo_path = store.init_repo(repo, repo_clone)
> +            new_repo_path = store.init_repo(repo, clone_from=repo_clone)
>              repo_name = os.path.basename(os.path.normpath(new_repo_path))
>              return {'repo_url': '/'.join([self.request.url, repo_name])}
>          except GitError:
> @@ -168,6 +169,43 @@
>          return patch
>  
>  
> +@resource(path='/repo/{name}:{other_name}/diff/{commit}:{other_commit}')
> +class RepoDiffAPI(BaseAPI):
> +    """Provides HTTP API for cross repository diffs."""
> +
> +    def __init__(self, request):
> +        super(RepoDiffAPI, self).__init__()
> +        self.request = request
> +
> +    @repo_path
> +    def get(self, repo_path):
> +        context_lines = int(self.request.params.get('context_lines', 3))
> +        other_name = self.request.matchdict['other_name']
> +        commit = self.request.matchdict['commit']
> +        other_commit = self.request.matchdict['other_commit']
> +
> +        other_repo_path = os.path.join(self.repo_store, other_name)

You need to run is_valid_path on this, as repo_path does for name.

> +        tmp_repo_path = os.path.join(self.repo_store, uuid.uuid4().hex)

Can you use some kind of prefix here so it's clear that it can be safely deleted if it somehow gets left around? It could even be in a different directory from the real repos, though that matters less.

> +        # create a new ephemeral repo with alternates set from {name}
> +        # and {other_name}
> +        store.init_repo(
> +            tmp_repo_path,
> +            alternate_repo_paths=[repo_path, other_repo_path])
> +        try:
> +            patch = store.get_diff(
> +                tmp_repo_path, commit, other_commit, context_lines)
> +        except (ValueError, GitError):
> +            # invalid pygit2 sha1's return ValueError: 1: Ambiguous lookup
> +            return exc.HTTPNotFound()
> +        # delete ephemeral repo
> +        try:
> +            if is_valid_path(self.repo_store, tmp_repo_path):
> +                store.delete_repo(tmp_repo_path)

This needs to be in a big try/finally from immediately after the init_repo, otherwise eg. bad commit identifiers will cause the repo to be left around on disk.

> +        except (IOError, OSError):
> +            return exc.HTTPNotFound()  # 404

404ing on delete seems unfortunate. We created it, so it's not user error if it doesn't exist.

> +        return patch
> +
> +
>  @resource(collection_path='/repo/{name}/commits',
>            path='/repo/{name}/commits/{sha1}')
>  class CommitAPI(BaseAPI):
> 


-- 
https://code.launchpad.net/~blr/turnip/api-init-with-alternates/+merge/256715
Your team Launchpad code reviewers is subscribed to branch lp:turnip.