Implement reverse mangling of svn properties.

rietveld.py

 # 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.
 """Defines class Rietveld to easily access a rietveld instance.
 
 Security implications:
 
 The following hypothesis are made:
 - Rietveld enforces:
   - Nobody else than issue owner can upload a patch set
   - Verifies the issue owner credentials when creating new issues
   - A issue owner can't change once the issue is created
   - A patch set cannot be modified
 """
 
 import logging
 import os
+import re
 import sys
 import time
 import urllib2
 
 try:
   import simplejson as json  # pylint: disable=F0401
 except ImportError:
   try:
     import json  # pylint: disable=F0401
   except ImportError:
@@ skipping 96 matching lines @@
         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':
         # Ignore the diff.
         out.append(patch.FilePatchDelete(filename, state['is_binary']))
       elif status in ('A', 'M'):
-        # TODO(maruel): Rietveld support is still weird, add this line once it's
-        # safe to use.
-        # props = state.get('property_changes', '').splitlines() or []
-        props = []
+        svn_props = self.parse_svn_properties(
+            state.get('property_changes', ''), filename)
         if state['is_binary']:
           out.append(patch.FilePatchBinary(
               filename,
               self.get_file_content(issue, patchset, state['id']),
-              props,
+              svn_props,
               is_new=(status[0] == 'A')))
         else:
           # Ignores num_chunks since it may only contain an header.
           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.')
-          out.append(patch.FilePatchDiff(filename, diff, props))
+          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:
-        # Line too long (N/80)
-        # pylint: disable=C0301
-        # TODO: Add support for MM, A+, etc. Rietveld removes the svn properties
-        # from the diff.
-        # Example of mergeinfo across branches:
-        # http://codereview.chromium.org/202046/diff/1/third_party/libxml/xmlcatalog_dummy.cc
-        # svn:eol-style property that is lost in the diff
-        # http://codereview.chromium.org/202046/diff/1/third_party/libxml/xmllint_dummy.cc
-        # Change with no diff, only svn property change:
-        # http://codereview.chromium.org/6462019/
+        # TODO(maruel): Add support for MM, A+, etc. Rietveld removes the svn
+        # properties from the diff.
         raise patch.UnsupportedPatchFormat(filename, status)
     return patch.PatchSet(out)
 
+  @staticmethod
+  def parse_svn_properties(rietveld_svn_props, filename):
+    """Returns a list of tuple [('property', 'newvalue')]."""
+    rietveld_svn_props = rietveld_svn_props.splitlines()
+    svn_props = []
+    if not rietveld_svn_props:
+      return svn_props
+    # 1. Ignore svn:mergeinfo.
+    # 2. Accept svn:eol-style and svn:executable.
+    # 3. Refuse any other.
+    # \n
+    # Added: svn:ignore\n
+    #    + LF\n
+    while rietveld_svn_props:
+      if rietveld_svn_props.pop(0) or not rietveld_svn_props:

[Dirk Pranke, 2011/08/30 20:44:58]

What is the format of the the rietveld_svn_props list? is the first line
guaranteed to be empty?

Also, I find the use of pop() inside method calls and if branches to be a bit
confusing; perhaps it would be clearer if you pulled the popped values into
local variables?

[M-A Ruel, 2011/08/30 21:01:33]

It's the exact format of a svn diff.
Done.

+        raise patch.UnsupportedPatchFormat(
+            filename, 'Failed to parse svn properties.')
+      match = re.match(r'^(\w+): (.+)$', rietveld_svn_props.pop(0))
+      if not match or not rietveld_svn_props:
+        raise patch.UnsupportedPatchFormat(
+            filename, 'Failed to parse svn properties.')
+      if match.group(2) == 'svn:mergeinfo':
+        # Silently ignore.
+        rietveld_svn_props.pop(0)
+        continue
+      if match.group(1) not in ('Added', 'Modified'):
+        raise patch.UnsupportedPatchFormat(
+            filename, 'Unsupported svn property operation.')
+      if match.group(2) in ('svn:eol-style', 'svn:executable'):
+        svn_props.append((match.group(2), rietveld_svn_props.pop(0)[5:]))
+    return svn_props
+
   def update_description(self, issue, description):
     """Sets the description for an issue on Rietveld."""
     logging.info('new description for issue %s' % issue)
     self.post('/%s/description' % issue, [
         ('description', description),
         ('xsrf_token', self.xsrf_token())])
 
   def add_comment(self, issue, message):
     logging.info('issue %s; comment: %s' % (issue, message))
     return self.post('/%s/publish' % issue, [
@@ skipping 36 matching lines @@
         if retry >= (maxtries - 1):
           raise
         if not 'Name or service not known' in e.reason:
           # Usually internal GAE flakiness.
           raise
       # If reaching this line, loop again. Uses a small backoff.
       time.sleep(1+maxtries*2)
 
   # DEPRECATED.
   Send = get