[Findit] Plain objects to represent the returned result from running the algorithm,

tools/findit/blame.py

+# Copyright (c) 2014 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.
+from threading import Lock, Thread

[stgao, 2014/08/04 20:44:56]

Please add an empty line before this one.

[jeun, 2014/08/06 21:33:03]

Done.

+
+import findit_for_crash_utils as findit_utils

[stgao, 2014/08/04 20:44:56]

Seems not up-to-day.

[jeun, 2014/08/06 21:33:03]

Done.

+import gitparser
+import svnparser

[stgao, 2014/08/04 20:44:56]

Hmm, why we use the parsers here?

I remember blame.Blame was imported by the parsers instead.

[jeun, 2014/08/06 21:33:04]

I have changed the code that parsers do not use them anymore.

+import utils
+
+
+class Blame(object):
+  """Represents a blame object.
+
+  The object contains blame information for one line of stack, and this
+  information is shown when there are no CLs that change the crashing files.
+  Attributes:
+    content: the content of the line to find the blame for
+    component: the component this line is in
+    stack_frame_index: stack frame index of this file
+    file_name: name of the file
+    line: line that caused a crash

[stgao, 2014/08/04 20:44:56]

line_number?

[jeun, 2014/08/06 21:33:03]

Done.

+    author: the author of this line on the latest revision
+    crash_revision: revision that caused the crash
+    revision: the latest revision of this line before the crash revision
+    url: url of this revision

[stgao, 2014/08/04 20:44:56]

url to the file change or the whole CL change?

[jeun, 2014/08/06 21:33:04]

This revision, the whole CL change.

[stgao, 2014/08/09 19:51:02]

In that case, please update the documentation here and make it a little clear.

[jeun, 2014/08/12 18:17:33]

Done.

+    regression: the regression range of the component, if it exists
+
+  """
+
+  def __init__(self, content, component, stack_frame_index, file_name, line,
+               author, crash_revision, revision, url, regression):
+    # Set all the variables from the arguments

[stgao, 2014/08/04 20:44:56]

Style: should end with a dot.

[jeun, 2014/08/06 21:33:03]

Done.

+    self.content = content
+    self.component = component
+    self.stack_frame_index = stack_frame_index
+    self.file = file_name
+    self.line = line
+    self.author = author
+    self.revision = revision
+    self.url = url
+    self.regression = regression
+    self.distance = float('inf')
+    revision = int(revision)
+
+    # Calculate the distance, where it measures how far the last revision is
+    # from the regression range
+    if regression:
+      range1 = int(regression[0])
+      range2 = int(regression[1])
+      self.distance = min(abs(range1 - revision), abs(range2 - revision))
+
+    # If the regression is in SVN but it does not have regression info, check
+    # how far the last revision is from crash revision
+    elif not utils.IsGitHash(crash_revision):
+      self.distance = abs(int(crash_revision) - revision)
+
+
+class BlameList(object):
+  """Represents a list of blame objects.
+
+  Thread-safe.
+  """
+
+  def __init__(self):
+    self.blame_list = []
+    self.blame_list_lock = Lock()
+
+  def __getitem__(self, index):
+    return self.blame_list[index]
+
+  def AddBlame(self, blame):
+    """Adds blame object to the set."""
+    with self.blame_list_lock:
+      self.blame_list.append(blame)
+
+  def sort(self, key=None):
+    return self.blame_list.sort(key=key)
+
+  def FindBlame(self, callstack, crash_revision_dic, revision_range_dic,
+                url_map):
+    """Given a stack within a stacktrace, retrieves blame information.
+
+    Only either first 10 or the length of stack, whichever is shorter,

[stgao, 2014/08/04 20:44:56]

How about making it configurable?

[jeun, 2014/08/06 21:33:03]

Done.

+    results are returned.
+
+    Args:
+      callstack: a list of stacktrace lines
+      crash_revision_dic: a dictionary that maps component to its crash revision
+      revision_range_dic: a dictionary that maps component to its revision range
+      url_map: a map from repository type to urls
+
+    Returns:
+    a list of blame object

[stgao, 2014/08/04 20:44:56]

indent?

[jeun, 2014/08/06 21:33:04]

Done.

+    """
+    # Only return blame information for first 10 frames
+    stack_list = callstack.GetFirstN(10)
+
+    threads = []
+    # Iterate through frames in stack
+    for stacktrace_line in stack_list:
+

[stgao, 2014/08/04 20:44:57]

This empty line could be removed.

[jeun, 2014/08/06 21:33:03]

Done.

+      # If the component this line is from does not have a crash revision,
+      # It is not possible to get blame information so ignore this line
+      component = stacktrace_line.component
+      if component not in crash_revision_dic:
+        continue
+
+      crash_revision = crash_revision_dic[component]
+      regression = None
+      is_svn = not utils.IsGitHash(crash_revision)
+
+      # If the revision is in SVN, and if revision information is available,
+      # get it. Not for Git because we cannot calculate the distance

[stgao, 2014/08/04 20:44:56]

style: no ending dot.

It is a common issue in this patch.
Please check all comments by doing a grep.

[jeun, 2014/08/06 21:33:04]

Done.

+      if is_svn:
+        repository_parser = svnparser.SVNParser(url_map['svn'])
+        if revision_range_dic and component in revision_range_dic:
+          regression = revision_range_dic[component]
+      else:
+        repository_parser = gitparser.GitParser(url_map['git'])
+
+      # Generate blame entry, one thread for one entry
+      blame_thread = Thread(
+          target=self.__GenerateBlameList,
+          args=[repository_parser, stacktrace_line, crash_revision, regression])
+      threads.append(blame_thread)
+      blame_thread.start()
+
+    # Join the results before returning
+    for blame_thread in threads:
+      blame_thread.join()

[stgao, 2014/08/04 20:44:57]

It seems nothing is returned.
If this is intended, the documentation of this function should be updated.

[jeun, 2014/08/06 21:33:04]

Done.

+
+  def __GenerateBlameList(self, repository_parser, stacktrace_line,

[stgao, 2014/08/04 20:44:56]

This function generate a single Blame only. The name is kinda misleading.

[jeun, 2014/08/06 21:33:03]

Done.

+                          crash_revision, regression):
+    """Generates blame list from the arguments."""
+    stack_frame_index = stacktrace_line.stack_frame_index
+    component = stacktrace_line.component
+    file_name = stacktrace_line.file_name
+    file_path = stacktrace_line.file_path
+    line = stacktrace_line.crashed_line
+
+    blame = repository_parser.ParseBlameInfo(
+        stack_frame_index, component, file_name, file_path, line,
+        crash_revision, regression)
+
+    if blame:
+      self.AddBlame(blame)
+
+  def PrettifyBlame(self):
+    """Returns a string representation of blame results.
+
+    Returns:
+      a string representation of blame_list
+    """
+    blame_list = self.blame_list
+    return_string = []
+    return_string.append('Below are the blame information of the stacktrace.\n')
+    return_string.append('[\n')
+
+    # If blame info is not available, return the message
+    if not blame_list:
+      return_string.append('No Blame Information Available.\n')
+      return ''.join(return_string)
+
+    # Sort the blame list by its distance, and its position in stack
+    blame_list.sort(key=lambda blame: (blame.diff, blame.stack))

[stgao, 2014/08/04 20:44:56]

blame.diff?

[jeun, 2014/08/06 21:33:03]

changed to distance

+
+    for blame in blame_list:
+

[stgao, 2014/08/04 20:44:57]

This empty line could be removed.

[jeun, 2014/08/06 21:33:03]

Done.

+      # If regression information is available, do some filtering
+      if blame.regression:
+        regressed = blame.regression
+
+        # Discards results that are too far from the regression range.
+        # For example, if regression is 10000:11000, it is very not
+        # likely that a commit from revision 1000 would have caused a crash.
+        if blame.distance > int(regressed[0]) / 4 and blame.distance > int(
+            regressed[1]) / 4:
+          continue
+
+      return_string.append('  {\n')
+      return_string.append(
+          '    suspected_cl: %s\n' % findit_utils.AddHyperlink(
+              blame.revision,
+              blame.url))
+      return_string.append('    component: %s\n' % blame.component)
+      return_string.append('    owner: %s\n' % blame.author)
+      reason = (
+          'The CL changes line %s of file %s from stack %d.' %
+          (blame.line, blame.file, blame.stack))
+      return_string.append('    reason: %s\n' % reason)
+      if blame.content:
+        return_string.append('    content: %s\n' % blame.content)
+      return_string.append('  }\n')
+
+    return_string.append(']\n')
+    return_string.append('-------------------------------------------\n')
+    return ''.join(return_string)