Bug 28880 - svn-apply --force doesn't actually work
Summary: svn-apply --force doesn't actually work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 11:31 PDT by Brady Eidson
Modified: 2009-09-03 12:07 PDT (History)
2 users (show)

See Also:


Attachments
Patch v1 (1.17 KB, patch)
2009-09-01 11:40 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch 2 v1 (1.21 KB, patch)
2009-09-01 11:50 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch 2 v2 (1.94 KB, patch)
2009-09-02 08:51 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch 2 v3 (8.61 KB, patch)
2009-09-02 16:41 PDT, David Kilzer (:ddkilzer)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2009-09-01 11:31:52 PDT
Which the changes landed in https://bugs.webkit.org/show_bug.cgi?id=26299, svn-apply didn't do --force by default, but rather added it as an option.

Unfortunately, that option was never actually hooked up.

It needs to be.
Comment 1 David Kilzer (:ddkilzer) 2009-09-01 11:40:28 PDT
Created attachment 38872 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2009-09-01 11:43:35 PDT
Committed r47945: <http://trac.webkit.org/changeset/47945>
Comment 3 David Kilzer (:ddkilzer) 2009-09-01 11:48:20 PDT
We also need to ignore $exitCode if $force is true.
Comment 4 David Kilzer (:ddkilzer) 2009-09-01 11:48:43 PDT
Comment on attachment 38872 [details]
Patch v1

Clearing beidson's r+ for a second patch.
Comment 5 David Kilzer (:ddkilzer) 2009-09-01 11:50:53 PDT
Created attachment 38873 [details]
Patch 2 v1
Comment 6 Eric Seidel (no email) 2009-09-02 00:25:07 PDT
Comment on attachment 38873 [details]
Patch 2 v1

Would be nice to have unit tests of this. :)  Would be possible with scm.py, but also not required.  cq- for ChangeLog diff and because you're a committer.
Comment 7 David Kilzer (:ddkilzer) 2009-09-02 08:13:23 PDT
Comment on attachment 38873 [details]
Patch 2 v1

(In reply to comment #6)
> (From update of attachment 38873 [details])
> Would be nice to have unit tests of this. :)  Would be possible with scm.py,
> but also not required.  cq- for ChangeLog diff and because you're a committer.

I'm clearing the review flag to (1) make --force work as expected from the command-line so that it applies all of the patches and (2) makes svn-apply exit with a non-zero status if any forced patches didn't apply cleanly.

That should satisfy both the command-line usage and bugzilla-tool's commit-queu mode.
Comment 8 David Kilzer (:ddkilzer) 2009-09-02 08:51:03 PDT
Created attachment 38926 [details]
Patch 2 v2
Comment 9 David Kilzer (:ddkilzer) 2009-09-02 08:54:44 PDT
Comment on attachment 38926 [details]
Patch 2 v2

Oops, I wanted to write a scm.py unit test as well.
Comment 10 David Kilzer (:ddkilzer) 2009-09-02 16:41:03 PDT
Created attachment 38947 [details]
Patch 2 v3
Comment 11 David Kilzer (:ddkilzer) 2009-09-02 18:42:20 PDT
(In reply to comment #10)
> Created an attachment (id=38947) [details]
> Patch 2 v3

The only "issue" with this patch is that the layout tests print to stderr when they run.  I couldn't figure out a nice way to turn that off.
Comment 12 Eric Seidel (no email) 2009-09-02 23:10:03 PDT
Comment on attachment 38947 [details]
Patch 2 v3

LGTM.  I don't understand the command about stderr, that seems to be another pathc.
Comment 13 David Kilzer (:ddkilzer) 2009-09-03 03:16:59 PDT
(In reply to comment #12)
> (From update of attachment 38947 [details])
> LGTM.  I don't understand the comment about stderr, that seems to be another
> patch.

No, when scm.apply_patch() runs, it produces log() output to stderr during the unit tests that's slightly annoying.
Comment 14 David Kilzer (:ddkilzer) 2009-09-03 12:07:53 PDT
Committed r48027: <http://trac.webkit.org/changeset/48027>