Bug 38094 – svn-apply: Add support for binary patches to new patch-parsing code

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

parseDiffHeader() needs to handle binary patches.

[Chris Jerdonek]

Created attachment 54240 [details]
Proposed patch

This might not apply correctly until after the patch for bug 38093 lands.

[Eric Seidel (no email)]

I'm confused by what this does.

[Chris Jerdonek]

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

[Chris Jerdonek]

Created attachment 54664 [details]
Proposed patch 2

Re-attaching after landing the patch for bug 38093 and rebasing.

[Eric Seidel (no email)]

Comment on attachment 54664 [details]
Proposed patch 2

I'm confused by what we do with binary patches.  How does this change behavior?

[Chris Jerdonek]

(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.

[Eric Seidel (no email)]

Comment on attachment 54664 [details]
Proposed patch 2

OK.  Thanks for the additional explanation.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

Comment on attachment 54664 [details]
Proposed patch 2

Clearing flags on attachment: 54664

Committed r58478: <http://trac.webkit.org/changeset/58478>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.