Make rietveld status handling saner

rietveld.py

Index: rietveld.py
diff --git a/rietveld.py b/rietveld.py
index df529ad6efb02bef5d3cbf9ee017d8edfee103ed..2320f3d2b974c31840c3ac3899443dfeea7d957b 100644
--- a/rietveld.py
+++ b/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():

[Dirk Pranke, 2011/09/06 20:21:48]

Nit: wonder if this might be clearer with 'if status != 'D': ? The strip() and
the [0] seem a bit unnecessary.

[M-A Ruel, 2011/09/06 20:23:59]

No because status is a svn status line, which contains 6 characters, by default
they are spaces, e.g. 'A     '.

[Dirk Pranke, 2011/09/06 22:35:36]

Ah, right. LGTM, then.

+          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