From eb8426e2ac6e8b03837a8d093863008fbec79860 Mon Sep 17 00:00:00 2001
From: Bruce Dawson <brucedawson@chromium.org>
Date: Fri, 5 Aug 2022 23:58:15 +0000
Subject: [PATCH] Improve presubmit no-network handling

Recent gerrit issues made it obvious that "git cl presubmit" relies on
gerrit much more than most people would expect (the expectation is zero
for many people). This makes presubmits flaky or much slower under poor
network conditions, and it means that the presubmit step may drastically
underestimate how long it takes to run because of a
cl.FetchDescription() that may occur outside of the timed portion of the
presubmits.

This change wraps more network-touching steps in try/except blocks, to
make them robust. It also gets them to check for the existence of a
PRESUBMIT_SKIP_NETWORK environment variable. And, it prints the elapsed
time to get the CL description if this is inordinately long.

Bug: 1350227
Change-Id: I7954fd50e928fd24975a4f61a316cb280542ebbd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3813095
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
---
 git_cl.py                  | 20 ++++++++++++++------
 presubmit_canned_checks.py | 20 ++++++++++++--------
 presubmit_support.py       |  2 +-
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/git_cl.py b/git_cl.py
index 7a5f66736..6725f4a5c 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1772,9 +1772,9 @@ class Changelist(object):
 
     status = self._GetChangeDetail()['status']
     if status == 'ABANDONED':
-       DieWithError(
-           'Change %s has been abandoned, new uploads are not allowed' %
-           (self.GetIssueURL()))
+      DieWithError(
+          'Change %s has been abandoned, new uploads are not allowed' %
+          (self.GetIssueURL()))
     if status == 'MERGED':
       answer = gclient_utils.AskForData(
           'Change %s has been submitted, new uploads are not allowed. '
@@ -4132,10 +4132,18 @@ def CMDpresubmit(parser, args):
     # Default to diffing against the common ancestor of the upstream branch.
     base_branch = cl.GetCommonAncestorWithUpstream()
 
-  if cl.GetIssue():
-    description = cl.FetchDescription()
-  else:
+  start = time.time()
+  try:
+    if not 'PRESUBMIT_SKIP_NETWORK' in os.environ and cl.GetIssue():
+      description = cl.FetchDescription()
+    else:
+      description = _create_description_from_log([base_branch])
+  except Exception as e:
+    print('Failed to fetch CL description - %s' % str(e))
     description = _create_description_from_log([base_branch])
+  elapsed = time.time() - start
+  if elapsed > 5:
+    print('%.1f s to get CL description.' % elapsed)
 
   if not base_branch:
     if not options.force:
diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py
index b0945c565..38a2682ff 100644
--- a/presubmit_canned_checks.py
+++ b/presubmit_canned_checks.py
@@ -705,7 +705,8 @@ def CheckTreeIsOpen(input_api, output_api,
     closed: regex to match for closed status.
     json_url: url to download json style status.
   """
-  if not input_api.is_committing:
+  if not input_api.is_committing or \
+      'PRESUBMIT_SKIP_NETWORK' in _os.environ:
     return []
   try:
     if json_url:
@@ -1439,13 +1440,16 @@ def PanProjectChecks(input_api, output_api,
 
   snapshot("checking owners files format")
   try:
-    results.extend(input_api.canned_checks.CheckOwnersFormat(
-        input_api, output_api))
-
-    if owners_check:
-      snapshot("checking owners")
-      results.extend(input_api.canned_checks.CheckOwners(
-          input_api, output_api, source_file_filter=None))
+    if not 'PRESUBMIT_SKIP_NETWORK' in _os.environ:
+      results.extend(
+          input_api.canned_checks.CheckOwnersFormat(input_api, output_api))
+
+      if owners_check:
+        snapshot("checking owners")
+        results.extend(
+            input_api.canned_checks.CheckOwners(input_api,
+                                                output_api,
+                                                source_file_filter=None))
   except Exception as e:
     print('Failed to check owners - %s' % str(e))
 
diff --git a/presubmit_support.py b/presubmit_support.py
index 55e986254..553b1603f 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -677,7 +677,7 @@ class InputApi(object):
     self._named_temporary_files = []
 
     self.owners_client = None
-    if self.gerrit:
+    if self.gerrit and not 'PRESUBMIT_SKIP_NETWORK' in self.environ:
       try:
         self.owners_client = owners_client.GetCodeOwnersClient(
             root=change.RepositoryRoot(),