Bug 34260 - check-webkit-style: is not reporting the carriage-return error for patches
Summary: check-webkit-style: is not reporting the carriage-return error for patches
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: 34379
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-28 06:19 PST by Chris Jerdonek
Modified: 2010-02-08 15:46 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (27.93 KB, patch)
2010-02-01 13:30 PST, Chris Jerdonek
hamaji: review+
eric: commit-queue+
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-28 06:19:45 PST
It looks like there is a bug in StyleChecker.check_patch() which is causing the carriage-return error never to be reported for patches.

The reason is that StyleChecker._process_file() associates the carriage-return error with line zero, but line zero will never be a modified line of a patch file (line numbers of patches start with 1).  I discovered this a week or two ago and made a note of it in the code as a FIXME.

Since it looks like we've never turned this on, it's unknown to me what will happen if we correct this error and turn it on (i.e. it's unknown to me how prevalent it is).
Comment 1 Chris Jerdonek 2010-02-01 13:30:11 PST
Created attachment 47867 [details]
Proposed patch

I addressed the FIXME by adding the ability to limit the number of error reports per category, per file.  (The essence of the existing carriage return logic is that a carriage return error should be reported at most once per file.)

It might be nice to enable this for the tab check, too.  See this, for example--

https://bugs.webkit.org/show_bug.cgi?id=33475#c2

Note also that the submitted fix preserves the prevailing behavior that errors for patch files should only be reported if they occur in a modified line.  This wasn't being done with the existing "line 0" code (even if the existing code were working).
Comment 2 Chris Jerdonek 2010-02-01 13:39:14 PST
I also have a question.  I'm wondering if, in our case, it makes sense to do the following check prior to checking for carriage returns:

+        # FIXME: Shouldn't we always do this check?
+        should_check_no_carriage_return = (os.linesep != '\r\n')
+
+        for line_number in range(len(lines)):
+            if should_check_no_carriage_return:
+                check_no_carriage_return(lines[line_number], line_number,
+                                         handle_style_error)

The reason is that it seems like whether the files in source control should have carriage returns has nothing to do with the operating system of the machine that is running check-webkit-style (i.e. check-webkit-style should behave the same on Windows machines as it does on Macs, etc).

By the way, I already addressed this FIXME in checker_unittest.py and forgot to delete it:

+# FIXME: Check that the keys are in _style_categories().
+#
+# The maximum number of errors to report per file, per category.
+# If a category is not a key, then it has no maximum.
+MAX_REPORTS_PER_CATEGORY = {
+    "whitespace/carriage_return": 1
+}

I will correct that later.
Comment 3 Shinichiro Hamaji 2010-02-01 21:32:41 PST
Comment on attachment 47867 [details]
Proposed patch

>      def __init__(self, output_format="emacs", verbosity=1, filter=None,
> -                 git_commit=None, extra_flag_values=None):
> -        if filter is None:
> -            filter = CategoryFilter()
> +                 max_reports_per_category=None,
> +                 git_commit=None,
> +                 extra_flag_values=None):

Though this code is OK as is, I'd break line for each arguments to improve the consistency a bit:

      def __init__(self,
                   output_format="emacs",
                   verbosity=1,
                   filter=None,
                   max_reports_per_category=None,
                   git_commit=None,
                   extra_flag_values=None):

> +        # FIXME: Shouldn't we always do this check?

Agreed. I guess we need to use svn property "eol-style" or a whitelist instead of this check.

> +    def test_max_reports_per_category(self):
> +        """Check that MAX_REPORTS_PER_CATEGORY is valid."""
> +        categories = style.style_categories()
> +        for category in style.MAX_REPORTS_PER_CATEGORY.keys():

I slightly prefer iterkeys, but it's OK as is.

> +    def test_ends_with_carriage_newline(self):
> +        # Check_no_carriage_return only checks the final character.
> +        self.assert_carriage_return("no carriage return\r\n", is_error=False)

This test case looks weird as we want to warn for this case. I think we'll never pass strings which contain "\n" as a parameter of check_no_carriage_return. How about using "carriage return\r in a string" or something like this?
Comment 4 Chris Jerdonek 2010-02-01 21:49:29 PST
(In reply to comment #3)
> (From update of attachment 47867 [details])
> >      def __init__(self, output_format="emacs", verbosity=1, filter=None,
> > -                 git_commit=None, extra_flag_values=None):
> > -        if filter is None:
> > -            filter = CategoryFilter()
> > +                 max_reports_per_category=None,
> > +                 git_commit=None,
> > +                 extra_flag_values=None):
> 
> Though this code is OK as is, I'd break line for each arguments to improve the
> consistency a bit:
> 
>       def __init__(self,
>                    output_format="emacs",
>                    verbosity=1,
>                    filter=None,
>                    max_reports_per_category=None,
>                    git_commit=None,
>                    extra_flag_values=None):

Thanks.  I prefer this style, too.  Maybe this should be part of the new style guide? :)

> 
> > +        # FIXME: Shouldn't we always do this check?
> 
> Agreed. I guess we need to use svn property "eol-style" or a whitelist instead
> of this check.

Sounds good.  I'll go ahead and remove it and add a FIXME re: eol-style instead.

> > +    def test_max_reports_per_category(self):
> > +        """Check that MAX_REPORTS_PER_CATEGORY is valid."""
> > +        categories = style.style_categories()
> > +        for category in style.MAX_REPORTS_PER_CATEGORY.keys():
> 
> I slightly prefer iterkeys, but it's OK as is.

Good point.  Thanks.

> > +    def test_ends_with_carriage_newline(self):
> > +        # Check_no_carriage_return only checks the final character.
> > +        self.assert_carriage_return("no carriage return\r\n", is_error=False)
> 
> This test case looks weird as we want to warn for this case. I think we'll
> never pass strings which contain "\n" as a parameter of
> check_no_carriage_return. How about using "carriage return\r in a string" or
> something like this?

Sounds good.  By the way, will we ever have a "\r" in the middle of a string?  Should that also be an error and not just occurrences at the end?
Comment 5 Chris Jerdonek 2010-02-01 22:42:54 PST
Manually committed r54206 (via "git svn dcommit"):

http://trac.webkit.org/changeset/54206
Comment 6 Shinichiro Hamaji 2010-02-02 02:37:37 PST
> Sounds good.  By the way, will we ever have a "\r" in the middle of a string? 
> Should that also be an error and not just occurrences at the end?

We have at least one file: http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/script-tests/parse-backslash-before-newline.js
Comment 7 Chris Jerdonek 2010-02-03 23:21:31 PST
FYI, looks like the fix worked. :)

https://bugs.webkit.org/show_bug.cgi?id=34464#c9