From 238d2e8b3deb90706d2f27fd1365a37183f6847c Mon Sep 17 00:00:00 2001 From: "eric@webkit.org" Date: Thu, 26 Jan 2012 00:54:44 +0000 Subject: [PATCH] webkit-patch apply-* should always continue after failures https://bugs.webkit.org/show_bug.cgi?id=77057 Reviewed by Adam Barth. As far as I can tell there is only one potential drawback to always forcing: that if you're somehow in the wrong directory it will create new directories for new files. Since webkit-patch always cd's to the root it seems that's not a drawback. The drawback of not using --force for svn-apply, is that it will stop after the first failure, which is rarely (if ever) the desired behavior. This just removes the force option (which was strangely hidden behind --non-interactive). This should make for a better user experiance. * Scripts/webkitpy/common/checkout/checkout.py: (Checkout.apply_patch): * Scripts/webkitpy/common/checkout/checkout_mock.py: (MockCheckout.apply_patch): * Scripts/webkitpy/common/checkout/checkout_unittest.py: (CheckoutTest.test_chromium_deps): (CheckoutTest): (CheckoutTest.test_apply_patch): * Scripts/webkitpy/tool/commands/download_unittest.py: (DownloadCommandsTest._default_options): * Scripts/webkitpy/tool/steps/applypatch.py: (ApplyPatch.options): (ApplyPatch.run): * Scripts/webkitpy/tool/steps/options.py: (Options): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@105945 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Tools/ChangeLog | 32 ++++++++++++++++++++++ Tools/Scripts/webkitpy/common/checkout/checkout.py | 7 ++--- .../webkitpy/common/checkout/checkout_mock.py | 2 +- .../webkitpy/common/checkout/checkout_unittest.py | 11 ++++++++ .../webkitpy/tool/commands/download_unittest.py | 1 - Tools/Scripts/webkitpy/tool/steps/applypatch.py | 3 +- Tools/Scripts/webkitpy/tool/steps/options.py | 1 - 7 files changed, 48 insertions(+), 9 deletions(-) diff --git a/Tools/ChangeLog b/Tools/ChangeLog index b137c91..f254531 100644 --- a/Tools/ChangeLog +++ b/Tools/ChangeLog @@ -1,3 +1,35 @@ +2012-01-25 Eric Seidel + + webkit-patch apply-* should always continue after failures + https://bugs.webkit.org/show_bug.cgi?id=77057 + + Reviewed by Adam Barth. + + As far as I can tell there is only one potential drawback to always + forcing: that if you're somehow in the wrong directory it will create new + directories for new files. Since webkit-patch always cd's to the root + it seems that's not a drawback. The drawback of not using --force for + svn-apply, is that it will stop after the first failure, which is rarely + (if ever) the desired behavior. This just removes the force option + (which was strangely hidden behind --non-interactive). This should + make for a better user experiance. + + * Scripts/webkitpy/common/checkout/checkout.py: + (Checkout.apply_patch): + * Scripts/webkitpy/common/checkout/checkout_mock.py: + (MockCheckout.apply_patch): + * Scripts/webkitpy/common/checkout/checkout_unittest.py: + (CheckoutTest.test_chromium_deps): + (CheckoutTest): + (CheckoutTest.test_apply_patch): + * Scripts/webkitpy/tool/commands/download_unittest.py: + (DownloadCommandsTest._default_options): + * Scripts/webkitpy/tool/steps/applypatch.py: + (ApplyPatch.options): + (ApplyPatch.run): + * Scripts/webkitpy/tool/steps/options.py: + (Options): + 2012-01-25 Dirk Pranke nrwt: should be able to run --platform test interactively diff --git a/Tools/Scripts/webkitpy/common/checkout/checkout.py b/Tools/Scripts/webkitpy/common/checkout/checkout.py index 93a205a..5b30627 100644 --- a/Tools/Scripts/webkitpy/common/checkout/checkout.py +++ b/Tools/Scripts/webkitpy/common/checkout/checkout.py @@ -150,15 +150,14 @@ class Checkout(object): def chromium_deps(self): return DEPS(self._scm.absolute_path(self._filesystem.join("Source", "WebKit", "chromium", "DEPS"))) - def apply_patch(self, patch, force=False): + def apply_patch(self, patch): # It's possible that the patch was not made from the root directory. # We should detect and handle that case. # FIXME: Move _scm.script_path here once we get rid of all the dependencies. - args = [self._scm.script_path('svn-apply')] + # --force (continue after errors) is the common case, so we always use it. + args = [self._scm.script_path('svn-apply'), "--force"] if patch.reviewer(): args += ['--reviewer', patch.reviewer().full_name] - if force: - args.append('--force') self._executive.run_command(args, input=patch.contents()) def apply_reverse_diff(self, revision): diff --git a/Tools/Scripts/webkitpy/common/checkout/checkout_mock.py b/Tools/Scripts/webkitpy/common/checkout/checkout_mock.py index 039db1f..25b05c3 100644 --- a/Tools/Scripts/webkitpy/common/checkout/checkout_mock.py +++ b/Tools/Scripts/webkitpy/common/checkout/checkout_mock.py @@ -84,7 +84,7 @@ class MockCheckout(object): def chromium_deps(self): return MockDEPS() - def apply_patch(self, patch, force=False): + def apply_patch(self, patch): pass def apply_reverse_diffs(self, revision): diff --git a/Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py b/Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py index 7a4b454..3eb327d 100644 --- a/Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py +++ b/Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py @@ -42,6 +42,7 @@ from webkitpy.common.system.executive import Executive, ScriptError from webkitpy.common.system.filesystem import FileSystem # FIXME: This should not be needed. from webkitpy.common.system.filesystem_mock import MockFileSystem from webkitpy.common.system.executive_mock import MockExecutive +from webkitpy.common.system.outputcapture import OutputCapture from webkitpy.thirdparty.mock import Mock @@ -252,3 +253,13 @@ class CheckoutTest(unittest.TestCase): checkout = self._make_checkout() checkout._scm.checkout_root = "/foo/bar" self.assertEqual(checkout.chromium_deps()._path, '/foo/bar/Source/WebKit/chromium/DEPS') + + def test_apply_patch(self): + checkout = self._make_checkout() + checkout._executive = MockExecutive(should_log=True) + checkout._scm.script_path = lambda script: script + mock_patch = Mock() + mock_patch.contents = lambda: "foo" + mock_patch.reviewer = lambda: None + expected_stderr = "MOCK run_command: ['svn-apply', '--force'], cwd=None\n" + OutputCapture().assert_outputs(self, checkout.apply_patch, [mock_patch], expected_stderr=expected_stderr) diff --git a/Tools/Scripts/webkitpy/tool/commands/download_unittest.py b/Tools/Scripts/webkitpy/tool/commands/download_unittest.py index 4239ecc..55a2286 100644 --- a/Tools/Scripts/webkitpy/tool/commands/download_unittest.py +++ b/Tools/Scripts/webkitpy/tool/commands/download_unittest.py @@ -82,7 +82,6 @@ class DownloadCommandsTest(CommandsTest): options.clean = True options.close_bug = True options.force_clean = False - options.force_patch = True options.non_interactive = False options.parent_command = 'MOCK parent command' options.quiet = False diff --git a/Tools/Scripts/webkitpy/tool/steps/applypatch.py b/Tools/Scripts/webkitpy/tool/steps/applypatch.py index 327ac09..5c36169 100644 --- a/Tools/Scripts/webkitpy/tool/steps/applypatch.py +++ b/Tools/Scripts/webkitpy/tool/steps/applypatch.py @@ -35,9 +35,8 @@ class ApplyPatch(AbstractStep): def options(cls): return AbstractStep.options() + [ Options.non_interactive, - Options.force_patch, ] def run(self, state): log("Processing patch %s from bug %s." % (state["patch"].id(), state["patch"].bug_id())) - self._tool.checkout().apply_patch(state["patch"], force=self._options.non_interactive or self._options.force_patch) + self._tool.checkout().apply_patch(state["patch"]) diff --git a/Tools/Scripts/webkitpy/tool/steps/options.py b/Tools/Scripts/webkitpy/tool/steps/options.py index fc781a6..1141df7 100644 --- a/Tools/Scripts/webkitpy/tool/steps/options.py +++ b/Tools/Scripts/webkitpy/tool/steps/options.py @@ -44,7 +44,6 @@ class Options(object): description = make_option("-m", "--description", action="store", type="string", dest="description", help="Description string for the attachment") email = make_option("--email", action="store", type="string", dest="email", help="Email address to use in ChangeLogs.") force_clean = make_option("--force-clean", action="store_true", dest="force_clean", default=False, help="Clean working directory before applying patches (removes local changes and commits)") - force_patch = make_option("--force-patch", action="store_true", dest="force_patch", default=False, help="Forcefully applies the patch, continuing past errors.") git_commit = make_option("-g", "--git-commit", action="store", dest="git_commit", help="Operate on a local commit. If a range, the commits are squashed into one. HEAD.. operates on working copy changes only.") local_commit = make_option("--local-commit", action="store_true", dest="local_commit", default=False, help="Make a local commit for each applied patch") non_interactive = make_option("--non-interactive", action="store_true", dest="non_interactive", default=False, help="Never prompt the user, fail as fast as possible.") -- 2.7.4