Bug 33475 – Create a unit-tested subroutine to parse a patch created by svn-create-patch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

This subroutine will be able to replace the main while loop at the top of svn-apply and svn-unapply.

[Chris Jerdonek]

Created attachment 46744 [details]
Proposed patch

Note that there is about 50 lines of near cut-and-paste between the two helper subroutines at the bottom of the two unit test files parseDiffHeader.pl and parseDiff.pl.  While it would be possible to share some of this code, I view it more as a (temporary) coincidence that they can be so similar.  If the signature, return value, or test logic for either parseDiff() or parseDiffHeader() were to change slightly, then for readability reasons it would probably not be worthwhile to share the two helper functions.  I am open to breaking some of that into a separate file though if you feel it's important.  Thanks in advance for reviewing!

[WebKit Review Bot]

Attachment 46744 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
test/parseDiff.pl:91:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:105:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:106:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:126:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:127:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:135:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:136:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:153:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:154:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:159:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:165:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:166:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:183:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:184:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:197:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:198:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:250:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:261:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:51:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:52:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:57:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:58:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:68:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:69:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:81:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl:82:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 31

[Chris Jerdonek]

(In reply to comment #2)
> Attachment 46744 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Last 3072 characters of output:
> test/parseDiff.pl:91:  Line contains tab character.  [whitespace/tab] [5]
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:105:  Line
> contains tab character.  [whitespace/tab] [5]
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:106:  Line
> ...

Whoa, there!

[Eric Seidel (no email)]

Does Perl require tabs?  Why would this file have them?

[Chris Jerdonek]

(In reply to comment #4)
> Does Perl require tabs?  Why would this file have them?

Tabs are present because the file has unit tests containing svn-create-patch output as input, and svn-create-patch creates output with tabs:

> print "--- ${file}\t(revision ${sourceRevision})\t(from ${sourceFile}:${sourceRevision})\n";
> print "+++ ${file}\t(working copy)\n";

(from svn-create-patch's manufacturePatchForAdditionWithHistory())

The tabs are in heredoc strings and so are covered by this report:

https://bugs.webkit.org/show_bug.cgi?id=33737

Strictly speaking, we could remove the tabs from the tests and not test the exact ouput of svn-create-patch (or at least limit the number of test cases with tabs to one).

And if David doesn't see any reason not to, I could go ahead and change svn-create-patch so as not to output tabs at all.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Strictly speaking, we could remove the tabs from the tests and not test the
> exact ouput of svn-create-patch (or at least limit the number of test cases
> with tabs to one).
> 
> And if David doesn't see any reason not to, I could go ahead and change
> svn-create-patch so as not to output tabs at all.

I think we should keep the tabs where they are.  There's no need to change the format, and this issue will only come up as long as Bug 33737 isn't fixed or any time the unit test file is updated (which probably won't be that often after it's written).

[David Kilzer (:ddkilzer)]

Comment on attachment 46744 [details]
Proposed patch

> diff --git a/WebKitTools/Scripts/VCSUtils.pm b/WebKitTools/Scripts/VCSUtils.pm
> +# Returns ($diffHashRef, $lastReadLine):
> +#   $diffHashRef:
> +#     copiedFromPath: if a file copy, the path from which the file was
> +#                     copied. Otherwise, undefined.
> +#     indexPath: the path in the "Index:" line.
> +#     sourceRevision: the revision number of the source. This is the same
> +#                     as the revision number the file was copied from, in
> +#                     the case of a file copy.
> +#     svnConvertedText: the diff converted to SVN format.
> +#   $line: the line last read from $fileHandle

"$line" should be "$lastReadLine" here.

> diff --git a/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl b/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#     * Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> +# its contributors may be used to endorse or promote products derived
> +# from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

This is the "old" BSD license.  I probably didn't catch this before, but you should use the text from WebCore/LICENSE-APPLE instead if you have no objections.

> +use strict;
> +use warnings;
> +
> +use Test::More;
> +use VCSUtils;

This doesn't require FindBin?  I guess it gets its include path from the script that includes it.

> diff --git a/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl b/WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parsePatch.pl
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#     * Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> +# its contributors may be used to endorse or promote products derived
> +# from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Ditto for the license.

r=me

[Chris Jerdonek]

(In reply to comment #7)

Thanks for the review, David!

> This is the "old" BSD license.  I probably didn't catch this before, but you
> should use the text from WebCore/LICENSE-APPLE instead if you have no
> objections.

Is this the canonical license text for new files?  Maybe I can add a pointer to this in the web site documentation, so other people can know.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (In reply to comment #7)
> > This is the "old" BSD license.  I probably didn't catch this before, but you
> > should use the text from WebCore/LICENSE-APPLE instead if you have no
> > objections.
> 
> Is this the canonical license text for new files?  Maybe I can add a pointer to
> this in the web site documentation, so other people can know.

Yes, I believe so.  Maciej will correct me if I'm wrong.

[Chris Jerdonek]

I'm going to manually commit this (after making David's suggested changes locally) since I need to add the allow-tabs property to a couple files.

This will be my first commit, so I have a couple questions.

I'm planning to do this:

(1) Use svn-apply to apply the Git patch to an SVN checkout on my local machine.
(2) Set the allow-tabs property on the appropriate files.
(3) Run "svn commit" with no args and my SVN_EDITOR environment variable set to commit-log-editor so the commit message will get properly populated.

Is there any easier way than what I've outlined above?  Also, is svn commit smart enough to put the ChangeLog entry at the top, or do I need to svn update immediately before committing (or is there another way to ensure this)?  Thanks -- any help is appreciated!

[Eric Seidel (no email)]

(In reply to comment #10)
> This will be my first commit, so I have a couple questions.

Please see http://trac.webkit.org/wiki/CommitterTips and update it if necessary.

> (1) Use svn-apply to apply the Git patch to an SVN checkout on my local
> machine.

Personally I would use:
webkit-patch apply-from-bug 33475
as that will find the patch, and set the reviewer for you.  It uses svn-apply under the covers.  (For help on this and other commands see webkit-patch help or webkit-patch help --all-commands.)

> (2) Set the allow-tabs property on the appropriate files.

Yes.  You have to do that manually.  At least until someone fixes svn-apply (nudge, nudge). :)
https://bugs.webkit.org/show_bug.cgi?id=27204

> (3) Run "svn commit" with no args and my SVN_EDITOR environment variable set to
> commit-log-editor so the commit message will get properly populated.

You could do that.  I find:
webkit-patch land --no-build
to be easier.

The only trouble you might run into is that if SVN has never asked for your credentials, that command will hang.  See https://bugs.webkit.org/show_bug.cgi?id=31500

> Is there any easier way than what I've outlined above?  Also, is svn commit
> smart enough to put the ChangeLog entry at the top, or do I need to svn update
> immediately before committing (or is there another way to ensure this)?

Unlikely.  resolve-ChangeLogs -f is used for this.  In general, assume all tools do the wrong things with ChangeLogs.  You'll be right 99% of the time. :)

[Eric Seidel (no email)]

(In reply to comment #11)
> > Also, is svn commit
> > smart enough to put the ChangeLog entry at the top, or do I need to svn update
> > immediately before committing (or is there another way to ensure this)?
> 
> Unlikely.  resolve-ChangeLogs -f is used for this.  In general, assume all
> tools do the wrong things with ChangeLogs.  You'll be right 99% of the time. :)

update-webkit is smart enough to run resolve-ChangeLogs when necessary (at least for SVN).  Eventually we'd like to make it smart enough for Git users too. :)

[Chris Jerdonek]

Manually committed r53667: http://trac.webkit.org/changeset/53667

Thanks for the advice, Eric!  I will be updating the wiki committer tips with info.

[Chris Jerdonek]

(In reply to comment #11)
> (In reply to comment #10)
> > This will be my first commit, so I have a couple questions.
> 
> Please see http://trac.webkit.org/wiki/CommitterTips and update it if
> necessary.

FYI, I wrote a first draft for the section of the committer tips page called "Walking you through your first commit":

http://trac.webkit.org/wiki/CommitterTips

Feel free to update if anything is incorrect or misleading.  Thanks.

[Chris Jerdonek]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > This is the "old" BSD license.  I probably didn't catch this before, but you
> > > should use the text from WebCore/LICENSE-APPLE instead if you have no
> > > objections.
> > 
> > Is this the canonical license text for new files?  Maybe I can add a pointer to
> > this in the web site documentation, so other people can know.
> 
> Yes, I believe so.  Maciej will correct me if I'm wrong.

FYI, it looks like this license file exists in several places (which you probably knew):

http://trac.webkit.org/changeset/50154

So there is no one "master copy".

[Chris Jerdonek]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > This is the "old" BSD license.  I probably didn't catch this before, but you
> > > should use the text from WebCore/LICENSE-APPLE instead if you have no
> > > objections.
> > 
> > Is this the canonical license text for new files?  Maybe I can add a pointer to
> > this in the web site documentation, so other people can know.
> 
> Yes, I believe so.  Maciej will correct me if I'm wrong.

I filed a bug for this issue here:

https://bugs.webkit.org/show_bug.cgi?id=34017