File index appended to previous file in svn-create-patch with SVN 1.9

[Sam Weinig]

The EWS bot does not seem to be able to apply the patch in 167806. The patch posted using webkit-patch post from an up-to-date tree.

[Radar WebKit Bug Importer]

<rdar://problem/30387395>

[Jonathan Bedard]

*** This bug has been marked as a duplicate of bug 167906 ***

[Alexey Proskuryakov]

Let's track the issue in this older bug.

[Alexey Proskuryakov]

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

[Jonathan Bedard]

Actually, this is an issue I've seen locally, it's just now that I've noticed what the root cause is.

SVN 1.9 doesn't always emit newlines at the end of patches, and we don't explicitly add them.  Sometimes the index line of the next file gets appended to the last line of the previous file.

[Jonathan Bedard]

Created attachment 300822 [details]
Patch

[Jonathan Bedard]

If you search for 'Index: Source/WebCore/bindings/js/JSDOMConvertBase.h' in https://bug-167806-attachments.webkit.org/attachment.cgi?id=300583, you will see an example of a file index being appended to the previous change.

[Jonathan Bedard]

Created attachment 300829 [details]
Patch

[Daniel Bates]

Comment on attachment 300829 [details]
Patch

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

> Tools/Scripts/svn-create-patch:245
> +    $patch eq "" or (substr $patch, -1) eq "\n" or die;

We need to make this work for arbitrary end-of-line characters. I would also write this in the form:

die if ($patch && $patch =~ /\r?$/);

(The regular expression used may not be correct. Please check.)

[Daniel Bates]

Comment on attachment 300829 [details]
Patch

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

> Tools/Scripts/VCSUtils.pm:1782
> +    return join($lineEnding, @lines) . $lineEnding;

Is it OK to use $lineEnding here?fixChangeLogPatch() has similar logic and emits a new-line character because "patch(1) expects an extra trailing newline".

[Daniel Bates]

(In reply to comment #9)
> Comment on attachment 300829 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300829&action=review
> 
> > Tools/Scripts/svn-create-patch:245
> > +    $patch eq "" or (substr $patch, -1) eq "\n" or die;
> 
> We need to make this work for arbitrary end-of-line characters. I would also
> write this in the form:
> 
> die if ($patch && $patch =~ /\r?$/);
> 

Assuming the comment in fixChangeLogPatch() is true [1], we should write this as:

die if ($patch && substr($patch, -1) ne "\n");

[1] <https://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm?rev=211828#L1935>

[Daniel Bates]

Comment on attachment 300829 [details]
Patch

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

> Tools/Scripts/webkitperl/VCSUtils_unittest/fixSVNPatchForAdditionWithHistory.pl:47
> +

Actually we shouldn't add this line here. Instead we should remove the chomp() from <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/fixSVNPatchForAdditionWithHistory.pl?rev=211828#L285>.

[Daniel Bates]

Created attachment 300832 [details]
Patch and unit tests

[Daniel Bates]

Created attachment 300833 [details]
Patch and unit tests

[WebKit Commit Bot]

Comment on attachment 300833 [details]
Patch and unit tests

Clearing flags on attachment: 300833

Committed r211833: <http://trac.webkit.org/changeset/211833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.