From 99df04e8aa2421d6872d2177e92f5444d3467c82 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Thu, 5 Mar 2020 19:39:43 +0000 Subject: [PATCH] git-cl: Check if author exists before adding to presubmit command line. When user.email is not configured in git, git-cl tries to call presubmit support with --author None, which makes git-cl crash. Bug: b/150870673 Change-Id: Idc42ba2b970340ed93e1e92f65850fc1a12336d9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2090375 Reviewed-by: Anthony Polito Commit-Queue: Edward Lesmes --- git_cl.py | 10 +++++---- tests/git_cl_test.py | 51 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/git_cl.py b/git_cl.py index 2144791d4..2348620ec 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1405,22 +1405,24 @@ class Changelist(object): def _GetCommonPresubmitArgs(self, verbose, upstream): args = [ - '--author', self.GetAuthor(), '--root', settings.GetRoot(), '--upstream', upstream, ] args.extend(['--verbose'] * verbose) + author = self.GetAuthor() + gerrit_url = self.GetCodereviewServer() issue = self.GetIssue() patchset = self.GetPatchset() - gerrit_url = self.GetCodereviewServer() + if author: + args.extend(['--author', author]) + if gerrit_url: + args.extend(['--gerrit_url', gerrit_url]) if issue: args.extend(['--issue', str(issue)]) if patchset: args.extend(['--patchset', str(patchset)]) - if gerrit_url: - args.extend(['--gerrit_url', gerrit_url]) return args diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index c817d5a50..b7243e2ac 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2842,13 +2842,13 @@ class ChangelistTest(unittest.TestCase): self.assertEqual(expected_results, results) subprocess2.Popen.assert_called_once_with([ 'vpython', 'PRESUBMIT_SUPPORT', - '--author', 'author', '--root', 'root', '--upstream', 'upstream', '--verbose', '--verbose', + '--author', 'author', + '--gerrit_url', 'https://chromium-review.googlesource.com', '--issue', '123456', '--patchset', '7', - '--gerrit_url', 'https://chromium-review.googlesource.com', '--commit', '--may_prompt', '--parallel', @@ -2864,6 +2864,49 @@ class ChangelistTest(unittest.TestCase): 'exit_code': 0, }) + def testRunHook_FewerOptions(self): + expected_results = { + 'more_cc': ['more@example.com', 'cc@example.com'], + 'should_continue': True, + } + gclient_utils.FileRead.return_value = json.dumps(expected_results) + git_cl.time_time.side_effect = [100, 200] + mockProcess = mock.Mock() + mockProcess.wait.return_value = 0 + subprocess2.Popen.return_value = mockProcess + + git_cl.Changelist.GetAuthor.return_value = None + git_cl.Changelist.GetIssue.return_value = None + git_cl.Changelist.GetPatchset.return_value = None + git_cl.Changelist.GetCodereviewServer.return_value = None + + cl = git_cl.Changelist() + results = cl.RunHook( + committing=False, + may_prompt=False, + verbose=0, + parallel=False, + upstream='upstream', + description='description', + all_files=False) + + self.assertEqual(expected_results, results) + subprocess2.Popen.assert_called_once_with([ + 'vpython', 'PRESUBMIT_SUPPORT', + '--root', 'root', + '--upstream', 'upstream', + '--upload', + '--json_output', '/tmp/fake-temp2', + '--description_file', '/tmp/fake-temp1', + ]) + gclient_utils.FileWrite.assert_called_once_with( + '/tmp/fake-temp1', 'description') + metrics.collector.add_repeated('sub_commands', { + 'command': 'presubmit', + 'execution_time': 100, + 'exit_code': 0, + }) + @mock.patch('sys.exit', side_effect=SystemExitMock) def testRunHook_Failure(self, _mock): git_cl.time_time.side_effect = [100, 200] @@ -2890,13 +2933,13 @@ class ChangelistTest(unittest.TestCase): subprocess2.Popen.assert_called_once_with([ 'vpython', 'PRESUBMIT_SUPPORT', - '--author', 'author', '--root', 'root', '--upstream', 'upstream', '--verbose', '--verbose', + '--author', 'author', + '--gerrit_url', 'https://chromium-review.googlesource.com', '--issue', '123456', '--patchset', '7', - '--gerrit_url', 'https://chromium-review.googlesource.com', '--post_upload', '--description_file', '/tmp/fake-temp1', ])