launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18392
Re: [Merge] lp:~blr/turnip/api-init-with-alternates into lp:turnip
Review: Needs Fixing code
Diff comments:
> === modified file 'turnip/api/store.py'
> --- turnip/api/store.py 2015-04-25 17:46:51 +0000
> +++ turnip/api/store.py 2015-04-28 10:11:43 +0000
> @@ -1,8 +1,10 @@
> # Copyright 2015 Canonical Ltd. All rights reserved.
>
> +from contextlib import contextmanager
> import itertools
> import os
> import shutil
> +import uuid
>
> from pygit2 import (
> clone_repository,
> @@ -63,6 +65,10 @@
> }
>
>
> +def is_bare_repo(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 +76,25 @@
> return True
>
>
> -def init_repo(repo_path, clone_from=None, clone_refs=False, 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_repo(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, clone_refs=False,
> + alternate_repo_paths=None, is_bare=True):
> """Initialise a new git repository or clone from existing."""
> assert is_valid_new_path(repo_path)
> init_repository(repo_path, is_bare)
> @@ -97,12 +121,41 @@
> for ref in from_repo.listall_references():
> to_repo.create_reference(
> ref, from_repo.lookup_reference(ref).target)
> +
> + if alternate_repo_paths:
> + write_alternates(repo_path, alternate_repo_paths)
> return repo_path
>
>
> +@contextmanager
> def open_repo(repo_path):
> - """Open an existing git repository."""
> - return Repository(repo_path)
> + """Open an existing git repository. Optionally create an
> + ephemeral repository with alternates if repo_path contains ':'.
> + """
> + (repo_store, name) = os.path.split(repo_path)
This seems prone to failure. Can you pass the store root and repo name as separate arguments?
> + if ':' in name:
> + try:
> + # create ephemeral repo with alternates set from both
> + repos = [os.path.join(repo_store, repo)
> + for repo in name.split(':')]
> + tmp_repo_path = os.path.join(repo_store,
> + 'ephemeral-' + uuid.uuid4().hex)
> + ephemeral_repo_path = init_repo(
> + tmp_repo_path,
> + alternate_repo_paths=repos)
> + repo = Repository(ephemeral_repo_path)
> + repo.ephemeral = True
> + yield repo
> + finally:
> + cleanup_repo(repo)
> + else:
> + yield Repository(repo_path)
> +
> +
> +def cleanup_repo(repo):
> + """Remove ephemeral repo."""
> + if hasattr(repo, 'ephemeral'):
> + delete_repo(repo.path)
This function and repo.ephemeral can go away now that it's only called in the ephemeral case. You can just use delete_repo directly.
>
>
> def delete_repo(repo_path):
> @@ -112,27 +165,27 @@
>
> def get_refs(repo_path):
> """Return all refs for a git repository."""
> - repo = open_repo(repo_path)
> - refs = {}
> - for ref in repo.listall_references():
> - git_object = repo.lookup_reference(ref).peel()
> - # Filter non-unicode refs, as refs are treated as unicode
> - # given json is unable to represent arbitrary byte strings.
> - try:
> - ref.decode('utf-8')
> - except UnicodeDecodeError:
> - pass
> - else:
> - refs.update(format_ref(ref, git_object))
> - return refs
> + with open_repo(repo_path) as repo:
> + refs = {}
> + for ref in repo.listall_references():
> + git_object = repo.lookup_reference(ref).peel()
> + # Filter non-unicode refs, as refs are treated as unicode
> + # given json is unable to represent arbitrary byte strings.
> + try:
> + ref.decode('utf-8')
> + except UnicodeDecodeError:
> + pass
> + else:
> + refs.update(format_ref(ref, git_object))
> + return refs
>
>
> def get_ref(repo_path, ref):
> """Return a specific ref for a git repository."""
> - repo = open_repo(repo_path)
> - git_object = repo.lookup_reference(ref.encode('utf-8')).peel()
> - ref_obj = format_ref(ref, git_object)
> - return ref_obj
> + with open_repo(repo_path) as repo:
> + git_object = repo.lookup_reference(ref.encode('utf-8')).peel()
> + ref_obj = format_ref(ref, git_object)
> + return ref_obj
>
>
> def get_common_ancestor_diff(repo_path, sha1_target, sha1_source,
> @@ -143,9 +196,9 @@
> :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, context_lines)
> + with open_repo(repo_path) as repo:
> + common_ancestor = repo.merge_base(sha1_target, sha1_source)
> + return get_diff(repo_path, common_ancestor, sha1_source, context_lines)
>
>
> def get_merge_diff(repo_path, sha1_base, sha1_head, context_lines=3):
> @@ -155,23 +208,25 @@
> :param sha1_head: source sha1 for merge.
> :param context_lines: num unchanged lines that define a hunk boundary.
> """
> - repo = open_repo(repo_path)
> - merged_index = repo.merge_commits(sha1_base, sha1_head)
> - conflicts = set()
> - if merged_index.conflicts is not None:
> - for conflict in list(merged_index.conflicts):
> - path = [entry for entry in conflict if entry is not None][0].path
> - conflicts.add(path)
> - merged_file = repo.merge_file_from_index(*conflict)
> - blob_oid = repo.create_blob(merged_file)
> - merged_index.add(IndexEntry(path, blob_oid, GIT_FILEMODE_BLOB))
> - del merged_index.conflicts[path]
> - diff = merged_index.diff_to_tree(
> - repo[sha1_base].tree, context_lines=context_lines).patch
> - shas = [sha1_base, sha1_head]
> - commits = [get_commit(repo_path, sha, repo) for sha in shas]
> - diff = {'commits': commits, 'patch': diff, 'conflicts': sorted(conflicts)}
> - return diff
> + with open_repo(repo_path) as repo:
> + merged_index = repo.merge_commits(sha1_base, sha1_head)
> + conflicts = set()
> + if merged_index.conflicts is not None:
> + for conflict in list(merged_index.conflicts):
> + path = [entry for entry in conflict
> + if entry is not None][0].path
> + conflicts.add(path)
> + merged_file = repo.merge_file_from_index(*conflict)
> + blob_oid = repo.create_blob(merged_file)
> + merged_index.add(IndexEntry(path, blob_oid, GIT_FILEMODE_BLOB))
> + del merged_index.conflicts[path]
> + diff = merged_index.diff_to_tree(
> + repo[sha1_base].tree, context_lines=context_lines).patch
> + shas = [sha1_base, sha1_head]
> + commits = [get_commit(repo_path, sha, repo) for sha in shas]
> + diff = {'commits': commits, 'patch': diff,
> + 'conflicts': sorted(conflicts)}
> + return diff
>
>
> def get_diff(repo_path, sha1_from, sha1_to, context_lines=3):
> @@ -181,15 +236,15 @@
> :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
> + with open_repo(repo_path) as repo:
> + 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
> + return diff
>
>
> def get_log(repo_path, start=None, limit=None, stop=None):
> @@ -199,37 +254,34 @@
> :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 > 0:
> - walker = itertools.islice(walker, limit)
> - commits = [format_commit(commit) for commit in walker]
> - return commits
> + with open_repo(repo_path) as repo:
> + 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 > 0:
> + walker = itertools.islice(walker, limit)
> + return [format_commit(commit) for commit in walker]
>
>
> def get_commit(repo_path, commit_oid, repo=None):
> """Return a single commit object from an oid."""
> - if not repo:
> - repo = open_repo(repo_path)
> - git_object = repo.get(commit_oid)
> - if git_object is None:
> - raise GitError('Object {} does not exist in repository {}.'.format(
> - commit_oid, repo_path))
> - commit = format_commit(git_object)
> - return commit
> + with open_repo(repo_path) as repo:
This now ignores the repo argument. If nothing uses it, it can be removed from the declaration.
> + git_object = repo.get(commit_oid)
> + if git_object is None:
> + raise GitError('Object {} does not exist in repository {}.'.format(
> + commit_oid, repo_path))
> + return format_commit(git_object)
>
>
> def get_commits(repo_path, commit_oids):
> """Return a collection of commit objects from a list of oids."""
> - repo = open_repo(repo_path)
> - commits = []
> - for commit in commit_oids:
> - try:
> - commits.append(get_commit(repo_path, commit, repo))
> - except GitError:
> - pass
> - return commits
> + with open_repo(repo_path) as repo:
> + commits = []
> + for commit in commit_oids:
> + try:
> + commits.append(get_commit(repo_path, commit, repo))
> + except GitError:
> + pass
> + return commits
>
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py 2015-04-27 13:16:08 +0000
> +++ turnip/api/tests/test_api.py 2015-04-28 10:11:43 +0000
> @@ -74,6 +74,59 @@
> self.assertEqual(200, resp.status_code)
> self.assertIn(new_repo_path, resp.json['repo_url'])
>
> + def test_cross_repo_merge_diff(self):
> + """Merge diff can be requested across 2 repositories."""
> + factory = RepoFactory(self.repo_store)
> + c1 = factory.add_commit('foo', 'foobar.txt')
> + factory.set_head(c1)
> +
> + repo2_name = uuid.uuid4().hex
> + factory2 = RepoFactory(
> + os.path.join(self.repo_root, repo2_name), clone_from=factory)
> + c2 = factory.add_commit('bar', 'foobar.txt', parents=[c1])
> + c3 = factory2.add_commit('baz', 'foobar.txt', parents=[c1])
> +
> + resp = self.app.get('/repo/{}:{}/compare-merge/{}:{}'.format(
> + self.repo_path, repo2_name, c2, c3))
> + self.assertIn('-bar', resp.json['patch'])
> +
> + def test_cross_repo_diff(self):
> + """Diff can be requested across 2 repositories."""
> + factory = RepoFactory(self.repo_store)
> + c1 = factory.add_commit('foo', 'foobar.txt')
> + factory.set_head(c1)
> +
> + repo2_name = uuid.uuid4().hex
> + factory2 = RepoFactory(
> + os.path.join(self.repo_root, repo2_name), clone_from=factory)
> + c2 = factory.add_commit('bar', 'foobar.txt', parents=[c1])
> + c3 = factory2.add_commit('baz', 'foobar.txt', parents=[c1])
> +
> + resp = self.app.get('/repo/{}:{}/compare/{}..{}'.format(
> + self.repo_path, repo2_name, c2, c3))
> + self.assertIn('-bar', resp.json['patch'])
> + self.assertIn('+baz', resp.json['patch'])
> +
> + def test_cross_repo_diff_invalid_repo(self):
> + """Cross repo diff with invalid repo returns HTTP 404."""
> + resp = self.app.get('/repo/1:2/compare-merge/3:4', expect_errors=True)
> + self.assertEqual(404, resp.status_code)
> +
> + def test_cross_repo_diff_invalid_commit(self):
> + """Cross repo diff with an invalid commit returns HTTP 404."""
> + factory = RepoFactory(self.repo_store)
> + c1 = factory.add_commit('foo', 'foobar.txt')
> + factory.set_head(c1)
> +
> + repo2_name = uuid.uuid4().hex
> + RepoFactory(
> + os.path.join(self.repo_root, repo2_name), clone_from=factory)
> + c2 = factory.add_commit('bar', 'foobar.txt', parents=[c1])
> +
> + resp = self.app.get('/repo/{}:{}/diff/{}:{}'.format(
> + self.repo_path, repo2_name, c2, 'invalid'), expect_errors=True)
> + self.assertEqual(404, resp.status_code)
> +
> def test_repo_delete(self):
> self.app.post_json('/repo', {'repo_path': self.repo_path})
> resp = self.app.delete('/repo/{}'.format(self.repo_path))
> @@ -244,12 +297,15 @@
> """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(
> + resp = self.app.get('/repo/{}/compare-merge/{}:{}'.format(
> self.repo_path, c3_right, c3_left))
> self.assertIn(' quux', resp.json_body['patch'])
> self.assertIn('-baz', resp.json_body['patch'])
> @@ -264,7 +320,7 @@
> c2_left = repo.add_commit('foo\nbar\n', 'blah.txt', parents=[c1])
> c2_right = repo.add_commit('foo\nbaz\n', 'blah.txt', parents=[c1])
>
> - resp = self.app.get('/repo/{}/compare-merge/{}/{}'.format(
> + resp = self.app.get('/repo/{}/compare-merge/{}:{}'.format(
> self.repo_path, c2_left, c2_right))
> self.assertIn(dedent("""\
> +<<<<<<< blah.txt
>
> === 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-28 10:11:43 +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):
> @@ -62,6 +68,9 @@
> blob_content, tree_id, parents)
> return oid
>
> + def set_head(self, oid):
> + self.repo.create_reference('refs/heads/master', oid)
> +
> def add_branch(self, name, oid):
> commit = self.repo.get(oid)
> branch = self.repo.create_branch('branch-{}'.format(name), commit)
> @@ -133,6 +142,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:
>
> === modified file 'turnip/api/tests/test_store.py'
> --- turnip/api/tests/test_store.py 2015-04-25 12:50:07 +0000
> +++ turnip/api/tests/test_store.py 2015-04-28 10:11:43 +0000
> @@ -8,20 +8,29 @@
>
> import os.path
> import subprocess
> +import uuid
>
> -from fixtures import TempDir
> +from fixtures import (
> + EnvironmentVariable,
> + TempDir,
> + )
> import pygit2
> from testtools import TestCase
>
> from turnip.api import store
> -from turnip.api.tests.test_helpers import RepoFactory
> +from turnip.api.tests.test_helpers import (
> + open_repo,
> + RepoFactory,
> + )
>
>
> class InitTestCase(TestCase):
>
> def setUp(self):
> super(InitTestCase, self).setUp()
> - self.root = self.useFixture(TempDir()).path
> + 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 assertAllLinkCounts(self, link_count, path):
> count = 0
> @@ -44,19 +53,57 @@
> self.assertNotIn(absent, out)
>
> def makeOrig(self):
> - self.orig_path = os.path.join(self.root, 'orig/')
> + self.orig_path = os.path.join(self.repo_store, 'orig/')
> orig = RepoFactory(
> self.orig_path, num_branches=3, num_commits=2).build()
> self.orig_refs = orig.listall_references()
> self.master_oid = orig.lookup_reference('refs/heads/master').target
> self.orig_objs = os.path.join(self.orig_path, '.git/objects')
>
> + def assert_alternate_exists(self, alternate_path, repo_path):
> + """Assert alternate_path exists in alternates at repo_path."""
> + objects_path = '{}\n'.format(
> + os.path.join(alternate_path, 'objects'))
> + with open(store.alternates_path(repo_path), 'r') as alts:
> + alts_content = alts.read()
> + self.assertIn(objects_path, alts_content)
> +
> def test_from_scratch(self):
> - path = os.path.join(self.root, 'repo/')
> + path = os.path.join(self.repo_store, 'repo/')
> self.assertEqual(path, store.init_repo(path))
> r = pygit2.Repository(path)
> self.assertEqual([], r.listall_references())
>
> + def test_open_ephemeral_repo(self):
> + """Opening a repo where a repo name contains ':' should return
> + a new ephemeral repo.
> + """
> + repos = [uuid.uuid4().hex, uuid.uuid4().hex]
> + repo_name = '{}:{}'.format(repos[0], repos[1])
> + repo_path = os.path.join(self.repo_store, repo_name)
> + alt_path = os.path.join(self.repo_store, repos[0])
> + with store.open_repo(repo_path) as repo:
> + self.assert_alternate_exists(alt_path, repo.path)
> + self.assertTrue(repo.ephemeral)
> +
> + 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 = store.init_repo(
> + new_repo_path, alternate_repo_paths=[factory.repo.path])
> + self.assert_alternate_exists(factory.repo.path, repo_path_with_alt)
> +
> + 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 = store.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)
> +
> def test_clone_with_refs(self):
> self.makeOrig()
> self.assertAllLinkCounts(1, self.orig_objs)
> @@ -64,7 +111,7 @@
> # init_repo with clone_from=orig and clone_refs=True creates a
> # repo with the same set of refs. And the objects are copied
> # too.
> - to_path = os.path.join(self.root, 'to/')
> + to_path = os.path.join(self.repo_store, 'to/')
> store.init_repo(to_path, clone_from=self.orig_path, clone_refs=True)
> to = pygit2.Repository(to_path)
> self.assertIsNot(None, to[self.master_oid])
> @@ -88,7 +135,7 @@
>
> # init_repo with clone_from=orig and clone_refs=False creates a
> # repo without any refs, but the objects are copied.
> - to_path = os.path.join(self.root, 'to/')
> + to_path = os.path.join(self.repo_store, 'to/')
> store.init_repo(to_path, clone_from=self.orig_path, clone_refs=False)
> to = pygit2.Repository(to_path)
> self.assertIsNot(None, to[self.master_oid])
>
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py 2015-04-27 13:16:08 +0000
> +++ turnip/api/views.py 2015-04-28 10:11:43 +0000
> @@ -121,6 +121,7 @@
> Two dots provides a simple diff, equivalent to `git diff A B`.
> Three dots provides the symmetric or common ancestor diff, equivalent
> to `git diff $(git-merge-base A B) B`.
> + {name} can be two : separated repositories, for a cross repository diff.
> """
> def __init__(self, request):
> super(DiffAPI, self).__init__()
> @@ -146,11 +147,12 @@
> return patch
>
>
> -@resource(path='/repo/{name}/compare-merge/{base}/{head}')
> +@resource(path='/repo/{name}/compare-merge/{base}:{head}')
> class DiffMergeAPI(BaseAPI):
> """Provides an HTTP API for merge previews.
>
> {head} will be merged into {base} and the diff from {base} returned.
> + {name} can be two : separated repositories, for a cross repository diff.
> """
> def __init__(self, request):
> super(DiffMergeAPI, self).__init__()
>
--
https://code.launchpad.net/~blr/turnip/api-init-with-alternates/+merge/256715
Your team Launchpad code reviewers is subscribed to branch lp:turnip.
References
