Bug 38094

Created attachment 54240 [details] Proposed patch This might not apply correctly until after the patch for bug 38093 lands.

I'm confused by what this does.

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

Created attachment 54664 [details] Proposed patch 2 Re-attaching after landing the patch for bug 38093 and rebasing.

Comment on attachment 54664 [details] Proposed patch 2 I'm confused by what we do with binary patches. How does this change behavior?

(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 on attachment 54664 [details] Proposed patch 2 OK. Thanks for the additional explanation.

(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 on attachment 54664 [details] Proposed patch 2 Clearing flags on attachment: 54664 Committed r58478: <http://trac.webkit.org/changeset/58478>

All reviewed patches have been landed. Closing bug.