Make rietveld status handling saner

Add support for a few svn:* properties.
Silently ignore svn:mergeinfo. It's useless once we're switched to git-land anyway.

More testing.

R=dpranke@chromium.org
BUG=
TEST=


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

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

rietveld.py

@@ -125,13 +125,13 @@ class Rietveld(object):
         raise patch.UnsupportedPatchFormat(
             filename, 'File\'s status is None, patchset upload is incomplete.')
 
-      # TODO(maruel): That's bad, it confuses property change.
-      status = status.strip()
-
-      if status == 'D':
+      if status[0] == 'D':
+        if status[0] != status.strip():
+          raise patch.UnsupportedPatchFormat(
+              filename, 'Deleted file shouldn\'t have property change.')
         # Ignore the diff.
         out.append(patch.FilePatchDelete(filename, state['is_binary']))
-      elif status in ('A', 'M'):
+      elif status[0] in ('A', 'M'):
         svn_props = self.parse_svn_properties(
             state.get('property_changes', ''), filename)
         if state['is_binary']:
@@ -142,20 +142,27 @@ class Rietveld(object):
               is_new=(status[0] == 'A')))
         else:
           # Ignores num_chunks since it may only contain an header.
+          diff = None
           try:
             diff = self.get_file_diff(issue, patchset, state['id'])
           except urllib2.HTTPError, e:
             if e.code == 404:
               raise patch.UnsupportedPatchFormat(
                   filename, 'File doesn\'t have a diff.')
+            raise
+          # It may happen on property-only change or svn copy without diff.
+          if not diff:
+            # Support this use case if it ever happen.
+            raise patch.UnsupportedPatchFormat(
+                filename, 'Empty diff is not supported yet.\n')
           out.append(patch.FilePatchDiff(filename, diff, svn_props))
           if status[0] == 'A':
             # It won't be set for empty file.
             out[-1].is_new = True
       else:
-        # TODO(maruel): Add support for MM, A+, etc. Rietveld removes the svn
-        # properties from the diff.
-        raise patch.UnsupportedPatchFormat(filename, status)
+        raise patch.UnsupportedPatchFormat(
+            filename, 'Change with status \'%s\' is not supported.' % status)
+
     return patch.PatchSet(out)
 
   @staticmethod

tests/rietveld_test.py

@@ -18,6 +18,25 @@ import rietveld
 
 # Access to a protected member XX of a client class
 # pylint: disable=W0212
+GIT_COPY_FULL = (
+    'diff --git a/PRESUBMIT.py b/file_a\n'
+    'similarity index 100%\n'
+    'copy from PRESUBMIT.py\n'
+    'copy to file_a\n')
+
+
+NORMAL_DIFF = (
+    '--- file_a\n'
+    '+++ file_a\n'
+    '@@ -80,10 +80,13 @@\n'
+    ' // Foo\n'
+    ' // Bar\n'
+    ' void foo() {\n'
+    '-   return bar;\n'
+    '+   return foo;\n'
+    ' }\n'
+    ' \n'
+    ' \n')
 
 
 def _api(files):
@@ -137,19 +156,63 @@ class RietveldTest(unittest.TestCase):
     except patch.UnsupportedPatchFormat, e:
       self.assertEquals('file_a', e.filename)
 
-  def test_add_plus(self):
+  def test_add_plus_merge(self):
+    # svn:mergeinfo is dropped.
+    diff = GIT_COPY_FULL
     properties = (
         '\nAdded: svn:mergeinfo\n'
         '   Merged /branches/funky/file_b:r69-2775\n')
     self.requests = [
         ('/api/123/456',
           _api({'file_a': _file('A+', property_changes=properties)})),
+        ('/download/issue123_456_789.diff', diff),
     ]
-    try:
-      self.rietveld.get_patch(123, 456)
-      self.fail()
-    except patch.UnsupportedPatchFormat, e:
-      self.assertEquals('file_a', e.filename)
+    patches = self.rietveld.get_patch(123, 456)
+    self.assertEquals(1, len(patches.patches))
+    self._check_patch(
+        patches.patches[0],
+        'file_a',
+        diff,
+        is_git_diff=True,
+        is_new=True,
+        patchlevel=1)
+
+  def test_add_plus_eol_style(self):
+    diff = GIT_COPY_FULL
+    properties = '\nAdded: svn:eol-style\n   + LF\n'
+    self.requests = [
+        ('/api/123/456',
+          _api({'file_a': _file('A+', property_changes=properties)})),
+        ('/download/issue123_456_789.diff', diff),
+    ]
+    patches = self.rietveld.get_patch(123, 456)
+    self.assertEquals(1, len(patches.patches))
+    self._check_patch(
+        patches.patches[0],
+        'file_a',
+        diff,
+        is_git_diff=True,
+        is_new=True,
+        patchlevel=1,
+        svn_properties=[('svn:eol-style', 'LF')])
+
+  def test_add_empty(self):
+    # http://codereview.chromium.org/api/7530007/5001
+    # http://codereview.chromium.org/download/issue7530007_5001_4011.diff
+    diff = (
+        'Index: scripts/master/factory/skia/__init__.py\n'
+        '===================================================================\n')
+    self.requests = [
+        ('/api/123/456', _api({'__init__.py': _file('A    ', num_chunks=0)})),
+        ('/download/issue123_456_789.diff', diff),
+    ]
+    patches = self.rietveld.get_patch(123, 456)
+    self.assertEquals(1, len(patches.patches))
+    self._check_patch(
+        patches.patches[0],
+        '__init__.py',
+        diff,
+        is_new=True)
 
   def test_delete(self):
     self.requests = [
@@ -160,7 +223,23 @@ class RietveldTest(unittest.TestCase):
     self._check_patch(patches.patches[0], 'file_a', None, is_delete=True)
 
   def test_m_plus(self):
+    diff = NORMAL_DIFF
     properties = '\nAdded: svn:eol-style\n   + LF\n'
+    self.requests = [
+        ('/api/123/456',
+          _api({'file_a': _file('M+', property_changes=properties)})),
+        ('/download/issue123_456_789.diff', diff),
+    ]
+    patches = self.rietveld.get_patch(123, 456)
+    self.assertEquals(1, len(patches.patches))
+    self._check_patch(
+        patches.patches[0],
+        'file_a',
+        diff,
+        svn_properties=[('svn:eol-style', 'LF')])
+
+  def test_m_plus_unknown_prop(self):
+    properties = '\nAdded: svn:foobar\n   + stuff\n'
     self.requests = [
         ('/api/123/456',
           _api({'file_a': _file('M+', property_changes=properties)})),