Reland "git-cl: Keep git push traces"

This is a reland of dc8e23d356 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 <ehmaldonado@chromium.org> > Reviewed-by: Dirk Pranke <dpranke@chromium.org> Bug: 955206 Change-Id: Ie63a3ebfe3024d48ad7e0c8492a2939e635371aa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1600246 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
6 years ago · 1b52d87ab5
parent 2e48312247
commit 1b52d87ab5
2 changed files with 253 additions and 57 deletions
--- 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 '<untitled>',
+        '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.*')
--- 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 '<untitled>'
+    # 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'