Correctly set the repo remote URL

I8e1b5b26f03cb75727d2b2e3c9310214a3eac447 introduced a regression that
prevented us from re-cloning a repo that no longer exists on the file
system (e.g. deleted by an operator) but where we still have the cached
`Repo` object.

The problem was that we only updated the remote URL of the repo object
after we wrote it to the Git config. Unfortunately, if the repo no
longer existed on the file system we would attempt to re-clone it with a
possibly outdated remote URL.

`test_set_remote_url` is a regression test for the issue described
above.

`test_set_remote_url_invalid` verifies that the original issue is fixes,
where we updated the remote URL attribute of the repo object, but fail
to update the Git config.

Change-Id: I311842ccc7af38664c28177450ea9e80e1371638

tests/unit/test_merger_repo.py

@@ -16,6 +16,8 @@
 import datetime
 import logging
 import os
+import shutil
+from unittest import mock
 
 import git
 import testtools
@@ -678,6 +680,59 @@ class TestMergerRepo(ZuulTestCase):
         new_result = work_repo_underlying.commit('testtag')
         self.assertEqual(new_commit, new_result)
 
+    def test_set_remote_url_clone(self):
+        """Test that we always use the new Git URL for cloning.
+
+        This is a regression test to make sure we always use the new
+        Git URL when a clone of the repo is necessary before updating
+        the config.
+        """
+        parent_path = os.path.join(self.upstream_root, 'org/project1')
+        work_repo = Repo(parent_path, self.workspace_root,
+                         'none@example.org', 'User Name', '0', '0')
+
+        # Simulate an invalid/outdated remote URL with the repo no
+        # longer existing on the file system.
+        work_repo.remote_url = "file:///dev/null"
+        shutil.rmtree(work_repo.local_path)
+
+        # Setting a valid remote URL should update the attribute and
+        # clone the repository.
+        work_repo.setRemoteUrl(parent_path)
+        self.assertEqual(work_repo.remote_url, parent_path)
+        self.assertTrue(os.path.exists(work_repo.local_path))
+
+    def test_set_remote_url_invalid(self):
+        """Test that we don't store the Git URL when failing to set it.
+
+        This is a regression test to make sure we will always update
+        the Git URL after a previously failed attempt.
+        """
+        parent_path = os.path.join(self.upstream_root, 'org/project1')
+        work_repo = Repo(parent_path, self.workspace_root,
+                         'none@example.org', 'User Name', '0', '0')
+
+        # Set the Git remote URL to an invalid value.
+        invalid_url = "file:///dev/null"
+        repo = work_repo.createRepoObject(None)
+        work_repo._git_set_remote_url(repo, invalid_url)
+        work_repo.remote_url = invalid_url
+
+        # Simulate a failed attempt to update the remote URL
+        with mock.patch.object(work_repo, "_git_set_remote_url",
+                               side_effect=RuntimeError):
+            with testtools.ExpectedException(RuntimeError):
+                work_repo.setRemoteUrl(parent_path)
+
+        # Make sure we cleared out the remote URL.
+        self.assertIsNone(work_repo.remote_url)
+
+        # Setting a valid remote URL should update the attribute and
+        # clone the repository.
+        work_repo.setRemoteUrl(parent_path)
+        self.assertEqual(work_repo.remote_url, parent_path)
+        self.assertTrue(os.path.exists(work_repo.local_path))
+
 
 class TestMergerWithAuthUrl(ZuulTestCase):
     config_file = 'zuul-github-driver.conf'

zuul/merger/merger.py

@@ -757,8 +757,19 @@ class Repo(object):
             return
         log = get_annotated_logger(self.log, zuul_event_id)
         log.debug("Set remote url to %s", redact_url(url))
-        self._git_set_remote_url(self.createRepoObject(zuul_event_id), url)
-        self.remote_url = url
+        try:
+            # Update the remote URL as it is used for the clone if the
+            # repo doesn't exist.
+            self.remote_url = url
+            self._git_set_remote_url(
+                self.createRepoObject(zuul_event_id), self.remote_url)
+        except Exception:
+            # Clear out the stored remote URL so we will always set
+            # the Git URL after a failed attempt. This prevents us from
+            # using outdated credentials that might still be stored in
+            # the Git config as part of the URL.
+            self.remote_url = None
+            raise
 
     def mapLine(self, commit, filename, lineno, zuul_event_id=None):
         repo = self.createRepoObject(zuul_event_id)