From 45f349df9b1989df7aab9407ad782f3d09d5280c Mon Sep 17 00:00:00 2001 From: MrMha <114929652+MrMha@users.noreply.github.com> Date: Sun, 11 Oct 2026 18:21:09 +0400 Subject: [PATCH] fix(conventions): handle unusual Git paths and escaped links --- tools/check_conventions.py | 35 +++--- .../tests/test_check_conventions_comments.py | 101 ++++++++++++++++++ 2 files changed, 124 insertions(+), 12 deletions(-) diff --git a/tools/check_conventions.py b/tools/check_conventions.py index 8254975e8..961b82c97 100644 --- a/tools/check_conventions.py +++ b/tools/check_conventions.py @@ -7,6 +7,7 @@ import subprocess import sys from collections import defaultdict from pathlib import PurePosixPath +from urllib.parse import unquote RULES = { "comment": ("error", "docs/dev/CONVENTIONS.md", "Comments are only for technical debt, so they need a change to docs/dev/TechnicalDebt.md in the same pull request; #endif and namespace ends are always allowed"), @@ -55,21 +56,26 @@ class Check: self.added = defaultdict(list) self.status = {} self.numstat = {} - self.tree = set(git("ls-tree", "-r", "--name-only", head).splitlines()) + entries = [entry.split("\t", 1) for entry in git("ls-tree", "-rz", head).split("\0") if entry] + self.modes = {path: metadata.split()[0] for metadata, path in entries} + self.tree = set(self.modes) self.dirs = {str(parent) for path in self.tree for parent in PurePosixPath(path).parents} - for line in git("diff", "--name-status", "--no-renames", self.base, head).splitlines(): - status, path = line.split("\t", 1) + statuses = git("diff", "--name-status", "--no-renames", "-z", self.base, head).split("\0") + for status, path in zip(statuses[::2], statuses[1::2]): self.status[path] = status - for line in git("diff", "--numstat", "--no-renames", self.base, head).splitlines(): + for line in git("diff", "--numstat", "--no-renames", "-z", self.base, head).split("\0"): + if not line: + continue added, deleted, path = line.split("\t", 2) self.numstat[path] = (added, deleted) - path = None - for line in git("diff", "-U0", "--no-color", "--no-renames", "--no-prefix", self.base, head).splitlines(): - if line.startswith("+++ "): - path = None if line == "+++ /dev/null" else line[4:].rstrip("\t") + paths = iter(self.status) + path, number = None, None + for line in git("diff", "-U0", "--no-color", "--no-renames", self.base, head).splitlines(): + if line.startswith("diff --git "): + path, number = next(paths), None elif line.startswith("@@"): number = int(re.search(r"\+(\d+)", line).group(1)) - elif line.startswith("+") and path: + elif path and number is not None and line.startswith("+"): self.added[path].append((number, line[1:])) number += 1 @@ -141,6 +147,9 @@ class Check: for path, (added, deleted) in self.numstat.items(): if path.startswith("3rdparty/"): continue + if self.modes.get(path) == "160000": + self.report("system-dependency", path, 0, "submodule outside 3rdparty/") + continue suffix = PurePosixPath(path).suffix.lower() if self.status.get(path) != "D" and (added == "-" or suffix in IMAGES or not utf8(git("show", f"{self.head}:{path}", text=False))): self.report("binary", path, 0) @@ -188,10 +197,11 @@ def utf8(data): def links(path, text): result = [] for target in LINK.findall(text): - if re.match(r"[a-z]+:|#", target): + if re.match(r"[A-Za-z][A-Za-z0-9+.-]*:|//|#", target): continue target = target.split("#", 1)[0].split("?", 1)[0] - result.append((target, posixpath.normpath(target.lstrip("/") if target.startswith("/") else str(PurePosixPath(path).parent / target)))) + decoded = unquote(target) + result.append((target, posixpath.normpath(decoded.lstrip("/") if decoded.startswith("/") else str(PurePosixPath(path).parent / decoded)))) return result @@ -256,7 +266,8 @@ def output(check, repo): errors += level == "error" text = f"{message}" + (f": {detail}" if detail else "") if actions: - location = f" file={path},line={max(line, 1)}," if path else " " + escaped_path = path.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A").replace(":", "%3A").replace(",", "%2C") + location = f" file={escaped_path},line={max(line, 1)}," if path else " " print(f"::{level}{location}title={rule}::" + text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")) else: print(f"{path or '(pull request)'}{f':{line}' if line else ''}: {level}: [{rule}] {text}") diff --git a/tools/tests/test_check_conventions_comments.py b/tools/tests/test_check_conventions_comments.py index 9c943fe7b..a584fcd23 100644 --- a/tools/tests/test_check_conventions_comments.py +++ b/tools/tests/test_check_conventions_comments.py @@ -1,3 +1,5 @@ +import contextlib +import io import os import subprocess import sys @@ -7,6 +9,7 @@ from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from check_conventions import STUB_BODY, SYSTEM_DEPENDENCY, Check, function_body +from check_conventions import Check, STUB_BODY, function_body, links, output class FunctionBodyTests(unittest.TestCase): @@ -270,5 +273,103 @@ class SystemDependencyTests(unittest.TestCase): self.assertIsNone(SYSTEM_DEPENDENCY.search(line)) +class RepositoryChecksTests(unittest.TestCase): + def setUp(self): + self.directory = tempfile.TemporaryDirectory() + self.root = Path(self.directory.name) + self.original = Path.cwd() + os.chdir(self.root) + self.git("init", "-q") + (self.root / "README.md").write_text("Project\n", encoding="utf-8") + self.commit() + + def tearDown(self): + os.chdir(self.original) + self.directory.cleanup() + + def git(self, *args): + return subprocess.run(["git", "-c", "user.name=Conventions Test", "-c", + "user.email=test@example.com", "-c", "commit.gpgsign=false", *args], + capture_output=True, text=True, check=True).stdout.strip() + + def commit(self): + self.git("add", ".") + self.git("commit", "-qm", "test: add convention fixture") + + def test_quoted_utf8_paths_are_checked(self): + name = "docs/dev/測試guide.md" if os.name == "nt" else "docs/dev/測試\tguide.md" + path = self.root / name + path.parent.mkdir(parents=True) + path.write_text("[missing](absent.md)\n", encoding="utf-8") + self.commit() + check = Check("HEAD~1", "HEAD") + check.repository() + check.docs() + self.assertEqual(check.findings, [("doc-link", name, 1, "absent.md")]) + + def test_mixed_patch_types_preserve_file_order(self): + name = "a b.cpp" if os.name == "nt" else "a\nb.cpp" + (self.root / "README.md").unlink() + (self.root / "a.bin").write_bytes(b"binary\x00data") + (self.root / name).write_text("// debt\n", encoding="utf-8") + self.commit() + check = Check("HEAD~1", "HEAD") + check.code() + check.repository() + self.assertIn(("comment", name, 1, ""), check.findings) + self.assertIn(("binary", "a.bin", 0, ""), check.findings) + self.assertNotIn("README.md", check.added) + + def test_increment_line_is_not_a_patch_file_header(self): + path = self.root / "core/example.cpp" + path.parent.mkdir() + path.write_text("void example() {\n++ counter;\n// debt\n}\n", encoding="utf-8") + self.commit() + check = Check("HEAD~1", "HEAD") + check.code() + self.assertIn(("comment", "core/example.cpp", 3, ""), check.findings) + + def test_submodule_outside_thirdparty_reports_without_crashing(self): + head = self.git("rev-parse", "HEAD") + self.git("update-index", "--add", "--cacheinfo", "160000", head, "vendor") + self.git("commit", "-qm", "build: add invalid submodule") + check = Check("HEAD~1", "HEAD") + check.repository() + self.assertIn(("system-dependency", "vendor", 0, "submodule outside 3rdparty/"), check.findings) + + def test_thirdparty_submodule_remains_allowed(self): + head = self.git("rev-parse", "HEAD") + self.git("update-index", "--add", "--cacheinfo", "160000", head, "3rdparty/vendor") + self.git("commit", "-qm", "build: add allowed submodule") + check = Check("HEAD~1", "HEAD") + check.repository() + self.assertEqual(check.findings, []) + + def test_relative_link_url_decoding(self): + self.assertEqual(links("docs/dev/index.md", "[guide](./space%20name.md#details)"), + [("./space%20name.md", "docs/dev/space name.md")]) + self.assertEqual(links("docs/dev/index.md", "[reference](HTTPS://example.com/x)"), []) + self.assertEqual(links("docs/dev/index.md", "[reference](//example.com/x)"), []) + + def test_percent_encoded_fragment_is_part_of_filename(self): + self.assertEqual(links("docs/dev/index.md", "[guide](./part%23one.md#details)"), + [("./part%23one.md", "docs/dev/part#one.md")]) + + def test_annotation_filename_properties_are_escaped(self): + check = type("Findings", (), {"findings": [("doc-link", "docs/name,with:colon%\n.md", 2, "missing") ]})() + original = os.environ.get("GITHUB_ACTIONS") + os.environ["GITHUB_ACTIONS"] = "true" + try: + with contextlib.redirect_stdout(io.StringIO()) as captured: + output(check, "owner/repo") + finally: + if original is None: + os.environ.pop("GITHUB_ACTIONS", None) + else: + os.environ["GITHUB_ACTIONS"] = original + annotation = captured.getvalue().splitlines()[0] + self.assertIn("file=docs/name%2Cwith%3Acolon%25%0A.md,line=2", annotation) + + if __name__ == "__main__": unittest.main()