Changed FindSuspects to take a Repository factory, rather than mutating it

appengine/findit/crash/changelist_classifier.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import logging
 from collections import defaultdict
 from collections import namedtuple
 
-from common import chrome_dependency_fetcher
+from common.chrome_dependency_fetcher import ChromeDependencyFetcher
 from crash import crash_util
 from crash.suspect import StackInfo
 from crash.suspect import Suspect
 from crash.suspect import SuspectMap
 from crash.scorers.aggregated_scorer import AggregatedScorer
 from crash.scorers.min_distance import MinDistance
 from crash.scorers.top_frame_index import TopFrameIndex
 from crash.stacktrace import CallStack
 from crash.stacktrace import Stacktrace
 from libs.gitiles.diff import ChangeType
 
 
 class ChangelistClassifier(namedtuple('ChangelistClassifier',
-    ['repository', 'top_n_results', 'confidence_threshold'])):
+    ['repository', 'get_repository', 'top_n_results', 'confidence_threshold'])):

[Sharu Jiang, 2016/12/28 00:45:33]

The ``get_repository`` here is not actually a factory, it is to mutate
repository, so this name is a little misleading. Better add comments to explain
that it will be cleaned in the future. 

[Sharu Jiang, 2016/12/28 00:45:33]

So after we cleaned the mutation in ``get_repository``(make it allocates new
repository object instead of doing mutation), we will get rid of this
``repository`` parameter, right?

[wrengr, 2016/12/28 18:23:33]

The ``get_repository`` argument is indeed (intended to be) a factory.
``FindSuspects`` doesn't rely on/require it to return novel objects (e.g., it
doesn't save them anywhere or anything requiring longevity beyond from when
``get_repository`` is called to when ``FindSuspects`` returns). Which is why we
can get away with passing a fake "factory" that doesn't actually build anything.
Yes, passing a fake factory is bad; but that's firmly the fault of the caller
not the callee.

[wrengr, 2016/12/28 18:23:33]

In principle we could... but since the main ``repository`` is used frequently
and longlived-ly, we should probably keep it. This way the caller of
ChangelistClassifier.__init__ can build the repo object once, pass it to the
constructor, and then forget about it, the url associated with it, etc (if
needed, the caller can always retrieve that information from the
ChangelistClassifier object itself).

The simplest alternative to keeping it around would be to pass the url to the
constructor, and understand that the constructor will call ``get_repository`` on
it and save the result. That seems less clean than passing the repo object, imo

   __slots__ = ()
 
-  def __new__(cls, repository, top_n_results=3, confidence_threshold=0.999):
+  def __new__(cls, repository, get_repository, top_n_results=3,
+              confidence_threshold=0.999):
     """Args:
-      repository (Repository): the Git repository for getting CLs to classify.
+      repository (Repository): the Git repository of the main project
+        we're trying to classify CLs from.
+      get_repository (callable): a function from DEP urls to ``Repository``
+        objects, so we can get changelogs and blame for each dep. Notably,
+        to keep the code here generic, we make no assumptions about
+        which subclass of ``Repository`` this function returns. Thus,
+        it is up to the caller to decide what class to return and handle
+        any other arguments that class may require (e.g., an http client
+        for ``GitilesRepository``).
       top_n_results (int): maximum number of results to return.
       confidence_threshold (float): In [0,1], above which we only return
         the first suspect.
     """
     return super(cls, ChangelistClassifier).__new__(
-        cls, repository, top_n_results, confidence_threshold)
+        cls, repository, get_repository, top_n_results, confidence_threshold)
 
   def __str__(self): # pragma: no cover
     return ('%s(top_n_results=%d, confidence_threshold=%g)'
         % (self.__class__.__name__,
            self.top_n_results,
            self.confidence_threshold))
 
   def __call__(self, report):
     """Finds changelists suspected of being responsible for the crash report.
 
     This function assumes the report's stacktrace has already had any necessary
     preprocessing (like filtering or truncating) applied.
 
     Args:
       report (CrashReport): the report to be analyzed.
 
     Returns:
       List of ``Suspect``s, sorted by confidence from highest to lowest.
     """
     if not report.regression_range:
       logging.warning('ChangelistClassifier.__call__: Missing regression range '
           'for report: %s', str(report))
       return []
     last_good_version, first_bad_version = report.regression_range
     logging.info('ChangelistClassifier.__call__: Regression range %s:%s',
         last_good_version, first_bad_version)
 
+    dependency_fetcher = ChromeDependencyFetcher(self.repository)

[Sharu Jiang, 2016/12/28 00:45:33]

I think ``ChromeDependencyFetcher`` should also take a repository factory as
construct parameter instead of repository instance, because it's ``Chrome``
dependency fetcher, which means the dep url has to be chromium dep url, so
repository should be created inside ``ChromeDependencyFetcher`` with dep url set
to chromium url.

[wrengr, 2016/12/28 18:23:33]

I'm not sure I follow. Perhaps you could write up a CL demonstrating the change
to ChromeDependencyFetcher you have in mind

[wrengr, 2016/12/28 19:09:24]

ah, nm. I think I see what you mean now. I'm working on cleaning u the
ChromeDependencyFetcher stuff too in the CL that gets rid of the mutation

[wrengr, 2016/12/28 20:55:52]

See https://codereview.chromium.org/2605943002 for cleaning all this up

+
     # We are only interested in the deps in crash stack (the callstack that
     # caused the crash).
     # TODO(wrengr): we may want to receive the crash deps as an argument,
     # so that when this method is called via Findit.FindCulprit, we avoid
     # doing redundant work creating it.
     stack_deps = GetDepsInCrashStack(
         report.stacktrace.crash_stack,
-        chrome_dependency_fetcher.ChromeDependencyFetcher(
-            self.repository).GetDependency(report.crashed_version,
-                                           report.platform))
+        dependency_fetcher.GetDependency(
+            report.crashed_version, report.platform))
 
     # Get dep and file to changelogs, stack_info and blame dicts.
-    dep_rolls = chrome_dependency_fetcher.ChromeDependencyFetcher(
-        self.repository).GetDependencyRollsDict(
-            last_good_version, first_bad_version, report.platform)
+    dep_rolls = dependency_fetcher.GetDependencyRollsDict(
+        last_good_version, first_bad_version, report.platform)
 
     # Regression of a dep added/deleted (old_revision/new_revision is None) can
     # not be known for sure and this case rarely happens, so just filter them
     # out.
     regression_deps_rolls = {}
     for dep_path, dep_roll in dep_rolls.iteritems():
       if not dep_roll.old_revision or not dep_roll.new_revision:
         logging.info('Skip %s denpendency %s',
                      'added' if dep_roll.new_revision else 'deleted', dep_path)
         continue
       regression_deps_rolls[dep_path] = dep_roll
 
     dep_to_file_to_changelogs, ignore_cls = GetChangeLogsForFilesGroupedByDeps(
         regression_deps_rolls, stack_deps, self.repository)
     dep_to_file_to_stack_infos = GetStackInfosForFilesGroupedByDeps(
         report.stacktrace, stack_deps)
 
-    # TODO: argument order is inconsistent from others. Repository should
-    # be last argument.
     suspects = FindSuspects(dep_to_file_to_changelogs,
                             dep_to_file_to_stack_infos,
-                            stack_deps, self.repository, ignore_cls)
+                            stack_deps, self.get_repository, ignore_cls)
     if not suspects:
       return []
 
     # Set confidence, reasons, and changed_files.
     aggregated_scorer = AggregatedScorer([TopFrameIndex(), MinDistance()])
     map(aggregated_scorer.Score, suspects)
 
     # Filter all the 0 confidence results.
     suspects = filter(lambda suspect: suspect.confidence != 0, suspects)
     if not suspects:
@@ skipping 141 matching lines @@
 
       dep_to_file_to_stack_infos[frame.dep_path][frame.file_path].append(
           StackInfo(frame, callstack.priority))
 
   return dep_to_file_to_stack_infos
 
 
 # TODO(katesonia): Remove the repository argument after refatoring cl committed.
 def FindSuspects(dep_to_file_to_changelogs,
                  dep_to_file_to_stack_infos,
-                 stack_deps, repository,
+                 stack_deps, get_repository,
                  ignore_cls=None):
   """Finds suspects by matching stacktrace and changelogs in regression range.
 
   This method only applies to those crashes with regression range.
 
   Args:
     dep_to_file_to_changelogs (dict): Maps dep_path to a dict mapping file path
       to ChangeLogs that touched this file.
     dep_to_file_to_stack_infos (dict): Maps dep path to a dict mapping file path
       to a list of stack information of this file. A file may occur in several
       frames, one stack info consist of a StackFrame and the callstack priority
       of it.
     stack_deps (dict): Represents all the dependencies shown in the crash stack.
-    repository (Repository): Repository to get changelogs and blame from.
+    get_repository (callable): a function from urls to ``Repository``
+      objects, so we can get changelogs and blame for each dep.
     ignore_cls (set): Set of reverted revisions.
 
   Returns:
     A list of ``Suspect`` instances with confidence and reason unset.
   """
   suspects = SuspectMap(ignore_cls)
 
   for dep, file_to_stack_infos in dep_to_file_to_stack_infos.iteritems():
     file_to_changelogs = dep_to_file_to_changelogs[dep]
 
     for crashed_file_path, stack_infos in file_to_stack_infos.iteritems():
       for touched_file_path, changelogs in file_to_changelogs.iteritems():
         if not crash_util.IsSameFilePath(crashed_file_path, touched_file_path):
           continue
 
-        repository.repo_url = stack_deps[dep].repo_url
+        repository = get_repository(stack_deps[dep].repo_url)
         blame = repository.GetBlame(touched_file_path,
                                     stack_deps[dep].revision)
 
         # Generate/update each suspect(changelog) in changelogs, blame is used
         # to calculate distance between touched lines and crashed lines in file.
         suspects.GenerateSuspects(
             touched_file_path, dep, stack_infos, changelogs, blame)
 
   return suspects.values()