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

recipe_engine/fetch.py

 # Copyright 2016 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 import calendar
 import httplib
 import json
 import logging
 import os
 import re
@@ skipping 13 matching lines @@
 from . import env
 from . import requests_ssl
 from .requests_ssl import requests
 
 import subprocess42
 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
 
 
 class FetchNotAllowedError(FetchError):
   pass
 
 
 class UnresolvedRefspec(Exception):
   pass
 
 
 # revision (str): the revision of this commit (i.e. hash)
 # author_email (str|None): the email of the author of this commit
 # 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):
   @staticmethod
   def class_for_type(repo_type):
     """
     Args:
       repo_type (package_pb2.DepSpec.RepoType)
 
     Returns Backend (class): Returns the Backend appropriate for the
@@ skipping 43 matching lines @@
   _GIT_METADATA_CACHE = {}
 
   # This matches git commit hashes.
   _COMMIT_RE = re.compile(r'^[a-fA-F0-9]{40}$')
 
   def commit_metadata(self, refspec):
     """Cached version of _commit_metadata_impl.
 
     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, {})
     if revision not in cache:
       cache[revision] = self._commit_metadata_impl(revision)
     return cache[revision]
 
   @classmethod
   def is_resolved_revision(cls, revision):
     return cls._COMMIT_RE.match(revision)
 
   @classmethod
   def assert_resolved(cls, revision):
     if not cls.is_resolved_revision(revision):
       raise UnresolvedRefspec('unresolved refspec %r' % revision)
 
   def resolve_refspec(self, refspec):
     if self.is_resolved_revision(refspec):
       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.
 
   @property
   def repo_type(self):
     """Returns package_pb2.DepSpec.RepoType."""
     raise NotImplementedError()
 
   def fetch(self, refspec):
     """Does a fetch for the provided refspec (e.g. get all data from remote), if
@@ skipping 13 matching lines @@
         remote git repo, e.g. 'refs/heads/master', 'deadbeef...face', etc.
     """
     # TODO(iannucci): Alter the contract for this method so that it only checks
     # out the files referred to according to the rules that the bundle
     # subcommand uses.
     raise NotImplementedError()
 
   ### 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
 
     Returns list(CommitMetadata) - The commit metadata in the range
       [revision,other_revision].
     """
     raise NotImplementedError()
 
   def _resolve_refspec_impl(self, refspec):
@@ skipping 116 matching lines @@
       self.fetch(refspec)
 
     # reset touches index.lock which is problematic when multiple processes are
     # accessing the recipes at the same time. To allieviate this, we do a quick
     # diff, which will exit if `revision` is not already checked out.
     try:
       self._git('diff', '--quiet', revision)
     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')
       if bool(rev)
     ]
 
   def _resolve_refspec_impl(self, revision):
     self._ensure_local_repo_exists()
     self.assert_remote('resolve refspec %r' % revision)
     rslt = self._git('ls-remote', self.repo_url, revision).split()[0]
@@ skipping 11 matching lines @@
     #   %`Body`
     meta = self._git(
       'show', '-s', '--format=%aE%n%ct%n%B', revision).rstrip('\n').splitlines()
 
     try:
       spec = package_io.parse(self._git(
         'cat-file', 'blob', '%s:infra/config/recipes.cfg' % revision))
     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."""
 
   def __init__(self, status, message):
     super(GitilesFetchError, self).__init__(
         'Gitiles error code (%d): %s' % (status, message))
     self.status = status
     self.message = message
 
   @staticmethod
   def transient(e):
     """
     Returns (bool): True if "e" is a GitilesFetchError with transient HTTP code.
     """
     return (isinstance(e, GitilesFetchError) and
             e.status >= httplib.INTERNAL_SERVER_ERROR)
 
 
 # Internal cache object for GitilesBackend.
 # 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),
     )
 
 
 class GitilesBackend(Backend):
   """GitilesBackend uses a repo served by Gitiles."""
 
   # Prefix at the beginning of Gerrit/Gitiles JSON API responses.
   _GERRIT_XSRF_HEADER = ')]}\'\n'
 
   @util.exponential_retry(condition=GitilesFetchError.transient)
@@ skipping 43 matching lines @@
 
   def _fetch_commit_json(self, refspec):
     """Returns _GitilesCommitJson for the refspec.
 
     If refspec is resolved then this value is cached.
     """
     c = self._COMMIT_JSON_CACHE.setdefault(self.repo_url, {})
     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
 
     return ret
 
 
   ### Backend implementations
 
 
@@ skipping 38 matching lines @@
 
     # TODO(iannucci): This implementation may be slow if we need to retieve
     # multiple files/archives from the remote server. Should possibly consider
     # using a thread pool here.
 
     archive_response = self._fetch_gitiles(
       '+archive/%s/%s.tar.gz', revision, recipes_path_rel)
     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
 
     # To include info about touched paths (tree_diff), pass name-status=1 below.
     log_json = self._fetch_gitiles_json(
       '+log/%s..%s?name-status=1&format=JSON', revision, other_revision)
 
     c = self._COMMIT_JSON_CACHE.setdefault(self.repo_url, {})
 
     results = []
     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)
 
   def _resolve_refspec_impl(self, refspec):
     if self.is_resolved_revision(refspec):
       return self.commit_metadata(refspec).commit
     return self._fetch_commit_json(refspec).commit
 
   def _commit_metadata_impl(self, revision):
     self.assert_remote('_commit_metadata_impl')
     rev_json = self._fetch_commit_json(revision)
 
     recipes_cfg_text = self._fetch_gitiles(
       '+/%s/infra/config/recipes.cfg?format=TEXT', revision
     ).text.decode('base64')
     spec = json_format.Parse(
       recipes_cfg_text, package_pb2.Package(), ignore_unknown_fields=True)
 
     return CommitMetadata(
       revision,
       rev_json.author_email,
       rev_json.commit_timestamp,
       rev_json.message_lines,
-      spec)
+      spec,
+      has_interesting_changes(spec, rev_json.changed_files))