diff --git a/git_cl.py b/git_cl.py index a8fa6e393..84f6d54d1 100755 --- a/git_cl.py +++ b/git_cl.py @@ -140,9 +140,8 @@ def DieWithError(message, change_desc=None): def SaveDescriptionBackup(change_desc): backup_path = os.path.join(DEPOT_TOOLS, DESCRIPTION_BACKUP_FILE) print('\nsaving CL description to %s\n' % backup_path) - backup_file = open(backup_path, 'w') - backup_file.write(change_desc.description) - backup_file.close() + with open(backup_path, 'w') as backup_file: + backup_file.write(change_desc.description) def GetNoGitPagerEnv(): @@ -862,6 +861,7 @@ class Settings(object): cr_settings_file = FindCodereviewSettingsFile() if autoupdate != 'false' and cr_settings_file: LoadCodereviewSettingsFromFile(cr_settings_file) + cr_settings_file.close() self.updated = True @staticmethod @@ -1344,8 +1344,9 @@ class Changelist(object): '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, self.GetBranch(), url) + 'Remote "%(remote)s" for branch "%(branch)s" points to "%(url)s", ' + 'but it doesn\'t exist.', + {'remote': remote, 'branch': self.GetBranch(), 'url': url}) return None cache_path = url @@ -1703,7 +1704,7 @@ class Changelist(object): """Returns Gerrit project name based on remote git URL.""" remote_url = self.GetRemoteUrl() if remote_url is None: - logging.warn('can\'t detect Gerrit project.') + logging.warning('can\'t detect Gerrit project.') return None project = urllib.parse.urlparse(remote_url).path.strip('/') if project.endswith('.git'): @@ -1756,11 +1757,14 @@ class Changelist(object): remote_url = self.GetRemoteUrl() if remote_url is None: - print('WARNING: invalid remote') + logging.warning('invalid remote') return if urllib.parse.urlparse(remote_url).scheme != 'https': - print('WARNING: Ignoring branch %s with non-https remote %s' % - (self.branch, self.GetRemoteUrl())) + logging.warning('Ignoring branch %(branch)s with non-https remote ' + '%(remote)s', { + 'branch': self.branch, + 'remote': self.GetRemoteUrl() + }) return # Lazy-loader to identify Gerrit and Git hosts. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 3101aa33b..6ad36d5c4 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2094,9 +2094,17 @@ class TestGitCl(TestCase): ((['git', 'config', 'branch.master.merge'],), 'refs/heads/master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), ((['git', 'config', 'remote.origin.url'],), 'custom-scheme://repo'), + (('logging.warning', + 'Ignoring branch %(branch)s with non-https remote ' + '%(remote)s', { + 'branch': 'master', + 'remote': 'custom-scheme://repo'} + ), None), ] self.mock(git_cl.gerrit_util, 'CookiesAuthenticator', CookiesAuthenticatorMockFactory(hosts_with_creds={})) + self.mock(logging, 'warning', + lambda *a: self._mocked_call('logging.warning', *a)) cl = git_cl.Changelist() cl.branch = 'master' cl.branchref = 'refs/heads/master' @@ -2111,9 +2119,18 @@ class TestGitCl(TestCase): ((['git', 'config', 'branch.master.remote'], ), 'origin'), ((['git', 'config', 'remote.origin.url'], ), 'git@somehost.example:foo/bar.git'), + (('logging.error', + 'Remote "%(remote)s" for branch "%(branch)s" points to "%(url)s", ' + 'but it doesn\'t exist.', { + 'remote': 'origin', + 'branch': 'master', + 'url': 'git@somehost.example:foo/bar.git'} + ), None), ] self.mock(git_cl.gerrit_util, 'CookiesAuthenticator', CookiesAuthenticatorMockFactory(hosts_with_creds={})) + self.mock(logging, 'error', + lambda *a: self._mocked_call('logging.error', *a)) cl = git_cl.Changelist() cl.branch = 'master' cl.branchref = 'refs/heads/master' @@ -3031,7 +3048,7 @@ class TestGitCl(TestCase): self.mock(os.path, 'isdir', selective_os_path_isdir_mock) self.mock(logging, 'error', - lambda fmt, *a: self._mocked_call('logging.error', fmt % a)) + lambda *a: self._mocked_call('logging.error', *a)) self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'master'), @@ -3042,8 +3059,12 @@ class TestGitCl(TestCase): (('os.path.isdir', '/cache/this-dir-doesnt-exist'), False), (('logging.error', - 'Remote "origin" for branch "master" points to' - ' "/cache/this-dir-doesnt-exist", but it doesn\'t exist.'), None), + 'Remote "%(remote)s" for branch "%(branch)s" points to "%(url)s", ' + 'but it doesn\'t exist.', { + 'remote': 'origin', + 'branch': 'master', + 'url': '/cache/this-dir-doesnt-exist'} + ), None), ] cl = git_cl.Changelist(issue=1) self.assertIsNone(cl.GetRemoteUrl()) @@ -3094,11 +3115,21 @@ class TestGitCl(TestCase): self.assertEqual(cl._GerritChangeIdentifier(), 'my%2Frepo~123456') def test_gerrit_change_identifier_without_project(self): + 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'],), CERR1), + (('logging.error', + 'Remote "%(remote)s" for branch "%(branch)s" points to "%(url)s", ' + 'but it doesn\'t exist.', { + 'remote': 'origin', + 'branch': 'master', + 'url': ''} + ), None), ] cl = git_cl.Changelist(issue=123456) self.assertEqual(cl._GerritChangeIdentifier(), '123456')