Increase coverage to 91%; Much stricter about header parsing.

patch.py

 # coding=utf8
 # Copyright (c) 2011 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.
 """Utility functions to handle patches."""
 
 import posixpath
 import os
 import re
 
@@ skipping 11 matching lines @@
     return out
 
 
 class FilePatchBase(object):
   """Defines a single file being modified.
 
   '/' is always used instead of os.sep for consistency.
   """
   is_delete = False
   is_binary = False
+  is_new = False
 
   def __init__(self, filename):
     self.filename = None
     self._set_filename(filename)
 
   def _set_filename(self, filename):
     self.filename = filename.replace('\\', '/')
     # Blacklist a few characters for simplicity.
     for i in ('%', '$', '..', '\'', '"'):
       if i in self.filename:
         self._fail('Can\'t use \'%s\' in filename.' % i)
     for i in ('/', 'CON', 'COM'):
       if self.filename.startswith(i):
         self._fail('Filename can\'t start with \'%s\'.' % i)
 
-  def get(self):
+  def get(self):  # pragma: no coverage
     raise NotImplementedError('Nothing to grab')
 
   def set_relpath(self, relpath):
     if not relpath:
       return
     relpath = relpath.replace('\\', '/')
     if relpath[0] == '/':
       self._fail('Relative path starts with %s' % relpath[0])
     self._set_filename(posixpath.join(relpath, self.filename))
 
@@ skipping 10 matching lines @@
     self.is_binary = is_binary
 
   def get(self):
     raise NotImplementedError('Nothing to grab')
 
 
 class FilePatchBinary(FilePatchBase):
   """Content of a new binary file."""
   is_binary = True
 
-  def __init__(self, filename, data, svn_properties):
+  def __init__(self, filename, data, svn_properties, is_new):
     super(FilePatchBinary, self).__init__(filename)
     self.data = data
     self.svn_properties = svn_properties or []
+    self.is_new = is_new
 
   def get(self):
     return self.data
 
 
 class FilePatchDiff(FilePatchBase):
   """Patch for a single file."""
 
   def __init__(self, filename, diff, svn_properties):
     super(FilePatchDiff, self).__init__(filename)
@@ skipping 54 matching lines @@
     # Rename partial change:
     # http://codereview.chromium.org/download/issue6250123_3013_6010.diff
     # Rename no change:
     # http://codereview.chromium.org/download/issue6287022_3001_4010.diff
     return any(l.startswith('diff --git') for l in diff_header.splitlines())
 
   def mangle(self, string):
     """Mangle a file path."""
     return '/'.join(string.replace('\\', '/').split('/')[self.patchlevel:])
 
   def _verify_git_header(self):

[Dirk Pranke, 2011/06/03 19:17:27]

This function is getting awfully long. Can it be broken up?

[M-A Ruel, 2011/06/03 19:42:31]

done

     """Sanity checks the header.
 
     Expects the following format:
 
     <garbagge>
     diff --git (|a/)<filename> (|b/)<filename>
     <similarity>
     <filemode changes>
     <index>
     <copy|rename from>
@@ skipping 21 matching lines @@
       # The rename is about the new file so the old file can be anything.
       if new not in (self.filename, 'dev/null'):
         self._fail('Unexpected git diff output name %s.' % new)
       if old == 'dev/null' and new == 'dev/null':
         self._fail('Unexpected /dev/null git diff.')
       break
 
     if not old or not new:
       self._fail('Unexpected git diff; couldn\'t find git header.')
 
-    # Handle these:
-    # new file mode \d{6}
-    # rename from <>
-    # rename to <>
-    # copy from <>
-    # copy to <>
-    while lines:
-      if lines[0].startswith('--- '):
-        break
-      line = lines.pop(0)
+    last_line = ''
+
+    def process_line(line):

[Dirk Pranke, 2011/06/03 19:17:27]

Does this really need to be a nested function?

[M-A Ruel, 2011/06/03 19:42:31]

not anymore. so much state to keep

+      """Processes a single line of the header.
+
+      Returns True if it should continue looping.
+      """
+      # Handle these:
+      #  rename from <>
+      #  copy from <>
       match = re.match(r'^(rename|copy) from (.+)$', line)
       if match:
         if old != match.group(2):
-          self._fail('Unexpected git diff input name for %s.' % match.group(1))
-        if not lines:
-          self._fail('Missing git diff output name for %s.' % match.group(1))
-        match = re.match(r'^(rename|copy) to (.+)$', lines.pop(0))
-        if not match:
-          self._fail('Missing git diff output name for %s.' % match.group(1))
+          self._fail('Unexpected git diff input name for line %s.' % line)
+        if not lines or not lines[0].startswith('%s to ' % match.group(1)):
+          self._fail(
+              'Confused %s from/to git diff for line %s.' %
+                  (match.group(1), line))
+        return
+
+      # Handle these:
+      #  rename to <>
+      #  copy to <>
+      match = re.match(r'^(rename|copy) to (.+)$', line)
+      if match:
         if new != match.group(2):
-          self._fail('Unexpected git diff output name for %s.' % match.group(1))
-        continue
+          self._fail('Unexpected git diff output name for line %s.' % line)
+        if not last_line.startswith('%s from ' % match.group(1)):
+          self._fail(
+              'Confused %s from/to git diff for line %s.' %
+                  (match.group(1), line))
+        return
 
+      # Handle "new file mode \d{6}"
       match = re.match(r'^new file mode (\d{6})$', line)
       if match:
         mode = match.group(1)
         # Only look at owner ACL for executable.
         if bool(int(mode[4]) & 4):
           self.svn_properties.append(('svn:executable', '*'))
 
-    # Handle ---/+++
+      # Handle "--- "
+      match = re.match(r'^--- (.*)$', line)
+      if match:
+        if last_line[:3] in ('---', '+++'):
+          self._fail('--- and +++ are reversed')
+        self.is_new = match.group(1) == '/dev/null'
+        # TODO(maruel): Use self.source_file.
+        if old != self.mangle(match.group(1)) and match.group(1) != '/dev/null':
+          self._fail('Unexpected git diff: %s != %s.' % (old, match.group(1)))
+        if not lines or not lines[0].startswith('+++'):
+          self._fail('Missing git diff output name.')
+        return
+
+      # Handle "+++ "
+      match = re.match(r'^\+\+\+ (.*)$', line)
+      if match:
+        if not last_line.startswith('---'):
+          self._fail('Unexpected git diff: --- not following +++.')
+        # TODO(maruel): new == self.filename.
+        if new != self.mangle(match.group(1)) and '/dev/null' != match.group(1):
+          # TODO(maruel): Can +++ be /dev/null? If so, assert self.is_delete ==
+          # True.
+          self._fail('Unexpected git diff: %s != %s.' % (new, match.group(1)))
+        if lines:
+          self._fail('Crap after +++')
+        # We're done.
+        return
+
     while lines:
-      match = re.match(r'^--- (.*)$', lines.pop(0))
-      if not match:
-        continue
-      if old != self.mangle(match.group(1)) and match.group(1) != '/dev/null':
-        self._fail('Unexpected git diff: %s != %s.' % (old, match.group(1)))
-      if not lines:
-        self._fail('Missing git diff output name.')
-      match = re.match(r'^\+\+\+ (.*)$', lines.pop(0))
-      if not match:
-        self._fail('Unexpected git diff: --- not following +++.')
-      if new != self.mangle(match.group(1)) and '/dev/null' != match.group(1):
-        self._fail('Unexpected git diff: %s != %s.' % (new, match.group(1)))
-      assert not lines, '_split_header() is broken'
-      break
+      line = lines.pop(0)
+      process_line(line)
+      last_line = line
+
+    # Cheap check to make sure the file name is at least mentioned in the
+    # 'diff' header. That the only remaining invariant.
+    if not self.filename in self.diff_header:
+      self._fail('Diff seems corrupted.')
 
   def _verify_svn_header(self):
     """Sanity checks the header.
 
     A svn diff can contain only property changes, in that case there will be no
     proper header. To make things worse, this property change header is
     localized.
     """
     lines = self.diff_header.splitlines()
+    last_line = ''
+
+    def process_line(line):
+      """Processes a single line of the header.
+
+      Returns True if it should continue looping.
+      """
+      match = re.match(r'^--- ([^\t]+).*$', line)
+      if match:
+        if last_line[:3] in ('---', '+++'):
+          self._fail('--- and +++ are reversed')
+        self.is_new = match.group(1) == '/dev/null'
+        # For copy and renames, it's possible that the -- line doesn't match
+        # +++, so don't check match.group(1) to match self.filename or
+        # '/dev/null', it can be anything else.
+        # TODO(maruel): Handle rename/copy explicitly.
+        # if (self.mangle(match.group(1)) != self.filename and
+        #     match.group(1) != '/dev/null'):
+        #  self.source_file = match.group(1)
+        if not lines or not lines[0].startswith('+++'):
+          self._fail('Nothing after header.')
+        return
+
+      match = re.match(r'^\+\+\+ ([^\t]+).*$', line)
+      if match:
+        if not last_line.startswith('---'):
+          self._fail('Unexpected diff: --- not following +++.')
+        if (self.mangle(match.group(1)) != self.filename and
+            match.group(1) != '/dev/null'):
+          # TODO(maruel): Can +++ be /dev/null? If so, assert self.is_delete ==
+          # True.
+          self._fail('Unexpected diff: %s.' % match.group(1))
+        if lines:
+          self._fail('Crap after +++')
+        # We're done.
+        return
+
     while lines:
-      match = re.match(r'^--- ([^\t]+).*$', lines.pop(0))
-      if not match:
-        continue
-      # For copy and renames, it's possible that the -- line doesn't match +++,
-      # so don't check match.group(1) to match self.filename or '/dev/null', it
-      # can be anything else.
-      # TODO(maruel): Handle rename/copy explicitly.
-      # if match.group(1) not in (self.filename, '/dev/null'):
-      #  self.source_file = match.group(1)
-      if not lines:
-        self._fail('Nothing after header.')
-      match = re.match(r'^\+\+\+ ([^\t]+).*$', lines.pop(0))
-      if not match:
-        self._fail('Unexpected diff: --- not following +++.')
-      if match.group(1) not in (self.filename, '/dev/null'):
-        self._fail('Unexpected diff: %s.' % match.group(1))
-      assert not lines, '_split_header() is broken'
-      break
-    else:
-      # Cheap check to make sure the file name is at least mentioned in the
-      # 'diff' header. That the only remaining invariant.
-      if not self.filename in self.diff_header:
-        self._fail('Diff seems corrupted.')
+      line = lines.pop(0)
+      process_line(line)
+      last_line = line
 
+    # Cheap check to make sure the file name is at least mentioned in the
+    # 'diff' header. That the only remaining invariant.
+    if not self.filename in self.diff_header:
+      self._fail('Diff seems corrupted.')
 
 class PatchSet(object):
   """A list of FilePatch* objects."""
 
   def __init__(self, patches):
     self.patches = patches
     for p in self.patches:
       assert isinstance(p, FilePatchBase)
 
   def set_relpath(self, relpath):
     """Used to offset the patch into a subdirectory."""
     for patch in self.patches:
       patch.set_relpath(relpath)
 
   def __iter__(self):
     for patch in self.patches:
       yield patch
 
   @property
   def filenames(self):
     return [p.filename for p in self.patches]