From e7f0b4c62c551cde2723d1456e007fcb10f53ac2 Mon Sep 17 00:00:00 2001 From: Michael Moss Date: Wed, 8 May 2019 04:36:24 +0000 Subject: [PATCH] Revert "git-cl: Keep git push traces" This reverts commit dc8e23d35612f213aa1ee554196f640cc315e4d5. Reason for revert: Breaking chrome releases crbug.com/960638 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 TBR=dpranke@chromium.org,ehmaldonado@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 955206 Change-Id: Id56dfb1b32a54070d761ef75bc26bbb9081f86fc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1600535 Reviewed-by: Michael Moss Commit-Queue: Michael Moss --- git_cl.py | 148 ++++++++++++------------------------------- tests/git_cl_test.py | 137 +++++---------------------------------- 2 files changed, 58 insertions(+), 227 deletions(-) diff --git a/git_cl.py b/git_cl.py index 66b205337f..8f82be47b7 100755 --- a/git_cl.py +++ b/git_cl.py @@ -79,30 +79,12 @@ 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 = ( -'\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 +'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') COMMIT_BOT_EMAIL = 'commit-bot@chromium.org' POSTUPSTREAM_HOOK = '.git/hooks/post-cl-land' @@ -223,12 +205,6 @@ 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) @@ -2520,73 +2496,18 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): else: print('OK, will keep Gerrit commit-msg hook in place.') - 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.""" + def _RunGitPushWithTraces(self, change_desc, refspec, refspec_opts): 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') - 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() - 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' @@ -2597,10 +2518,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): try: push_returncode = 0 - remote_url = self.GetRemoteUrl() before_push = time_time() push_stdout = gclient_utils.CheckCallAndFilter( - ['git', 'push', remote_url, refspec], + ['git', 'push', self.GetRemoteUrl(), refspec], env=env, print_stdout=True, # Flush after every line: useful for seeing progress when running as @@ -2612,7 +2532,8 @@ 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', + ' git cl creds-check\n' + + TRACES_MESSAGE % (traces_zip, git_info_zip), change_desc) finally: execution_time = time_time() - before_push @@ -2623,11 +2544,31 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'arguments': metrics_utils.extract_known_subcommand_args(refspec_opts), }) - git_push_metadata['execution_time'] = execution_time - git_push_metadata['exit_code'] = push_returncode - self._WriteGitPushTraces(traces_dir, git_push_metadata) - - self._CleanUpOldTraces() + 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) + + # 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) gclient_utils.rmtree(traces_dir) return push_stdout @@ -2881,14 +2822,7 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'spaces not allowed in refspec: "%s"' % refspec_suffix) refspec = '%s:refs/for/%s%s' % (ref_to_push, branch, refspec_suffix) - 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) + push_stdout = self._RunGitPushWithTraces(change_desc, refspec, refspec_opts) 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 386ca8ef51..36c099706a 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -909,8 +909,7 @@ class TestGitCl(TestCase): post_amend_description=None, issue=None, cc=None, custom_cl_base=None, tbr=None, short_hostname='chromium', - labels=None, change_id=None, original_title=None, - final_description=None): + labels=None): if post_amend_description is None: post_amend_description = description cc = cc or [] @@ -1108,67 +1107,6 @@ 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. - ((['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, - ), - ((['FileRead', '~/.gitcookies'],), - 'gitcookies 1/SECRET', - ), - ((['FileWrite', 'TEMP_DIR/gitcookies', 'gitcookies REDACTED'],), - None, - ), - # 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'],), @@ -1209,10 +1147,7 @@ class TestGitCl(TestCase): custom_cl_base=None, tbr=None, short_hostname='chromium', - labels=None, - change_id=None, - original_title=None, - final_description=None): + labels=None): """Generic gerrit upload test framework.""" if squash_mode is None: if '--no-squash' in upload_args: @@ -1234,17 +1169,6 @@ 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.calls = self._gerrit_base_calls( issue=issue, @@ -1266,10 +1190,7 @@ class TestGitCl(TestCase): issue=issue, cc=cc, custom_cl_base=custom_cl_base, tbr=tbr, short_hostname=short_hostname, - labels=labels, - change_id=change_id, - original_title=original_title, - final_description=final_description) + labels=labels) # Uncomment when debugging. # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls)))) git_cl.main(['upload'] + upload_args) @@ -1280,8 +1201,7 @@ class TestGitCl(TestCase): 'desc\n\nBUG=\n', [], squash=False, - post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx', - change_id='Ixxx') + post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx') def test_gerrit_upload_without_change_id_override_nosquash(self): self._run_gerrit_upload_test( @@ -1290,8 +1210,7 @@ class TestGitCl(TestCase): [], squash=False, squash_mode='override_nosquash', - post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx', - change_id='Ixxx') + post_amend_description='desc\n\nBUG=\n\nChange-Id: Ixxx') def test_gerrit_no_reviewer(self): self._run_gerrit_upload_test( @@ -1299,8 +1218,7 @@ class TestGitCl(TestCase): 'desc\n\nBUG=\n\nChange-Id: I123456789\n', [], squash=False, - squash_mode='override_nosquash', - change_id='I123456789') + squash_mode='override_nosquash') def test_gerrit_no_reviewer_non_chromium_host(self): # TODO(crbug/877717): remove this test case. @@ -1310,8 +1228,7 @@ class TestGitCl(TestCase): [], squash=False, squash_mode='override_nosquash', - short_hostname='other', - change_id='I123456789') + short_hostname='other') def test_gerrit_patchset_title_special_chars(self): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) @@ -1320,9 +1237,7 @@ 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', - change_id='I123456789', - original_title='We\'ll escape ^_ ^ special chars...@{u}') + title='We%27ll_escape_%5E%5F_%5E_special_chars%2E%2E%2E%40%7Bu%7D') def test_gerrit_reviewers_cmd_line(self): self._run_gerrit_upload_test( @@ -1331,10 +1246,7 @@ class TestGitCl(TestCase): ['foo@example.com'], squash=False, squash_mode='override_nosquash', - notify=True, - change_id='I123456789', - final_description= - 'desc\n\nBUG=\nR=foo@example.com\n\nChange-Id: I123456789') + notify=True) def test_gerrit_reviewer_multiple(self): self.mock(git_cl.gerrit_util, 'GetCodeReviewTbrScore', @@ -1348,18 +1260,14 @@ class TestGitCl(TestCase): expected_upstream_ref='origin/master', cc=['more@example.com', 'people@example.com'], tbr='reviewer@example.com', - labels={'Code-Review': 2}, - change_id='123456789', - original_title='Initial upload') + labels={'Code-Review': 2}) 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', - change_id='123456789', - original_title='Initial upload') + expected_upstream_ref='origin/master') def test_gerrit_upload_squash_first(self): self._run_gerrit_upload_test( @@ -1367,9 +1275,7 @@ class TestGitCl(TestCase): 'desc\nBUG=\n\nChange-Id: 123456789', [], squash=True, - expected_upstream_ref='origin/master', - change_id='123456789', - original_title='Initial upload') + expected_upstream_ref='origin/master') def test_gerrit_upload_squash_first_with_labels(self): self._run_gerrit_upload_test( @@ -1378,9 +1284,7 @@ class TestGitCl(TestCase): [], squash=True, expected_upstream_ref='origin/master', - labels={'Commit-Queue': 1, 'Auto-Submit': 1}, - change_id='123456789', - original_title='Initial upload') + labels={'Commit-Queue': 1, 'Auto-Submit': 1}) def test_gerrit_upload_squash_first_against_rev(self): custom_cl_base = 'custom_cl_base_rev_or_branch' @@ -1390,9 +1294,7 @@ class TestGitCl(TestCase): [], squash=True, expected_upstream_ref='origin/master', - custom_cl_base=custom_cl_base, - change_id='123456789', - original_title='Initial upload') + custom_cl_base=custom_cl_base) self.assertIn( 'If you proceed with upload, more than 1 CL may be created by Gerrit', sys.stdout.getvalue()) @@ -1405,9 +1307,7 @@ class TestGitCl(TestCase): [], squash=True, expected_upstream_ref='origin/master', - issue=123456, - change_id='123456789', - original_title='User input') + issue=123456) def test_gerrit_upload_squash_reupload_to_abandoned(self): self.mock(git_cl, 'DieWithError', @@ -1421,8 +1321,7 @@ class TestGitCl(TestCase): squash=True, expected_upstream_ref='origin/master', issue=123456, - fetched_status='ABANDONED', - change_id='123456789') + fetched_status='ABANDONED') def test_gerrit_upload_squash_reupload_to_not_owned(self): self.mock(git_cl.gerrit_util, 'GetAccountDetails', @@ -1435,9 +1334,7 @@ class TestGitCl(TestCase): squash=True, expected_upstream_ref='origin/master', issue=123456, - other_cl_owner='other@example.com', - change_id='123456789', - original_title='User input') + other_cl_owner='other@example.com') self.assertIn( 'WARNING: Change 123456 is owned by other@example.com, but you ' 'authenticate to Gerrit as yet-another@example.com.\n'