Bug 27363 – Add a parser of patches for linter

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

[Shinichiro Hamaji]

To implement a linter for patches (Bug 27291), we need a parser for diff file.

[Shinichiro Hamaji]

Created attachment 32919 [details]
Patch v2


---
 9 files changed, 137 insertions(+), 5 deletions(-)

[Shinichiro Hamaji]

Created attachment 32920 [details]
Patch v1


---
 3 files changed, 312 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

(In reply to comment #1)
> Created an attachment (id=32919) [details]
> Patch v2

Oops... I've sent a wrong patch. Please just ignore this.

[Shinichiro Hamaji]

Created attachment 32921 [details]
Patch v2


---
 1 files changed, 2 insertions(+), 1 deletions(-)

[Shinichiro Hamaji]

Created attachment 32922 [details]
Patch v3


---
 3 files changed, 313 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

I found an unnecessary local variable and eliminated it. Again, patch v2 is my mistake and sorry for this (it shows the difference between "patch v1" and "patch v3" unintentionally though).

[David Levin]

Comment on attachment 32922 [details]
Patch v3

Nice to see this patch.  Just a few things to address.


> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
...
> +        Now, the parser doesn't have full information of patches.
> +        It only knows the line number and line contents of each lines in
> +        chunks of a patch.  In case the line is newly added or deleted, it
> +        sets zero as the line number.

In isolation from the code, this comment is a bit confusing.  I think it is sufficient to say something like this here:

Adds a simple parser for unified diff format.

> diff --git a/WebKitTools/Scripts/modules/diff_parser.py b/WebKitTools/Scripts/modules/diff_parser.py
...
> +def match(pattern, string):
> +    """Matches the string with the pattern, caching the compiled regexp."""
> +    # The regexp compilation caching is inlined in both match and search for
> +    # performance reasons; factoring it out into a separate function turns out
> +    # to be noticeably expensive.

I'd remove this comment as there is no search function.  (The comment is a carry over from the cpplint.py file where there is.)

> +# FIXME: We may need more states if we want to check the integrity of input patches.

I think it is fine to omit this fixme.  If someone wants to check integrity, they can do the appropriate changes.

> +class DiffFile:
> +    """Hold information of a file in a patch.

Consider: Contains the information for one file in a patch.

> +    The field "lines" is a list which contains tuples which represent

Consider: The field "lines" is a list which contains tuples in this format:

> +    # FIXME: We only have information of lines in chunks. Need more info?

I think it is fine to omit this FIXME.  If someone wants more information, they can enhance this code.

> +    def add_new_line(self, line_number, line):
> +        """Adds a newly added line."""

I would just omit this doc string as it doesn't add any information.

> +    def add_old_line(self, line_number, line):

add_deleted_line seems like a better name.

> +        """Adds a deleted line."""

Remove this doc string.

> +    def add_unchanged_line(self, old_line_number, new_line_number, line):
> +        """Adds an unchanged line."""

Remove this doc string.

> +    def __init__(self, diff_input):
> +        """Parses a diff.
> +
> +        Args:
> +          diff_input: An iteratable object.

An iterable object

> +                elif line == '\\ No newline at end of file\n':
> +                    # Nothing to do.  We may still have some added lines.
> +                    # FIXME: Check if the following lines only contain added lines?

I don't understand this FIXME. (I guess I really don't understand how there are added lines after the "no newline at end of line" in the diff.)

> +            # The following kinds of lines will be ignored silently:
> +            #
> +            # - Lines git-diff-tree produces before filename
> +            #   declaration, which contains a hash of a patch.
> +            # - '='*67 lines between filename declaration and file
> +            #   revision declaration.
> +            # - Lines for revision declaration.
> +            #
> +            # FIXME: Need these info?

I think it is fine to omit this FIXME and perhaps the whole note.  It seems clear that this information isn't used.


> diff --git a/WebKitTools/Scripts/modules/diff_parser_unittest.py b/WebKitTools/Scripts/modules/diff_parser_unittest.py
...
> +    def test_diff_parser(self):
...
> +        # The first file looks OK, let's check the next, more complicated file.

Add a period here:
        # The first file looks OK. Let's check the next, more complicated file.

> +        # Look through the last chunk, which contains both add/delete.
Consider this wording:
        # Look through the last chunk, which contains both add's and delete's.

[Shinichiro Hamaji]

Created attachment 32927 [details]
Patch v4


---
 3 files changed, 291 insertions(+), 0 deletions(-)

[David Levin]

Comment on attachment 32927 [details]
Patch v4

I still like add_deleted_line better than add_old_line, but add_old_line is ok. :)

[David Levin]

Committed as http://trac.webkit.org/changeset/46046