From 6cdac9ef456e7ab662adf47a43b37cd86c1cd99a Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Wed, 7 Sep 2011 14:25:40 +0000 Subject: [PATCH] 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 | 23 +++++++---- tests/rietveld_test.py | 91 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 100 insertions(+), 14 deletions(-) diff --git a/rietveld.py b/rietveld.py index df529ad6e..2320f3d2b 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(): + 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 diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py index 47af5a0c5..5412cf26c 100755 --- a/tests/rietveld_test.py +++ b/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)})),