address cpp-linter/cpp-linter-action#178 (#45)

uses legacy approach (pure python) to parsing a diff str when pygit2 fails.

added unit test to notify when bug is fixed

prints an ERROR and a WARNING log message in users' CI runs when legacy approach was taken

Author: 2bndy5

cpp_linter/git.py

@@ -15,8 +15,10 @@
     GIT_STATUS_INDEX_NEW,
     GIT_STATUS_INDEX_MODIFIED,
     GIT_STATUS_INDEX_RENAMED,
+    GitError,
 )
 from . import logger, CACHE_PATH, FileObj
+from .git_str import parse_diff as legacy_parse_diff
 
 
 def get_sha(repo: Repository, parent: Optional[int] = None) -> GitObject:
@@ -86,7 +88,11 @@ def parse_diff(diff_obj: Union[Diff, str]) -> List[FileObj]:
     """
     file_objects: List[FileObj] = []
     if isinstance(diff_obj, str):
-        diff_obj = Diff.parse_diff(diff_obj)
+        try:
+            diff_obj = Diff.parse_diff(diff_obj)
+        except GitError as exc:
+            logger.warning(f"pygit2.Diff.parse_diff() threw {exc}")
+            return legacy_parse_diff(diff_obj)
     for patch in diff_obj:
         if patch.delta.status not in ADDITIVE_STATUS:
             continue

cpp_linter/git_str.py

@@ -0,0 +1,92 @@
+"""This was reintroduced to deal with any bugs in pygit2 (or the libgit2 C library it
+binds to). The `parse_diff()` function here is only used when
+`pygit2.Diff.parse_diff()` function fails in `cpp_linter.git.parse_diff()`"""
+import re
+from typing import Optional, List, Tuple, cast
+from . import FileObj, logger
+
+
+DIFF_FILE_DELIMITER = re.compile(r"^diff --git a/.*$", re.MULTILINE)
+DIFF_FILE_NAME = re.compile(r"^\+\+\+\sb?/(.*)$", re.MULTILINE)
+DIFF_RENAMED_FILE = re.compile(r"^rename to (.*)$", re.MULTILINE)
+DIFF_BINARY_FILE = re.compile(r"^Binary\sfiles\s", re.MULTILINE)
+HUNK_INFO = re.compile(r"@@\s\-\d+,\d+\s\+(\d+,\d+)\s@@", re.MULTILINE)
+
+
+def _get_filename_from_diff(front_matter: str) -> Optional[re.Match]:
+    """Get the filename from content in the given diff front matter."""
+    filename_match = DIFF_FILE_NAME.search(front_matter)
+    if filename_match is not None:
+        return filename_match
+
+    # check for renamed file name
+    rename_match = DIFF_RENAMED_FILE.search(front_matter)
+    if rename_match is not None and front_matter.lstrip().startswith("similarity"):
+        return rename_match
+    # We may need to compensate for other instances where the filename is
+    # not directly after `+++ b/`. Binary files are another example of this.
+    if DIFF_BINARY_FILE.search(front_matter) is None:
+        # log the case and hope it helps in the future
+        logger.warning(  # pragma: no cover
+            "Unrecognized diff starting with:\n%s",
+            "\n".join(front_matter.splitlines()),
+        )
+    return None
+
+def parse_diff(full_diff: str) -> List[FileObj]:
+    """Parse a given diff into file objects.
+
+    :param full_diff: The complete diff for an event.
+    :returns: A `list` of `FileObj` instances containing information about the files
+        changed.
+    """
+    file_objects: List[FileObj] = []
+    logger.error("Using pure python to parse diff because pygit2 failed!")
+    file_diffs = DIFF_FILE_DELIMITER.split(full_diff.lstrip("\n"))
+    for diff in file_diffs:
+        if not diff or diff.lstrip().startswith("deleted file"):
+            continue
+        first_hunk = HUNK_INFO.search(diff)
+        hunk_start = -1 if first_hunk is None else first_hunk.start()
+        diff_front_matter = diff[:hunk_start]
+        filename_match = _get_filename_from_diff(diff_front_matter)
+        if filename_match is None:
+            continue
+        filename = cast(str, filename_match.groups(0)[0])
+        if first_hunk is None:
+            continue
+        diff_chunks, additions = _parse_patch(diff[first_hunk.start() :])
+        file_objects.append(FileObj(filename, additions, diff_chunks))
+    return file_objects
+
+
+def _parse_patch(full_patch: str) -> Tuple[List[List[int]], List[int]]:
+    """Parse a diff's patch accordingly.
+
+    :param full_patch: The entire patch of hunks for 1 file.
+    :returns:
+        A `tuple` of lists where
+
+        - Index 0 is the ranges of lines in the diff. Each item in this `list` is a
+          2 element `list` describing the starting and ending line numbers.
+        - Index 1 is a `list` of the line numbers that contain additions.
+    """
+    ranges: List[List[int]] = []
+    # additions is a list line numbers in the diff containing additions
+    additions: List[int] = []
+    line_numb_in_diff: int = 0
+    chunks = HUNK_INFO.split(full_patch)
+    for index, chunk in enumerate(chunks):
+        if index % 2 == 1:
+            # each odd element holds the starting line number and number of lines
+            start_line, hunk_length = [int(x) for x in chunk.split(",")]
+            ranges.append([start_line, hunk_length + start_line])
+            line_numb_in_diff = start_line
+            continue
+        # each even element holds the actual line changes
+        for i, line in enumerate(chunk.splitlines()):
+            if line.startswith("+"):
+                additions.append(line_numb_in_diff)
+            if not line.startswith("-") and i:  # don't increment on first line
+                line_numb_in_diff += 1
+    return (ranges, additions)

tests/test_git_str.py

@@ -0,0 +1,67 @@
+import logging
+import pytest
+from cpp_linter import logger
+from cpp_linter.git import parse_diff
+from cpp_linter.git_str import parse_diff as parse_diff_str
+
+
+def test_pygit2_bug1260(caplog: pytest.LogCaptureFixture):
+    """This test the legacy approach of parsing a diff str using pure python regex
+    patterns.
+
+    See https://github.com/libgit2/pygit2/issues/1260 for details
+    """
+    diff_str = "\n".join(
+        [
+            "diff --git a/path/for/Some file.cpp b/path/to/Some file.cpp",
+            "similarity index 99%",
+            "rename from path/for/Some file.cpp",
+            "rename to path/to/Some file.cpp",
+        ]
+    )
+    caplog.set_level(logging.WARNING, logger=logger.name)
+    # the bug in libgit2 should trigger a call to
+    # cpp_linter.git_str.legacy_parse_diff()
+    files = parse_diff(diff_str)
+    assert caplog.messages, "this test is no longer needed; bug was fixed in pygit2"
+    # if we get here test, then is satisfied
+    assert not files  # no line changes means no file to focus on
+
+def test_typical_diff():
+    """For coverage completeness. Also tests for files with spaces in the names."""
+    diff_str = "\n".join(
+        [
+            "diff --git a/path/for/Some file.cpp b/path/to/Some file.cpp",
+            "--- a/path/for/Some file.cpp",
+            "+++ b/path/to/Some file.cpp",
+            "@@ -3,7 +3,7 @@",
+            " ",
+            " ",
+            " ",
+            "-#include <some_lib/render/animation.hpp>",
+            "+#include <some_lib/render/animations.hpp>",
+            " ",
+            " ",
+            " \n",
+        ]
+    )
+    from_c = parse_diff(diff_str)
+    from_py = parse_diff_str(diff_str)
+    assert [f.serialize() for f in from_c] == [f.serialize() for f in from_py]
+    for file_obj in from_c:
+        # file name should have spaces
+        assert " " in file_obj.name
+
+
+def test_binary_diff():
+    """For coverage completeness"""
+    diff_str = "\n".join(
+        [
+            "diff --git a/some picture.png b/some picture.png",
+            "new file mode 100644",
+            "Binary files /dev/null and b/some picture.png differ",
+        ]
+    )
+    files = parse_diff_str(diff_str)
+    # binary files are ignored during parsing
+    assert not files

tests/test_misc.py

@@ -81,7 +81,7 @@ def test_list_src_files(
     extensions: List[str],
 ):
     """List the source files in the demo folder of this repo."""
-    monkeypatch.setattr(Globals, "FILES",  [])
+    monkeypatch.setattr(Globals, "FILES", [])
     monkeypatch.chdir(Path(__file__).parent.as_posix())
     caplog.set_level(logging.DEBUG, logger=cpp_linter.logger.name)
     list_source_files(ext_list=extensions, ignored_paths=[], not_ignored=[])