[autoroller] All commits in updates(), only roll interesting ones.

recipe_engine/fetch.py

Index: recipe_engine/fetch.py
diff --git a/recipe_engine/fetch.py b/recipe_engine/fetch.py
index 8f417bf34906bf9da46a318720484a0ff8546544..1cc39248f1830eadf7e82a0ef5c548625a2a9256 100644
--- a/recipe_engine/fetch.py
+++ b/recipe_engine/fetch.py
@@ -31,6 +31,14 @@ from google.protobuf import json_format
 LOGGER = logging.getLogger(__name__)
 
 
+def has_interesting_changes(spec, changed_files):
+  # TODO(iannucci): analyze bundle_extra_paths.txt  too.
+  return (
+    'infra/config/recipes.cfg' in changed_files or
+    any(f.startswith(spec.recipes_path) for f in changed_files)
+  )
+
+
 class FetchError(Exception):
   pass
 
@@ -48,9 +56,12 @@ class UnresolvedRefspec(Exception):
 # commit_timestamp (int): the unix commit timestamp for this commit
 # message_lines (tuple(str)): the message of this commit
 # spec (package_pb2.Package): the parsed infra/config/recipes.cfg file or None.
+# roll_candidate (bool): if this commit contains changes which are known to
+#   affect the behavior of the recipes (i.e. modifications within recipe_path
+#   and/or modifications to recipes.cfg)
 CommitMetadata = namedtuple(
   '_CommitMetadata',
-  'revision author_email commit_timestamp message_lines spec')
+  'revision author_email commit_timestamp message_lines spec roll_candidate')
 
 
 class Backend(object):
@@ -114,11 +125,7 @@ class Backend(object):
 
     The refspec will be resolved if it's not absolute.
 
-    Returns {
-      'author': '<author name>',
-      'message': '<commit message>',
-      'spec': package_pb2.Package or None,  # the parsed recipes.cfg file.
-    }
+    Returns (CommitMetadata).
     """
     revision = self.resolve_refspec(refspec)
     cache = self._GIT_METADATA_CACHE.setdefault(self.repo_url, {})
@@ -140,18 +147,16 @@ class Backend(object):
       return refspec
     return self._resolve_refspec_impl(refspec)
 
-  def updates(self, revision, other_revision, paths):
+  def updates(self, revision, other_revision):
     """Returns a list of revisions |revision| through |other_revision|
     (inclusive).
 
-    If |paths| is a non-empty list, the history is scoped just to these paths.
-
     Returns list(CommitMetadata) - The commit metadata in the range
       (revision,other_revision].
     """
     self.assert_resolved(revision)
     self.assert_resolved(other_revision)
-    return self._updates_impl(revision, other_revision, paths)
+    return self._updates_impl(revision, other_revision)
 
   ### direct overrides. These are public methods which must be overridden.
 
@@ -185,12 +190,10 @@ class Backend(object):
   ### private overrides. Override these in the implementations, but don't call
   ### externally.
 
-  def _updates_impl(self, revision, other_revision, paths):
+  def _updates_impl(self, revision, other_revision):
     """Returns a list of revisions |revision| through |other_revision|. This
     includes |revision| and |other_revision|.
 
-    If |paths| is a non-empty list, the history is scoped just to these paths.
-
     Args:
       revision (str) - the first git commit
       other_revision (str) - the second git commit
@@ -327,15 +330,13 @@ class GitBackend(Backend):
     except GitError:
       self._git('reset', '-q', '--hard', revision)
 
-  def _updates_impl(self, revision, other_revision, paths):
+  def _updates_impl(self, revision, other_revision):
     args = [
         'rev-list',
         '--reverse',
         '--topo-order',
         '%s..%s' % (revision, other_revision),
     ]
-    if paths:
-      args.extend(['--'] + paths)
     return [
       self.commit_metadata(rev)
       for rev in self._git(*args).strip().split('\n')
@@ -367,9 +368,14 @@ class GitBackend(Backend):
     except GitError:
       spec = None
 
+    # check diff to see if it touches anything interesting.
+    changed_files = set(self._git(
+      'diff-tree', '-r', '--no-commit-id', '--name-only', '%s^!' % revision)
+      .splitlines())
+
     return CommitMetadata(revision, meta[0],
                           int(meta[1]), tuple(meta[2:]),
-                          spec)
+                          spec, has_interesting_changes(spec, changed_files))
 
 class GitilesFetchError(FetchError):
   """An HTTP error that occurred during Gitiles fetching."""
@@ -393,16 +399,22 @@ class GitilesFetchError(FetchError):
 # commit (str) - the git commit hash
 # author_email (str) - the author email for this commit
 # message_lines (tuple) - the lines of the commit message
+# changed_files (frozenset) - all paths touched by this commit
 class _GitilesCommitJson(namedtuple(
     '_GitilesCommitJson',
-    'commit author_email commit_timestamp message_lines')):
+    'commit author_email commit_timestamp message_lines changed_files')):
   @classmethod
   def from_raw_json(cls, raw):
+    mod_files = set()
+    for entry in raw['tree_diff']:
+      mod_files.add(entry['old_path'])

[dnj, 2017/04/20 18:16:21]

Not sure if all of these fields are guaranteed to exist for every commit. Maybe
use ".get" to be safe?

[iannucci, 2017/04/20 21:53:20]

They are, if there are entries in tree_diff.

+      mod_files.add(entry['new_path'])
     return cls(
       raw['commit'],
       raw['author']['email'],
       calendar.timegm(time.strptime(raw['committer']['time'])),
       tuple(raw['message'].splitlines()),
+      frozenset(mod_files),
     )
 
 
@@ -466,7 +478,7 @@ class GitilesBackend(Backend):
     if refspec in c:
       return c[refspec]
 
-    raw = self._fetch_gitiles_json('+/%s?format=JSON', refspec)
+    raw = self._fetch_gitiles_json('+/%s?name-status=1&format=JSON', refspec)

[dnj, 2017/04/20 18:16:21]

Can we document what "name-status=1" does? format=JSON is simple enough, but
this showing the diff tree isn't obvious.

[iannucci, 2017/04/20 21:53:20]

Done.

     ret = _GitilesCommitJson.from_raw_json(raw)
     if self.is_resolved_revision(refspec):
       c[refspec] = ret
@@ -525,7 +537,7 @@ class GitilesBackend(Backend):
     with tarfile.open(fileobj=StringIO(archive_response.content)) as tf:
       tf.extractall(recipes_path)
 
-  def _updates_impl(self, revision, other_revision, paths):
+  def _updates_impl(self, revision, other_revision):
     self.assert_remote('_updates_impl')
 
     # TODO(iannucci): implement paging
@@ -540,18 +552,7 @@ class GitilesBackend(Backend):
     for entry in log_json['log']:
       commit = entry['commit']
       c[commit] = _GitilesCommitJson.from_raw_json(entry)
-
-      matched = False
-      for path in paths:
-        for diff_entry in entry['tree_diff']:
-          if (diff_entry['old_path'].startswith(path) or
-              diff_entry['new_path'].startswith(path)):
-            matched = True
-            break
-        if matched:
-          break
-      if matched or not paths:
-        results.append(commit)
+      results.append(commit)
 
     results.reverse()
     return map(self.commit_metadata, results)
@@ -576,4 +577,5 @@ class GitilesBackend(Backend):
       rev_json.author_email,
       rev_json.commit_timestamp,
       rev_json.message_lines,
-      spec)
+      spec,
+      has_interesting_changes(spec, rev_json.changed_files))