From 1b52d87ab5367886500c91a92cf310d5aa76e284 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Thu, 9 May 2019 21:12:12 +0000 Subject: [PATCH] Reland "git-cl: Keep git push traces" This is a reland of dc8e23d35612f213aa1ee554196f640cc315e4d5 Original change's description: > git-cl: Keep git push traces > > Keep the last N git push traces. > Name them after the time when they were collected, and add a > README file to each one to provide some context to developers. > > Bug: 955206 > Change-Id: Ib5fcf2f78fb65f6ddd80a93619c14e1ef70c5564 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1595108 > Commit-Queue: Edward Lesmes > Reviewed-by: Dirk Pranke Bug: 955206 Change-Id: Ie63a3ebfe3024d48ad7e0c8492a2939e635371aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1600246 Reviewed-by: Dirk Pranke Commit-Queue: Edward Lesmes --- git_cl.py | 148 ++++++++++++++++++++++++++++----------- tests/git_cl_test.py | 162 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 253 insertions(+), 57 deletions(-) diff --git a/git_cl.py b/git_cl.py index 8f82be47b..5e5993a2e 100755 --- a/git_cl.py +++ b/git_cl.py @@ -79,12 +79,30 @@ GIT_HASH_RE = re.compile(r'\b([a-f0-9]{6})[a-f0-9]{34}\b', flags=re.I) # Used to redact the cookies from the gitcookies file. GITCOOKIES_REDACT_RE = re.compile(r'1/.*') +# The maximum number of traces we will keep. Multiplied by 3 since we store +# 3 files per trace. +MAX_TRACES = 3 * 10 +# Message to display to the user after git-cl has run, to inform them of the +# traces we just collected. TRACES_MESSAGE = ( -'When filing a bug, be sure to include the traces found at:\n' -' %s.zip\n' -'Consider including the git config and gitcookies,\n' -'which we have packed for you at:\n' -' %s.zip\n') +'\n' +'When filing a bug for this push, be sure to include the traces found at:\n' +' %(trace_name)s-traces.zip\n' +'Consider including the git config and gitcookies, which we have packed for \n' +'you at:\n' +' %(trace_name)s-git-info.zip\n') +# Format of the message to be stored as part of the traces to give developers a +# better context when they go through traces. +TRACES_README_FORMAT = ( +'Date: %(now)s\n' +'\n' +'Change: https://%(gerrit_host)s/q/%(change_id)s\n' +'Title: %(title)s\n' +'\n' +'%(description)s\n' +'\n' +'Execution time: %(execution_time)s\n' +'Exit code: %(exit_code)s\n') + TRACES_MESSAGE COMMIT_BOT_EMAIL = 'commit-bot@chromium.org' POSTUPSTREAM_HOOK = '.git/hooks/post-cl-land' @@ -205,6 +223,12 @@ def time_time(): return time.time() +def datetime_now(): + # Use this so that it can be mocked in tests without interfering with python + # system machinery. + return datetime.datetime.now() + + def ask_for_data(prompt): try: return raw_input(prompt) @@ -2496,18 +2520,75 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): else: print('OK, will keep Gerrit commit-msg hook in place.') - def _RunGitPushWithTraces(self, change_desc, refspec, refspec_opts): + def _CleanUpOldTraces(self): + """Keep only the last |MAX_TRACES| traces.""" + try: + traces = sorted([ + os.path.join(TRACES_DIR, f) + for f in os.listdir(TRACES_DIR) + if (os.path.isfile(os.path.join(TRACES_DIR, f)) + and not f.startswith('tmp')) + ]) + traces_to_delete = traces[:-MAX_TRACES] + for trace in traces_to_delete: + os.remove(trace) + except OSError: + print('WARNING: Failed to remove old git traces from\n' + ' %s' + 'Consider removing them manually.' % TRACES_DIR) + + def _WriteGitPushTraces(self, traces_dir, git_push_metadata): + """Zip and write the git push traces stored in traces_dir.""" gclient_utils.safe_makedirs(TRACES_DIR) + now = datetime_now() + trace_name = os.path.join(TRACES_DIR, now.strftime('%Y%m%dT%H%M%S.%f')) + traces_zip = trace_name + '-traces' + traces_readme = trace_name + '-README' + # Create a temporary dir to store git config and gitcookies in. It will be + # compressed and stored next to the traces. + git_info_dir = tempfile.mkdtemp() + git_info_zip = trace_name + '-git-info' + + git_push_metadata['now'] = now.strftime('%c') + git_push_metadata['trace_name'] = trace_name + gclient_utils.FileWrite( + traces_readme, TRACES_README_FORMAT % git_push_metadata) + + # Keep only the first 6 characters of the git hashes on the packet + # trace. This greatly decreases size after compression. + packet_traces = os.path.join(traces_dir, 'trace-packet') + if os.path.isfile(packet_traces): + contents = gclient_utils.FileRead(packet_traces) + gclient_utils.FileWrite( + packet_traces, GIT_HASH_RE.sub(r'\1', contents)) + shutil.make_archive(traces_zip, 'zip', traces_dir) + + # Collect and compress the git config and gitcookies. + git_config = RunGit(['config', '-l']) + gclient_utils.FileWrite( + os.path.join(git_info_dir, 'git-config'), + git_config) + cookie_auth = gerrit_util.Authenticator.get() + if isinstance(cookie_auth, gerrit_util.CookiesAuthenticator): + gitcookies_path = cookie_auth.get_gitcookies_path() + if os.path.isfile(gitcookies_path): + gitcookies = gclient_utils.FileRead(gitcookies_path) + gclient_utils.FileWrite( + os.path.join(git_info_dir, 'gitcookies'), + GITCOOKIES_REDACT_RE.sub('REDACTED', gitcookies)) + shutil.make_archive(git_info_zip, 'zip', git_info_dir) + + print(TRACES_MESSAGE % {'trace_name': trace_name}) + + gclient_utils.rmtree(git_info_dir) + + def _RunGitPushWithTraces( + self, change_desc, refspec, refspec_opts, git_push_metadata): + """Run git push and collect the traces resulting from the execution.""" # Create a temporary directory to store traces in. Traces will be compressed # and stored in a 'traces' dir inside depot_tools. traces_dir = tempfile.mkdtemp() - trace_name = os.path.basename(traces_dir) - traces_zip = os.path.join(TRACES_DIR, trace_name + '-traces') - # Create a temporary dir to store git config and gitcookies in. It will be - # compressed and stored next to the traces. - git_info_dir = tempfile.mkdtemp() - git_info_zip = os.path.join(TRACES_DIR, trace_name + '-git-info') env = os.environ.copy() env['GIT_REDACT_COOKIES'] = 'o,SSO,GSSO_Uberproxy' @@ -2518,9 +2599,10 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): try: push_returncode = 0 + remote_url = self.GetRemoteUrl() before_push = time_time() push_stdout = gclient_utils.CheckCallAndFilter( - ['git', 'push', self.GetRemoteUrl(), refspec], + ['git', 'push', remote_url, refspec], env=env, print_stdout=True, # Flush after every line: useful for seeing progress when running as @@ -2532,8 +2614,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'for the reason of the failure.\n' 'Hint: run command below to diagnose common Git/Gerrit ' 'credential problems:\n' - ' git cl creds-check\n' + - TRACES_MESSAGE % (traces_zip, git_info_zip), + ' git cl creds-check', change_desc) finally: execution_time = time_time() - before_push @@ -2544,31 +2625,11 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'arguments': metrics_utils.extract_known_subcommand_args(refspec_opts), }) - if push_returncode != 0: - # Keep only the first 6 characters of the git hashes on the packet - # trace. This greatly decreases size after compression. - packet_traces = os.path.join(traces_dir, 'trace-packet') - contents = gclient_utils.FileRead(packet_traces) - gclient_utils.FileWrite( - packet_traces, GIT_HASH_RE.sub(r'\1', contents)) - shutil.make_archive(traces_zip, 'zip', traces_dir) + git_push_metadata['execution_time'] = execution_time + git_push_metadata['exit_code'] = push_returncode + self._WriteGitPushTraces(traces_dir, git_push_metadata) - # Collect and compress the git config and gitcookies. - git_config = RunGit(['config', '-l']) - gclient_utils.FileWrite( - os.path.join(git_info_dir, 'git-config'), - git_config) - - cookie_auth = gerrit_util.Authenticator.get() - if isinstance(cookie_auth, gerrit_util.CookiesAuthenticator): - gitcookies_path = cookie_auth.get_gitcookies_path() - gitcookies = gclient_utils.FileRead(gitcookies_path) - gclient_utils.FileWrite( - os.path.join(git_info_dir, 'gitcookies'), - GITCOOKIES_REDACT_RE.sub('REDACTED', gitcookies)) - shutil.make_archive(git_info_zip, 'zip', git_info_dir) - - gclient_utils.rmtree(git_info_dir) + self._CleanUpOldTraces() gclient_utils.rmtree(traces_dir) return push_stdout @@ -2822,7 +2883,14 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'spaces not allowed in refspec: "%s"' % refspec_suffix) refspec = '%s:refs/for/%s%s' % (ref_to_push, branch, refspec_suffix) - push_stdout = self._RunGitPushWithTraces(change_desc, refspec, refspec_opts) + git_push_metadata = { + 'gerrit_host': self._GetGerritHost(), + 'title': title or '', + 'change_id': change_id, + 'description': change_desc.description, + } + push_stdout = self._RunGitPushWithTraces( + change_desc, refspec, refspec_opts, git_push_metadata) if options.squash: regex = re.compile(r'remote:\s+https?://[\w\-\.\+\/#]*/(\d+)\s.*') diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 36c099706..91b68568c 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -909,7 +909,8 @@ class TestGitCl(TestCase): post_amend_description=None, issue=None, cc=None, custom_cl_base=None, tbr=None, short_hostname='chromium', - labels=None): + labels=None, change_id=None, original_title=None, + final_description=None, gitcookies_exists=True): if post_amend_description is None: post_amend_description = description cc = cc or [] @@ -1107,6 +1108,78 @@ class TestGitCl(TestCase): None,), ] + final_description = final_description or post_amend_description.strip() + original_title = original_title or title or '' + # Trace-related calls + calls += [ + # Write a description with context for the current trace. + ((['FileWrite', 'TRACES_DIR/20170316T200041.000000-README', + 'Date: Thu Mar 16 20:00:41 2017\n\n' + 'Change: https://%(short_hostname)s-review.googlesource.com/' + 'q/%(change_id)s\n' + 'Title: %(title)s\n\n' + '%(description)s\n\n' + 'Execution time: 1000\n' + 'Exit code: 0\n\n' + 'When filing a bug for this push, be sure to include the traces ' + 'found at:\n' + ' TRACES_DIR/20170316T200041.000000-traces.zip\n' + 'Consider including the git config and gitcookies, which we have ' + 'packed for \nyou at:\n' + ' TRACES_DIR/20170316T200041.000000-git-info.zip\n' % { + 'short_hostname': short_hostname, + 'change_id': change_id, + 'description': final_description, + 'title': original_title, + }],), + None, + ), + # Read traces and shorten git hashes. + ((['os.path.isfile', 'TEMP_DIR/trace-packet'],), + True, + ), + ((['FileRead', 'TEMP_DIR/trace-packet'],), + ('git-hash: 0123456789012345678901234567890123456789\n' + 'git-hash: abcdeabcdeabcdeabcdeabcdeabcdeabcdeabcde\n'), + ), + ((['FileWrite', 'TEMP_DIR/trace-packet', + 'git-hash: 012345\n' + 'git-hash: abcdea\n'],), + None, + ), + # Make zip file for the git traces. + ((['make_archive', 'TRACES_DIR/20170316T200041.000000-traces', 'zip', + 'TEMP_DIR'],), + None, + ), + # Collect git config and gitcookies. + ((['git', 'config', '-l'],), + 'git-config-output', + ), + ((['FileWrite', 'TEMP_DIR/git-config', 'git-config-output'],), + None, + ), + ((['os.path.isfile', '~/.gitcookies'],), + gitcookies_exists, + ), + ] + if gitcookies_exists: + calls += [ + ((['FileRead', '~/.gitcookies'],), + 'gitcookies 1/SECRET', + ), + ((['FileWrite', 'TEMP_DIR/gitcookies', 'gitcookies REDACTED'],), + None, + ), + ] + calls += [ + # Make zip file for the git config and gitcookies. + ((['make_archive', 'TRACES_DIR/20170316T200041.000000-git-info', 'zip', + 'TEMP_DIR'],), + None, + ), + ] + if squash: calls += [ ((['git', 'config', 'branch.master.gerritissue', '123456'],), @@ -1147,7 +1220,11 @@ class TestGitCl(TestCase): custom_cl_base=None, tbr=None, short_hostname='chromium', - labels=None): + labels=None, + change_id=None, + original_title=None, + final_description=None, + gitcookies_exists=True): """Generic gerrit upload test framework.""" if squash_mode is None: if '--no-squash' in upload_args: @@ -1169,6 +1246,19 @@ class TestGitCl(TestCase): lambda *_, **__: self._mocked_call(['RunEditor'])) self.mock(git_cl, 'DownloadGerritHook', lambda force: self._mocked_call( 'DownloadGerritHook', force)) + self.mock(git_cl.gclient_utils, 'FileRead', + lambda path: self._mocked_call(['FileRead', path])) + self.mock(git_cl.gclient_utils, 'FileWrite', + lambda path, contents: self._mocked_call( + ['FileWrite', path, contents])) + self.mock(git_cl, 'datetime_now', + lambda: datetime.datetime(2017, 3, 16, 20, 0, 41, 0)) + self.mock(git_cl.tempfile, 'mkdtemp', lambda: 'TEMP_DIR') + self.mock(git_cl, 'TRACES_DIR', 'TRACES_DIR') + self.mock(git_cl.shutil, 'make_archive', + lambda *args: self._mocked_call(['make_archive'] + list(args))) + self.mock(os.path, 'isfile', + lambda path: self._mocked_call(['os.path.isfile', path])) self.calls = self._gerrit_base_calls( issue=issue, @@ -1190,18 +1280,33 @@ class TestGitCl(TestCase): issue=issue, cc=cc, custom_cl_base=custom_cl_base, tbr=tbr, short_hostname=short_hostname, - labels=labels) + labels=labels, + change_id=change_id, + original_title=original_title, + final_description=final_description, + gitcookies_exists=gitcookies_exists) # Uncomment when debugging. # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls)))) git_cl.main(['upload'] + upload_args) + def test_gerrit_upload_traces_no_gitcookies(self): + self._run_gerrit_upload_test( + ['--no-squash'], + 'desc\n\nBUG=\n', + [], + squash=False, + post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx', + change_id='Ixxx', + gitcookies_exists=False) + def test_gerrit_upload_without_change_id(self): self._run_gerrit_upload_test( ['--no-squash'], 'desc\n\nBUG=\n', [], squash=False, - post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx') + post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx', + change_id='Ixxx') def test_gerrit_upload_without_change_id_override_nosquash(self): self._run_gerrit_upload_test( @@ -1210,7 +1315,8 @@ class TestGitCl(TestCase): [], squash=False, squash_mode='override_nosquash', - post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx') + post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx', + change_id='Ixxx') def test_gerrit_no_reviewer(self): self._run_gerrit_upload_test( @@ -1218,7 +1324,8 @@ class TestGitCl(TestCase): 'desc\n\nBUG=\n\nChange-Id: I123456789\n', [], squash=False, - squash_mode='override_nosquash') + squash_mode='override_nosquash', + change_id='I123456789') def test_gerrit_no_reviewer_non_chromium_host(self): # TODO(crbug/877717): remove this test case. @@ -1228,7 +1335,8 @@ class TestGitCl(TestCase): [], squash=False, squash_mode='override_nosquash', - short_hostname='other') + short_hostname='other', + change_id='I123456789') def test_gerrit_patchset_title_special_chars(self): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) @@ -1237,7 +1345,9 @@ class TestGitCl(TestCase): 'desc\n\nBUG=\n\nChange-Id: I123456789', squash=False, squash_mode='override_nosquash', - title='We%27ll_escape_%5E%5F_%5E_special_chars%2E%2E%2E%40%7Bu%7D') + title='We%27ll_escape_%5E%5F_%5E_special_chars%2E%2E%2E%40%7Bu%7D', + change_id='I123456789', + original_title='We\'ll escape ^_ ^ special chars...@{u}') def test_gerrit_reviewers_cmd_line(self): self._run_gerrit_upload_test( @@ -1246,7 +1356,10 @@ class TestGitCl(TestCase): ['foo@example.com'], squash=False, squash_mode='override_nosquash', - notify=True) + notify=True, + change_id='I123456789', + final_description= + 'desc\n\nBUG=\nR=foo@example.com\n\nChange-Id: I123456789') def test_gerrit_reviewer_multiple(self): self.mock(git_cl.gerrit_util, 'GetCodeReviewTbrScore', @@ -1260,14 +1373,18 @@ class TestGitCl(TestCase): expected_upstream_ref='origin/master', cc=['more@example.com', 'people@example.com'], tbr='reviewer@example.com', - labels={'Code-Review': 2}) + labels={'Code-Review': 2}, + change_id='123456789', + original_title='Initial upload') def test_gerrit_upload_squash_first_is_default(self): self._run_gerrit_upload_test( [], 'desc\nBUG=\n\nChange-Id: 123456789', [], - expected_upstream_ref='origin/master') + expected_upstream_ref='origin/master', + change_id='123456789', + original_title='Initial upload') def test_gerrit_upload_squash_first(self): self._run_gerrit_upload_test( @@ -1275,7 +1392,9 @@ class TestGitCl(TestCase): 'desc\nBUG=\n\nChange-Id: 123456789', [], squash=True, - expected_upstream_ref='origin/master') + expected_upstream_ref='origin/master', + change_id='123456789', + original_title='Initial upload') def test_gerrit_upload_squash_first_with_labels(self): self._run_gerrit_upload_test( @@ -1284,7 +1403,9 @@ class TestGitCl(TestCase): [], squash=True, expected_upstream_ref='origin/master', - labels={'Commit-Queue': 1, 'Auto-Submit': 1}) + labels={'Commit-Queue': 1, 'Auto-Submit': 1}, + change_id='123456789', + original_title='Initial upload') def test_gerrit_upload_squash_first_against_rev(self): custom_cl_base = 'custom_cl_base_rev_or_branch' @@ -1294,7 +1415,9 @@ class TestGitCl(TestCase): [], squash=True, expected_upstream_ref='origin/master', - custom_cl_base=custom_cl_base) + custom_cl_base=custom_cl_base, + change_id='123456789', + original_title='Initial upload') self.assertIn( 'If you proceed with upload, more than 1 CL may be created by Gerrit', sys.stdout.getvalue()) @@ -1307,7 +1430,9 @@ class TestGitCl(TestCase): [], squash=True, expected_upstream_ref='origin/master', - issue=123456) + issue=123456, + change_id='123456789', + original_title='User input') def test_gerrit_upload_squash_reupload_to_abandoned(self): self.mock(git_cl, 'DieWithError', @@ -1321,7 +1446,8 @@ class TestGitCl(TestCase): squash=True, expected_upstream_ref='origin/master', issue=123456, - fetched_status='ABANDONED') + fetched_status='ABANDONED', + change_id='123456789') def test_gerrit_upload_squash_reupload_to_not_owned(self): self.mock(git_cl.gerrit_util, 'GetAccountDetails', @@ -1334,7 +1460,9 @@ class TestGitCl(TestCase): squash=True, expected_upstream_ref='origin/master', issue=123456, - other_cl_owner='other@example.com') + other_cl_owner='other@example.com', + change_id='123456789', + original_title='User input') self.assertIn( 'WARNING: Change 123456 is owned by other@example.com, but you ' 'authenticate to Gerrit as yet-another@example.com.\n'