Bug 38094 - svn-apply: Add support for binary patches to new patch-parsing code
Summary: svn-apply: Add support for binary patches to new patch-parsing code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on: 38093
Blocks: 34033
  Show dependency treegraph
 
Reported: 2010-04-25 06:46 PDT by Chris Jerdonek
Modified: 2010-04-28 23:20 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (4.82 KB, patch)
2010-04-25 07:02 PDT, Chris Jerdonek
cjerdonek: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 2 (4.91 KB, patch)
2010-04-28 19:51 PDT, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-04-25 06:46:45 PDT
parseDiffHeader() needs to handle binary patches.
Comment 1 Chris Jerdonek 2010-04-25 07:02:32 PDT
Created attachment 54240 [details]
Proposed patch

This might not apply correctly until after the patch for bug 38093 lands.
Comment 2 Eric Seidel (no email) 2010-04-26 14:04:47 PDT
I'm confused by what this does.
Comment 3 Chris Jerdonek 2010-04-26 14:39:27 PDT
(In reply to comment #2)
> I'm confused by what this does.

This is needed before landing the patch here:

https://bugs.webkit.org/show_bug.cgi?id=34033

The parsePatch() code that bug 34033 would activate currently detects the ending of a diff header by looking for a regular expression like the following:

+++ $indexPath

The problem is that not all diff headers end like this.  Git and SVN diffs for binary files don't have a +++ line to end the header (and possibly not at all).  Instead they end in "GIT binary patch" or a similar line for SVN.  This patch simply adds those possibilities so that the new parsePatch() code will detect the header ending in all cases.

Without this change, svn-apply will throw an error that the "header ending could not be found" or something to that effect when running the Python svn-apply unit tests for binary patches.  (I have since added binary diff test cases to the Perl unit tests as well.)
Comment 4 Chris Jerdonek 2010-04-28 19:51:48 PDT
Created attachment 54664 [details]
Proposed patch 2

Re-attaching after landing the patch for bug 38093 and rebasing.
Comment 5 Eric Seidel (no email) 2010-04-28 21:03:17 PDT
Comment on attachment 54664 [details]
Proposed patch 2

I'm confused by what we do with binary patches.  How does this change behavior?
Comment 6 Chris Jerdonek 2010-04-28 22:40:19 PDT
(In reply to comment #5)
> (From update of attachment 54664 [details])
> I'm confused by what we do with binary patches.  How does this change behavior?

This patch won't change behavior in two senses.  First, it won't actually go live when this patch lands.  Only test-webkitperl executes its code paths right now.  Second, when it does go live (via bug 34033), there will be no change in behavior.  Bug 34033 essentially swaps out a chunk of code in svn-apply and svn-unapply with a bunch of unit-tested code with the same functionality.

As for how we currently handle binary patches, there are separate code paths in svn-apply and svn-unapply that look for special strings in the patch header signifying a binary patch.  If those special strings are there, then it handles the contents differently.
Comment 7 Eric Seidel (no email) 2010-04-28 22:42:35 PDT
Comment on attachment 54664 [details]
Proposed patch 2

OK.  Thanks for the additional explanation.
Comment 8 Chris Jerdonek 2010-04-28 23:05:34 PDT
(In reply to comment #7)
> (From update of attachment 54664 [details])
> OK.  Thanks for the additional explanation.

Thanks for the review, Eric!

Some of that explanation will also be available to others in the ChangeLog being cq+'ed.  I added extra information to the ChangeLog (namely that the code will go live in a subsequent revision) after reading your comment 2.
Comment 9 Chris Jerdonek 2010-04-28 23:19:53 PDT
Comment on attachment 54664 [details]
Proposed patch 2

Clearing flags on attachment: 54664

Committed r58478: <http://trac.webkit.org/changeset/58478>
Comment 10 Chris Jerdonek 2010-04-28 23:20:04 PDT
All reviewed patches have been landed.  Closing bug.