Bug 33476 - Change VCSUtils::parseDiffHeader() to skip over unrecognized lines
Summary: Change VCSUtils::parseDiffHeader() to skip over unrecognized lines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-11 10:39 PST by Chris Jerdonek
Modified: 2010-01-15 13:26 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (12.52 KB, patch)
2010-01-11 14:05 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 2 (15.58 KB, patch)
2010-01-14 01:36 PST, Chris Jerdonek
cjerdonek: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 3 (15.72 KB, patch)
2010-01-14 01:54 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-01-11 10:39:16 PST
There is also some other clean-up of parseDiffHeader() that can take place here.
Comment 1 Chris Jerdonek 2010-01-11 14:05:16 PST
Created attachment 46307 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-01-11 14:06:47 PST
Attachment 46307 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:149:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:150:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:158:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:159:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:175:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:176:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:184:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:185:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:201:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:210:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:211:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11
Comment 3 Chris Jerdonek 2010-01-11 14:16:08 PST
I'm not sure if skipping over unrecognized lines is better than allowing them to pass through because we lose information by skipping lines.  I could imagine this preventing the --force option from working in cases where it might otherwise work (e.g. if the parse subroutine couldn't find the end of the header block due to some slight mis-formatting and so wound up skipping the entire file).  Isn't the patch command pretty lenient with respect to junk lines, anyways?  I'm not sure if you voiced an opinion on this issue before.

But the other changes I would like to make for sure.  Thanks!
Comment 4 Chris Jerdonek 2010-01-13 08:54:32 PST
Comment on attachment 46307 [details]
Proposed patch

Working on further changes here.
Comment 5 David Kilzer (:ddkilzer) 2010-01-13 10:50:20 PST
Comment on attachment 46307 [details]
Proposed patch

> Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl
> ===================================================================
> --- WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl	(revision 53081)
> +++ WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl	(working copy)
> @@ -266,15 +258,12 @@ END
>      # Header keys to check
>      svnConvertedText => <<'END',
>  Index: LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> -new file mode 100644
> -index 0000000..3c9f114
>  --- LayoutTests/http/tests/security/listener/xss-inactive-closure.html
>  +++ LayoutTests/http/tests/security/listener/xss-inactive-closure.html
>  END

Chris, I know you're working on a new patch, but I wanted to note that this result is incorrect (it's missing the '====' line).
Comment 6 Chris Jerdonek 2010-01-14 01:36:46 PST
Created attachment 46548 [details]
Proposed patch 2

FYI, this is my first Git patch.  Can these not be cq'ed?
Comment 7 WebKit Review Bot 2010-01-14 01:37:46 PST
Attachment 46548 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:129:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:130:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:138:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:139:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:154:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:155:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:163:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:164:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:179:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:188:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:189:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11
Comment 8 Chris Jerdonek 2010-01-14 01:54:45 PST
Created attachment 46550 [details]
Proposed patch 3

Corrected my e-mail in Changelog and corrected comment at end of unit test file.
Comment 9 WebKit Review Bot 2010-01-14 01:58:49 PST
Attachment 46550 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:129:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:130:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:138:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:139:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:154:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:155:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:163:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:164:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:179:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:188:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl:189:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11
Comment 10 David Kilzer (:ddkilzer) 2010-01-14 09:25:40 PST
(In reply to comment #6)
> Created an attachment (id=46548) [details]
> Proposed patch 2
> 
> FYI, this is my first Git patch.  Can these not be cq'ed?

I think they can, but the same no-tabs restriction is present.  :(
Comment 11 Eric Seidel (no email) 2010-01-14 17:39:36 PST
In this case, I believe the file in question already has "allow-tabs" so this could be cq+'d just fine.
Comment 12 David Kilzer (:ddkilzer) 2010-01-15 11:30:01 PST
Comment on attachment 46550 [details]
Proposed patch 3

> -plan(tests => @diffTests * 8); # Eight assertions per call to doDiffTest().
> +plan(tests => @diffTests * 6); # Multiply by number of assertions per call to doDiffTest().

It would be really cool if there was a way to "count" the number of assertions in code, but it's not necessary for this patch.  (I'm not sure if it's possible, either, unless you make the assertions an array of strings that are eval-ed.)

r=me

Let's try with the commit queue per Eric's Comment #11.
Comment 13 WebKit Commit Bot 2010-01-15 12:36:37 PST
Comment on attachment 46550 [details]
Proposed patch 3

Clearing flags on attachment: 46550

Committed r53340: <http://trac.webkit.org/changeset/53340>
Comment 14 WebKit Commit Bot 2010-01-15 12:36:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 David Kilzer (:ddkilzer) 2010-01-15 13:20:53 PST
(In reply to comment #11)
> In this case, I believe the file in question already has "allow-tabs" so this
> could be cq+'d just fine.

W00t!  It worked!
Comment 16 Chris Jerdonek 2010-01-15 13:24:22 PST
(In reply to comment #12)
> (From update of attachment 46550 [details])
> > -plan(tests => @diffTests * 8); # Eight assertions per call to doDiffTest().
> > +plan(tests => @diffTests * 6); # Multiply by number of assertions per call to doDiffTest().
> 
> It would be really cool if there was a way to "count" the number of assertions
> in code, but it's not necessary for this patch.  (I'm not sure if it's
> possible, either, unless you make the assertions an array of strings that are
> eval-ed.)

Thanks for the review, David.  It looks like this is possible without eval since all the assertions are calls to is().  We could write a function that returns an array of the parameters to use for each call to is().  The number of assertions per call would then be the length of the return value of this function on a sample input.
Comment 17 Chris Jerdonek 2010-01-15 13:26:30 PST
(In reply to comment #15)
> (In reply to comment #11)
> > In this case, I believe the file in question already has "allow-tabs" so this
> > could be cq+'d just fine.
> 
> W00t!  It worked!

Great!  By the way, I just got granted commit access today, so either way you would have been off the hook. ;)