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

presubmit_support.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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.
 
 """Enables directory-specific presubmit checks to run at upload and/or commit.
 """
 
 __version__ = '1.6.2'
 
@@ skipping 471 matching lines @@
       pool = multiprocessing.Pool()
       # async recipe works around multiprocessing bug handling Ctrl-C
       msgs.extend(pool.map_async(CallCommand, tests).get(99999))
       pool.close()
       pool.join()
     else:
       msgs.extend(map(CallCommand, tests))
     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
     self._is_directory = None
     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):
     """Returns a path string that identifies the file in the SCM system.
 
     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.
     """
     return normpath(self._path)
 
   def AbsoluteLocalPath(self):
     """Returns the absolute path of this file on the local disk.
     """
     return os.path.abspath(os.path.join(self._local_root, self.LocalPath()))
@@ skipping 36 matching lines @@
     if self._cached_new_contents is None:
       self._cached_new_contents = []
       if not self.IsDirectory():
         try:
           self._cached_new_contents = gclient_utils.FileRead(
               self.AbsoluteLocalPath(), 'rU').splitlines()
         except IOError:
           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.
 
      This relies on the scm diff output describing each changed code section
      with a line of the form
 
      ^@@ <old line num>,<old size> <new line num>,<new size> @@$
     """
     if self._cached_changed_contents is not None:
       return self._cached_changed_contents[:]
@@ skipping 11 matching lines @@
       if line.startswith('+') and not line.startswith('++'):
         self._cached_changed_contents.append((line_num, line[1:]))
       if not line.startswith('-'):
         line_num += 1
     return self._cached_changed_contents[:]
 
   def __str__(self):
     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):
   """Representation of a file in a change out of a Subversion checkout."""
   # 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:
       self._server_path = scm.SVN.CaptureLocalInfo(
           [self.LocalPath()], self._local_root).get('URL', '')
     return self._server_path
 
   def IsDirectory(self):
     if self._is_directory is None:
       path = self.AbsoluteLocalPath()
@@ skipping 19 matching lines @@
         # A deleted file is not a text file.
         self._is_text_file = False
       elif self.IsDirectory():
         self._is_text_file = False
       else:
         mime_type = scm.SVN.GetFileProperty(
             self.LocalPath(), 'svn:mime-type', self._local_root)
         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:
       raise NotImplementedError('TODO(maruel) Implement.')
     return self._server_path
 
   def IsDirectory(self):
     if self._is_directory is None:
       path = self.AbsoluteLocalPath()
       if os.path.exists(path):
@@ skipping 13 matching lines @@
     if self._is_text_file is None:
       if self.Action() == 'D':
         # A deleted file is not a text file.
         self._is_text_file = False
       elif self.IsDirectory():
         self._is_text_file = False
       else:
         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.
 
   Used directly by the presubmit scripts to query the current change being
   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']
   """
 
   _AFFECTED_FILES = AffectedFile
 
   # Matches key/value (or "tag") lines in changelist descriptions.
   TAG_LINE_RE = re.compile(
       '^[ \t]*(?P<key>[A-Z][A-Z_0-9]*)[ \t]*=[ \t]*(?P<value>.*?)[ \t]*$')
   scm = ''
 
@@ skipping 20 matching lines @@
       else:
         description_without_tags.append(line)
 
     # Change back to text and remove whitespace at end.
     self._description_without_tags = (
         '\n'.join(description_without_tags).rstrip())
 
     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):
     """Returns the change name."""
     return self._name
 
   def DescriptionText(self):
     """Returns the user-entered changelist description, minus tags.
 
     Any line in the user-provided description starting with e.g. "FOO="
@@ skipping 598 matching lines @@
   except PresubmitFailure, e:
     print >> sys.stderr, e
     print >> sys.stderr, 'Maybe your depot_tools is out of date?'
     print >> sys.stderr, 'If all fails, contact maruel@'
     return 2
 
 
 if __name__ == '__main__':
   fix_encoding.fix_encoding()
   sys.exit(Main(None))