Bug 35804 – svn-apply: should provide an error message if --binary should be used

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

See the failures from https://bugs.webkit.org/show_bug.cgi?id=25648.  The failure message is:

Failed to run "['/mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply', '--force']" exit_code: 2
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/repaint/slider-thumb-change-height.html
patching file LayoutTests/fast/repaint/slider-update-value.html
patching file LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum
patch: **** Only garbage was found in the patch input.
patching file LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.txt
patching file LayoutTests/platform/mac/fast/repaint/slider-update-value-expected.checksum
patch: **** Only garbage was found in the patch input.
patching file LayoutTests/platform/mac/fast/repaint/slider-update-value-expected.txt
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/rendering/RenderObject.h
patching file WebCore/rendering/RenderSlider.cpp

This is because 'patch' complains that the diffs for the .checksum files are nothing but garbage.  The diffs look like this:

diff --git a/LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum b/LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum
new file mode 100644
index 0000000..82cbe9b
--- /dev/null
+++ b/LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum
@@ -0,0 +1 @@
+6e3355f2ea671c3956c85dcb3b0c97a0
\ No newline at end of file

A patch generated from SVN applies cleanly.  The .checksum portion of this patch is:

Index: LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum
===================================================================
--- LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum  (revision 0)
+++ LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum  (revision 0)
@@ -0,0 +1 @@
+6e3355f2ea671c3956c85dcb3b0c97a0
\ No newline at end of file

[Eric Seidel (no email)]

EWS (and the commit-queue, etc) all use svn-apply under the covers.  So the real bug is there.

Do we have reproduction steps?

Something like:

cat "1232434" > foo
git diff --binary > test.patch
svn-apply test.patch

Reproduction steps will help us understand what's actually going on.

This would be where the bug would be in svn-apply, if we determine this is a real bug:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/VCSUtils.pm#L364

[James Robinson]

Ah, it's actually dying on this part:

diff --git a/LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.png b/LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.png
new file mode 100644
index 0000000..661d863
Binary files /dev/null and b/LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.png differ


That makes more sense.  I guess the script should just skip binary diffs completely?

[Eric Seidel (no email)]

We just need a better error message.  Your diff is missing the git diff --binary flag.  So there is no binary data!  Thus svn-apply can't actually apply the patch.

If you regenerate the diff with git diff --binary or using webkit-patch upload it will "do the right thing" and svn-apply will work just fine.

svn-apply can handle "git diff --binary" patches, but if you have binary changes and don't pass --binary,  svn-apply can't handle the patch (because there is not enough data in the patch!).

[Chris Jerdonek]

Created attachment 54982 [details]
Proposed patch

[Eric Seidel (no email)]

Comment on attachment 54982 [details]
Proposed patch

YES!  This will be hugely helpful for the cq.  This is a common mistake, and the cq should be able to tell people automatically instead of me having to manually.

[Chris Jerdonek]

Comment on attachment 54982 [details]
Proposed patch

Clearing flags on attachment: 54982

Committed r58732: <http://trac.webkit.org/changeset/58732>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.