Make git pulls less error prone due to history changes. (#1052)

LP: #1614520

.travis.yml

@@ -15,7 +15,7 @@ env:
   - secure: "gqtqTji8cie0Q2O+sRhE4MbTXGI0qTq8yPgRGFd9XlT/lB/EttFQKu72qycr/jyrvt809wjWM13QVqa5/71SoJd+Xzrmr1/leevx9Z/Wnv+IYkRAuGHW7iIDQb7MhQvpq3tw8hbGJzGxw03cUmjKJ89AAlGbwaURMat47lPsRXus8R7pl9S6r5owhBbmrQNaP9io0oPQDOAUf4pmJma1FTHAjXg0EdUwdXFUWToj15c7UJtB/MQNNTfjlwGA+/sPDqgthUEAzXmvUfXAZWnjQFZmq4ebvBIJEOQEPdLCXWGYdN2DAL7zp7WthrwFfgFFZb579rOBh0ETIMebUgBLoVSiPcn/bfzdYHcYKGf7lTJpoug5QENl+kZcuVyK7GUjf8O9tamhkYeMtUOy5Ubrcnv+Lfy9NsDPhKY05n+7tzzUVB1dePTrMHPuRZLl4OKku1AUN/S3A2xMrLO8vsWPVxcfxeb+4Y5ikYiHHpOozJHHDdPmj5raRIf3IH87W2PX0nJhg+gEgNHV1v3HBoyeqOPl4hl6/Fb9sCS/JAbbfcixkC54MHHI+opNSgZRvY0RORGHmuhHRGvfMxnwHmeOD51oV+SRGJS6A7qUq6GlBIy3/YlAY3LqqkWrHwm4EYttd4yM1FZ5s9pVnoSSJkgQ5vYK7A8a9AgZJlCcZnbzXtA="
   matrix:
   - TEST_SUITE=static DEPENDENCIES="apt install -y python3-pip && python3 -m pip install -r requirements-devel.txt"
-  - TEST_SUITE=unit DEPENDENCIES="apt install -y libnacl-dev libsodium-dev libffi-dev libapt-pkg-dev libarchive-dev python3-pip squashfs-tools xdelta3 && python3 -m pip install -r requirements-devel.txt -r requirements.txt && python3 -m pip uninstall -y coverage && apt install -y python3-coverage"
+  - TEST_SUITE=unit DEPENDENCIES="apt install -y git libnacl-dev libsodium-dev libffi-dev libapt-pkg-dev libarchive-dev python3-pip squashfs-tools xdelta3 && python3 -m pip install -r requirements-devel.txt -r requirements.txt && python3 -m pip uninstall -y coverage && apt install -y python3-coverage"
   - TEST_SUITE=integration TEST_STORE=fake DEPENDENCIES="apt install -y bzr curl git libnacl-dev libsodium-dev libffi-dev libapt-pkg-dev libarchive-dev mercurial python3-pip subversion squashfs-tools sudo snapd xdelta3 && python3 -m pip install -r requirements-devel.txt -r requirements.txt"
 
 matrix:

snapcraft/internal/sources/_git.py

@@ -24,7 +24,7 @@
 class Git(Base):
 
     def __init__(self, source, source_dir, source_tag=None, source_commit=None,
-                 source_branch=None, source_depth=None):
+                 source_branch=None, source_depth=None, silent=False):
         super().__init__(source, source_dir, source_tag, source_commit,
                          source_branch, source_depth, 'git')
         if source_tag and source_branch:
@@ -39,6 +39,10 @@ def __init__(self, source, source_dir, source_tag=None, source_commit=None,
             raise errors.IncompatibleOptionsError(
                 'can\'t specify both source-branch and source-commit for '
                 'a git source')
+        self.kwargs = {}
+        if silent:
+            self.kwargs['stdout'] = subprocess.DEVNULL
+            self.kwargs['stderr'] = subprocess.DEVNULL
 
     def _pull_existing(self):
         refspec = 'HEAD'
@@ -49,14 +53,18 @@ def _pull_existing(self):
         elif self.source_commit:
             refspec = self.source_commit
 
-        # Pull changes to this repository and any submodules.
+        reset_spec = refspec if refspec != 'HEAD' else 'origin/master'
+
+        subprocess.check_call([self.command, '-C', self.source_dir,
+                               'fetch', '--prune',
+                               '--recurse-submodules=yes'], **self.kwargs)
         subprocess.check_call([self.command, '-C', self.source_dir,
-                               'pull', '--recurse-submodules=yes',
-                               self.source, refspec])
+                               'reset', '--hard', reset_spec], **self.kwargs)
 
         # Merge any updates for the submodules (if any).
         subprocess.check_call([self.command, '-C', self.source_dir,
-                              'submodule', 'update'])
+                              'submodule', 'update', '--recursive',
+                               '--remote'], **self.kwargs)
 
     def _clone_new(self):
         command = [self.command, 'clone', '--recursive']
@@ -65,11 +73,13 @@ def _clone_new(self):
                 '--branch', self.source_tag or self.source_branch])
         if self.source_depth:
             command.extend(['--depth', str(self.source_depth)])
-        subprocess.check_call(command + [self.source, self.source_dir])
+        subprocess.check_call(command + [self.source, self.source_dir],
+                              **self.kwargs)
 
         if self.source_commit:
             subprocess.check_call([self.command, '-C', self.source_dir,
-                                  'checkout', self.source_commit])
+                                  'checkout', self.source_commit],
+                                  **self.kwargs)
 
     def pull(self):
         if os.path.exists(os.path.join(self.source_dir, '.git')):

snapcraft/tests/sources/test_git.py

@@ -14,11 +14,15 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import os
+import shutil
+import subprocess
 from unittest import mock
 
 from snapcraft.internal import sources
 
 from snapcraft.tests.sources import SourceTestCase
+from snapcraft import tests
 
 
 class TestGit(SourceTestCase):
@@ -78,9 +82,12 @@ def test_pull_existing(self):
         git.pull()
 
         self.mock_run.assert_has_calls([
-            mock.call(['git', '-C', 'source_dir', 'pull',
-                       '--recurse-submodules=yes', 'git://my-source', 'HEAD']),
-            mock.call(['git', '-C', 'source_dir', 'submodule', 'update'])
+            mock.call(['git', '-C', 'source_dir', 'fetch',
+                       '--prune', '--recurse-submodules=yes']),
+            mock.call(['git', '-C', 'source_dir', 'reset', '--hard',
+                       'origin/master']),
+            mock.call(['git', '-C', 'source_dir', 'submodule', 'update',
+                       '--recursive', '--remote'])
         ])
 
     def test_pull_existing_with_tag(self):
@@ -90,10 +97,12 @@ def test_pull_existing_with_tag(self):
         git.pull()
 
         self.mock_run.assert_has_calls([
-            mock.call(['git', '-C', 'source_dir', 'pull',
-                       '--recurse-submodules=yes', 'git://my-source',
+            mock.call(['git', '-C', 'source_dir', 'fetch', '--prune',
+                       '--recurse-submodules=yes']),
+            mock.call(['git', '-C', 'source_dir', 'reset', '--hard',
                        'refs/tags/tag']),
-            mock.call(['git', '-C', 'source_dir', 'submodule', 'update'])
+            mock.call(['git', '-C', 'source_dir', 'submodule', 'update',
+                       '--recursive', '--remote'])
         ])
 
     def test_pull_existing_with_commit(self):
@@ -105,10 +114,12 @@ def test_pull_existing_with_commit(self):
         git.pull()
 
         self.mock_run.assert_has_calls([
-            mock.call(['git', '-C', 'source_dir', 'pull',
-                       '--recurse-submodules=yes', 'git://my-source',
+            mock.call(['git', '-C', 'source_dir', 'fetch', '--prune',
+                       '--recurse-submodules=yes']),
+            mock.call(['git', '-C', 'source_dir', 'reset', '--hard',
                        '2514f9533ec9b45d07883e10a561b248497a8e3c']),
-            mock.call(['git', '-C', 'source_dir', 'submodule', 'update'])
+            mock.call(['git', '-C', 'source_dir', 'submodule', 'update',
+                       '--recursive', '--remote'])
         ])
 
     def test_pull_existing_with_branch(self):
@@ -119,10 +130,12 @@ def test_pull_existing_with_branch(self):
         git.pull()
 
         self.mock_run.assert_has_calls([
-            mock.call(['git', '-C', 'source_dir', 'pull',
-                       '--recurse-submodules=yes', 'git://my-source',
+            mock.call(['git', '-C', 'source_dir', 'fetch', '--prune',
+                       '--recurse-submodules=yes']),
+            mock.call(['git', '-C', 'source_dir', 'reset', '--hard',
                        'refs/heads/my-branch']),
-            mock.call(['git', '-C', 'source_dir', 'submodule', 'update'])
+            mock.call(['git', '-C', 'source_dir', 'submodule', 'update',
+                       '--recursive', '--remote'])
         ])
 
     def test_init_with_source_branch_and_tag_raises_exception(self):
@@ -161,3 +174,126 @@ def test_init_with_source_tag_and_commit_raises_exception(self):
             'can\'t specify both source-tag and source-commit for ' \
             'a git source'
         self.assertEqual(raised.message, expected_message)
+
+
+class TestGitConflicts(tests.TestCase):
+    """Test that git pull errors don't kill the parser"""
+
+    def call(self, cmd):
+        subprocess.check_call(
+            cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+
+    def rm_dir(self, dir):
+        if os.path.exists(dir):
+            shutil.rmtree(dir)
+
+    def clean_dir(self, dir):
+        self.rm_dir(dir)
+        os.mkdir(dir)
+        self.addCleanup(self.rm_dir, dir)
+
+    def clone_repo(self, repo, tree):
+        self.clean_dir(tree)
+        self.call(['git', 'clone', repo, tree])
+        os.chdir(tree)
+        self.call(['git', 'config', '--local', 'user.name',
+                   '"Example Dev"'])
+        self.call(['git', 'config', '--local', 'user.email',
+                   'dev@example.com'])
+
+    def add_file(self, filename, body, message):
+        with open(filename, 'w') as fp:
+            fp.write(body)
+
+        self.call(['git', 'add', filename])
+        self.call(['git', 'commit', '-am', message])
+
+    def check_file_contents(self, path, expected):
+        body = None
+        with open(path) as fp:
+            body = fp.read()
+        self.assertEqual(body, expected)
+
+    def test_git_conflicts(self):
+
+        repo = '/tmp/conflict-test.git'
+        working_tree = '/tmp/git-conflict-test'
+        conflicting_tree = '{}-conflict'.format(working_tree)
+        git = sources.Git(repo, working_tree, silent=True)
+
+        self.clean_dir(repo)
+        self.clean_dir(working_tree)
+        self.clean_dir(conflicting_tree)
+
+        os.chdir(repo)
+        self.call(['git', 'init', '--bare'])
+
+        self.clone_repo(repo, working_tree)
+
+        # check out the original repo
+        self.clone_repo(repo, conflicting_tree)
+
+        # add a file to the repo
+        os.chdir(working_tree)
+        self.add_file('fake', 'fake 1', 'fake 1')
+        self.call(['git', 'push', repo])
+
+        git.pull()
+
+        os.chdir(conflicting_tree)
+        self.add_file('fake', 'fake 2', 'fake 2')
+        self.call(['git', 'push', '-f', repo])
+
+        os.chdir(working_tree)
+        git.pull()
+
+        body = None
+        with open(os.path.join(working_tree, 'fake')) as fp:
+            body = fp.read()
+
+        self.assertEqual(body, 'fake 2')
+
+    def test_git_submodules(self):
+        """Test that updates to submodules are pulled"""
+        repo = '/tmp/submodules.git'
+        sub_repo = '/tmp/subrepo'
+        working_tree = '/tmp/git-submodules'
+        sub_working_tree = '/tmp/git-submodules-sub'
+        git = sources.Git(repo, working_tree, silent=True)
+
+        self.clean_dir(repo)
+        self.clean_dir(sub_repo)
+        self.clean_dir(working_tree)
+        self.clean_dir(sub_working_tree)
+
+        os.chdir(sub_repo)
+        self.call(['git', 'init', '--bare'])
+
+        self.clone_repo(sub_repo, sub_working_tree)
+        self.add_file('sub-file', 'sub-file', 'sub-file')
+        self.call(['git', 'push', sub_repo])
+
+        os.chdir(repo)
+        self.call(['git', 'init', '--bare'])
+
+        self.clone_repo(repo, working_tree)
+        self.call(['git', 'submodule', 'add', sub_repo])
+        self.call(['git', 'commit', '-am', 'added submodule'])
+        self.call(['git', 'push', repo])
+
+        git.pull()
+
+        self.check_file_contents(os.path.join(working_tree,
+                                              'subrepo', 'sub-file'),
+                                 'sub-file')
+
+        # add a file to the repo
+        os.chdir(sub_working_tree)
+        self.add_file('fake', 'fake 1', 'fake 1')
+        self.call(['git', 'push', sub_repo])
+
+        os.chdir(working_tree)
+        git.pull()
+
+        self.check_file_contents(os.path.join(working_tree, 'subrepo', 'fake'),
+                                 'fake 1')