git cl: Print a clear error message when we fail to find the remote url.

Bug: 914655
Change-Id: I06a3ff39d884327fd407a5c50cc9730a375bef0b
Reviewed-on: https://chromium-review.googlesource.com/c/1382974
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

git_cl.py

@@ -1343,11 +1343,39 @@ class Changelist(object):
     remote, _ = self.GetRemoteBranch()
     url = RunGit(['config', 'remote.%s.url' % remote], error_ok=True).strip()
 
-    # If URL is pointing to a local directory, it is probably a git cache.
-    if os.path.isdir(url):
-      url = RunGit(['config', 'remote.%s.url' % remote],
-                   error_ok=True,
-                   cwd=url).strip()
+    # Check if the remote url can be parsed as an URL.
+    host = urlparse.urlparse(url).netloc
+    if host:
+      self._cached_remote_url = (True, url)
+      return url
+
+    # If it cannot be parsed as an url, assume it is a local directory, probably
+    # a git cache.
+    logging.warning('"%s" doesn\'t appear to point to a git host. '
+                    'Interpreting it as a local directory.', url)
+    if not os.path.isdir(url):
+      logging.error(
+          'Remote "%s" for branch "%s" points to "%s", but it doesn\'t exist.',
+          remote, url, self.GetBranch())
+      return None
+
+    cache_path = url
+    url = RunGit(['config', 'remote.%s.url' % remote],
+                 error_ok=True,
+                 cwd=url).strip()
+
+    host = urlparse.urlparse(url).netloc
+    if not host:
+      logging.error(
+          'Remote "%(remote)s" for branch "%(branch)s" points to '
+          '"%(cache_path)s", but it is misconfigured.\n'
+          '"%(cache_path)s" must be a git repo and must have a remote named '
+          '"%(remote)s" pointing to the git host.', {
+              'remote': remote,
+              'cache_path': cache_path,
+              'branch': self.GetBranch()})
+      return None
+
     self._cached_remote_url = (True, url)
     return url
 
@@ -1863,7 +1891,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
 
   def _GetGitHost(self):
     """Returns git host to be used when uploading change to Gerrit."""
-    return urlparse.urlparse(self.GetRemoteUrl()).netloc
+    remote_url = self.GetRemoteUrl()
+    if not remote_url:
+      return None
+    return urlparse.urlparse(remote_url).netloc
 
   def GetCodereviewServer(self):
     if not self._gerrit_server:
@@ -1941,7 +1972,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     # Lazy-loader to identify Gerrit and Git hosts.
     self.GetCodereviewServer()
     git_host = self._GetGitHost()
-    assert self._gerrit_server and self._gerrit_host
+    assert self._gerrit_server and self._gerrit_host and git_host
 
     gerrit_auth = cookie_auth.get_auth_header(self._gerrit_host)
     git_auth = cookie_auth.get_auth_header(git_host)

tests/git_cl_test.py

@@ -3050,6 +3050,64 @@ class TestGitCl(TestCase):
     self.assertEqual(cl.GetRemoteUrl(), url)
     self.assertEqual(cl.GetRemoteUrl(), url)  # Must be cached.
 
+  def test_get_remote_url_non_existing_mirror(self):
+    original_os_path_isdir = os.path.isdir
+    def selective_os_path_isdir_mock(path):
+      if path == '/cache/this-dir-doesnt-exist':
+        return self._mocked_call('os.path.isdir', path)
+      return original_os_path_isdir(path)
+    self.mock(os.path, 'isdir', selective_os_path_isdir_mock)
+    self.mock(logging, 'error',
+              lambda fmt, *a: self._mocked_call('logging.error', fmt % a))
+
+    self.calls = [
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'config', 'branch.master.merge'],), 'master'),
+      ((['git', 'config', 'branch.master.remote'],), 'origin'),
+      ((['git', 'config', 'remote.origin.url'],),
+       '/cache/this-dir-doesnt-exist'),
+      (('os.path.isdir', '/cache/this-dir-doesnt-exist'),
+       False),
+      (('logging.error',
+        'Remote "origin" for branch "/cache/this-dir-doesnt-exist" points to'
+        ' "master", but it doesn\'t exist.'), None),
+    ]
+    cl = git_cl.Changelist(codereview='gerrit', issue=1)
+    self.assertIsNone(cl.GetRemoteUrl())
+
+  def test_get_remote_url_misconfigured_mirror(self):
+    original_os_path_isdir = os.path.isdir
+    def selective_os_path_isdir_mock(path):
+      if path == '/cache/this-dir-exists':
+        return self._mocked_call('os.path.isdir', path)
+      return original_os_path_isdir(path)
+    self.mock(os.path, 'isdir', selective_os_path_isdir_mock)
+    self.mock(logging, 'error',
+              lambda *a: self._mocked_call('logging.error', *a))
+
+    self.calls = [
+      ((['git', 'symbolic-ref', 'HEAD'],), 'master'),
+      ((['git', 'config', 'branch.master.merge'],), 'master'),
+      ((['git', 'config', 'branch.master.remote'],), 'origin'),
+      ((['git', 'config', 'remote.origin.url'],),
+       '/cache/this-dir-exists'),
+      (('os.path.isdir', '/cache/this-dir-exists'), True),
+      # Runs in /cache/this-dir-exists.
+      ((['git', 'config', 'remote.origin.url'],), ''),
+      (('logging.error',
+        'Remote "%(remote)s" for branch "%(branch)s" points to '
+        '"%(cache_path)s", but it is misconfigured.\n'
+        '"%(cache_path)s" must be a git repo and must have a remote named '
+        '"%(remote)s" pointing to the git host.', {
+              'remote': 'origin',
+              'cache_path': '/cache/this-dir-exists',
+              'branch': 'master'}
+        ), None),
+    ]
+    cl = git_cl.Changelist(codereview='gerrit', issue=1)
+    self.assertIsNone(cl.GetRemoteUrl())
+
+
   def test_gerrit_change_identifier_with_project(self):
     self.calls = [
       ((['git', 'symbolic-ref', 'HEAD'],), 'master'),