Bug 34260 – check-webkit-style: is not reporting the carriage-return error for patches

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Chris Jerdonek]

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.

[Shinichiro Hamaji]

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?

[Chris Jerdonek]

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

[Chris Jerdonek]

Manually committed r54206 (via "git svn dcommit"):

http://trac.webkit.org/changeset/54206

[Shinichiro Hamaji]

> 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

[Chris Jerdonek]

FYI, looks like the fix worked. :)

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