presubmit: Call 'git diff' once per change instead of once per file

presubmit_support.py

Index: presubmit_support.py
diff --git a/presubmit_support.py b/presubmit_support.py
index 9baee8c5c0b90d51ada1fa560dc338b90b44a802..27d85f303c6659e7c05fcb9320424806d51b3404 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -489,11 +489,75 @@ class InputApi(object):
     return [m for m in msgs if m]
 
 
+class _DiffCache(object):

[iannucci, 2013/06/07 02:17:36]

Why not build this into the scm interface and hide the logic in GenerateDiff? 

[ncarter (slow), 2013/06/07 19:51:15]

It seemed risky, since there are many other callers to GenerateDiff() and I
didn't know what their needs were, or how to appropriately test them.

Here's what I didn't know: It wasn't clear to me whether any current or future
scripts might edit the files on disk and then try to recompute a diff. I've
heard rumors that whitespace errors are auto-corrected on commit or upload or
something

It also would mean that the cache would be global, and I'd likely have to
support the full interface of GenerateDiff (all its arguments would be a key
into the cache) as opposed to only needing to test and validate the subset of
GenerateDiff used here. With a global cache, would that cause any of the
existing unittests (which seem to create git repositories on the fly) fail
because of stale caching?

[iannucci, 2013/06/07 21:30:16]

Hmm.. fair point. This code is pretty labyrinthine :/

+  """Caches diffs retrieved from a particular SCM."""
+
+  def GetScmDiff(self, path, local_root):

[iannucci, 2013/06/07 02:17:36]

Maybe 'GetDiff'?

[ncarter (slow), 2013/06/07 19:51:15]

Done.

+    """Get the diff for a particular path."""
+    raise NotImplementedError()
+
+
+class _SvnDiffCache(_DiffCache):
+  """DiffCache implementation for subversion."""
+  def __init__(self):
+    _DiffCache.__init__(self)

[iannucci, 2013/06/07 02:17:36]

super(_SvnDiffCache, self).__init__()

[ncarter (slow), 2013/06/07 19:51:15]

D'oh, how rusty of me. I knew this looked funny but couldn't put my finger on
it.

+    self._diffs_by_file = {}
+
+  def GetScmDiff(self, path, local_root):
+    if path not in self._diffs_by_file:
+      self._diffs_by_file[path] = scm.SVN.GenerateDiff([path], local_root,
+                                                       False, None)
+    return self._diffs_by_file[path]
+
+
+class _GitDiffCache(_DiffCache):
+  """DiffCache implementation for git; gets all file diffs at once."""
+  def __init__(self):
+    _DiffCache.__init__(self)

[iannucci, 2013/06/07 02:17:36]

super(_GitDiffCache, self).__init__()

[ncarter (slow), 2013/06/07 19:51:15]

Done.

+    self._diffs_by_file = None
+
+  def GetScmDiff(self, path, local_root):
+    if not self._diffs_by_file:
+      # Compute a single diff for all files and parse the output; should
+      # with git this is much faster than computing one diff for each file.
+      diffs = {}
+
+      # Don't specify any filenames below, because there are command line length
+      # limits on some platforms and GenerateDiff would fail.
+      unified_diff = scm.GIT.GenerateDiff(local_root, files=[], full_move=True)
+
+      # This regex matches the path twice, separated by a space. Note that
+      # filename itself may contain spaces.
+      file_marker = re.compile('^diff --git (?P<filename>.*) (?P=filename)$')

[iannucci, 2013/06/07 02:17:36]

won't there be a/ and b/ here? Or does scm.GIT do magic?

[ncarter (slow), 2013/06/07 19:51:15]

the a/ and b/ are omitted when the --no-prefix option to git diff is used.
GenerateDiff always uses the --no-prefix option.

+      current_diff = []
+      for x in unified_diff.splitlines(True):

[iannucci, 2013/06/07 02:17:36]

Since keepends is infrequently used, let's invoke this as
splitlines(keepends=True)

[ncarter (slow), 2013/06/07 19:51:15]

I agree, but actually splitlines is one of those python C functions whose
optional arguments actually can only be specified positionally: (
http://bugs.python.org/issue13012 ). Next best thing I could do was to create a
var called keep_line_endings = True, for the sake of readability. Done.

[iannucci, 2013/06/07 21:30:16]

That's a bummer. New version lg though.

+        match = file_marker.match(x)
+        if match:
+          # Marks the start of a new per-file section.
+          diffs[match.group('filename')] = current_diff = [x]
+        elif x.startswith('diff --git'):
+          raise PresubmitFailure('Unexpected diff line: %s' % x)
+        else:
+          current_diff.append(x)
+
+      self._diffs_by_file = dict(
+        (normpath(path), ''.join(diff)) for path, diff in diffs.items())
+
+    if path not in self._diffs_by_file:
+      raise PresubmitFailure(
+          'Unified diff did not contain entry for file %s' % path)
+
+    return self._diffs_by_file[path]
+
+
 class AffectedFile(object):
   """Representation of a file in a change."""
+
+  DIFF_CACHE = _DiffCache
+
   # Method could be a function
   # pylint: disable=R0201
-  def __init__(self, path, action, repository_root):
+  def __init__(self, path, action, repository_root, diff_cache=None):
     self._path = path
     self._action = action
     self._local_root = repository_root
@@ -501,6 +565,10 @@ class AffectedFile(object):
     self._properties = {}
     self._cached_changed_contents = None
     self._cached_new_contents = None
+    if diff_cache:
+      self._diff_cache = diff_cache
+    else:
+      self._diff_cache = self.DIFF_CACHE()
     logging.debug('%s(%s)' % (self.__class__.__name__, self._path))
 
   def ServerPath(self):
@@ -508,7 +576,7 @@ class AffectedFile(object):
 
     Returns the empty string if the file does not exist in SCM.
     """
-    return ""
+    return ''
 
   def LocalPath(self):
     """Returns the path of this file on the local disk relative to client root.
@@ -565,22 +633,6 @@ class AffectedFile(object):
           pass  # File not found?  That's fine; maybe it was deleted.
     return self._cached_new_contents[:]
 
-  def OldContents(self):
-    """Returns an iterator over the lines in the old version of file.
-
-    The old version is the file in depot, i.e. the "left hand side".
-    """
-    raise NotImplementedError()  # Implement when needed
-
-  def OldFileTempPath(self):
-    """Returns the path on local disk where the old contents resides.
-
-    The old version is the file in depot, i.e. the "left hand side".
-    This is a read-only cached copy of the old contents. *DO NOT* try to
-    modify this file.
-    """
-    raise NotImplementedError()  # Implement if/when needed.
-
   def ChangedContents(self):
     """Returns a list of tuples (line number, line text) of all new lines.
 
@@ -612,7 +664,7 @@ class AffectedFile(object):
     return self.LocalPath()
 
   def GenerateScmDiff(self):
-    raise NotImplementedError()  # Implemented in derived classes.
+    return self._diff_cache.GetScmDiff(self.LocalPath(), self._local_root)
 
 
 class SvnAffectedFile(AffectedFile):
@@ -620,11 +672,12 @@ class SvnAffectedFile(AffectedFile):
   # Method 'NNN' is abstract in class 'NNN' but is not overridden
   # pylint: disable=W0223
 
+  DIFF_CACHE = _SvnDiffCache
+
   def __init__(self, *args, **kwargs):
     AffectedFile.__init__(self, *args, **kwargs)
     self._server_path = None
     self._is_text_file = None
-    self._diff = None
 
   def ServerPath(self):
     if self._server_path is None:
@@ -664,23 +717,18 @@ class SvnAffectedFile(AffectedFile):
         self._is_text_file = (not mime_type or mime_type.startswith('text/'))
     return self._is_text_file
 
-  def GenerateScmDiff(self):
-    if self._diff is None:
-      self._diff = scm.SVN.GenerateDiff(
-          [self.LocalPath()], self._local_root, False, None)
-    return self._diff
-
 
 class GitAffectedFile(AffectedFile):
   """Representation of a file in a change out of a git checkout."""
   # Method 'NNN' is abstract in class 'NNN' but is not overridden
   # pylint: disable=W0223
 
+  DIFF_CACHE = _GitDiffCache
+
   def __init__(self, *args, **kwargs):
     AffectedFile.__init__(self, *args, **kwargs)
     self._server_path = None
     self._is_text_file = None
-    self._diff = None
 
   def ServerPath(self):
     if self._server_path is None:
@@ -714,12 +762,6 @@ class GitAffectedFile(AffectedFile):
         self._is_text_file = os.path.isfile(self.AbsoluteLocalPath())
     return self._is_text_file
 
-  def GenerateScmDiff(self):
-    if self._diff is None:
-      self._diff = scm.GIT.GenerateDiff(
-          self._local_root, files=[self.LocalPath(),])
-    return self._diff
-
 
 class Change(object):
   """Describe a change.
@@ -728,7 +770,7 @@ class Change(object):
   tested.
 
   Instance members:
-    tags: Dictionnary of KEY=VALUE pairs found in the change description.
+    tags: Dictionary of KEY=VALUE pairs found in the change description.
     self.KEY: equivalent to tags['KEY']
   """
 
@@ -769,9 +811,10 @@ class Change(object):
     assert all(
         (isinstance(f, (list, tuple)) and len(f) == 2) for f in files), files
 
+    diff_cache = self._AFFECTED_FILES.DIFF_CACHE()
     self._affected_files = [
-        self._AFFECTED_FILES(info[1], info[0].strip(), self._local_root)
-        for info in files
+        self._AFFECTED_FILES(path, action.strip(), self._local_root, diff_cache)
+        for action, path in files
     ]
 
   def Name(self):