Skip to content

C
Chromium_Depot_tools
Commit cf602553 authored by maruel@chromium.org's avatar maruel@chromium.org
Browse files
Options

Fix svn delete handling (again). 

Fix RAW.NEW_NOT_NULL parsing to detect correctly it's a new file.

Add proper Hunk parsing to fix these 2 bugs.

R=dpranke@chromium.org
BUG=109715
TEST=CQ'ing a patch generated with svn that delete files is applied properly, e.g. the file is deleted and not simple 0-length.


Review URL: http://codereview.chromium.org/9167015

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@117084 0039d316-1c4b-4281-b951-d872f2087c98
parent 0e023174
Hide whitespace changes
Inline Side-by-side
Showing with 251 additions and 36 deletions
patch.py
View file @ cf602553
# coding=utf8
# Copyright (c) 2011 The Chromium Authors. All rights reserved.
# Copyright (c) 2012 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."""
......@@ -86,7 +86,10 @@ class FilePatchBase(object):
out += 'R'
else:
out += ' '
return out + ' %s->%s' % (self.source_filename, self.filename)
out += ' '
if self.source_filename:
out += '%s->' % self.source_filename
return out + str(self.filename)
class FilePatchDelete(FilePatchBase):
......@@ -112,6 +115,18 @@ class FilePatchBinary(FilePatchBase):
return self.data
class Hunk(object):
"""Parsed hunk data container."""
def __init__(self, start_src, lines_src, start_dst, lines_dst):
self.start_src = start_src
self.lines_src = lines_src
self.start_dst = start_dst
self.lines_dst = lines_dst
self.variation = self.lines_dst - self.lines_src
self.text = []
class FilePatchDiff(FilePatchBase):
"""Patch for a single file."""
......@@ -127,6 +142,7 @@ class FilePatchDiff(FilePatchBase):
self._verify_git_header()
else:
self._verify_svn_header()
self.hunks = self._split_hunks()
if self.source_filename and not self.is_new:
self._fail('If source_filename is set, is_new must be also be set')
......@@ -198,6 +214,65 @@ class FilePatchDiff(FilePatchBase):
# http://codereview.chromium.org/download/issue6287022_3001_4010.diff
return any(l.startswith('diff --git') for l in diff_header.splitlines())
def _split_hunks(self):
"""Splits the hunks and does verification."""
hunks = []
for line in self.diff_hunks.splitlines(True):
if line.startswith('@@'):
match = re.match(r'^@@ -(\d+),(\d+) \+([\d,]+) @@.*$', line)
# File add will result in "-0,0 +1" but file deletion will result in
# "-1,N +0,0" where N is the number of lines deleted. That's from diff
# and svn diff. git diff doesn't exhibit this behavior.
if not match:
self._fail('Hunk header is unparsable')
if ',' in match.group(3):
start_dst, lines_dst = map(int, match.group(3).split(',', 1))
else:
start_dst = int(match.group(3))
lines_dst = 0
new_hunk = Hunk(int(match.group(1)), int(match.group(2)),
start_dst, lines_dst)
if hunks:
if new_hunk.start_src <= hunks[-1].start_src:
self._fail('Hunks source lines are not ordered')
if new_hunk.start_dst <= hunks[-1].start_dst:
self._fail('Hunks destination lines are not ordered')
hunks.append(new_hunk)
continue
hunks[-1].text.append(line)
if len(hunks) == 1:
if hunks[0].start_src == 0 and hunks[0].lines_src == 0:
self.is_new = True
if hunks[0].start_dst == 0 and hunks[0].lines_dst == 0:
self.is_delete = True
if self.is_new and self.is_delete:
self._fail('Hunk header is all 0')
if not self.is_new and not self.is_delete:
for hunk in hunks:
variation = (
len([1 for i in hunk.text if i.startswith('+')]) -
len([1 for i in hunk.text if i.startswith('-')]))
if variation != hunk.variation:
self._fail(
'Hunk header is incorrect: %d vs %d' % (
variation, hunk.variation))
if not hunk.start_src:
self._fail(
'Hunk header start line is incorrect: %d' % hunk.start_src)
if not hunk.start_dst:
self._fail(
'Hunk header start line is incorrect: %d' % hunk.start_dst)
hunk.start_src -= 1
hunk.start_dst -= 1
if self.is_new and hunks:
hunks[0].start_dst -= 1
if self.is_delete and hunks:
hunks[0].start_src -= 1
return hunks
def mangle(self, string):
"""Mangle a file path."""
return '/'.join(string.replace('\\', '/').split('/')[self.patchlevel:])
......
testing_support/patches_data.py
View file @ cf602553
# Copyright (c) 2011 The Chromium Authors. All rights reserved.
# Copyright (c) 2012 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.
......@@ -54,12 +54,63 @@ class RAW(object):
'--- chrome/file.cc\tbar\n'
'+++ /dev/null\tfoo\n')
DELETE2 = (
'Index: browser/extensions/extension_sidebar_api.cc\n'
'===================================================================\n'
'--- browser/extensions/extension_sidebar_api.cc\t(revision 116830)\n'
'+++ browser/extensions/extension_sidebar_api.cc\t(working copy)\n'
'@@ -1,19 +0,0 @@\n'
'-// Copyright (c) 2011 The Chromium Authors. All rights reserved.\n'
'-// Use of this source code is governed by a BSD-style license that\n'
'-// found in the LICENSE file.\n'
'-\n'
'-#include "base/command_line.h"\n'
'-#include "chrome/browser/extensions/extension_apitest.h"\n'
'-#include "chrome/common/chrome_switches.h"\n'
'-\n'
'-class SidebarApiTest : public ExtensionApiTest {\n'
'- public:\n'
'- void SetUpCommandLine(CommandLine* command_line) {\n'
'- ExtensionApiTest::SetUpCommandLine(command_line);\n'
'- command_line->AppendSwitch(switches::Bleh);\n'
'- }\n'
'-};\n'
'-\n'
'-IN_PROC_BROWSER_TEST_F(SidebarApiTest, Sidebar) {\n'
'- ASSERT_TRUE(RunExtensionTest("sidebar")) << message_;\n'
'-}\n')
# http://codereview.chromium.org/api/7530007/5001
# http://codereview.chromium.org/download/issue7530007_5001_4011.diff
CRAP_ONLY = (
'Index: scripts/master/factory/skia/__init__.py\n'
'===================================================================\n')
TWO_HUNKS = (
'Index: chrome/app/generated_resources.grd\n'
'===================================================================\n'
'--- chrome/app/generated_resources.grd\t(revision 116830)\n'
'+++ chrome/app/generated_resources.grd\t(working copy)\n'
'@@ -4169,9 +4169,6 @@\n'
' <message name="IDS_EXTENSION_LOAD_OPTIONS_PAGE_FAILED" desc="">\n'
' Could not load options page \'<ph name="OPTIONS_PAGE">$1<ex....\n'
' </message>\n'
'- <message name="IDS_EXTENSION_LOAD_SIDEBAR_PAGE_FAILED" desc="">\n'
'- Could not load sidebar page \'<ph name="SIDEBAR_PAGE">$1<e...\n'
'- </message>\n'
' <if expr="is_win">\n'
' <message name="IDS_EXTENSION_UNPACK_FAILED" desc="On wind...\n'
' Can not unpack extension. To safely unpack an extensio...\n'
'@@ -5593,9 +5590,6 @@\n'
' <message name="IDS_ACCNAME_WEB_CONTENTS" desc="The acces...\n'
' Web Contents\n'
' </message>\n'
'- <message name="IDS_ACCNAME_SIDE_BAR" desc="The acces...\n'
'- Sidebar\n'
'- </message>\n'
' \n'
' <!-- Browser Hung Plugin Detector -->\n'
' <message name="IDS_UNKNOWN_PLUGIN_NAME" ...\n')
class GIT(object):
"""Sample patches generated by git diff."""
......@@ -206,3 +257,51 @@ class GIT(object):
'diff --git a/git_cl/git-cl b/git_cl/git-cl\n'
'old mode 100644\n'
'new mode 100755\n')
FOUR_HUNKS = (
'Index: presubmit_support.py\n'
'diff --git a/presubmit_support.py b/presubmit_support.py\n'
'index 52416d3f..d56512f2 100755\n'
'--- a/presubmit_support.py\n'
'+++ b/presubmit_support.py\n'
'@@ -558,6 +558,7 @@ class SvnAffectedFile(AffectedFile):\n'
' AffectedFile.__init__(self, *args, **kwargs)\n'
' self._server_path = None\n'
' self._is_text_file = None\n'
'+ self._diff = None\n'
' \n'
' def ServerPath(self):\n'
' if self._server_path is None:\n'
'@@ -598,8 +599,10 @@ class SvnAffectedFile(AffectedFile):\n'
' return self._is_text_file\n'
' \n'
' def GenerateScmDiff(self):\n'
'- return scm.SVN.GenerateDiff(\n'
'- [self.LocalPath()], self._local_root, False, None)\n'
'+ if self._diff is None:\n'
'+ self._diff = scm.SVN.GenerateDiff(\n'
'+ [self.LocalPath()], self._local_root, False, None)\n'
'+ return self._diff\n'
' \n'
' \n'
' class GitAffectedFile(AffectedFile):\n'
'@@ -611,6 +614,7 @@ class GitAffectedFile(AffectedFile):\n'
' AffectedFile.__init__(self, *args, **kwargs)\n'
' self._server_path = None\n'
' self._is_text_file = None\n'
'+ self._diff = None\n'
' \n'
' def ServerPath(self):\n'
' if self._server_path is None:\n'
'@@ -645,7 +649,10 @@ class GitAffectedFile(AffectedFile):\n'
' return self._is_text_file\n'
' \n'
' def GenerateScmDiff(self):\n'
'- return scm.GIT.GenerateDiff(self._local_root, files=[self.Lo...\n'
'+ if self._diff is None:\n'
'+ self._diff = scm.GIT.GenerateDiff(\n'
'+ self._local_root, files=[self.LocalPath(),])\n'
'+ return self._diff\n'
' \n'
' \n'
' class Change(object):\n')
tests/patch_test.py
View file @ cf602553
#!/usr/bin/env python
# Copyright (c) 2011 The Chromium Authors. All rights reserved.
# Copyright (c) 2012 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.
......@@ -29,7 +29,8 @@ class PatchTest(unittest.TestCase):
is_git_diff=False,
is_new=False,
patchlevel=0,
svn_properties=None):
svn_properties=None,
nb_hunks=None):
svn_properties = svn_properties or []
self.assertEquals(p.filename, filename)
self.assertEquals(p.source_filename, source_filename)
......@@ -45,8 +46,10 @@ class PatchTest(unittest.TestCase):
self.assertEquals(p.get(), diff)
else:
self.assertEquals(p.get(True), diff)
if hasattr(p, 'svn_properties'):
self.assertEquals(p.svn_properties, svn_properties)
if hasattr(p, 'hunks'):
self.assertEquals(len(p.hunks), nb_hunks)
else:
self.assertEquals(None, nb_hunks)
def testFilePatchDelete(self):
p = patch.FilePatchDelete('foo', False)
......@@ -66,32 +69,33 @@ class PatchTest(unittest.TestCase):
def testFilePatchDiff(self):
p = patch.FilePatchDiff('chrome/file.cc', RAW.PATCH, [])
self._check_patch(p, 'chrome/file.cc', RAW.PATCH)
self._check_patch(p, 'chrome/file.cc', RAW.PATCH, nb_hunks=1)
def testFilePatchDiffHeaderMode(self):
p = patch.FilePatchDiff('git_cl/git-cl', GIT.MODE_EXE, [])
self._check_patch(
p, 'git_cl/git-cl', GIT.MODE_EXE, is_git_diff=True, patchlevel=1,
svn_properties=[('svn:executable', '*')])
svn_properties=[('svn:executable', '*')], nb_hunks=0)
def testFilePatchDiffHeaderModeIndex(self):
p = patch.FilePatchDiff('git_cl/git-cl', GIT.MODE_EXE_JUNK, [])
self._check_patch(
p, 'git_cl/git-cl', GIT.MODE_EXE_JUNK, is_git_diff=True, patchlevel=1,
svn_properties=[('svn:executable', '*')])
svn_properties=[('svn:executable', '*')], nb_hunks=0)
def testFilePatchDiffSvnNew(self):
# The code path is different for git and svn.
p = patch.FilePatchDiff('foo', RAW.NEW, [])
self._check_patch(p, 'foo', RAW.NEW, is_new=True)
self._check_patch(p, 'foo', RAW.NEW, is_new=True, nb_hunks=1)
def testFilePatchDiffGitNew(self):
# The code path is different for git and svn.
p = patch.FilePatchDiff('foo', GIT.NEW, [])
self._check_patch(
p, 'foo', GIT.NEW, is_new=True, is_git_diff=True, patchlevel=1)
p, 'foo', GIT.NEW, is_new=True, is_git_diff=True, patchlevel=1,
nb_hunks=1)
def testValidSvn(self):
def testSvn(self):
# Should not throw.
p = patch.FilePatchDiff('chrome/file.cc', RAW.PATCH, [])
lines = RAW.PATCH.splitlines(True)
......@@ -102,21 +106,21 @@ class PatchTest(unittest.TestCase):
self.assertEquals(RAW.PATCH, p.get(True))
self.assertEquals(RAW.PATCH, p.get(False))
def testValidSvnNew(self):
def testSvnNew(self):
p = patch.FilePatchDiff('chrome/file.cc', RAW.MINIMAL_NEW, [])
self.assertEquals(RAW.MINIMAL_NEW, p.diff_header)
self.assertEquals('', p.diff_hunks)
self.assertEquals(RAW.MINIMAL_NEW, p.get(True))
self.assertEquals(RAW.MINIMAL_NEW, p.get(False))
def testValidSvnDelete(self):
def testSvnDelete(self):
p = patch.FilePatchDiff('chrome/file.cc', RAW.MINIMAL_DELETE, [])
self.assertEquals(RAW.MINIMAL_DELETE, p.diff_header)
self.assertEquals('', p.diff_hunks)
self.assertEquals(RAW.MINIMAL_DELETE, p.get(True))
self.assertEquals(RAW.MINIMAL_DELETE, p.get(False))
def testValidSvnRename(self):
def testSvnRename(self):
p = patch.FilePatchDiff('file_b', RAW.MINIMAL_RENAME, [])
self.assertEquals(RAW.MINIMAL_RENAME, p.diff_header)
self.assertEquals('', p.diff_hunks)
......@@ -192,54 +196,85 @@ class PatchTest(unittest.TestCase):
self.assertEquals(RAW.PATCH, patches.patches[0].get(True))
self.assertEquals(RAW.PATCH, patches.patches[0].get(False))
def testTwoHunks(self):
name = 'chrome/app/generated_resources.grd'
p = patch.FilePatchDiff(name, RAW.TWO_HUNKS, [])
self._check_patch(p, name, RAW.TWO_HUNKS, nb_hunks=2)
def testGitThreeHunks(self):
p = patch.FilePatchDiff('presubmit_support.py', GIT.FOUR_HUNKS, [])
self._check_patch(
p, 'presubmit_support.py', GIT.FOUR_HUNKS, is_git_diff=True,
patchlevel=1,
nb_hunks=4)
def testDelete(self):
p = patch.FilePatchDiff('tools/clang_check/README.chromium', RAW.DELETE, [])
self._check_patch(
p, 'tools/clang_check/README.chromium', RAW.DELETE, is_delete=True)
p, 'tools/clang_check/README.chromium', RAW.DELETE, is_delete=True,
nb_hunks=1)
def testDelete2(self):
name = 'browser/extensions/extension_sidebar_api.cc'
p = patch.FilePatchDiff(name, RAW.DELETE2, [])
self._check_patch(p, name, RAW.DELETE2, is_delete=True, nb_hunks=1)
def testGitDelete(self):
p = patch.FilePatchDiff('tools/clang_check/README.chromium', GIT.DELETE, [])
self._check_patch(
p, 'tools/clang_check/README.chromium', GIT.DELETE, is_delete=True,
is_git_diff=True, patchlevel=1)
is_git_diff=True, patchlevel=1, nb_hunks=1)
def testGitRename(self):
p = patch.FilePatchDiff('tools/run_local_server.sh', GIT.RENAME, [])
self._check_patch(p, 'tools/run_local_server.sh', GIT.RENAME,
is_git_diff=True, patchlevel=1,
source_filename='tools/run_local_server.PY', is_new=True)
self._check_patch(
p,
'tools/run_local_server.sh',
GIT.RENAME,
is_git_diff=True,
patchlevel=1,
source_filename='tools/run_local_server.PY',
is_new=True,
nb_hunks=0)
def testGitRenamePartial(self):
p = patch.FilePatchDiff(
'chromeos/views/webui_menu_widget.h', GIT.RENAME_PARTIAL, [])
self._check_patch(
p, 'chromeos/views/webui_menu_widget.h', GIT.RENAME_PARTIAL,
source_filename='chromeos/views/DOMui_menu_widget.h', is_git_diff=True,
patchlevel=1, is_new=True)
p,
'chromeos/views/webui_menu_widget.h',
GIT.RENAME_PARTIAL,
source_filename='chromeos/views/DOMui_menu_widget.h',
is_git_diff=True,
patchlevel=1,
is_new=True,
nb_hunks=1)
def testGitCopy(self):
p = patch.FilePatchDiff('pp', GIT.COPY, [])
self._check_patch(p, 'pp', GIT.COPY, is_git_diff=True, patchlevel=1,
source_filename='PRESUBMIT.py', is_new=True)
self._check_patch(
p, 'pp', GIT.COPY, is_git_diff=True, patchlevel=1,
source_filename='PRESUBMIT.py', is_new=True, nb_hunks=0)
def testOnlyHeader(self):
p = patch.FilePatchDiff('file_a', RAW.MINIMAL, [])
self._check_patch(p, 'file_a', RAW.MINIMAL)
self._check_patch(p, 'file_a', RAW.MINIMAL, nb_hunks=0)
def testSmallest(self):
p = patch.FilePatchDiff('file_a', RAW.NEW_NOT_NULL, [])
self._check_patch(p, 'file_a', RAW.NEW_NOT_NULL)
self._check_patch(p, 'file_a', RAW.NEW_NOT_NULL, is_new=True, nb_hunks=1)
def testRenameOnlyHeader(self):
p = patch.FilePatchDiff('file_b', RAW.MINIMAL_RENAME, [])
self._check_patch(
p, 'file_b', RAW.MINIMAL_RENAME, source_filename='file_a', is_new=True)
p, 'file_b', RAW.MINIMAL_RENAME, source_filename='file_a', is_new=True,
nb_hunks=0)
def testGitCopyPartial(self):
p = patch.FilePatchDiff('wtf2', GIT.COPY_PARTIAL, [])
self._check_patch(
p, 'wtf2', GIT.COPY_PARTIAL, source_filename='wtf', is_git_diff=True,
patchlevel=1, is_new=True)
patchlevel=1, is_new=True, nb_hunks=1)
def testGitCopyPartialAsSvn(self):
p = patch.FilePatchDiff('wtf2', GIT.COPY_PARTIAL, [])
......@@ -263,14 +298,20 @@ class PatchTest(unittest.TestCase):
def testGitNewExe(self):
p = patch.FilePatchDiff('natsort_test.py', GIT.NEW_EXE, [])
self._check_patch(
p, 'natsort_test.py', GIT.NEW_EXE, is_new=True, is_git_diff=True,
patchlevel=1, svn_properties=[('svn:executable', '*')])
p,
'natsort_test.py',
GIT.NEW_EXE,
is_new=True,
is_git_diff=True,
patchlevel=1,
svn_properties=[('svn:executable', '*')],
nb_hunks=1)
def testGitNewMode(self):
p = patch.FilePatchDiff('natsort_test.py', GIT.NEW_MODE, [])
self._check_patch(
p, 'natsort_test.py', GIT.NEW_MODE, is_new=True, is_git_diff=True,
patchlevel=1)
patchlevel=1, nb_hunks=1)
def testPatchsetOrder(self):
# Deletes must be last.
......
tests/rietveld_test.py
View file @ cf602553
#!/usr/bin/env python
# Copyright (c) 2011 The Chromium Authors. All rights reserved.
# Copyright (c) 2012 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.
......@@ -104,8 +104,8 @@ class RietveldTest(unittest.TestCase):
]
patches = self.rietveld.get_patch(123, 456)
self.assertEquals(2, len(patches.patches))
# TODO(maruel): svn sucks.
self._check_patch(patches.patches[0], 'file_a', RAW.NEW_NOT_NULL)
self._check_patch(
patches.patches[0], 'file_a', RAW.NEW_NOT_NULL, is_new=True)
self._check_patch(patches.patches[1], 'foo', RAW.NEW, is_new=True)
def test_get_patch_add(self):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment