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-13 03:44:09 +0000
> +++ turnip/api/store.py	2015-03-24 21:30:45 +0000
> @@ -3,6 +3,8 @@
>  import itertools
>  import os
>  import shutil
> +import urllib
> +import urlparse
>  
>  from pygit2 import (
>      GitError,
> @@ -10,6 +12,7 @@
>      GIT_OBJ_COMMIT,
>      GIT_OBJ_TREE,
>      GIT_OBJ_TAG,
> +    clone_repository,
>      init_repository,
>      Repository,
>      )
> @@ -60,23 +63,33 @@
>          }
>  
>  
> -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 is_valid_new_path(path):
> +    """Verify repo path is new, or raise Exception."""
> +    if os.path.exists(path):
> +        raise Exception("Repository '%s' already exists" % path)

This should be a more class.

> +    return True
> +
> +
> +def init_repo(repo_path, clone_path=None, is_bare=True):
> +    """Initialise a new git repository or clone from existing."""
> +    if clone_path:
> +        assert is_valid_new_path(clone_path)
> +        repo_url = urlparse.urljoin('file:', urllib.pathname2url(repo_path))
> +        repo = clone_repository(repo_url, clone_path, is_bare)
> +    else:
> +        assert is_valid_new_path(repo_path)
> +        repo = init_repository(repo_path, is_bare)
> +    return repo.path

repo_path is the new repo in the init case, but the old repo in the clone case. That's a bit confusing -- can you make clone_path the old repo instead? Maybe even call it clone_from?

>  
>  
>  def open_repo(repo_path):
>      """Open an existing git repository."""
> -    repo = Repository(repo_path)
> -    return repo
> -
> -
> -def delete_repo(repo):
> +    return Repository(repo_path)
> +
> +
> +def delete_repo(repo_path):
>      """Permanently delete a git repository from repo store."""
> -    shutil.rmtree(repo)
> +    shutil.rmtree(repo_path)
>  
>  
>  def get_refs(repo_path):
> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-03-13 03:24:22 +0000
> +++ turnip/api/tests/test_api.py	2015-03-24 21:30:45 +0000
> @@ -14,7 +14,11 @@
>  from webtest import TestApp
>  
>  from turnip import api
> -from turnip.api.tests.test_helpers import RepoFactory
> +from turnip.api.tests.test_helpers import (
> +    get_revlist,
> +    open_repo,
> +    RepoFactory,
> +    )
>  
>  
>  class ApiTestCase(TestCase):
> @@ -24,8 +28,9 @@
>          repo_store = self.useFixture(TempDir()).path
>          self.useFixture(EnvironmentVariable("REPO_STORE", repo_store))
>          self.app = TestApp(api.main({}))
> -        self.repo_path = str(uuid.uuid1())
> +        self.repo_path = uuid.uuid1().hex
>          self.repo_store = os.path.join(repo_store, self.repo_path)
> +        self.repo_root = repo_store
>          self.commit = {'ref': 'refs/heads/master', 'message': 'test commit.'}
>          self.tag = {'ref': 'refs/tags/tag0', 'message': 'tag message'}
>  
> @@ -33,9 +38,31 @@
>          resp = self.app.get('/repo/{}/{}'.format(self.repo_path, ref))
>          return resp.json
>  
> -    def test_repo_create(self):
> +    def test_repo_init(self):
>          resp = self.app.post_json('/repo', {'repo_path': self.repo_path})
> -        self.assertEqual(resp.status_code, 200)
> +        self.assertIn(self.repo_path, resp.json['repo_url'])
> +        self.assertEqual(resp.status_code, 200)
> +
> +    def test_repo_init_with_invalid_repo_path(self):
> +        resp = self.app.post_json('/repo', {'repo_path': '../1234'},
> +                                  expect_errors=True)
> +        self.assertEqual(resp.status_code, 500)
> +
> +    def test_repo_init_with_clone(self):
> +        """Repo can be initialised with optional clone."""
> +        factory = RepoFactory(self.repo_store, num_commits=2)
> +        factory.build()
> +        new_repo_path = uuid.uuid1().hex
> +        resp = self.app.post_json('/repo', {'repo_path': self.repo_path,
> +                                            'clone_path': new_repo_path})
> +        repo1_revlist = get_revlist(factory.repo)
> +        clone_path = resp.json['repo_url'].split('/')[-1]
> +        repo2 = open_repo(os.path.join(self.repo_root, clone_path))
> +        repo2_revlist = get_revlist(repo2)
> +
> +        self.assertEqual(repo1_revlist, repo2_revlist)
> +        self.assertEqual(resp.status_code, 200)
> +        self.assertIn(new_repo_path, resp.json['repo_url'])
>  
>      def test_repo_delete(self):
>          self.app.post_json('/repo', {'repo_path': self.repo_path})
> 
> === modified file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	2015-03-13 03:44:09 +0000
> +++ turnip/api/tests/test_helpers.py	2015-03-24 21:30:45 +0000
> @@ -8,10 +8,21 @@
>      GIT_OBJ_COMMIT,
>      GIT_SORT_TIME,
>      IndexEntry,
> +    Repository,
>      Signature,
>      )
>  
>  
> +def get_revlist(repo):
> +    """Return revlist for a given pygit2 repo object."""
> +    return [commit.oid.hex for commit in repo.walk(repo.head.target)]
> +
> +
> +def open_repo(repo_path):
> +    """Return a pygit2 repo object for a given path."""
> +    return Repository(repo_path)
> +
> +
>  class RepoFactory():
>      """Builds a git repository in a user defined state."""
>  
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-03-13 03:44:09 +0000
> +++ turnip/api/views.py	2015-03-24 21:30:45 +0000
> @@ -11,6 +11,11 @@
>  from turnip.api import store
>  
>  
> +def is_valid_path(repo_store, repo_path):
> +    """Ensure path in within repo root and has not been subverted."""
> +    return os.path.realpath(repo_path).startswith(os.path.realpath(repo_store))
> +
> +
>  def repo_path(func):
>      """Decorator builds repo path from request name and repo_store."""
>      def repo_path_decorator(self):
> @@ -19,10 +24,10 @@
>              self.request.errors.add('body', 'name', 'repo name is missing')
>              return
>          repo_path = os.path.join(self.repo_store, name)
> -        if not os.path.realpath(repo_path).startswith(
> -                os.path.realpath(self.repo_store)):
> +        if not is_valid_path(self.repo_store, repo_path):
>              self.request.errors.add('body', 'name', 'invalid path.')
>              raise exc.HTTPInternalServerError()
> +
>          return func(self, repo_path)
>      return repo_path_decorator
>  
> @@ -42,15 +47,28 @@
>          self.request = request
>  
>      def collection_post(self):
> -        """Initialise a new git repository."""
> +        """Initialise a new git repository, or clone from an existing repo."""
>          repo_path = extract_json_data(self.request).get('repo_path')
> +        clone_path = extract_json_data(self.request).get('clone_path')
> +
>          if not repo_path:
>              self.request.errors.add('body', 'repo_path',
>                                      'repo_path is missing')
>              return
>          repo = os.path.join(self.repo_store, repo_path)
> +        if not is_valid_path(self.repo_store, repo):
> +            self.request.errors.add('body', 'name', 'invalid path.')
> +            raise exc.HTTPInternalServerError()

This could probably be a 400 rather than a 500.

> +
> +        if clone_path:
> +            repo_clone = os.path.join(self.repo_store, clone_path)
> +        else:
> +            repo_clone = None
> +
>          try:
> -            store.init_repo(repo)
> +            new_repo_path = store.init_repo(repo, repo_clone)

This is weirdly reversed again. When POSTing to /repos, repo_path is the new path except in the clone case. clone should be identical to init, except that there's an additional argument which specifies where to clone from.

> +            repo_name = os.path.basename(os.path.normpath(new_repo_path))
> +            return {'repo_url': '/'.join([self.request.url, repo_name])}
>          except GitError:
>              return exc.HTTPConflict()  # 409
>  
> 


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