Refactor get_footers

It will be easier to support centralized management of what footers exist if we
require all callers to specifically declare which footers they want to check.
This is a partial step towards allowing that without increasing the number of
queries required. It introduced cache staleness as a potential issue, but should
not matter in practice.

Bug: chromium:1079219
Change-Id: I678fd366202bac5a4efa4258ffe02e70c64589bb
Recipe-Nontrivial-Roll: infra
Recipe-Nontrivial-Roll: chromiumos
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2664790
Commit-Queue: Jacob Kopczynski <jkop@chromium.org>
Auto-Submit: Jacob Kopczynski <jkop@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>

recipes/README.recipes.md

@@ -797,38 +797,52 @@ Returns:
 
 #### **class [TryserverApi](/recipes/recipe_modules/tryserver/api.py#11)([RecipeApi][recipe_engine/wkt/RecipeApi]):**
 
-&emsp; **@property**<br>&mdash; **def [gerrit\_change](/recipes/recipe_modules/tryserver/api.py#27)(self):**
+&emsp; **@property**<br>&mdash; **def [gerrit\_change](/recipes/recipe_modules/tryserver/api.py#29)(self):**
 
 Returns current gerrit change, if there is exactly one.
 
 Returns a self.m.buildbucket.common_pb2.GerritChange or None.
 
-&emsp; **@property**<br>&mdash; **def [gerrit\_change\_fetch\_ref](/recipes/recipe_modules/tryserver/api.py#104)(self):**
+&emsp; **@property**<br>&mdash; **def [gerrit\_change\_fetch\_ref](/recipes/recipe_modules/tryserver/api.py#106)(self):**
 
 Returns gerrit patch ref, e.g. "refs/heads/45/12345/6, or None.
 
 Populated iff gerrit_change is populated.
 
-&emsp; **@property**<br>&mdash; **def [gerrit\_change\_owner](/recipes/recipe_modules/tryserver/api.py#43)(self):**
+&emsp; **@property**<br>&mdash; **def [gerrit\_change\_number](/recipes/recipe_modules/tryserver/api.py#124)(self):**
+
+Returns gerrit change patchset, e.g. 12345 for a patch ref of
+"refs/heads/45/12345/6".
+
+Populated iff gerrit_change is populated. Returns None if not populated.
+
+&emsp; **@property**<br>&mdash; **def [gerrit\_change\_owner](/recipes/recipe_modules/tryserver/api.py#45)(self):**
 
 Returns owner of the current Gerrit CL.
 
 Populated iff gerrit_change is populated.
 Is a dictionary with keys like "name".
 
-&emsp; **@property**<br>&mdash; **def [gerrit\_change\_repo\_url](/recipes/recipe_modules/tryserver/api.py#35)(self):**
+&emsp; **@property**<br>&mdash; **def [gerrit\_change\_repo\_url](/recipes/recipe_modules/tryserver/api.py#37)(self):**
 
 Returns canonical URL of the gitiles repo of the current Gerrit CL.
 
 Populated iff gerrit_change is populated.
 
-&emsp; **@property**<br>&mdash; **def [gerrit\_change\_target\_ref](/recipes/recipe_modules/tryserver/api.py#113)(self):**
+&emsp; **@property**<br>&mdash; **def [gerrit\_change\_target\_ref](/recipes/recipe_modules/tryserver/api.py#115)(self):**
 
 Returns gerrit change destination ref, e.g. "refs/heads/master".
 
 Populated iff gerrit_change is populated.
 
-&mdash; **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#143)(self, patch_root, report_files_via_property=None, \*\*kwargs):**
+&emsp; **@property**<br>&mdash; **def [gerrit\_patchset\_number](/recipes/recipe_modules/tryserver/api.py#136)(self):**
+
+Returns gerrit change patchset, e.g. 6 for a patch ref of
+"refs/heads/45/12345/6".
+
+Populated iff gerrit_change is populated Returns None if not populated..
+
+&mdash; **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#169)(self, patch_root, report_files_via_property=None, \*\*kwargs):**
 
 Returns list of paths to files affected by the patch.
 
@@ -840,43 +854,43 @@ Args:
 
 Returned paths will be relative to to patch_root.
 
-&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#265)(self, tag, patch_text=None):**
+&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#306)(self, tag, patch_text=None):**
 
 Gets a specific tag from a CL description
 
-&mdash; **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#245)(self, patch_text=None):**
+&mdash; **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#273)(self, patch_text=None):**
 
 Retrieves footers from the patch description.
 
 footers are machine readable tags embedded in commit messages. See
 git-footers documentation for more information.
 
-&mdash; **def [initialize](/recipes/recipe_modules/tryserver/api.py#22)(self):**
+&mdash; **def [initialize](/recipes/recipe_modules/tryserver/api.py#24)(self):**
 
-&emsp; **@property**<br>&mdash; **def [is\_gerrit\_issue](/recipes/recipe_modules/tryserver/api.py#127)(self):**
+&emsp; **@property**<br>&mdash; **def [is\_gerrit\_issue](/recipes/recipe_modules/tryserver/api.py#153)(self):**
 
 Returns true iff the properties exist to match a Gerrit issue.
 
-&emsp; **@property**<br>&mdash; **def [is\_patch\_in\_git](/recipes/recipe_modules/tryserver/api.py#137)(self):**
+&emsp; **@property**<br>&mdash; **def [is\_patch\_in\_git](/recipes/recipe_modules/tryserver/api.py#163)(self):**
 
-&emsp; **@property**<br>&mdash; **def [is\_tryserver](/recipes/recipe_modules/tryserver/api.py#122)(self):**
+&emsp; **@property**<br>&mdash; **def [is\_tryserver](/recipes/recipe_modules/tryserver/api.py#148)(self):**
 
 Returns true iff we have a change to check out.
 
-&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#269)(self, footer):**
+&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#310)(self, footer):**
 
-&mdash; **def [set\_change](/recipes/recipe_modules/tryserver/api.py#272)(self, change):**
+&mdash; **def [set\_change](/recipes/recipe_modules/tryserver/api.py#313)(self, change):**
 
 Set the gerrit change for this module.
 
 Args:
   * change: a self.m.buildbucket.common_pb2.GerritChange.
 
-&mdash; **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#208)(self):**
+&mdash; **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#234)(self):**
 
 Mark the tryjob result as a compile failure.
 
-&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#220)(self):**
+&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#246)(self):**
 
 Mark the tryjob result as having invalid test results.
 
@@ -884,32 +898,32 @@ This means we run some tests, but the results were not valid
 (e.g. no list of specific test cases that failed, or too many
 tests failing, etc).
 
-&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#204)(self):**
+&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#230)(self):**
 
 Mark the tryjob result as failure to apply the patch.
 
-&mdash; **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#182)(self, subproject_tag):**
+&mdash; **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#208)(self, subproject_tag):**
 
 Adds a subproject tag to the build.
 
 This can be used to distinguish between builds that execute different steps
 depending on what was patched, e.g. blink vs. pure chromium patches.
 
-&mdash; **def [set\_test\_expired\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#237)(self):**
+&mdash; **def [set\_test\_expired\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#263)(self):**
 
 Mark the tryjob result as a test expiration.
 
 This means a test task expired and was never scheduled, most likely due to
 lack of capacity.
 
-&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#212)(self):**
+&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#238)(self):**
 
 Mark the tryjob result as a test failure.
 
 This means we started running actual tests (not prerequisite steps
 like checkout or compile), and some of these tests have failed.
 
-&mdash; **def [set\_test\_timeout\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#229)(self):**
+&mdash; **def [set\_test\_timeout\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#255)(self):**
 
 Mark the tryjob result as a test timeout.
 

recipes/recipe_modules/tryserver/api.py

@@ -18,6 +18,8 @@ class TryserverApi(recipe_api.RecipeApi):
     self._gerrit_change_target_ref = None
     self._gerrit_change_fetch_ref = None
     self._gerrit_change_owner = None
+    self._change_footers = None
+    self._gerrit_commit_message = None
 
   def initialize(self):
     changes = self.m.buildbucket.build.input.gerrit_changes
@@ -119,6 +121,30 @@ class TryserverApi(recipe_api.RecipeApi):
     self._ensure_gerrit_change_info()
     return self._gerrit_change_target_ref
 
+  @property
+  def gerrit_change_number(self):
+    """Returns gerrit change patchset, e.g. 12345 for a patch ref of
+    "refs/heads/45/12345/6".
+
+    Populated iff gerrit_change is populated. Returns None if not populated.
+    """
+    self._ensure_gerrit_change_info()
+    if not self._gerrit_change:  #pragma: nocover
+      return None
+    return int(self._gerrit_change.change)
+
+  @property
+  def gerrit_patchset_number(self):
+    """Returns gerrit change patchset, e.g. 6 for a patch ref of
+    "refs/heads/45/12345/6".
+
+    Populated iff gerrit_change is populated Returns None if not populated..
+    """
+    self._ensure_gerrit_change_info()
+    if not self._gerrit_change:  #pragma: nocover
+      return None
+    return int(self._gerrit_change.patchset)
+
   @property
   def is_tryserver(self):
     """Returns true iff we have a change to check out."""
@@ -242,20 +268,35 @@ class TryserverApi(recipe_api.RecipeApi):
     """
     self._set_failure_type('TEST_EXPIRED')
 
+  # TODO(crbug.com/1179039): switch the test in examples/full.py to not use
+  # patch_text, and drop the argument entirely from all the get_footer variants.
   def get_footers(self, patch_text=None):
     """Retrieves footers from the patch description.
 
     footers are machine readable tags embedded in commit messages. See
     git-footers documentation for more information.
     """
-    if patch_text is None:
-      if self.gerrit_change:
-        # TODO: reuse _ensure_gerrit_change_info.
-        patch_text = self.m.gerrit.get_change_description(
-            'https://%s' % self.gerrit_change.host,
-            int(self.gerrit_change.change),
-            int(self.gerrit_change.patchset))
+    return self._get_footers(patch_text)
+
+  def _ensure_gerrit_commit_message(self):
+    """Fetch full commit message for Gerrit change."""
+    self._ensure_gerrit_change_info()
+    self._gerrit_commit_message = self.m.gerrit.get_change_description(
+        'https://%s' % self.gerrit_change.host, self.gerrit_change_number,
+        self.gerrit_patchset_number)
+
+  def _get_footers(self, patch_text=None):
+    if patch_text is not None:
+      return self._get_footer_step(patch_text)
+    if self._change_footers:  #pragma: nocover
+      return self._change_footers
+    if self.gerrit_change:
+      self._ensure_gerrit_commit_message()
+      self._change_footers = self._get_footer_step(self._gerrit_commit_message)
+      return self._change_footers
+    raise "No patch text or associated changelist, cannot get footers"  #pragma: nocover
 
+  def _get_footer_step(self, patch_text):
     result = self.m.python(
         'parse description', self.repo_resource('git_footers.py'),
         args=['--json', self.m.json.output()],
@@ -264,7 +305,7 @@ class TryserverApi(recipe_api.RecipeApi):
 
   def get_footer(self, tag, patch_text=None):
     """Gets a specific tag from a CL description"""
-    return self.get_footers(patch_text).get(tag, [])
+    return self._get_footers(patch_text).get(tag, [])
 
   def normalize_footer_name(self, footer):
     return '-'.join([ word.title() for word in footer.strip().split('-') ])