Revert of [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 roll_candidate')
+  'revision author_email commit_timestamp message_lines spec')
 
 
 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 (CommitMetadata).
+    Returns {
+      'author': '<author name>',
+      'message': '<commit message>',
+      'spec': package_pb2.Package or None,  # the parsed recipes.cfg file.
+    }
     """
     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):
+  def updates(self, revision, other_revision, paths):
     """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)
+    return self._updates_impl(revision, other_revision, paths)
 
   ### 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):
+  def _updates_impl(self, revision, other_revision, paths):
     """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):
+  def _updates_impl(self, revision, other_revision, paths):
     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, has_interesting_changes(spec, changed_files))
+                          spec)
 
 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 changed_files')):
+    'commit author_email commit_timestamp message_lines')):
   @classmethod
   def from_raw_json(cls, raw):
-    mod_files = set()
-    for entry in raw['tree_diff']:
-      mod_files.add(entry['old_path'])
-      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)
   def _fetch_gitiles(self, url_fmt, *args):
     """Fetches a remote URL path and returns the response object on success.
 
     Args:
       url_fmt (str) - the url path fragment as a python %format string, like
         '%s/foo/bar?something=value'
       *args (str) - the arguments to format url_fmt with. They will be URL
         escaped.
 
     Returns requests.Response.
     """
     url = '%s/%s' % (self.repo_url,
                      url_fmt % tuple(map(requests.utils.quote, args)))
     LOGGER.info('fetching %s' % url)
     resp = requests.get(url)
     if resp.status_code != httplib.OK:
       raise GitilesFetchError(resp.status_code, resp.text)
     return resp
 
-  def _fetch_gitiles_committish_json(self, url_fmt, *args):
+  def _fetch_gitiles_json(self, url_fmt, *args):
     """Fetches a remote URL path and expects a JSON object on success.
 
-    This appends two GET params to url_fmt:
-      format=JSON   - Does what you expect
-      name-status=1 - Ensures that commit objects returned have a 'tree_diff'
-        member which shows the diff for that commit.
-
     Args:
       url_fmt (str) - the url path fragment as a python %format string, like
         '%s/foo/bar?something=value'
       *args (str) - the arguments to format url_fmt with. They will be URL
         escaped.
 
     Returns the decoded JSON object
     """
-    resp = self._fetch_gitiles(url_fmt+'?name-status=1&format=JSON', *args)
+    resp = self._fetch_gitiles(url_fmt, *args)
     if not resp.text.startswith(self._GERRIT_XSRF_HEADER):
       raise GitilesFetchError(resp.status_code, 'Missing XSRF prefix')
     return json.loads(resp.text[len(self._GERRIT_XSRF_HEADER):])
 
   # This caches entries from _fetch_commit_json. It's populated by
   # _fetch_commit_json as well as _updates_impl.
   #
   # Mapping of:
   #   repo_url -> git_revision -> _GitilesCommitJson
   #
   # Only populated if _fetch_commit_json is passed a resolved commit.
   _COMMIT_JSON_CACHE = {}
 
   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_committish_json('+/%s', refspec)
+    raw = self._fetch_gitiles_json('+/%s?format=JSON', refspec)
     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):
+  def _updates_impl(self, revision, other_revision, paths):
     self.assert_remote('_updates_impl')
 
     # TODO(iannucci): implement paging
 
-    log_json = self._fetch_gitiles_committish_json(
-      '+log/%s..%s', revision, other_revision)
+    # 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)
-      results.append(commit)
+
+      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.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,
-      has_interesting_changes(spec, rev_json.changed_files))
+      spec)