From 298f2cf820dfe45dff53b0e41b11441dbf72b48c Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Fri, 22 Feb 2019 21:40:39 +0000 Subject: [PATCH] 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 Reviewed-by: Andrii Shyshkalov --- git_cl.py | 45 ++++++++++++++++++++++++++++------ tests/git_cl_test.py | 58 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/git_cl.py b/git_cl.py index 1631be49b..e4db62556 100755 --- a/git_cl.py +++ b/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) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 841a77a2a..f71a5d5dd 100755 --- a/tests/git_cl_test.py +++ b/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'),