Merge lp:~zyga/tarmac/git-support into lp:tarmac

[Zygmunt Krynicki]

aa97679 Rename Branch to BzrBranch
fb93a53 Ignore .pyc files

This branch is meant to get started with adding git support to tarmac. I'd like to see how conductive upstream is with this move and start making the necessary changes.

[dobey (dobey)]

This specific change looks OK to me, but it implies there is an idea about how to get to the end goal. I'd like to know what the ideas of how to get to the end goal of this are, before landing this change, though.

There's a lot of ugliness in the current tests that I think could/should be cleaned up first, for example. Abstracting the bzr integration away and switching to use mocking in the tests, instead of creating actual branches and performing actions in them, will make things more reliable, and easy to repeat across VCS implementations.

review: Needs Information

[Zygmunt Krynicki (zyga)]

Thanks for the feedback!

I'll continue working on this and I'll be pushing more branches as I
make progress. I'll focus on separating the bzr bits so that git can
co-exist. As for testing, do you intend to have a single set of tests
that work for git and bzr together?

Thanks
ZK

On Tue, Jul 7, 2015 at 7:14 PM, Rodney Dawes <email address hidden> wrote:
> Review: Needs Information
>
> This specific change looks OK to me, but it implies there is an idea about how to get to the end goal. I'd like to know what the ideas of how to get to the end goal of this are, before landing this change, though.
>
> There's a lot of ugliness in the current tests that I think could/should be cleaned up first, for example. Abstracting the bzr integration away and switching to use mocking in the tests, instead of creating actual branches and performing actions in them, will make things more reliable, and easy to repeat across VCS implementations.
> --
> https://code.launchpad.net/~zyga/tarmac/git-support/+merge/261528
> You are the owner of lp:~zyga/tarmac/git-support.

[dobey (dobey)]

I think for the tests, each layer will need to be tested separately. The
abstraction would have tests, the underlying bzrlib back-end with its
own tests, and the git back-end with its tests.

On Tue, 2015-07-07 at 17:27 +0000, Zygmunt Krynicki wrote:
> Thanks for the feedback!
>
> I'll continue working on this and I'll be pushing more branches as I
> make progress. I'll focus on separating the bzr bits so that git can
> co-exist. As for testing, do you intend to have a single set of tests
> that work for git and bzr together?
>
> Thanks
> ZK
>
> On Tue, Jul 7, 2015 at 7:14 PM, Rodney Dawes <email address hidden> wrote:
> > Review: Needs Information
> >
> > This specific change looks OK to me, but it implies there is an idea about how to get to the end goal. I'd like to know what the ideas of how to get to the end goal of this are, before landing this change, though.
> >
> > There's a lot of ugliness in the current tests that I think could/should be cleaned up first, for example. Abstracting the bzr integration away and switching to use mocking in the tests, instead of creating actual branches and performing actions in them, will make things more reliable, and easy to repeat across VCS implementations.
> > --
> > https://code.launchpad.net/~zyga/tarmac/git-support/+merge/261528
> > You are the owner of lp:~zyga/tarmac/git-support.
>

Unmerged revisions

434. By Zygmunt Krynicki on 2015-06-09

Ignore .pyc files

Signed-off-by: Zygmunt Krynicki <email address hidden>

433. By Zygmunt Krynicki on 2015-06-09

Rename Branch to BzrBranch

This patch is meant to test the waters. My goal is to add git support to
tarmac. This change is a harmless change of tarmac.branch.Branch to
tarmac.branch.BzrBranch, this is in anticipation of subsequent add to
GitBranch and subsequent changes as they show up.

Signed-off-by: Zygmunt Krynicki <email address hidden>

=== modified file '.bzrignore'
--- .bzrignore	2013-12-05 22:28:15 +0000
+++ .bzrignore	2015-06-09 15:08:40 +0000
@@ -5,3 +5,4 @@
 TODO
 debug*
 tarmac.egg-info
+*.pyc
 
=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2014-02-24 23:38:37 +0000
+++ tarmac/bin/commands.py	2015-06-09 15:08:40 +0000
@@ -27,7 +27,7 @@
     STAGING_SERVICE_ROOT)
 
 from tarmac.bin import options
-from tarmac.branch import Branch
+from tarmac.branch import BzrBranch
 from tarmac.hooks import tarmac_hooks
 from tarmac.log import set_up_debug_logging, set_up_logging
 from tarmac.exceptions import (
@@ -226,8 +226,8 @@
             return
 
         try:
-            target = Branch.create(lp_branch, self.config, create_tree=True,
-                                   launchpad=self.launchpad)
+            target = BzrBranch.create(lp_branch, self.config, create_tree=True,
+                                      launchpad=self.launchpad)
         except TarmacMergeError as failure:
             self._handle_merge_error(proposals[0], failure)
             return
@@ -267,7 +267,7 @@
                             u'No approved revision specified.')
 
 
-                    source = Branch.create(
+                    source = BzrBranch.create(
                         proposal.source_branch, self.config, target=target)
 
                     approved = source.bzr_branch.revision_id_to_revno(
@@ -449,7 +449,7 @@
             self.logger.debug('%(branch_url)s specified as branch_url' % {
                 'branch_url': branch_url})
             if not branch_url.startswith('lp:'):
-                raise TarmacCommandError('Branch urls must start with lp:')
+                raise TarmacCommandError('Bzr branch urls must start with lp:')
             self._do_merges(branch_url, source_mp=proposal)
 
         else:
 
=== modified file 'tarmac/branch.py'
--- tarmac/branch.py	2013-11-07 17:28:53 +0000
+++ tarmac/branch.py	2015-06-09 15:08:40 +0000
@@ -32,7 +32,7 @@
 )
 
 
-class Branch(object):
+class BzrBranch(object):
 
     def __init__(self, lp_branch, config=False, target=None, launchpad=None):
         self.lp_branch = lp_branch
@@ -112,7 +112,7 @@
         self.tree.update()
 
     def merge(self, branch, revid=None):
-        '''Merge from another tarmac.branch.Branch instance.'''
+        '''Merge from another tarmac.branch.BzrBranch instance.'''
         assert self.tree
         conflict_list = self.tree.merge_from_branch(
             branch.bzr_branch, to_revision=revid)
 
=== modified file 'tarmac/tests/__init__.py'
--- tarmac/tests/__init__.py	2013-12-05 22:28:15 +0000
+++ tarmac/tests/__init__.py	2015-06-09 15:08:40 +0000
@@ -203,13 +203,13 @@
         self.add_branch_config(branch2_dir)
 
         mock1 = MockLPBranch(branch1_dir)
-        branch1 = branch.Branch.create(mock1, self.config, create_tree=True)
+        branch1 = branch.BzrBranch.create(mock1, self.config, create_tree=True)
         branch1.commit("Reading, 'riting, 'rithmetic")
         branch1.lp_branch.revision_count += 1
 
         mock2 = MockLPBranch(branch2_dir, source_branch=branch1.lp_branch)
-        branch2 = branch.Branch.create(mock2, self.config, create_tree=True,
-                                       target=branch1)
+        branch2 = branch.BzrBranch.create(mock2, self.config, create_tree=True,
+                                          target=branch1)
         branch2.commit('ABC...')
 
         added_file = os.path.join(branch2.lp_branch.tree_dir, 'README')
 
=== modified file 'tarmac/tests/test_branch.py'
--- tarmac/tests/test_branch.py	2013-11-07 17:28:53 +0000
+++ tarmac/tests/test_branch.py	2015-06-09 15:08:40 +0000
@@ -33,15 +33,15 @@
 
 
 class TestBranch(BranchTestCase):
-    '''Test for Tarmac.branch.Branch.'''
+    '''Test for Tarmac.branch.BzrBranch.'''
 
     def test_create(self):
         '''Test the creation of a TarmacBranch instance.'''
         tree_dir = os.path.join(self.TEST_ROOT, 'test_create')
         self.add_branch_config(tree_dir)
 
-        a_branch = branch.Branch.create(MockLPBranch(tree_dir), self.config)
-        self.assertTrue(isinstance(a_branch, branch.Branch))
+        a_branch = branch.BzrBranch.create(MockLPBranch(tree_dir), self.config)
+        self.assertTrue(isinstance(a_branch, branch.BzrBranch))
         self.assertTrue(a_branch.lp_branch.bzr_identity is not None)
         self.assertFalse(hasattr(a_branch, 'tree'))
         self.remove_branch_config(tree_dir)
@@ -58,16 +58,16 @@
         # TarmacDirectoryFactory mocking will work.
         mock = MockLPBranch(os.path.join(self.TEST_ROOT, branch_name))
         self.assertFalse(os.path.exists(parent_dir))
-        a_branch = branch.Branch.create(mock, self.config, create_tree=True)
+        a_branch = branch.BzrBranch.create(mock, self.config, create_tree=True)
         self.assertTrue(os.path.exists(parent_dir))
-        self.assertTrue(isinstance(a_branch, branch.Branch))
+        self.assertTrue(isinstance(a_branch, branch.BzrBranch))
         self.assertTrue(a_branch.lp_branch.bzr_identity is not None)
         self.assertTrue(hasattr(a_branch, 'tree'))
         self.remove_branch_config(tree_dir)
 
     def test_create_with_tree(self):
         '''Test the creation of a TarmacBranch with a created tree.'''
-        self.assertTrue(isinstance(self.branch1, branch.Branch))
+        self.assertTrue(isinstance(self.branch1, branch.BzrBranch))
         self.assertTrue(self.branch1.lp_branch.bzr_identity is not None)
         self.assertTrue(hasattr(self.branch1, 'tree'))
 
@@ -75,7 +75,7 @@
         '''A merge on a branch with no tree will raise an exception.'''
         branch3_dir = os.path.join(self.TEST_ROOT, 'branch3')
         self.add_branch_config(branch3_dir)
-        branch3 = branch.Branch.create(MockLPBranch(
+        branch3 = branch.BzrBranch.create(MockLPBranch(
                 branch3_dir, source_branch=self.branch1.lp_branch),
                                        self.config)
 
@@ -89,7 +89,7 @@
         changes are present.'''
         branch3_dir = os.path.join(self.TEST_ROOT, 'branch3')
         self.add_branch_config(branch3_dir)
-        branch3 = branch.Branch.create(MockLPBranch(
+        branch3 = branch.BzrBranch.create(MockLPBranch(
                 branch3_dir, source_branch=self.branch1.lp_branch),
                                        self.config)
 
@@ -214,13 +214,13 @@
                           self.branch2.commit,
                           'Authors Merge test', authors=['\n'.join(authors)])
 
-    @patch('tarmac.branch.bzr_branch.Branch.user_url')
+    @patch('tarmac.branch.bzr_branch.BzrBranch.user_url')
     def test_create_tree_invalid_workingtree(self, mocked):
         """Test that InvalidWorkingTree is raised when it should be."""
         tree_dir = os.path.join(self.TEST_ROOT, 'test_invalid_workingtree')
         self.add_branch_config(tree_dir)
         self.addCleanup(self.remove_branch_config, tree_dir)
-        a_branch = branch.Branch(MockLPBranch(tree_dir), self.config)
+        a_branch = branch.BzrBranch(MockLPBranch(tree_dir), self.config)
         a_branch.bzr_branch.user_url = 'lp:invalid'
         self.assertRaises(InvalidWorkingTree,
                           a_branch.create_tree)
@@ -233,7 +233,7 @@
         self.addCleanup(shutil.rmtree, tree_dir)
 
         committer = 'LP User'
-        a_branch = branch.Branch.create(MockLPBranch(tree_dir), self.config,
+        a_branch = branch.BzrBranch.create(MockLPBranch(tree_dir), self.config,
                                         create_tree=True, launchpad=Thing(
                                             me=Thing(display_name=committer)))
         a_branch.commit('Test LP User.')
 
=== modified file 'tarmac/tests/test_commands.py'
--- tarmac/tests/test_commands.py	2014-02-24 23:41:32 +0000
+++ tarmac/tests/test_commands.py	2015-06-09 15:08:40 +0000
@@ -22,7 +22,7 @@
 from mock import patch, MagicMock
 from tarmac.bin import commands
 from tarmac.bin.registry import CommandRegistry
-from tarmac.branch import Branch
+from tarmac.branch import BzrBranch
 from tarmac.config import TarmacConfig
 from tarmac.exceptions import (
     InvalidWorkingTree,
@@ -190,8 +190,8 @@
         # Create a 3rd branch we'll use to test with
         branch3_dir = os.path.join(self.TEST_ROOT, name)
         mock3 = MockLPBranch(branch3_dir, source_branch=self.branch1.lp_branch)
-        branch3 = Branch.create(mock3, self.config, create_tree=True,
-                                target=self.branch1)
+        branch3 = BzrBranch.create(mock3, self.config, create_tree=True,
+                                   target=self.branch1)
         branch3.commit('Prerequisite commit.')
         branch3.lp_branch.revision_count += 1
 
@@ -270,8 +270,8 @@
         # Create a 3rd prerequisite branch we'll use to test with
         branch3_dir = os.path.join(self.TEST_ROOT, 'branch3')
         mock3 = MockLPBranch(branch3_dir, source_branch=self.branch1.lp_branch)
-        branch3 = Branch.create(mock3, self.config, create_tree=True,
-                                target=self.branch1)
+        branch3 = BzrBranch.create(mock3, self.config, create_tree=True,
+                                   target=self.branch1)
         branch3.commit('Prerequisite commit.')
         branch3.lp_branch.revision_count += 1
 
@@ -316,8 +316,8 @@
         # Create a 3rd prerequisite branch we'll use to test with
         branch3_dir = os.path.join(self.TEST_ROOT, 'branch3')
         mock3 = MockLPBranch(branch3_dir, source_branch=self.branch1.lp_branch)
-        branch3 = Branch.create(mock3, self.config, create_tree=True,
-                                target=self.branch1)
+        branch3 = BzrBranch.create(mock3, self.config, create_tree=True,
+                                   target=self.branch1)
         branch3.commit('Prerequisite commit.')
         branch3.lp_branch.revision_count += 1
 
@@ -365,8 +365,8 @@
         # Create a 3rd prerequisite branch we'll use to test with
         branch3_dir = os.path.join(self.TEST_ROOT, 'branch3')
         mock3 = MockLPBranch(branch3_dir, source_branch=self.branch1.lp_branch)
-        branch3 = Branch.create(mock3, self.config, create_tree=True,
-                                target=self.branch1)
+        branch3 = BzrBranch.create(mock3, self.config, create_tree=True,
+                                   target=self.branch1)
         branch3.commit('Prerequisite commit.')
         branch3.lp_branch.revision_count += 1