Merge lp:~acsone-openerp/anybox.recipe.openerp/git-merges into lp:anybox.recipe.openerp

[Stéphane Bidoul (Acsone)]

git merges and vcs-revert support

[Laurent Mignon (Acsone) (lmi)]

Hi,

The code is really more natural and simple with more features covered!

LGTM,

review: Approve (code, tested)

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

LGTM as long as vcs-merge wouldn't be used to update a local clone of OCB if OCB goes for continuous git-rebase.

NOTE: a further improvement to come after this merge to support shallow clones:
https://code.launchpad.net/~akretion-team/anybox.recipe.openerp/git-shallow-clone/+merge/222980

review: Approve

[Georges Racinet (gracinet)]

So… this is a major refactor of the Git subsystem, together with the addition of the merge feature.

On the formal side, there is not a single existing test that had to be adapted (including the switch-branch ones).
It also leverages local idioms such that utils.check_output, or the function to issue a git commit (keeps proper insulation with other tests, works on dumb buildslaves…)

I believe the end result is indeed clearer, especially the relation with remote branches.

The only caveat is that test_git has some pep8 errors (the kind I'll fix in a second).

So, a big thank you, let's test this in the trunk !

review: Approve

=== modified file 'anybox/recipe/openerp/base.py'
--- anybox/recipe/openerp/base.py	2014-06-08 21:55:32 +0000
+++ anybox/recipe/openerp/base.py	2014-06-10 09:23:53 +0000
@@ -494,12 +494,14 @@
             if not split:
                 return
             loc_type = split[0]
-            if loc_type != 'bzr':
-                raise UserError("Only merges of type 'bzr' are "
+            if not loc_type in ('bzr', 'git'):
+                raise UserError("Only merges of type 'bzr' and 'git' are "
                                 "currently supported.")
             options = dict(opt.split('=') for opt in split[4:])
             if loc_type == 'bzr':
                 options['bzr-init'] = 'merge'
+            else:
+                options['merge'] = True
 
             repo_url, local_dir, repo_rev = split[1:4]
             location_spec = (repo_url, repo_rev)

=== modified file 'anybox/recipe/openerp/vcs/base.py'
--- anybox/recipe/openerp/vcs/base.py	2014-06-08 12:37:29 +0000
+++ anybox/recipe/openerp/vcs/base.py	2014-06-10 09:23:53 +0000
@@ -176,3 +176,6 @@
                                on the VCS type.
         """
         raise NotImplementedError
+
+    def archive(self, target_path):
+        raise NotImplementedError

=== modified file 'anybox/recipe/openerp/vcs/bzr.py'
--- anybox/recipe/openerp/vcs/bzr.py	2014-06-08 12:37:29 +0000
+++ anybox/recipe/openerp/vcs/bzr.py	2014-06-10 09:23:53 +0000
@@ -255,6 +255,9 @@
         If target_dir already exists, does a simple pull.
         Offline-mode: no branch nor pull, but update.
         In all cases, an attempt to update is performed before any pull
+
+        Special case: if the 'merge' option is True,
+        merge revision into current branch.
         """
         target_dir = self.target_dir
         offline = self.offline

=== modified file 'anybox/recipe/openerp/vcs/git.py'
--- anybox/recipe/openerp/vcs/git.py	2014-06-08 12:37:29 +0000
+++ anybox/recipe/openerp/vcs/git.py	2014-06-10 09:23:53 +0000
@@ -1,5 +1,4 @@
 import os
-import sys
 import subprocess
 import logging
 import tempfile
@@ -8,13 +7,14 @@
 from .base import BaseRepo
 from .base import SUBPROCESS_ENV
 from anybox.recipe.openerp import utils
-import re
 
 logger = logging.getLogger(__name__)
 
+BUILDOUT_ORIGIN = 'origin'
+
 
 class GitRepo(BaseRepo):
-    """Represent a Git clone tied to a reference branch."""
+    """Represent a Git clone tied to a reference branch/commit/tag."""
 
     vcs_control_dir = '.git'
 
@@ -46,50 +46,69 @@
             return bool(out.strip())
 
     def get_update(self, revision):
-        """Ensure that target_dir is a branch of url at specified revision.
+        """Make it so that the target directory is at the prescribed revision.
 
-        If target_dir already exists, does a simple fetch.
-        Offline-mode: no branch nor fetch, but checkout.
+        Special case: if the 'merge' option is True,
+        merge revision into current branch.
         """
+        if self.options.get('merge'):
+            return self.merge(revision)
+
         target_dir = self.target_dir
         url = self.url
         offline = self.offline
-        rev_str = revision
 
         with working_directory_keeper:
-            is_target_dir_exists = os.path.exists(target_dir)
-            if not is_target_dir_exists:
-                # TODO case of local url ?
+            if not os.path.exists(target_dir):
                 if offline:
+                    # TODO case of local url ?
                     raise IOError(
-                        "git repository %s does not exist; cannot clone it "
-                        "from %s (offline mode)" % (target_dir, url))
-
-                os.chdir(os.path.split(target_dir)[0])
-                logger.info("Cloning %s ...", url)
-                subprocess.check_call(['git', 'clone', url, target_dir])
-            os.chdir(target_dir)
-
-            if is_target_dir_exists:
+                        "git repository %s does not exist; cannot clone "
+                        "it from %s (offline mode)" % (target_dir, url))
+                logger.info("%s> git init", target_dir)
+                subprocess.check_call(['git', 'init', target_dir])
+                os.chdir(target_dir)
+                logger.info("%s> git remote add %s %s",
+                            target_dir, BUILDOUT_ORIGIN, url)
+                subprocess.check_call(['git', 'remote', 'add',
+                                       BUILDOUT_ORIGIN, url])
+
+            if not offline:
                 # TODO what if remote repo is actually local fs ?
-                if not offline:
-                    logger.info("Fetch for git repo %s (rev %s)...",
-                                target_dir, rev_str)
-                    subprocess.check_call(['git', 'fetch'])
-
-            if revision and self._needToSwitchRevision(revision):
-                # switch to the expected revision
-                logger.info("Checkout %s to revision %s",
-                            target_dir, revision)
-                self._switch(revision)
-
-            if self._isATrackedBranch(revision):
-                if not offline:
-                    logger.info("Pull for git repo %s (rev %s)...",
-                                target_dir, rev_str)
-                    subprocess.check_call(['git', 'pull'])
+                os.chdir(target_dir)
+                logger.info("%s> git remote set-url %s %s",
+                            target_dir, BUILDOUT_ORIGIN, url)
+                subprocess.call(['git', 'remote', 'set-url',
+                                 BUILDOUT_ORIGIN, url])
+                logger.info("%s> git fetch %s",
+                            target_dir, BUILDOUT_ORIGIN)
+                subprocess.check_call(['git', 'fetch', BUILDOUT_ORIGIN])
+                # TODO: check what happens when there are local changes
+                # TODO: what about the 'clean' option
+                logger.info("%s> git checkout %s", target_dir, revision)
+                subprocess.check_call(['git', 'checkout', revision])
+                if self._is_a_branch(revision):
+                    # fast forward
+                    logger.info("%s> git merge merge %s/%s",
+                                target_dir, BUILDOUT_ORIGIN, revision)
+                    subprocess.check_call(['git', 'merge',
+                                           BUILDOUT_ORIGIN + '/' + revision])
+
+    def merge(self, revision):
+        """Merge revision into current branch"""
+        with working_directory_keeper:
+            if not self.is_versioned(self.target_dir):
+                raise RuntimeError("Cannot merge into non existent "
+                                   "or non git local directory %s" %
+                                   self.target_dir)
+            os.chdir(self.target_dir)
+            logger.info("%s> git pull %s %s",
+                        self.target_dir, self.url, revision)
+            subprocess.check_call(['git', 'pull', '--no-edit',
+                                   self.url, revision])
 
     def archive(self, target_path):
+        # TODO: does this work with merge-ins?
         revision = self.parents()[0]
         if not os.path.exists(target_path):
             os.makedirs(target_path)
@@ -104,81 +123,17 @@
                                    '-C', target_path])
             os.unlink(target_tar.name)
 
-    def _isATrackedBranch(self, revision):
-        rbp = self._remote_branch_prefix
-        branches = utils.check_output(["git", "branch", "-a"])
-        branch = revision
-        return re.search(
-            "^  " + re.escape(rbp) + "\/" + re.escape(branch) + "$", branches,
-            re.M)
-
-    def _needToSwitchRevision(self, revision):
-        """ Check if we need to checkout to an other branch
-        """
-        p = utils.check_output(['git', 'rev-parse', '--abbrev-ref', 'HEAD'])
-        rev = p.split()[0]  # remove \n
-        logger.info("Current revision '%s' - Expected revision '%s'",
-                    rev, revision)
-        return rev != revision
-
-    def _switch(self, revision):
-        rbp = self._remote_branch_prefix
-        branches = utils.check_output(["git", "branch", "-a"])
-        branch = revision
-        if re.search("^(\*| ) %s$" % re.escape(branch), branches, re.M):
-            # the branch is local, normal checkout will work
-            logger.info("The branch is local; normal checkout ")
-            argv = ["checkout", branch]
-        elif re.search(
-            "^  " + re.escape(rbp) + "\/" + re.escape(branch) + "$", branches,
-                re.M):
-            # the branch is not local, normal checkout won't work here
-            logger.info("The branch is not local; checkout remote branch ")
-            argv = ["checkout", "-b", branch, "%s/%s" % (rbp, branch)]
-        else:
-            # A tag or revision was specified instead of a branch
-            logger.info("Checkout tag or revision")
-            argv = ["checkout", revision]
-        # runs the checkout with predetermined arguments
-        argv.insert(0, "git")
-        subprocess.check_call(argv)
-
-    @property
-    def _remote_branch_prefix(self):
-        version = self._git_version
-        if version < (1, 6, 3):
-            return "origin"
-        else:
-            return "remotes/origin"
-
-    @property
-    def _git_version(self):
-        out = utils.check_output(["git", "--version"])
-        m = re.search("git version (\d+)\.(\d+)(\.\d+)?(\.\d+)?", out)
-        if m is None:
-            logger.error("Unable to parse git version output")
-            logger.error("'git --version' output was:\n%s\n%s", out)
-            sys.exit(1)
-        version = m.groups()
-
-        if version[3] is not None:
-            version = (
-                int(version[0]),
-                int(version[1]),
-                int(version[2][1:]),
-                int(version[3][1:])
-            )
-        elif version[2] is not None:
-            version = (
-                int(version[0]),
-                int(version[1]),
-                int(version[2][1:])
-            )
-        else:
-            version = (int(version[0]), int(version[1]))
-        if version < (1, 5):
-            logger.error(
-                "Git version %s is unsupported, please upgrade",
-                ".".join([str(v) for v in version]))
-            sys.exit(1)
-        return version
+    def revert(self, revision):
+        with working_directory_keeper:
+            os.chdir(self.target_dir)
+            subprocess.check_call(['git', 'checkout', revision])
+            if self._is_a_branch(revision):
+                subprocess.check_call(['git', 'reset', '--hard',
+                                       BUILDOUT_ORIGIN + '/' + revision])
+            else:
+                subprocess.check_call(['git', 'reset', '--hard', revision])
+
+    def _is_a_branch(self, revision):
+        branches = utils.check_output(["git", "branch"])
+        branches = branches.split()
+        return revision in branches

=== modified file 'anybox/recipe/openerp/vcs/tests/test_git.py'
--- anybox/recipe/openerp/vcs/tests/test_git.py	2014-06-08 12:37:29 +0000
+++ anybox/recipe/openerp/vcs/tests/test_git.py	2014-06-10 09:23:53 +0000
@@ -36,8 +36,8 @@
 
     def create_src(self):
         os.chdir(self.src_dir)
-        subprocess.call(['git', 'init', 'src-branch'])
-        self.src_repo = os.path.join(self.src_dir, 'src-branch')
+        subprocess.call(['git', 'init', 'src-repo'])
+        self.src_repo = os.path.join(self.src_dir, 'src-repo')
         self.commit_1_sha = git_write_commit(self.src_repo, 'tracked',
                                              "first", msg="initial commit")
         self.commit_2_sha = git_write_commit(self.src_repo, 'tracked',
@@ -229,3 +229,84 @@
         branch("remotebranch")
         with open(os.path.join(target_dir, 'tracked')) as f:
             self.assertEquals(f.read().strip(), "last after remote branch")
+
+
+class GitMergeTestCase(GitBaseTestCase):
+
+    def create_src(self):
+        GitBaseTestCase.create_src(self)
+        os.chdir(self.src_repo)
+
+        self.make_branch(self.src_repo, 'branch1')
+        self.checkout_branch(self.src_repo, 'branch1')
+        git_write_commit(self.src_repo, 'file_on_branch1',
+                         "file on branch 1", msg="on branch 1")
+        self.checkout_branch(self.src_repo, 'master')
+
+        self.make_branch(self.src_repo, 'branch2')
+        self.checkout_branch(self.src_repo, 'branch2')
+        git_write_commit(self.src_repo, 'file_on_branch2',
+                         "file on branch 2", msg="on branch 2")
+        self.checkout_branch(self.src_repo, 'master')
+
+    def make_branch(self, src_dir, name):
+        """create a branch
+        """
+        subprocess.check_call(['git', 'branch', name], cwd=src_dir)
+
+    def checkout_branch(self, src_dir, name):
+        """checkout a branch
+        """
+        subprocess.check_call(['git', 'checkout', name], cwd=src_dir)
+
+    def test_01_check_src_repo(self):
+        """test if the creation of source repo worked as expected"""
+        target_dir = os.path.join(self.dst_dir, "to_repo")
+        repo = GitRepo(target_dir, self.src_repo)
+
+        repo('master')
+        self.assertFalse(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                         'file_on_branch1 should not exist')
+        self.assertFalse(os.path.exists(os.path.join(target_dir, 'file_on_branch2')),
+                         'file_on_branch2 should not exist')
+
+        repo('branch1')
+        self.assertTrue(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                         'file_on_branch1 should exist')
+        self.assertFalse(os.path.exists(os.path.join(target_dir, 'file_on_branch2')),
+                         'file_on_branch2 should not exist')
+
+        repo('branch2')
+        self.assertFalse(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                         'file_on_branch1 should not exist')
+        self.assertTrue(os.path.exists(os.path.join(target_dir, 'file_on_branch2')),
+                        'file_on_branch2 should exist')
+
+    def test_02_merge(self):
+        target_dir = os.path.join(self.dst_dir, "to_repo")
+        repo = GitRepo(target_dir, self.src_repo)
+        repo('master')
+
+        repo.merge('branch1')
+        self.assertTrue(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                        'file_on_branch1 should exist')
+        self.assertFalse(os.path.exists(os.path.join(target_dir, 'file_on_branch2')),
+                        'file_on_branch2 should not exist')
+        repo.merge('branch2')
+        self.assertTrue(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                        'file_on_branch1 should exist')
+        self.assertTrue(os.path.exists(os.path.join(target_dir, 'file_on_branch2')),
+                        'file_on_branch2 should exist')
+
+    def test_03_revert(self):
+        target_dir = os.path.join(self.dst_dir, "to_repo")
+        repo = GitRepo(target_dir, self.src_repo)
+        repo('master')
+
+        repo.merge('branch1')
+        self.assertTrue(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                        'file_on_branch1 should exist')
+
+        repo.revert('master')
+        self.assertFalse(os.path.exists(os.path.join(target_dir, 'file_on_branch1')),
+                         'file_on_branch1 should not exist')