Bug 38864 – svn-apply: should support git binary delta diffs

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

[Chris Jerdonek]

It already supports literal.  Just not delta.

[Eric Seidel (no email)]

The current behavior is confusing:

patching file LayoutTests/fast/multicol/client-rects-expected.checksum
only literal type is supported now at /usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/svn-apply line 248.

Failed to run "[u'/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler']" exit_code: 255

We should at least fix svn-apply to point people to this bug when failing.

jamesr also wondered if we could teach webkit-patch to call git diff with the right args so that it never generated delta diffs.

[Chris Jerdonek]

(In reply to comment #1)
> 
> jamesr also wondered if we could teach webkit-patch to call git diff with the right args so that it never generated delta diffs.

See here.  The answer seems to be that it's not possible to force literal diffs:

https://bugs.webkit.org/show_bug.cgi?id=36747#c15

[Chris Jerdonek]

(In reply to comment #1)
> The current behavior is confusing:
> 
> patching file LayoutTests/fast/multicol/client-rects-expected.checksum
> only literal type is supported now at /usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/svn-apply line 248.
> 
> Failed to run "[u'/usr/local/home/jamesr/WebKit_svn/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler']" exit_code: 255
> 
> We should at least fix svn-apply to point people to this bug when failing.

I don't know how hard it would be to add support for delta diffs.  Maybe implementing svn-apply by calling "git-apply" would be a cheap way of adding support for delta diffs in the case of applying to a Git repo.

[Shane Stephens]

Created attachment 77963 [details]
Patch

[Shane Stephens]

The provided patch adds delta support to svn-apply.  The algorithm comes directly from https://github.com/git/git/blob/master/patch-delta.c - should I make a note of this in the code?

[James Robinson]

I haven't looked at the patch but the code you linked is GPLv2 which is not permitted in WebKit. We can't include it.

[Shane Stephens]

The code I linked is also C.  It's the algorithm I used, not the code itself.  However if this is still a problem then perhaps I can describe the data format in more abstract terms and someone else can perform a clean implementation.

[Eric Seidel (no email)]

I'm less concerned about the possible GPL code, than I am about the lack of unit testing. :)

IANAL, but I don't think this is a licensing issue -- you writing a version of the algorithm in a new language under a new license.  The code is copyright, not the algorithm, right?

[Shane Stephens]

Created attachment 78106 [details]
Patch

[Shane Stephens]

Created attachment 78107 [details]
Patch

[Shane Stephens]

Created attachment 78108 [details]
Patch

[Shane Stephens]

sorry about the attachment spam.  webkit-patch seems to require that I provide git commit ids now and it took me a bit of fiddling to get it right :(

There's now a unit test as well, and some extra functionality that was missing.

[Eric Seidel (no email)]

Your indent should be 4 spaces.  I guess we don't have a style checker for perl files?

[Eric Seidel (no email)]

Comment on attachment 78108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78108&action=review

In general this patch is *fantastic*, I can't state that enough.  This has been a huge thorn in our side for a long time.

But I'm concerned about the ability for someone else to hack on this code in 3 months time.  We need to be sure to document where they would check the algorithm, and possibly try to make the existing perl code cleaner so that if there is ever a bug in this code, it can be fixed by someone other than you. :)

> Tools/Scripts/VCSUtils.pm:1774
> +sub decodeGitBinaryPatchDeltaSize($)
> +{

Would be good to link to where this algorithm is documented.  Even if that means that link is a ViewCVS URL for GPL'd code.

> Tools/Scripts/VCSUtils.pm:1801
> +    # we don't use these but we need to know how big they are

Comments should be sentences (capital, period).

> Tools/Scripts/VCSUtils.pm:1816
> +            if ($cmd & 0x01) { $cpOff = ord(substr($binaryChunk, $i++, 1)); }
> +            if ($cmd & 0x02) { $cpOff |= ord(substr($binaryChunk, $i++, 1)) << 8; }
> +            if ($cmd & 0x04) { $cpOff |= ord(substr($binaryChunk, $i++, 1)) << 16; }
> +            if ($cmd & 0x08) { $cpOff |= ord(substr($binaryChunk, $i++, 1)) << 24; }

Should we be using named constants for these opcodes?  Should this if or loop body be its own function?

[Eric Seidel (no email)]

Comment on attachment 78108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78108&action=review

> Tools/Scripts/VCSUtils.pm:1813
> +            if ($cmd & 0x01) { $cpOff = ord(substr($binaryChunk, $i++, 1)); }

Maybe "ord(substr($binaryChunk, $i++, 1))" should be a function readByte()?  I'm not really sure what it's doing.

[Shane Stephens]

Created attachment 78214 [details]
Patch

[Shane Stephens]

Done and done.  I played around with extracting a function from the loop but couldn't really get it to look nice (e.g. I would have had to return the output string each time), so I added some more comments and renamed some variables to be clearer instead.

I'm not actually a perl hacker though, so someone else might be able to suggest a decent way to pass a string by reference and append to it in-place.

The opcodes are kind of meaningless apart from the concordances displayed in directly in the code (e.g. they would be names like READ_BITS_7_TO_14_OF_SIZE_VALUE), so I didn't think that there was any additional clarity to be gained from adding them.

[Eric Seidel (no email)]

Comment on attachment 78214 [details]
Patch

You sir, are my hero.  Thank you.

[WebKit Commit Bot]

Comment on attachment 78214 [details]
Patch

Clearing flags on attachment: 78214

Committed r75235: <http://trac.webkit.org/changeset/75235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.