Bug 33476 – Change VCSUtils::parseDiffHeader() to skip over unrecognized lines

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

There is also some other clean-up of parseDiffHeader() that can take place here.

[Chris Jerdonek]

Created attachment 46307 [details]
Proposed patch

[WebKit Review Bot]

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

[Chris Jerdonek]

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!

[Chris Jerdonek]

Comment on attachment 46307 [details]
Proposed patch

Working on further changes here.

[David Kilzer (:ddkilzer)]

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).

[Chris Jerdonek]

Created attachment 46548 [details]
Proposed patch 2

FYI, this is my first Git patch.  Can these not be cq'ed?

[WebKit Review Bot]

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

[Chris Jerdonek]

Created attachment 46550 [details]
Proposed patch 3

Corrected my e-mail in Changelog and corrected comment at end of unit test file.

[WebKit Review Bot]

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

[David Kilzer (:ddkilzer)]

(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.  :(

[Eric Seidel (no email)]

In this case, I believe the file in question already has "allow-tabs" so this could be cq+'d just fine.

[David Kilzer (:ddkilzer)]

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.

[WebKit Commit Bot]

Comment on attachment 46550 [details]
Proposed patch 3

Clearing flags on attachment: 46550

Committed r53340: <http://trac.webkit.org/changeset/53340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(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!

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(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. ;)