[damned-lies] Abstract VCS commands inside specific repository classes

[Claude Paroz <claudep src gnome org>]

commit 072465e5f89bcdc1f1de74e1e01e7afb6e41c317
Author: Claude Paroz <claude 2xlibre net>
Date:   Sun Sep 30 21:22:08 2018 +0200

    Abstract VCS commands inside specific repository classes

 stats/models.py      | 201 ++++++++-------------------------------------------
 stats/repos.py       | 171 +++++++++++++++++++++++++++++++++++++++++++
 stats/tests/tests.py |   5 +-
 stats/tests/utils.py |   2 +
 4 files changed, 204 insertions(+), 175 deletions(-)
---
diff --git a/stats/models.py b/stats/models.py
index f82c3c22..3836ebc0 100644
--- a/stats/models.py
+++ b/stats/models.py
@@ -24,7 +24,7 @@ from django.db import models
 
 from common.fields import DictionaryField, JSONField
 from common.utils import is_site_admin, run_shell_command
-from stats import utils, signals
+from stats import repos, signals, utils
 from stats.doap import update_doap_infos
 from people.models import Person
 from languages.models import Language
@@ -43,7 +43,6 @@ VCS_TYPE_CHOICES = (
     ('svn', 'Subversion'),
     ('git', 'Git'),
     ('hg', 'Mercurial'),
-    ('bzr', 'Bazaar')
 )
 
 BRANCH_HEAD_NAMES = (
@@ -206,6 +205,11 @@ class Branch(models.Model):
     def __str__(self):
         return "%s (%s)" % (self.name, self.module)
 
+    @cached_property
+    def _repo(self):
+        repo_class = repos.RepoBase.repo_class_by_type(self.module.vcs_type)
+        return repo_class(self)
+
     def clean(self):
         if self.checkout_on_creation and not self.module.archived:
             try:
@@ -227,20 +231,7 @@ class Branch(models.Model):
 
     def delete_checkout(self):
         # Remove the repo checkout
-        if self.module.vcs_type in ('cvs', 'svn'):
-            if os.access(self.co_path, os.W_OK):
-                shutil.rmtree(self.co_path)
-        elif self.module.vcs_type == 'git':
-            wdir = self.co_path
-            if os.access(str(wdir), os.W_OK):
-                if self.is_head():
-                    shutil.rmtree(str(wdir))
-                else:
-                    with ModuleLock(self.module):
-                        run_shell_command(['git', 'checkout', 'master'], cwd=wdir)
-                        run_shell_command(['git', 'branch', '-D', self.name], cwd=wdir)
-        #To be implemented for hg/bzr
-
+        self._repo.remove()
         # Remove the pot/po generated files
         if os.access(str(self.output_dir('ui')), os.W_OK):
             shutil.rmtree(str(self.output_dir('ui')))
@@ -297,16 +288,6 @@ class Branch(models.Model):
         """ Return True if the branch only appears in 'archived' releases """
         return bool(self.releases.filter(weight__gt=0).count())
 
-    def get_vcs_url(self):
-        if self.module.vcs_type in ('hg', 'git'):
-            return self.module.vcs_root
-        elif self.vcs_subpath:
-            return utils.url_join(self.module.vcs_root, self.module.name, self.vcs_subpath)
-        elif self.is_head():
-            return utils.url_join(self.module.vcs_root, self.module.name, "trunk")
-        else:
-            return utils.url_join(self.module.vcs_root, self.module.name, "branches", self.name)
-
     def is_vcs_readonly(self):
         return 'ssh://' not in self.module.vcs_root and not self.module.vcs_root.startswith('git@')
 
@@ -536,148 +517,39 @@ class Branch(models.Model):
             if self.is_head() and self.file_changed("%s.doap" % self.module.name):
                 update_doap_infos(self.module)
 
-    def _exists(self):
-        """ Determine if branch (self) already exists (i.e. already checked out) on local FS """
-        if self.module.vcs_type == 'git':
-            if not self.co_path.exists():
-                return False
-            command = "git branch | grep %s" % self.name
-            status, output, errs = run_shell_command(command, cwd=self.co_path)
-            return output != ""
-        elif self.module.vcs_type == 'hg':
-            return self.id != None and os.access(str(self.co_path), os.X_OK | os.W_OK)
-        else:
-            return os.access(str(self.co_path), os.X_OK | os.W_OK)
-
     def checkout(self):
         """ Do a checkout or an update of the VCS files """
-        module_name = self.module.name
-        vcs_type = self.module.vcs_type
-        localroot = os.path.join(settings.SCRATCHDIR, vcs_type)
-        modulepath = self.co_path
-        scmroot = self.module.vcs_root
-
-        os.makedirs(localroot, exist_ok=True)
-        command_list = []
-        if self._exists():
-            command_list.extend(self.update_repo(execute=False))
-        else:
-            # Checkout
-            vcs_path = self.get_vcs_url()
-            if vcs_type in ('hg', 'git'):
-                moduledir = self.module.name
-            else:
-                moduledir = self.module.name + "." + self.name
-
-            if vcs_type == "cvs":
-                command_list.append((localroot,
-                    ['cvs', '-d%s' % scmroot, '-z4', 'co', '-d%s' % moduledir, '-r%s' % self.name,
-                      module_name]))
-            elif vcs_type == "svn":
-                command_list.append((localroot, ['svn', 'co', '--non-interactive', vcs_path, moduledir]))
-            elif vcs_type == "hg":
-                command_list.append((localroot, ['hg', 'clone', vcs_path, moduledir]))
-                command_list.append((modulepath, ['hg', 'update', self.name]))
-            elif vcs_type == "git":
-                # We are assuming here that master is the first branch created
-                if self.name == "master":
-                    command_list.append((localroot, ['git', 'clone', vcs_path, moduledir]))
-                    command_list.append((modulepath, ['git', 'remote', 'update']))
-                    command_list.append((modulepath, ['git', 'checkout', self.name]))
-                else:
-                    command_list.append((modulepath, ['git', 'pull']))
-                    command_list.append((modulepath, ['git', 'checkout', '--track', '-b', self.name, 
'origin/%s' % self.name]))
-                # check if there are any submodules and init & update them
-                command_list.append((modulepath, "if [ -e .gitmodules ]; then git submodule update --init; 
fi"))
-            elif vcs_type == "bzr":
-                command_list.append((localroot, ['bzr', 'co', '--lightweight', vcs_path, moduledir]))
-
-        # Run command(s)
-        logging.debug("Checking '%s.%s' out to '%s'..." % (module_name, self.name, modulepath))
-        for working_dir, command in command_list:
-            run_shell_command(command, raise_on_error=True, cwd=working_dir)
-
-    def update_repo(self, execute=True):
+        logging.debug("Checking '%s.%s' out to '%s'…" % (self.module.name, self.name, self.co_path))
+        self._repo.checkout()
+
+    def update_repo(self):
         """
         Update existing repository checkout.
-        WARNING: if execute is True, the calling method should acquire a lock
+        WARNING: the calling method should acquire a lock
         for the module to not mix checkouts in different threads/processes.
         """
-        modulepath = self.co_path
-        logging.debug("Updating '%s.%s' (in '%s')..." % (self.module.name, self.name, modulepath))
-        command_list = []
-        if self.module.vcs_type == "cvs":
-            command_list.append((modulepath, ['cvs', '-z4', 'up', '-Pd']))
-        elif self.module.vcs_type == "svn":
-            command_list.append((modulepath, ['svn', 'up', '--non-interactive']))
-        elif self.module.vcs_type == "hg":
-            command_list.append((modulepath, ['hg', 'revert', '--all']))
-        elif self.module.vcs_type == "git":
-            # tester "git checkout %(branch)s && git clean -dfq && git pull origin/%(branch)s"
-            command_list.append((modulepath, ['git', 'checkout', '-f', self.name]))
-            command_list.append((modulepath, ['git', 'fetch']))
-            command_list.append((modulepath, ['git', 'reset', '--hard', 'origin/%s' % self.name]))
-            command_list.append((modulepath, ['git', 'clean', '-dfq']))
-            # check if there are any submodules and init & update them
-            command_list.append((modulepath, "if [ -e .gitmodules ]; then git submodule update --init; fi"))
-        elif self.module.vcs_type == "bzr":
-            command_list.append((modulepath, ['bzr', 'up']))
-        if execute:
-            for working_dir, command in command_list:
-                run_shell_command(command, raise_on_error=True, cwd=working_dir)
-        else:
-            return command_list
+        logging.debug("Updating '%s.%s' (in '%s')..." % (self.module.name, self.name, self.co_path))
+        self._repo.update()
 
     def commit_po(self, po_file, domain, language, author):
         """ Commit the file 'po_file' in the branch VCS repository """
-        vcs_type = self.module.vcs_type
-        if vcs_type not in ("git",):
-            raise NotImplementedError("Commit is not implemented for '%s'" % vcs_type)
-
         dest_full_path, existing, linguas_path = domain.commit_info(self, language)
 
         with ModuleLock(self.module):
             self.update_repo()
-            if vcs_type == "git":
-                base_path = self.co_path
-
-                # Copy file in repo
-                shutil.copyfile(str(po_file), str(dest_full_path))
-
-                # git add file.po
-                run_shell_command(
-                    ['git', 'add', str(dest_full_path.relative_to(base_path))],
-                    raise_on_error=True, cwd=base_path
-                )
-                if linguas_path:
-                    # Add locale to LINGUAS
-                    utils.insert_locale_in_linguas(linguas_path, language.locale)
-                    run_shell_command(
-                        ['git', 'add', str(linguas_path.relative_to(base_path))],
-                        raise_on_error=True, cwd=base_path
-                    )
-                if existing:
-                    msg = "Update %s translation" % language.name
-                else:
-                    msg = "Add %s translation" % language.name
-
-                commit_cmd = ['git', 'commit', '-m', msg]
-                if author:
-                    commit_cmd.extend(['--author', author])
-                run_shell_command(commit_cmd, raise_on_error=True, cwd=base_path)
-                # git push
-                try:
-                    run_shell_command(
-                        ['git', 'push', 'origin', self.name], raise_on_error=True, cwd=base_path)
-                except OSError:
-                    # Revert the commit
-                    run_shell_command(
-                        ['git', 'reset', '--hard', 'origin/%s' % self.name], cwd=base_path)
-                    raise
-                else:
-                    _, out, _ = run_shell_command(
-                        ['git', 'log', '-n1', '--format=oneline'], cwd=base_path)
-                    commit_hash = out.split()[0] if out else ''
+            # Copy file in repo
+            shutil.copyfile(str(po_file), str(dest_full_path))
+
+            files_to_commit = [dest_full_path.relative_to(self.co_path)]
+            if linguas_path:
+                # Add locale to LINGUAS
+                utils.insert_locale_in_linguas(linguas_path, language.locale)
+                files_to_commit.append(linguas_path.relative_to(self.co_path))
+            if existing:
+                msg = "Update %s translation" % language.name
+            else:
+                msg = "Add %s translation" % language.name
+            commit_hash = self._repo.commit_files(files_to_commit, msg, author=author)
 
         # Finish by updating stats
         if existing:
@@ -696,21 +568,10 @@ class Branch(models.Model):
         Try to cherry-pick a branch commit `commit_hash` into master.
         Return True if the cherry-pick succeeds, False otherwise.
         """
-        if self.module.vcs_type != "git":
-            raise NotImplementedError("Commit cherry-pick is not implemented for '%s'" % 
self.module.vcs_type)
         with ModuleLock(self.module):
             self.update_repo()
-            commit_dir = self.co_path
-            result = run_shell_command(
-                ['git', 'cherry-pick', '-x', commit_hash], cwd=commit_dir)
-            if result[0] == utils.STATUS_OK:
-                run_shell_command(
-                    ['git', 'push', 'origin', self.name], raise_on_error=True, cwd=commit_dir)
-                return True
-            else:
-                # Revert
-                run_shell_command(['git', 'cherry-pick', '--abort'], cwd=commit_dir)
-                return False
+            success = self._repo.cherry_pick(commit_hash)
+        return success
 
 
 DOMAIN_TYPE_CHOICES = (
@@ -1595,10 +1456,6 @@ class Statistics(models.Model):
             stats['prc'] = 100*stats['translated']/stats['total']
         return stats
 
-    def vcs_path(self):
-        """ Return the VCS path of file on remote vcs """
-        return utils.url_join(self.branch.get_vcs_url(), self.domain.base_dir)
-
     def vcs_web_path(self):
         """ Return the Web interface path of file on remote vcs """
         return utils.url_join(self.branch.get_vcs_web_url(), self.domain.base_dir)
diff --git a/stats/repos.py b/stats/repos.py
new file mode 100644
index 00000000..6913a98e
--- /dev/null
+++ b/stats/repos.py
@@ -0,0 +1,171 @@
+import os
+import shutil
+
+from common.utils import run_shell_command
+from stats.utils import STATUS_OK
+
+
+class RepoBase:
+    @classmethod
+    def repo_class_by_type(cls, tp):
+        return {'cvs': CVSRepo, 'svn': SVNRepo, 'git': GitRepo, 'hg': MercurialRepo}.get(tp)
+
+    def __init__(self, branch):
+        self.branch = branch
+
+    def exists():
+        return os.access(str(self.branch.co_path), os.X_OK | os.W_OK)
+
+    def checkout(self):
+        if self.exists():
+            self.update()
+            return
+        else:
+            self.init_checkout()
+
+    def init_checkout(self):
+        raise NotImplementedError
+
+    def update(self):
+        raise NotImplementedError
+
+    def commit_files(self, files):
+        raise NotImplementedError
+
+    def cherry_pick(self, commit_hash):
+        raise NotImplementedError
+
+    def remove(self):
+        raise NotImplementedError
+
+
+class GitRepo(RepoBase):
+    def exists(self):
+        if not self.branch.co_path.exists():
+            return False
+        command = "git branch | grep %s" % self.branch.name
+        status, output, errs = run_shell_command(command, cwd=self.branch.co_path)
+        return output != ""
+
+    def init_checkout(self):
+        # We are assuming here that master is the first branch created
+        if self.name == "master":
+            commands = [
+                ['git', 'clone', self.branch.module.vcs_root, self.branch.co_path.parent],
+                ['git', 'remote', 'update'],
+                ['git', 'checkout', self.branch.name],
+            ]
+        else:
+            commands = [
+                ['git', 'pull'],
+                ['git', 'checkout', '--track', '-b', self.branch.name, 'origin/%s' % self.branch.name],
+            ]
+        # check if there are any submodules and init & update them
+        commands.append("if [ -e .gitmodules ]; then git submodule update --init; fi")
+        for cmd in commands:
+            working_dir = self.branch.co_path.parent if 'clone' in cmd else self.branch.co_path
+            run_shell_command(command, raise_on_error=True, cwd=working_dir)
+
+    def update(self):
+        # test "git checkout %(branch)s && git clean -dfq && git pull origin/%(branch)s"?
+        commands = [
+            ['git', 'checkout', '-f', self.branch.name],
+            ['git', 'fetch'],
+            ['git', 'reset', '--hard', 'origin/%s' % self.branch.name],
+            ['git', 'clean', '-dfq'],
+            # check if there are any submodules and init & update them
+            "if [ -e .gitmodules ]; then git submodule update --init; fi",
+        ]
+        for cmd in commands:
+            run_shell_command(cmd, raise_on_error=True, cwd=self.branch.co_path)
+
+    def commit_files(self, files, message, author=None):
+        base_path = self.branch.co_path
+        for _file in files:
+            run_shell_command(['git', 'add', str(_file)], raise_on_error=True, cwd=base_path)
+        commit_cmd = ['git', 'commit', '-m', message]
+        if author:
+            commit_cmd.extend(['--author', author])
+        run_shell_command(commit_cmd, raise_on_error=True, cwd=base_path)
+        # git push
+        try:
+            run_shell_command(
+                ['git', 'push', 'origin', self.branch.name], raise_on_error=True, cwd=base_path
+            )
+        except OSError:
+            # Revert the commit
+            run_shell_command(['git', 'reset', '--hard', 'origin/%s' % self.branch.name], cwd=base_path)
+            raise
+        else:
+            _, out, _ = run_shell_command(['git', 'log', '-n1', '--format=oneline'], cwd=base_path)
+            commit_hash = out.split()[0] if out else ''
+        return commit_hash
+
+    def cherry_pick(self, commit_hash):
+        commit_dir = self.branch.co_path
+        result = run_shell_command(['git', 'cherry-pick', '-x', commit_hash], cwd=commit_dir)
+        if result[0] == STATUS_OK:
+            run_shell_command(
+                ['git', 'push', 'origin', self.branch.name], raise_on_error=True, cwd=commit_dir
+            )
+            return True
+        else:
+            # Revert
+            run_shell_command(['git', 'cherry-pick', '--abort'], cwd=commit_dir)
+            return False
+
+    def remove(self):
+        wdir = str(self.branch.co_path)
+        if os.access(wdir, os.W_OK):
+            if self.branch.name == 'master':
+                shutil.rmtree(wdir)
+            else:
+                run_shell_command(['git', 'checkout', 'master'], cwd=wdir)
+                run_shell_command(['git', 'branch', '-D', self.branch.name], cwd=wdir)
+
+
+class SVNRepo(RepoBase):
+    def init_checkout(self):
+        run_shell_command([
+            'svn', 'co', '--non-interactive',
+            self.branch.get_vcs_url(), self.branch.module.name + "." + self.branch.name
+        ])
+
+    def update(self):
+        run_shell_command(['svn', 'up', '--non-interactive'], raise_on_error=True, cwd=self.branch.co_path)
+
+    def remove(self):
+        if os.access(str(self.branch.co_path), os.W_OK):
+            shutil.rmtree(str(self.branch.co_path))
+
+
+class CVSRepo(RepoBase):
+    def init_checkout(self):
+        run_shell_command([
+            'cvs', '-d%s' % self.branch.module.vcs_root, '-z4', 'co',
+            '-d%s' % self.branch.module.name + "." + self.branch.name,
+            '-r%s' % self.branch.name, self.module.name
+        ], cwd=os.path.join(settings.SCRATCHDIR, self.branch.module.vcs_type))
+
+    def update(self):
+        run_shell_command(['cvs', '-z4', 'up', '-Pd'], raise_on_error=True, cwd=self.branch.co_path)
+
+    def remove(self):
+        if os.access(str(self.branch.co_path), os.W_OK):
+            shutil.rmtree(str(self.branch.co_path))
+
+
+class MercurialRepo(RepoBase):
+    def exists(self):
+        return self.branch.id != None and os.access(str(self.branch.co_path), os.X_OK | os.W_OK)
+
+    def init_checkout(self):
+        base_path = self.branch.co_path
+        run_shell_command(
+            ['hg', 'clone', base_path, self.module.name],
+            cwd=base_path.parent
+        )
+        run_shell_command(['hg', 'update', self.branch.name], cwd=base_path)
+
+    def update(self):
+        run_shell_command(['hg', 'revert', '--all'], raise_on_error=True, cwd=self.branch.co_path)
diff --git a/stats/tests/tests.py b/stats/tests/tests.py
index 48f3c86f..ec160aa4 100644
--- a/stats/tests/tests.py
+++ b/stats/tests/tests.py
@@ -99,7 +99,6 @@ class ModuleTestCase(TestCase):
 
     def test_branch_methods(self):
         self.assertTrue(self.branch.is_head())
-        self.assertEqual(self.branch.get_vcs_url(), "https://gitlab.gnome.org/GNOME/gnome-hello.git";)
         self.assertEqual(self.branch.get_vcs_web_url(), "https://gitlab.gnome.org/GNOME/gnome-hello/";)
 
     def test_branch_domains(self):
@@ -175,9 +174,9 @@ class ModuleTestCase(TestCase):
     @test_scratchdir
     def test_branch_exists(self):
         branch = Branch.objects.get(name='master', module__name='zenity')
-        self.assertFalse(branch._exists())
+        self.assertFalse(branch._repo.exists())
         branch = Branch.objects.get(name='master', module__name='gnome-hello')
-        self.assertTrue(branch._exists())
+        self.assertTrue(branch._repo.exists())
 
     @test_scratchdir
     def test_delete_branch(self):
diff --git a/stats/tests/utils.py b/stats/tests/utils.py
index e523866d..0930f4b9 100644
--- a/stats/tests/utils.py
+++ b/stats/tests/utils.py
@@ -22,6 +22,8 @@ class patch_shell_command:
         self.patcher1.start()
         self.patcher2 = patch('stats.utils.run_shell_command', side_effect=self.mocked_run_shell_command)
         self.patcher2.start()
+        self.patcher3 = patch('stats.repos.run_shell_command', side_effect=self.mocked_run_shell_command)
+        self.patcher3.start()
         return self.cmds
 
     def mocked_run_shell_command(self, cmd, *args, **kwargs):