From b3727a36e4874c7d8909fa300524e143d80193b0 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Mon, 4 Apr 2011 19:31:44 +0000 Subject: [PATCH] Move patch.py and rietveld.py from commit-queue. It will be used for: - git cl patch so binary files can be patched - try jobs instead of doing a curl | patch BUG= TEST= Review URL: http://codereview.chromium.org/6792028 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@80355 0039d316-1c4b-4281-b951-d872f2087c98 --- .gitignore | 5 +- patch.py | 184 ++++++++++++++++++++++++++++++++++++ pylintrc | 3 +- rietveld.py | 204 ++++++++++++++++++++++++++++++++++++++++ tests/local_rietveld.py | 2 +- tests/patch_test.py | 163 ++++++++++++++++++++++++++++++++ tests/rietveld_test.py | 89 ++++++++++++++++++ 7 files changed, 644 insertions(+), 6 deletions(-) create mode 100644 patch.py create mode 100644 rietveld.py create mode 100755 tests/patch_test.py create mode 100755 tests/rietveld_test.py diff --git a/.gitignore b/.gitignore index 277fda9db..7eee64554 100644 --- a/.gitignore +++ b/.gitignore @@ -1,8 +1,5 @@ *.pyc -/git_cl_repo -/tests/pymox -/tests/rietveld -/tests/_trial +/tests/_rietveld /python /python.bat /python_bin diff --git a/patch.py b/patch.py new file mode 100644 index 000000000..b22e32e55 --- /dev/null +++ b/patch.py @@ -0,0 +1,184 @@ +# coding=utf8 +# 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. +"""Utility functions to handle patches.""" + +import re + + +class UnsupportedPatchFormat(Exception): + def __init__(self, filename, status): + super(UnsupportedPatchFormat, self).__init__(filename, status) + self.filename = filename + self.status = status + + def __str__(self): + out = 'Can\'t process patch for file %s.' % self.filename + if self.status: + out += '\n%s' % self.status + return out + + +class FilePatchBase(object): + """Defines a single file being modified.""" + is_delete = False + is_binary = False + + def get(self): + raise NotImplementedError('Nothing to grab') + + +class FilePatchDelete(FilePatchBase): + """Deletes a file.""" + is_delete = True + + def __init__(self, filename, is_binary): + super(FilePatchDelete, self).__init__() + self.filename = filename + self.is_binary = is_binary + + def get(self): + raise NotImplementedError('Nothing to grab') + + +class FilePatchBinary(FilePatchBase): + """Content of a new binary file.""" + is_binary = True + + def __init__(self, filename, data, svn_properties): + super(FilePatchBinary, self).__init__() + self.filename = filename + self.data = data + self.svn_properties = svn_properties or [] + + def get(self): + return self.data + + +class FilePatchDiff(FilePatchBase): + """Patch for a single file.""" + + def __init__(self, filename, diff, svn_properties): + super(FilePatchDiff, self).__init__() + self.filename = filename + self.diff = diff + self.svn_properties = svn_properties or [] + self.is_git_diff = self._is_git_diff(diff) + if self.is_git_diff: + self.patchlevel = 1 + self._verify_git_patch(filename, diff) + assert not svn_properties + else: + self.patchlevel = 0 + self._verify_svn_patch(filename, diff) + + def get(self): + return self.diff + + @staticmethod + def _is_git_diff(diff): + """Returns True if the diff for a single files was generated with gt. + + Expects the following format: + + Index: + diff --git a/ b/ + + + --- + +++ + + + Index: is a rietveld specific line. + """ + # Delete: http://codereview.chromium.org/download/issue6368055_22_29.diff + # Rename partial change: + # http://codereview.chromium.org/download/issue6250123_3013_6010.diff + # Rename no change: + # http://codereview.chromium.org/download/issue6287022_3001_4010.diff + return any(l.startswith('diff --git') for l in diff.splitlines()[:3]) + + @staticmethod + def _verify_git_patch(filename, diff): + lines = diff.splitlines() + # First fine the git diff header: + while lines: + line = lines.pop(0) + match = re.match(r'^diff \-\-git a\/(.*?) b\/(.*)$', line) + if match: + a = match.group(1) + b = match.group(2) + if a != filename and a != 'dev/null': + raise UnsupportedPatchFormat( + filename, 'Unexpected git diff input name.') + if b != filename and b != 'dev/null': + raise UnsupportedPatchFormat( + filename, 'Unexpected git diff output name.') + if a == 'dev/null' and b == 'dev/null': + raise UnsupportedPatchFormat( + filename, 'Unexpected /dev/null git diff.') + break + else: + raise UnsupportedPatchFormat( + filename, 'Unexpected git diff; couldn\'t find git header.') + + while lines: + line = lines.pop(0) + match = re.match(r'^--- a/(.*)$', line) + if match: + if a != match.group(1): + raise UnsupportedPatchFormat( + filename, 'Unexpected git diff: %s != %s.' % (a, match.group(1))) + match = re.match(r'^\+\+\+ b/(.*)$', lines.pop(0)) + if not match: + raise UnsupportedPatchFormat( + filename, 'Unexpected git diff: --- not following +++.') + if b != match.group(1): + raise UnsupportedPatchFormat( + filename, 'Unexpected git diff: %s != %s.' % (b, match.group(1))) + break + # Don't fail if the patch header is not found, the diff could be a + # file-mode-change-only diff. + + @staticmethod + def _verify_svn_patch(filename, diff): + lines = diff.splitlines() + while lines: + line = lines.pop(0) + match = re.match(r'^--- ([^\t]+).*$', line) + if match: + if match.group(1) not in (filename, '/dev/null'): + raise UnsupportedPatchFormat( + filename, + 'Unexpected diff: %s != %s.' % (filename, match.group(1))) + # Grab next line. + line2 = lines.pop(0) + match = re.match(r'^\+\+\+ ([^\t]+).*$', line2) + if not match: + raise UnsupportedPatchFormat( + filename, + 'Unexpected diff: --- not following +++.:\n%s\n%s' % ( + line, line2)) + if match.group(1) not in (filename, '/dev/null'): + raise UnsupportedPatchFormat( + filename, + 'Unexpected diff: %s != %s.' % (filename, match.group(1))) + break + # Don't fail if the patch header is not found, the diff could be a + # svn-property-change-only diff. + + +class PatchSet(object): + """A list of FilePatch* objects.""" + + def __init__(self, patches): + self.patches = patches + + def __iter__(self): + for patch in self.patches: + yield patch + + @property + def filenames(self): + return [p.filename for p in self.patches] diff --git a/pylintrc b/pylintrc index e52ac279d..b760eb488 100644 --- a/pylintrc +++ b/pylintrc @@ -50,6 +50,7 @@ load-plugins= # R0913: Too many arguments (N/5) # R0914: Too many local variables (N/15) # R0915: Too many statements (N/50) +# R0921: Abstract class not referenced # R0922: Abstract class is only referenced 1 times # W0122: Use of the exec statement # W0141: Used builtin function '' @@ -61,7 +62,7 @@ load-plugins= # W0613: Unused argument '' # W0703: Catch "Exception" # W6501: Specify string format arguments as logging function parameters -disable=C0103,C0111,C0302,I0011,R0401,R0801,R0901,R0902,R0903,R0904,R0911,R0912,R0913,R0914,R0915,R0922,W0122,W0141,W0142,W0402,W0404,W0511,W0603,W0613,W0703,W6501 +disable=C0103,C0111,C0302,I0011,R0401,R0801,R0901,R0902,R0903,R0904,R0911,R0912,R0913,R0914,R0915,R0921,R0922,W0122,W0141,W0142,W0402,W0404,W0511,W0603,W0613,W0703,W6501 [REPORTS] diff --git a/rietveld.py b/rietveld.py new file mode 100644 index 000000000..6984d3934 --- /dev/null +++ b/rietveld.py @@ -0,0 +1,204 @@ +# 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 sys +import time +import urllib2 + +try: + import simplejson as json # pylint: disable=F0401 +except ImportError: + try: + import json # pylint: disable=F0401 + except ImportError: + # Import the one included in depot_tools. + sys.path.append(os.path.join(os.path.dirname(__file__), 'third_party')) + import simplejson as json # pylint: disable=F0401 + +from third_party import upload +import patch + +# Hack out upload logging.info() +upload.logging = logging.getLogger('upload') +upload.logging.setLevel(logging.WARNING) + + +class Rietveld(object): + """Accesses rietveld.""" + def __init__(self, url, email, password): + self.issue = None + self.user = email + self.url = url + self._get_creds = lambda: (email, password) + self._xsrf_token = None + self._xsrf_token_time = None + self.rpc_server = upload.HttpRpcServer( + self.url, + self._get_creds, + save_cookies=False) + + def xsrf_token(self): + if (not self._xsrf_token_time or + (time.time() - self._xsrf_token_time) > 30*60): + self._xsrf_token_time = time.time() + self._xsrf_token = self.get( + '/xsrf_token', + extra_headers={'X-Requesting-XSRF-Token': '1'}) + return self._xsrf_token + + def get_pending_issues(self): + """Returns an array of dict of all the pending issues on the server.""" + return json.loads(self.get( + '/search?format=json&commit=True&closed=False&keys_only=True') + )['results'] + + def close_issue(self, issue): + """Closes the Rietveld issue for this changelist.""" + logging.info('closing issue %s' % issue) + self.post("/%d/close" % issue, [('xsrf_token', self.xsrf_token())]) + + def get_description(self, issue): + """Returns the issue's description.""" + return self.get('/%d/description' % issue) + + def get_issue_properties(self, issue, messages): + """Returns all the issue's metadata as a dictionary.""" + url = '/api/%s' % issue + if messages: + url += '?messages=true' + return json.loads(self.get(url)) + + def get_patchset_properties(self, issue, patchset): + """Returns the patchset properties.""" + url = '/api/%s/%s' % (issue, patchset) + return json.loads(self.get(url)) + + def get_file_content(self, issue, patchset, item): + """Returns the content of a new file. + + Throws HTTP 302 exception if the file doesn't exist or is not a binary file. + """ + # content = 0 is the old file, 1 is the new file. + content = 1 + url = '/%s/image/%s/%s/%s' % (issue, patchset, item, content) + return self.get(url) + + def get_file_diff(self, issue, patchset, item): + """Returns the diff of the file. + + Returns a useless diff for binary files. + """ + url = '/download/issue%s_%s_%s.diff' % (issue, patchset, item) + return self.get(url) + + def get_patch(self, issue, patchset): + """Returns a PatchSet object containing the details to apply this patch.""" + props = self.get_patchset_properties(issue, patchset) or {} + out = [] + for filename, state in props.get('files', {}).iteritems(): + status = state.get('status') + if status is None: + 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 = [] + if state['is_binary']: + out.append(patch.FilePatchBinary( + filename, + self.get_file_content(issue, patchset, state['id']), + props)) + else: + if state['num_chunks']: + diff = self.get_file_diff(issue, patchset, state['id']) + else: + diff = None + out.append(patch.FilePatchDiff(filename, diff, props)) + 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/ + raise patch.UnsupportedPatchFormat(filename, status) + return patch.PatchSet(out) + + 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, [ + ('xsrf_token', self.xsrf_token()), + ('message', message), + ('message_only', 'True'), + ('send_mail', 'True'), + ('no_redirect', 'True')]) + + def set_flag(self, issue, patchset, flag, value): + return self.post('/%s/edit_flags' % issue, [ + ('last_patchset', str(patchset)), + ('xsrf_token', self.xsrf_token()), + (flag, value)]) + + def get(self, request_path, **kwargs): + return self._send(request_path, payload=None, **kwargs) + + def post(self, request_path, data, **kwargs): + ctype, body = upload.EncodeMultipartFormData(data, []) + return self._send(request_path, payload=body, content_type=ctype, **kwargs) + + def _send(self, request_path, **kwargs): + """Sends a POST/GET to Rietveld. Returns the response body.""" + maxtries = 5 + for retry in xrange(maxtries): + try: + result = self.rpc_server.Send(request_path, **kwargs) + # Sometimes GAE returns a HTTP 200 but with HTTP 500 as the content. How + # nice. + return result + except urllib2.HTTPError, e: + if retry >= (maxtries - 1): + raise + if e.code not in (500, 502, 503): + raise + except urllib2.URLError, e: + 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) diff --git a/tests/local_rietveld.py b/tests/local_rietveld.py index b2efb3ff6..a40271eda 100755 --- a/tests/local_rietveld.py +++ b/tests/local_rietveld.py @@ -55,7 +55,7 @@ class LocalRietveld(object): self.sdk_path = os.path.abspath( os.path.join(self.base_dir, '..', 'google_appengine')) self.dev_app = os.path.join(self.sdk_path, 'dev_appserver.py') - self.rietveld = os.path.join(self.base_dir, 'tests', 'rietveld') + self.rietveld = os.path.join(self.base_dir, 'tests', '_rietveld') self.test_server = None self.port = None diff --git a/tests/patch_test.py b/tests/patch_test.py new file mode 100755 index 000000000..c030b883b --- /dev/null +++ b/tests/patch_test.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python +# 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. + +"""Unit tests for patch.py.""" + +import os +import sys +import unittest + +ROOT_DIR = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, os.path.join(ROOT_DIR, '..')) + +import patch + + +class PatchTest(unittest.TestCase): + def testFilePatchDelete(self): + c = patch.FilePatchDelete('foo', False) + self.assertEquals(c.is_delete, True) + self.assertEquals(c.is_binary, False) + self.assertEquals(c.filename, 'foo') + try: + c.get() + self.fail() + except NotImplementedError: + pass + c = patch.FilePatchDelete('foo', True) + self.assertEquals(c.is_delete, True) + self.assertEquals(c.is_binary, True) + self.assertEquals(c.filename, 'foo') + try: + c.get() + self.fail() + except NotImplementedError: + pass + + def testFilePatchBinary(self): + c = patch.FilePatchBinary('foo', 'data', []) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_binary, True) + self.assertEquals(c.filename, 'foo') + self.assertEquals(c.get(), 'data') + + def testFilePatchDiff(self): + c = patch.FilePatchDiff('foo', 'data', []) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_binary, False) + self.assertEquals(c.filename, 'foo') + self.assertEquals(c.is_git_diff, False) + self.assertEquals(c.patchlevel, 0) + self.assertEquals(c.get(), 'data') + diff = ( + 'diff --git a/git_cl/git-cl b/git_cl/git-cl\n' + 'old mode 100644\n' + 'new mode 100755\n') + c = patch.FilePatchDiff('git_cl/git-cl', diff, []) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_binary, False) + self.assertEquals(c.filename, 'git_cl/git-cl') + self.assertEquals(c.is_git_diff, True) + self.assertEquals(c.patchlevel, 1) + self.assertEquals(c.get(), diff) + diff = ( + 'Index: Junk\n' + 'diff --git a/git_cl/git-cl b/git_cl/git-cl\n' + 'old mode 100644\n' + 'new mode 100755\n') + c = patch.FilePatchDiff('git_cl/git-cl', diff, []) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_binary, False) + self.assertEquals(c.filename, 'git_cl/git-cl') + self.assertEquals(c.is_git_diff, True) + self.assertEquals(c.patchlevel, 1) + self.assertEquals(c.get(), diff) + + def testInvalidFilePatchDiffGit(self): + try: + patch.FilePatchDiff('svn_utils_test.txt', ( + 'diff --git a/tests/svn_utils_test_data/svn_utils_test.txt ' + 'b/tests/svn_utils_test_data/svn_utils_test.txt\n' + 'index 0e4de76..8320059 100644\n' + '--- a/svn_utils_test.txt\n' + '+++ b/svn_utils_test.txt\n' + '@@ -3,6 +3,7 @@ bb\n' + 'ccc\n' + 'dd\n' + 'e\n' + '+FOO!\n' + 'ff\n' + 'ggg\n' + 'hh\n'), + []) + self.fail() + except patch.UnsupportedPatchFormat: + pass + try: + patch.FilePatchDiff('svn_utils_test2.txt', ( + 'diff --git a/svn_utils_test_data/svn_utils_test.txt ' + 'b/svn_utils_test.txt\n' + 'index 0e4de76..8320059 100644\n' + '--- a/svn_utils_test.txt\n' + '+++ b/svn_utils_test.txt\n' + '@@ -3,6 +3,7 @@ bb\n' + 'ccc\n' + 'dd\n' + 'e\n' + '+FOO!\n' + 'ff\n' + 'ggg\n' + 'hh\n'), + []) + self.fail() + except patch.UnsupportedPatchFormat: + pass + + def testInvalidFilePatchDiffSvn(self): + try: + patch.FilePatchDiff('svn_utils_test.txt', ( + '--- svn_utils_test.txt2\n' + '+++ svn_utils_test.txt\n' + '@@ -3,6 +3,7 @@ bb\n' + 'ccc\n' + 'dd\n' + 'e\n' + '+FOO!\n' + 'ff\n' + 'ggg\n' + 'hh\n'), + []) + self.fail() + except patch.UnsupportedPatchFormat: + pass + + def testValidSvn(self): + # pylint: disable=R0201 + # Method could be a function + # Should not throw. + patch.FilePatchDiff('chrome/file.cc', ( + 'Index: chrome/file.cc\n' + '===================================================================\n' + '--- chrome/file.cc\t(revision 74690)\n' + '+++ chrome/file.cc\t(working copy)\n' + '@@ -80,10 +80,13 @@\n' + ' // Foo\n' + ' // Bar\n' + ' void foo() {\n' + '- return bar;\n' + '+ return foo;\n' + ' }\n' + ' \n' + ' \n'), []) + patch.FilePatchDiff('chrome/file.cc', ( + '--- /dev/null\t2\n' + '+++ chrome/file.cc\tfoo\n'), []) + patch.FilePatchDiff('chrome/file.cc', ( + '--- chrome/file.cc\tbar\n' + '+++ /dev/null\tfoo\n'), []) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py new file mode 100755 index 000000000..6a32f64e4 --- /dev/null +++ b/tests/rietveld_test.py @@ -0,0 +1,89 @@ +#!/usr/bin/env python +# 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. + +"""Unit tests for rietveld.py.""" + +import logging +import os +import sys +import unittest + +ROOT_DIR = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, os.path.join(ROOT_DIR, '..')) + +import patch +import rietveld + +# Access to a protected member XX of a client class +# pylint: disable=W0212 + + +class RietveldTest(unittest.TestCase): + def setUp(self): + super(RietveldTest, self).setUp() + self._rietveld_send = rietveld.Rietveld._send + rietveld.Rietveld._send = None + + def tearDown(self): + super(RietveldTest, self).setUp() + rietveld.Rietveld._send = self._rietveld_send + + def test_get_patch_empty(self): + rietveld.Rietveld._send = lambda x, y, payload: '{}' + r = rietveld.Rietveld('url', 'email', 'password') + patches = r.get_patch(123, 456) + self.assertTrue(isinstance(patches, patch.PatchSet)) + self.assertEquals([], patches.patches) + + def test_get_patch_no_status(self): + rietveld.Rietveld._send = lambda x, y, payload: ( + '{' + ' "files":' + ' {' + ' "file_a":' + ' {' + ' }' + ' }' + '}') + r = rietveld.Rietveld('url', 'email', 'password') + try: + r.get_patch(123, 456) + self.fail() + except patch.UnsupportedPatchFormat, e: + self.assertEquals('file_a', e.filename) + + def test_get_patch_two_files(self): + output = ( + '{' + ' "files":' + ' {' + ' "file_a":' + ' {' + ' "status": "A",' + ' "is_binary": false,' + ' "num_chunks": 1,' + ' "id": 789' + ' }' + ' }' + '}') + rietveld.Rietveld._send = lambda x, y, payload: output + r = rietveld.Rietveld('url', 'email', 'password') + patches = r.get_patch(123, 456) + self.assertTrue(isinstance(patches, patch.PatchSet)) + self.assertEquals(1, len(patches.patches)) + obj = patches.patches[0] + self.assertEquals(patch.FilePatchDiff, obj.__class__) + self.assertEquals('file_a', obj.filename) + self.assertEquals([], obj.svn_properties) + self.assertEquals(False, obj.is_git_diff) + self.assertEquals(0, obj.patchlevel) + # This is because Rietveld._send() always returns the same buffer. + self.assertEquals(output, obj.get()) + + + +if __name__ == '__main__': + logging.basicConfig(level=logging.ERROR) + unittest.main()