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

Add is_new to be used in a later patch by checkout classes.

R=dpranke@chromium.org
BUG=
TEST=

Review URL: http://codereview.chromium.org/7054048

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@87838 0039d316-1c4b-4281-b951-d872f2087c98

patch.py

@@ -29,6 +29,7 @@ class FilePatchBase(object):
   """
   is_delete = False
   is_binary = False
+  is_new = False
 
   def __init__(self, filename):
     self.filename = None
@@ -44,7 +45,7 @@ class FilePatchBase(object):
       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):
@@ -75,10 +76,11 @@ 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
@@ -195,52 +197,86 @@ class FilePatchDiff(FilePatchBase):
     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 <>
+    last_line = ''
+
     while lines:
-      if lines[0].startswith('--- '):
-        break
       line = lines.pop(0)
-      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))
-        if new != match.group(2):
-          self._fail('Unexpected git diff output name for %s.' % match.group(1))
-        continue
+      # TODO(maruel): old should be replace with self.source_file
+      # TODO(maruel): new == self.filename and remove new
+      self._verify_git_header_process_line(lines, line, last_line, old, new)
+      last_line = line
 
-      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', '*'))
+    # 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.')
 
-    # Handle ---/+++
-    while lines:
-      match = re.match(r'^--- (.*)$', lines.pop(0))
-      if not match:
-        continue
+  def _verify_git_header_process_line(self, lines, line, last_line, old, new):
+    """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 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 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 "--- "
+    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:
+      if not lines or not lines[0].startswith('+++'):
         self._fail('Missing git diff output name.')
-      match = re.match(r'^\+\+\+ (.*)$', lines.pop(0))
-      if not match:
+      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)))
-      assert not lines, '_split_header() is broken'
-      break
+      if lines:
+        self._fail('Crap after +++')
+      # We're done.
+      return
 
   def _verify_svn_header(self):
     """Sanity checks the header.
@@ -250,30 +286,52 @@ class FilePatchDiff(FilePatchBase):
     localized.
     """
     lines = self.diff_header.splitlines()
+    last_line = ''
+
     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.
+      line = lines.pop(0)
+      self._verify_svn_header_process_line(lines, line, last_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_process_line(self, lines, line, last_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 match.group(1) not in (self.filename, '/dev/null'):
+      # if (self.mangle(match.group(1)) != self.filename and
+      #     match.group(1) != '/dev/null'):
       #  self.source_file = match.group(1)
-      if not lines:
+      if not lines or not lines[0].startswith('+++'):
         self._fail('Nothing after header.')
-      match = re.match(r'^\+\+\+ ([^\t]+).*$', lines.pop(0))
-      if not match:
+      return
+
+    match = re.match(r'^\+\+\+ ([^\t]+).*$', line)
+    if match:
+      if not last_line.startswith('---'):
         self._fail('Unexpected diff: --- not following +++.')
-      if match.group(1) not in (self.filename, '/dev/null'):
+      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))
-      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.')
+      if lines:
+        self._fail('Crap after +++')
+      # We're done.
+      return
 
 
 class PatchSet(object):

rietveld.py

@@ -133,7 +133,8 @@ class Rietveld(object):
           out.append(patch.FilePatchBinary(
               filename,
               self.get_file_content(issue, patchset, state['id']),
-              props))
+              props,
+              is_new=(status[0] == 'A')))
         else:
           if state['num_chunks']:
             diff = self.get_file_diff(issue, patchset, state['id'])

tests/checkout_test.py

@@ -151,7 +151,8 @@ class BaseTest(fake_repos.FakeReposTestBase):
     return patch.PatchSet([
         patch.FilePatchDiff(
             'svn_utils_test.txt', GIT_PATCH, []),
-        patch.FilePatchBinary('bin_file', '\x00', []),
+        # TODO(maruel): Test with is_new == False.
+        patch.FilePatchBinary('bin_file', '\x00', [], is_new=True),
         patch.FilePatchDelete('extra', False),
         patch.FilePatchDiff('new_dir/subdir/new_file', PATCH_ADD, []),
     ])

tests/patch_test.py

@@ -5,6 +5,7 @@
 
 """Unit tests for patch.py."""
 
+import logging
 import os
 import sys
 import unittest
@@ -125,21 +126,30 @@ GIT_NEW = (
     '+bar\n')
 
 
+NEW = (
+    '--- /dev/null\n'
+    '+++ foo\n'
+    '@@ -0,0 +1 @@\n'
+    '+bar\n')
+
+
 class PatchTest(unittest.TestCase):
   def testFilePatchDelete(self):
     c = patch.FilePatchDelete('foo', False)
-    self.assertEquals(c.is_delete, True)
-    self.assertEquals(c.is_binary, False)
     self.assertEquals(c.filename, 'foo')
+    self.assertEquals(c.is_binary, False)
+    self.assertEquals(c.is_delete, True)
+    self.assertEquals(c.is_new, False)
     try:
       c.get()
       self.fail()
     except NotImplementedError:
       pass
     c = patch.FilePatchDelete('foo', True)
-    self.assertEquals(c.is_delete, True)
-    self.assertEquals(c.is_binary, True)
     self.assertEquals(c.filename, 'foo')
+    self.assertEquals(c.is_binary, True)
+    self.assertEquals(c.is_delete, True)
+    self.assertEquals(c.is_new, False)
     try:
       c.get()
       self.fail()
@@ -147,59 +157,97 @@ class PatchTest(unittest.TestCase):
       pass
 
   def testFilePatchBinary(self):
-    c = patch.FilePatchBinary('foo', 'data', [])
-    self.assertEquals(c.is_delete, False)
+    c = patch.FilePatchBinary('foo', 'data', [], is_new=False)
+    self.assertEquals(c.filename, 'foo')
     self.assertEquals(c.is_binary, True)
+    self.assertEquals(c.is_delete, False)
+    self.assertEquals(c.is_new, False)
+    self.assertEquals(c.get(), 'data')
+
+  def testFilePatchBinaryNew(self):
+    c = patch.FilePatchBinary('foo', 'data', [], is_new=True)
     self.assertEquals(c.filename, 'foo')
+    self.assertEquals(c.is_binary, True)
+    self.assertEquals(c.is_delete, False)
+    self.assertEquals(c.is_new, True)
     self.assertEquals(c.get(), 'data')
 
   def testFilePatchDiff(self):
     c = patch.FilePatchDiff('chrome/file.cc', SVN_PATCH, [])
-    self.assertEquals(c.is_delete, False)
-    self.assertEquals(c.is_binary, False)
     self.assertEquals(c.filename, 'chrome/file.cc')
+    self.assertEquals(c.is_binary, False)
+    self.assertEquals(c.is_delete, False)
     self.assertEquals(c.is_git_diff, False)
+    self.assertEquals(c.is_new, False)
     self.assertEquals(c.patchlevel, 0)
     self.assertEquals(c.get(), SVN_PATCH)
+
+  def testFilePatchDiffHeaderMode(self):
     diff = (
         'diff --git a/git_cl/git-cl b/git_cl/git-cl\n'
         'old mode 100644\n'
         'new mode 100755\n')
     c = patch.FilePatchDiff('git_cl/git-cl', diff, [])
-    self.assertEquals(c.is_delete, False)
-    self.assertEquals(c.is_binary, False)
     self.assertEquals(c.filename, 'git_cl/git-cl')
+    self.assertEquals(c.is_binary, False)
+    self.assertEquals(c.is_delete, False)
     self.assertEquals(c.is_git_diff, True)
+    self.assertEquals(c.is_new, False)
     self.assertEquals(c.patchlevel, 1)
     self.assertEquals(c.get(), diff)
+
+  def testFilePatchDiffHeaderModeIndex(self):
     diff = (
         'Index: Junk\n'
         'diff --git a/git_cl/git-cl b/git_cl/git-cl\n'
         'old mode 100644\n'
         'new mode 100755\n')
     c = patch.FilePatchDiff('git_cl/git-cl', diff, [])
-    self.assertEquals(c.is_delete, False)
-    self.assertEquals(c.is_binary, False)
     self.assertEquals(c.filename, 'git_cl/git-cl')
+    self.assertEquals(c.is_binary, False)
+    self.assertEquals(c.is_delete, False)
     self.assertEquals(c.is_git_diff, True)
+    self.assertEquals(c.is_new, False)
     self.assertEquals(c.patchlevel, 1)
     self.assertEquals(c.get(), diff)
 
-  def testFilePatchBadDiff(self):
+  def testFilePatchDiffSvnNew(self):
+    # The code path is different for git and svn.
+    c = patch.FilePatchDiff('foo', NEW, [])
+    self.assertEquals(c.filename, 'foo')
+    self.assertEquals(c.is_binary, False)
+    self.assertEquals(c.is_delete, False)
+    self.assertEquals(c.is_git_diff, False)
+    self.assertEquals(c.is_new, True)
+    self.assertEquals(c.patchlevel, 0)
+    self.assertEquals(c.get(), NEW)
+
+  def testFilePatchDiffGitNew(self):
+    # The code path is different for git and svn.
+    c = patch.FilePatchDiff('foo', GIT_NEW, [])
+    self.assertEquals(c.filename, 'foo')
+    self.assertEquals(c.is_binary, False)
+    self.assertEquals(c.is_delete, False)
+    self.assertEquals(c.is_git_diff, True)
+    self.assertEquals(c.is_new, True)
+    self.assertEquals(c.patchlevel, 1)
+    self.assertEquals(c.get(), GIT_NEW)
+
+  def testFilePatchDiffBad(self):
     try:
       patch.FilePatchDiff('foo', 'data', [])
       self.fail()
     except patch.UnsupportedPatchFormat:
       pass
 
-  def testFilePatchNoDiff(self):
+  def testFilePatchDiffEmpty(self):
     try:
       patch.FilePatchDiff('foo', '', [])
       self.fail()
     except patch.UnsupportedPatchFormat:
       pass
 
-  def testFilePatchNoneDiff(self):
+  def testFilePatchDiffNone(self):
     try:
       patch.FilePatchDiff('foo', None, [])
       self.fail()
@@ -210,10 +258,60 @@ class PatchTest(unittest.TestCase):
     try:
       patch.FilePatchDiff('foo', SVN_PATCH, [])
       self.fail()
+    except patch.UnsupportedPatchFormat, e:
+      self.assertEquals(
+          "Can't process patch for file foo.\nUnexpected diff: chrome/file.cc.",
+          str(e))
+
+  def testFilePatchDiffBadHeader(self):
+    try:
+      diff = (
+        '+++ b/foo\n'
+        '@@ -0,0 +1 @@\n'
+        '+bar\n')
+      patch.FilePatchDiff('foo', diff, [])
+      self.fail()
     except patch.UnsupportedPatchFormat:
       pass
 
-  def testInvalidFilePatchDiffGit(self):
+  def testFilePatchDiffBadGitHeader(self):
+    try:
+      diff = (
+        'diff --git a/foo b/foo\n'
+        '+++ b/foo\n'
+        '@@ -0,0 +1 @@\n'
+        '+bar\n')
+      patch.FilePatchDiff('foo', diff, [])
+      self.fail()
+    except patch.UnsupportedPatchFormat:
+      pass
+
+  def testFilePatchDiffBadHeaderReversed(self):
+    try:
+      diff = (
+        '+++ b/foo\n'
+        '--- b/foo\n'
+        '@@ -0,0 +1 @@\n'
+        '+bar\n')
+      patch.FilePatchDiff('foo', diff, [])
+      self.fail()
+    except patch.UnsupportedPatchFormat:
+      pass
+
+  def testFilePatchDiffGitBadHeaderReversed(self):
+    try:
+      diff = (
+        'diff --git a/foo b/foo\n'
+        '+++ b/foo\n'
+        '--- b/foo\n'
+        '@@ -0,0 +1 @@\n'
+        '+bar\n')
+      patch.FilePatchDiff('foo', diff, [])
+      self.fail()
+    except patch.UnsupportedPatchFormat:
+      pass
+
+  def testFilePatchDiffInvalidGit(self):
     try:
       patch.FilePatchDiff('svn_utils_test.txt', (
         'diff --git a/tests/svn_utils_test_data/svn_utils_test.txt '
@@ -291,7 +389,7 @@ class PatchTest(unittest.TestCase):
         patch.FilePatchDiff('pp', GIT_COPY, []),
         patch.FilePatchDiff('foo', GIT_NEW, []),
         patch.FilePatchDelete('other/place/foo', True),
-        patch.FilePatchBinary('bar', 'data', []),
+        patch.FilePatchBinary('bar', 'data', [], is_new=False),
     ])
     expected = [
         'chrome/file.cc', 'tools/clang_check/README.chromium',
@@ -326,6 +424,16 @@ class PatchTest(unittest.TestCase):
     except patch.UnsupportedPatchFormat:
       pass
 
+  def testRelPathEmpty(self):
+    patches = patch.PatchSet([
+        patch.FilePatchDiff('chrome\\file.cc', SVN_PATCH, []),
+        patch.FilePatchDelete('other\\place\\foo', True),
+    ])
+    patches.set_relpath('')
+    self.assertEquals(
+        ['chrome/file.cc', 'other/place/foo'],
+        [f.filename for f in patches])
+
   def testBackSlash(self):
     mangled_patch = SVN_PATCH.replace('chrome/', 'chrome\\')
     patches = patch.PatchSet([
@@ -407,4 +515,7 @@ class PatchTest(unittest.TestCase):
 
 
 if __name__ == '__main__':
+  logging.basicConfig(level=
+      [logging.WARNING, logging.INFO, logging.DEBUG][
+        min(2, sys.argv.count('-v'))])
   unittest.main()