| Summary: | Change VCSUtils::parseDiffHeader() to skip over unrecognized lines | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||||
| Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cjerdonek, commit-queue, ddkilzer, eric, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Jerdonek
2010-01-11 10:39:16 PST
Created attachment 46307 [details]
Proposed patch
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
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 on attachment 46307 [details]
Proposed patch
Working on further changes here.
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). Created attachment 46548 [details]
Proposed patch 2
FYI, this is my first Git patch. Can these not be cq'ed?
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
Created attachment 46550 [details]
Proposed patch 3
Corrected my e-mail in Changelog and corrected comment at end of unit test file.
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
(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. :( In this case, I believe the file in question already has "allow-tabs" so this could be cq+'d just fine. 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 on attachment 46550 [details] Proposed patch 3 Clearing flags on attachment: 46550 Committed r53340: <http://trac.webkit.org/changeset/53340> All reviewed patches have been landed. Closing bug. (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! (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. (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. ;) |