fix(conventions): handle unusual Git paths and escaped links
This commit is contained in:
MrMha
parent
a11c8f7f70
commit
45f349df9b
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Reference in New Issue