Bug 38454

Summary: svn-apply: Have parseDiffHeader() call parseGitDiffHeader().
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: Chris Jerdonek <cjerdonek>
Status: RESOLVED FIXED    
Severity: Normal CC: cjerdonek, dbates, eric, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 38425    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
Proposed patch 2 none

Description Chris Jerdonek 2010-05-03 04:02:24 PDT
In this patch, I think it would make sense to recast the existing parseDiffHeader() as parseSvnDiffHeader() and then create a new parseDiffHeader() that calls the Git or SVN version based on the first line (whether it begins with "Index:" or "diff --git").  The unit tests for the new parseDiffHeader() can have enough tests just to ensure that it supports carrying over each diffHash key value correctly (since the more thorough tests will be in the tests for the Git and SVN versions).

This patch can also delete the gitdiff2svndiff() subroutine.
Comment 1 Chris Jerdonek 2010-05-03 13:57:59 PDT
Created attachment 54958 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-05-03 14:03:57 PDT
Attachment 54958 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:47:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:48:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:57:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:58:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:73:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:74:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:83:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:84:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:99:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:100:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:109:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:110:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:125:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:126:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:135:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:136:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:151:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:152:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:161:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:162:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 20 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Jerdonek 2010-05-03 14:07:09 PDT
Created attachment 54959 [details]
Proposed patch 2

A minor change to the ChangeLog and a code comment.

By the way, expect style errors for tabs in this patch.  This is because diffs contain tab characters, and the unit tests contain sample diffs.

Also marking cq- since this patch will require setting the svn:allow-tabs property.
Comment 4 WebKit Review Bot 2010-05-03 14:09:59 PDT
Attachment 54959 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:47:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:48:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:57:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:58:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:73:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:74:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:83:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:84:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:99:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:100:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:109:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:110:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:125:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:126:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:135:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:136:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:151:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:152:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:161:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnDiffHeader.pl:162:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 20 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Daniel Bates 2010-05-03 23:56:54 PDT
Comment on attachment 54959 [details]
Proposed patch 2

> +#     * 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.

Just wanted to point out you reference Apple in the copyright here.

r=me.
Comment 6 Chris Jerdonek 2010-05-04 00:37:06 PDT
(In reply to comment #5)
> (From update of attachment 54959 [details])
> > +#     * 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.
> 
> Just wanted to point out you reference Apple in the copyright here.
> 
> r=me.

Thanks, Dan.

I preserved the original copyright and license text since the new file is mostly a copy of the original, though perhaps I can change that text since I'm the sole copyright owner of the file. :)
Comment 7 Chris Jerdonek 2010-05-04 00:56:43 PDT
Comment on attachment 54959 [details]
Proposed patch 2

Clearing flags on attachment: 54959

Committed r58739: <http://trac.webkit.org/changeset/58739>
Comment 8 Chris Jerdonek 2010-05-04 00:56:58 PDT
All reviewed patches have been landed.  Closing bug.