Bug 26830 – svn-apply can not handle git binary diffs

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

svn-apply can not handle git binary diffs

https://bugs.webkit.org/attachment.cgi?id=32000
is one such example:

diff --git a/WebCore/English.lproj/localizedStrings.js b/WebCore/English.lproj/localizedStrings.js
index b85f91a91e5f88be8e7190e862bc615efe7487c5..f01d8a5e0266b361d11b4991f3aadcc8faf49a35 100644
GIT binary patch
delta 22
ecmaDfkMYtx#tk1lCRcfiOy==S+g#=Of)xOJZVC1P

delta 30
ocmV+(0O9}Ao&n;X0kH5$lVV96lbS{_linF3liWxYvtmi`1>Uv|q5uE@

[Eric Seidel (no email)]

There is a (sadly GPL) python module which seems to know how to deal with git binary patches here:
http://modular.math.washington.edu/home/nathan/sage-dev/local/lib/python/mercurial/patch.py

[Shinichiro Hamaji]

I'm interested in fixing git issues in svn-apply. I think we may be able to handle the git-style binary patches (base85 & zlib) and actually I could write base85 decoder in perl. However, I think the better way is just using git-apply(1) instead of patch(1) for git binary patches, of course, we still need to call svn add and rm after patching though. In this way we don't need to maintain the code to handle git patches. What do you think? If this plan sounds, I'm happy to work on this.

[Eric Seidel (no email)]

Another way would be to teach svn-create-patch to work with git, so that tools like bugzilla-tool patches could just upload svn patches always.  I'm not sure that's better, but it's another option.

I think either option (using git-apply, or teaching svn-apply how to decode git binary patches) woudl be fine.

[Mark Rowe (bdash)]

Using ‘git apply’ isn’t ideal since most people don’t have git on their system.  I’m not convinced that fixing svn-create-patch would help here either given that most Git users are unlikely to use that when creating patches.

[Shinichiro Hamaji]

(In reply to comment #4)
> Using ‘git apply’ isn’t ideal since most people don’t have git on their system.

Hmm... I thought the most typical user of svn-apply is commit-queue now. So, I guessed using git-apply is OK because we just need to make sure git is installed in the machine for commit-queue. Is my guess wrong? Is svn-apply often used even after commit-queue was developed?

[Eric Seidel (no email)]

Lots of folks who are not the commit-queue use svn-apply. :)  Many WebKit developers use svn and not git.  I don't know the exact breakdown of usage.

[Shinichiro Hamaji]

(In reply to comment #6)
> Lots of folks who are not the commit-queue use svn-apply. :)

Ah, I didn't imagine that folks are using svn-apply for their usual works...

OK, I'll learn how git is handling delta. I think it's not so difficult anyway. I hope git won't add other formats for binary diff in future.

[Shinichiro Hamaji]

Created attachment 42929 [details]
Patch v1

[Shinichiro Hamaji]

Let me add some comments on my patch.

This adds support of git style patch for binary addition, deletion, move, and some cases of modification. As for modification, git produces two types of patches for binary modification depending the size of difference. One is labeled "literal" (used when the difference is big) and another is "delta" (used when the difference is small). "literal" patch has both original files and new files as is, and "delta" patch has a binary diff and another binary diff for reverse patching. This patch only supports "literal" type for now. I think it's still useful because we may be able to handle most of expected.png modifications only with this change.

I removed two lines from gitdiff2svndiff function not to delete "new file mode..." line. I think it's OK to remove this code, but I'm not 100% sure. I hope someone confirm it.

I think it's nice if bugzilla can show binary difference in git patches like it is working for svn patches. What is the best way for this? I guess modifying WebKit/BugsSite would be the best, but I'm not sure if we can use modules in WebKitTools/Scripts from BugsSite.

Any comments will be appreciated. Thanks!

[Eric Seidel (no email)]

PrettyPatch (which shows the patch images) is written in ruby, this is written in perl, which would make it difficutl to share code.

This is going to need some sort of testing.  I should have added perl unit testing to run-webkit-unittests when I last changed this code.  You can also test it using scm_unittest.py if you prefer.

I think this is fantastic!  Even if we don't support "delta" yet, "literal" support is a big win.

[Shinichiro Hamaji]

Created attachment 42990 [details]
Patch v2

[Shinichiro Hamaji]

Thanks for the comment!

(In reply to comment #10)
> PrettyPatch (which shows the patch images) is written in ruby, this is written
> in perl, which would make it difficutl to share code.

Hmm, I see. By the way, what is the most recommended script language in WebKit? I was surprised that there are ruby code. I thought python is the current official language. Is it still allowed to write new code in ruby or perl?

> This is going to need some sort of testing.  I should have added perl unit
> testing to run-webkit-unittests when I last changed this code.  You can also
> test it using scm_unittest.py if you prefer.

OK, I wrote a unittest in scm_unittest.py. Which checks if we can apply git binary patches in both SVN repository and git repository.

[Eric Seidel (no email)]

I don't think WebKit has any "official" language for scripts.  I write in python due to personal preference.  I happen to have written many of the recent scripts.  Others have written in the language of their preference.  Personally I'd like to move more of our perl scripts over to python, but it's possible that others would say the opposite. :)

Thank you very much for taking this on!

[Shinichiro Hamaji]

Comment on attachment 42990 [details]
Patch v2

Clearing flags on attachment: 42990

Committed r50863: <http://trac.webkit.org/changeset/50863>

[Shinichiro Hamaji]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

(In reply to comment #13)
> I don't think WebKit has any "official" language for scripts.  I write in
> python due to personal preference.  I happen to have written many of the recent
> scripts.  Others have written in the language of their preference.  Personally
> I'd like to move more of our perl scripts over to python, but it's possible
> that others would say the opposite. :)

Interesting situation. Luckily, I know python, perl, and ruby and like to try several programming languages, but some people would feel uncomfortable... Anyway, thanks for the description!

[Shinichiro Hamaji]

*** Bug 28456 has been marked as a duplicate of this bug. ***