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 <url> | 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

 *.pyc
-/git_cl_repo
-/tests/pymox
-/tests/rietveld
-/tests/_trial
+/tests/_rietveld
 /python
 /python.bat
 /python_bin

patch.py

+# 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: <filename>
+    diff --git a/<filename> b/<filename>
+    <filemode changes>
+    <index>
+    --- <filename>
+    +++ <filename>
+    <hunks>
+
+    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]

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]

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 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)

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
 

tests/patch_test.py

+#!/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()

tests/rietveld_test.py

+#!/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()