From 3fc39e1bd31d8a24c7b8030727bdbb39917d2ac3 Mon Sep 17 00:00:00 2001
From: Antoine Musso <hashar@free.fr>
Date: Tue, 19 Mar 2019 11:15:21 +0100
Subject: [PATCH] Properly abort when git submodule processing fails

The logic to process MediaWiki extensions and skins submodules comes
from the old JJB macro ext-skins-submodules-update. It was written in
shell using find to run the command. Roughly:

 find extensions -name .gitmodules -execdir <git submodule commands> \;

When a git submodule command failed, find would keep processing anyway
and exit 0 regardless.  The reason is in find documentation:

-execdir command {}  ; returns true only if command returns 0.

The 'true' returns value is for the find predicate and does not affect
find exit status.

Port the logic to plain python using os.walk(). Which also mean that
Quibble no more depends on the 'find' utility which would help on
Windows.

Add a test to ensure a failing call causes an exception which is bubbled
up to ext_skin_submodule_update() (and thus abort QuibbleCmd).

Change-Id: I3ca48f10a66a8959c1a8b9f8f7d585823f40312a
---
 quibble/cmd.py    | 43 ++++++++++++++++++++++++++-----------------
 tests/test_cmd.py | 25 +++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/quibble/cmd.py b/quibble/cmd.py
index d7a1e42..dfdaac0 100644
--- a/quibble/cmd.py
+++ b/quibble/cmd.py
@@ -230,23 +230,32 @@ class QuibbleCmd(object):
 
     def ext_skin_submodule_update(self):
         self.log.info('Updating git submodules of extensions and skins')
-        # From JJB macro ext-skins-submodules-update
-        # jjb/mediawiki-extensions.yaml
-        subprocess.check_call([
-            # Do not add ., or that will process mediawiki/core submodules in
-            # wmf branches which is a mess.
-            'find', 'extensions', 'skins',
-            '-maxdepth', '2',
-            '-name', '.gitmodules',
-            '-print',
-            '-execdir', 'bash', '-xe', '-c',
-            '\n'.join([
-                 'git submodule foreach git clean -xdff -q',
-                 'git submodule update --init --recursive',
-                 'git submodule status',
-                 ]),
-            ';',  # end of -execdir
-             ], cwd=self.mw_install_path)
+
+        cmds = [
+            ['git', 'submodule', 'foreach', 'git', 'clean', '-xdff', '-q'],
+            ['git', 'submodule', 'update', '--init', '--recursive'],
+            ['git', 'submodule', 'status'],
+        ]
+
+        tops = [os.path.join(self.mw_install_path, top)
+                for top in ['extensions', 'skins']]
+
+        for top in tops:
+            for dirpath, dirnames, filenames in os.walk(top):
+                if dirpath not in tops:
+                    # Only look at the first level
+                    dirnames[:] = []
+                if '.gitmodules' not in filenames:
+                    continue
+
+                for cmd in cmds:
+                    try:
+                        subprocess.check_call(cmd, cwd=dirpath)
+                    except subprocess.CalledProcessError as e:
+                        self.log.error(
+                            "Failed to process git submodules for %s" %
+                            dirpath)
+                        raise e
 
     # Used to be bin/mw-create-composer-local.py
     def create_composer_local(self):
diff --git a/tests/test_cmd.py b/tests/test_cmd.py
index 1b68c20..b67063b 100644
--- a/tests/test_cmd.py
+++ b/tests/test_cmd.py
@@ -78,6 +78,31 @@ class CmdTest(unittest.TestCase):
                     'mediawiki/skins/Vector',
                     ], q.set_repos_to_clone())
 
+    def test_ext_skin_submodule_update(self):
+        q = cmd.QuibbleCmd()
+        q.mw_install_path = '/tmp'
+
+        with mock.patch('os.walk') as mock_walk:
+            somesubdir = ['includes']
+            # XXX the mock is also used for skins :(
+            mock_walk.return_value = [
+                ('/tmp/extensions', ['VisualEditor'], []),
+                ('/tmp/extensions/VisualEditor', somesubdir, ['.gitmodules']),
+            ]
+            with mock.patch('subprocess.check_call') as mock_check_call:
+                # A git command failling aborts.
+                mock_check_call.side_effect = Exception('mock a failure')
+                with self.assertRaisesRegex(Exception, '^mock a failure$'):
+                    q.ext_skin_submodule_update()
+
+                self.assertEquals(
+                    [], somesubdir,
+                    'Must skip sub directories in extensions and skins')
+                mock_check_call.assert_any_call(
+                    ['git', 'submodule', 'foreach',
+                        'git', 'clean', '-xdff', '-q'],
+                    cwd='/tmp/extensions/VisualEditor')
+
     @mock.patch('quibble.is_in_docker', return_value=False)
     def test_args_defaults(self, _):
         args = cmd.QuibbleCmd().parse_arguments([])
-- 
2.11.0