From 031524151a40ac64101fb716fc29a0cd18d86c8a Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Thu, 1 Sep 2011 14:42:49 +0000 Subject: [PATCH] Implement reverse mangling of svn properties. That means some properties will being to be applied. R=dpranke@chromium.org BUG= TEST= Review URL: http://codereview.chromium.org/7776015 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@99182 0039d316-1c4b-4281-b951-d872f2087c98 --- rietveld.py | 75 +++++++++++++++++++++++++++++++++--------- tests/rietveld_test.py | 37 +++++++++++++++++++++ 2 files changed, 96 insertions(+), 16 deletions(-) diff --git a/rietveld.py b/rietveld.py index c2045ac33..df529ad6e 100644 --- a/rietveld.py +++ b/rietveld.py @@ -15,6 +15,7 @@ The following hypothesis are made: import logging import os +import re import sys import time import urllib2 @@ -131,15 +132,13 @@ class Rietveld(object): # 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. @@ -149,24 +148,68 @@ class Rietveld(object): 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 is the exact format from 'svn diff'. + """ + 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: + spacer = rietveld_svn_props.pop(0) + if spacer or not rietveld_svn_props: + # svn diff always put a spacer between the unified diff and property + # diff + raise patch.UnsupportedPatchFormat( + filename, 'Failed to parse svn properties.') + + # Something like 'Added: svn:eol-style'. Note the action is localized. + # *sigh*. + action = rietveld_svn_props.pop(0) + match = re.match(r'^(\w+): (.+)$', action) + 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 the content. + rietveld_svn_props.pop(0) + continue + + if match.group(1) not in ('Added', 'Modified'): + # Will fail for our French friends. + raise patch.UnsupportedPatchFormat( + filename, 'Unsupported svn property operation.') + + if match.group(2) in ('svn:eol-style', 'svn:executable'): + # ' + foo' where foo is the new value. That's fragile. + content = rietveld_svn_props.pop(0) + match2 = re.match(r'^ \+ (.*)$', content) + if not match2: + raise patch.UnsupportedPatchFormat( + filename, 'Unsupported svn property format.') + svn_props.append((match.group(2), match2.group(1))) + 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) diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py index 6c81454ac..ab84a375b 100755 --- a/tests/rietveld_test.py +++ b/tests/rietveld_test.py @@ -73,6 +73,43 @@ class RietveldTest(unittest.TestCase): # This is because Rietveld._send() always returns the same buffer. self.assertEquals(output, obj.get()) + def testSvnProperties(self): + # Line too long (N/80) + # pylint: disable=C0301 + + # To test one of these, run something like + # import json, pprint, urllib + # url = 'http://codereview.chromium.org/api/202046/1' + # pprint.pprint(json.load(urllib.urlopen(url))['files']) + + # svn:mergeinfo across branches: + # http://codereview.chromium.org/202046/diff/1/third_party/libxml/xmlcatalog_dummy.cc + self.assertEquals( + [('svn:eol-style', 'LF')], + rietveld.Rietveld.parse_svn_properties( + u'\nAdded: svn:eol-style\n + LF\n', 'foo')) + + # svn:eol-style property that is lost in the diff + # http://codereview.chromium.org/202046/diff/1/third_party/libxml/xmllint_dummy.cc + self.assertEquals( + [], + rietveld.Rietveld.parse_svn_properties( + u'\nAdded: svn:mergeinfo\n' + ' Merged /branches/chrome_webkit_merge_branch/third_party/' + 'libxml/xmldummy_mac.cc:r69-2775\n', + 'foo')) + + self.assertEquals( + [], + rietveld.Rietveld.parse_svn_properties(u'', 'foo')) + + try: + rietveld.Rietveld.parse_svn_properties(u'\n', 'foo') + self.fail() + except rietveld.patch.UnsupportedPatchFormat, e: + self.assertEquals('foo', e.filename) + # TODO(maruel): Change with no diff, only svn property change: + # http://codereview.chromium.org/6462019/ if __name__ == '__main__':