diff --git a/pyproject.toml b/pyproject.toml index f7cfb9e..76dd3ac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,7 +21,7 @@ authors = [ dependencies = [ "PyYAML>=6.0", "tomli; python_version < \"3.11\"", - "jinja2>=3.1" + "jinja2>=3.1.6" ] [project.urls] @@ -52,3 +52,4 @@ include = ["pkgmgr*"] [tool.setuptools.package-data] "pkgmgr.config" = ["*.yml", "*.yaml"] +"pkgmgr.actions.review" = ["*.j2"] diff --git a/src/pkgmgr/actions/review/__init__.py b/src/pkgmgr/actions/review/__init__.py new file mode 100644 index 0000000..521d64d --- /dev/null +++ b/src/pkgmgr/actions/review/__init__.py @@ -0,0 +1,7 @@ +from __future__ import annotations + +from .collect import collect_review +from .model import ReviewError +from .render import render_review, write_review + +__all__ = ["ReviewError", "collect_review", "render_review", "write_review"] diff --git a/src/pkgmgr/actions/review/checks.py b/src/pkgmgr/actions/review/checks.py new file mode 100644 index 0000000..d3eacc5 --- /dev/null +++ b/src/pkgmgr/actions/review/checks.py @@ -0,0 +1,112 @@ +from __future__ import annotations + +import os + +from pkgmgr.actions.branch.gc import GITLINK, _unsafe_config +from pkgmgr.core.git.errors import GitBaseError, GitRunError +from pkgmgr.core.git.run import run + +from .model import FileView, ReviewError +from .repo import git_text + +ATTRIBUTE_BINARY = ( + "{} is shown as binary because {}, not because of its content: " + "run git diff --text to see it." +) +UNSET = "a gitattributes entry unsets its diff attribute" +DRIVER = "its gitattributes diff driver {0} sets diff.{0}.binary" +RUNS = "a command git would run on this host" + + +def hidden_entries(top: str) -> list[str]: + listing = git_text(["ls-files", "-v", "-z"], top) + return [ + f"Index entry {entry[2:]} is marked " + f"{'skip-worktree' if entry[0] == 'S' else 'assume-unchanged'}: " + "its worktree changes are not shown." + for entry in listing.split("\0") + if entry[:1] == "S" or entry[:1].islower() + ] + + +def _binary_drivers(top: str) -> set[str]: + try: + out = run( + ["config", "-z", "--type=bool", "--get-regexp", r"^diff\..*\.binary$"], + cwd=top, + scan=True, + ) + except GitRunError as exc: + if exc.returncode == 1: + return set() + raise ReviewError("git config failed reading diff..binary") from None + last: dict[str, str] = {} + for record in filter(None, out.split("\0")): + key, _, value = record.partition("\n") + last[key.removeprefix("diff.").removesuffix(".binary")] = value + return {driver for driver, value in last.items() if value == "true"} + + +def attribute_binary(files: list[FileView], top: str) -> list[str]: + paths = list(dict.fromkeys(view.diff.path for view in files if view.diff.binary)) + if not paths: + return [] + fields = git_text(["check-attr", "-z", "diff", "--", *paths], top).split("\0") + reasons = { + **{driver: DRIVER.format(driver) for driver in _binary_drivers(top)}, + "unset": UNSET, + } + return [ + ATTRIBUTE_BINARY.format(path, reasons[value]) + for path, value in zip(fields[0::3], fields[2::3]) + if value in reasons + ] + + +def _submodule_configs(top: str, prefix: str) -> list[str]: + """ + Args: + top: Real path of the top-level working tree. + prefix: Submodule path below ``top`` whose gitlinks are walked, or "" + for the top level itself. + + Returns: + One reason per populated submodule, nested ones included, whose + config defines a filter or cannot be read. A gitlink behind a symbolic + link or without a ``.git`` is skipped: git runs nothing there either. + """ + listing = git_text(["ls-files", "--stage", "-z"], os.path.join(top, prefix)) + gitlinks = dict.fromkeys( + path + for meta, _, path in (entry.partition("\t") for entry in listing.split("\0")) + if meta.startswith(f"{GITLINK} ") + ) + reasons: list[str] = [] + for path in gitlinks: + relative = os.path.join(prefix, path) + full = os.path.join(top, relative) + if os.path.realpath(full) != full or not os.path.lexists( + os.path.join(full, ".git") + ): + continue + try: + keys = _unsafe_config(full) + nested = _submodule_configs(top, relative) + except (GitBaseError, ReviewError): + reasons.append( + f"the config of submodule {relative} cannot be read, so its " + "filters cannot be ruled out" + ) + continue + if keys: + reasons.append( + f"the config of submodule {relative} defines {', '.join(keys)}, {RUNS}" + ) + reasons.extend(nested) + return reasons + + +def unsafe_configs(top: str) -> list[str]: + keys = _unsafe_config(top) + own = [f"the repository config defines {', '.join(keys)}, {RUNS}"] if keys else [] + return own + _submodule_configs(os.path.realpath(top), "") diff --git a/src/pkgmgr/actions/review/collect.py b/src/pkgmgr/actions/review/collect.py new file mode 100644 index 0000000..d47e682 --- /dev/null +++ b/src/pkgmgr/actions/review/collect.py @@ -0,0 +1,153 @@ +from __future__ import annotations + +import os + +from pkgmgr.actions.branch.gc import END_OF_OPTIONS + +from .checks import attribute_binary, hidden_entries, unsafe_configs +from .diff import parse_patch +from .files import read_note, untracked_views +from .model import Commit, Review, Section +from .repo import base_ref, git_optional, git_text, top_level +from .views import file_views + +DIFF_FLAGS = [ + "--no-color", + "--no-ext-diff", + "--no-textconv", + "-M", + "--submodule=short", + "--ignore-submodules=none", + "--src-prefix=a/", + "--dst-prefix=b/", +] + + +def _commit(sha: str, anchor: str, top: str, limit: int) -> Commit: + meta = git_text( + [ + "show", + "-s", + "--no-color", + "--no-show-signature", + "--format=%h%x00%an <%ae>%x00%s%x00%b", + END_OF_OPTIONS, + sha, + ], + top, + ) + short, author, subject, body = meta.split("\0", 3) + patch = git_text( + [ + "show", + "--format=", + "--no-show-signature", + "--diff-merges=first-parent", + *DIFF_FLAGS, + END_OF_OPTIONS, + sha, + ], + top, + ) + return Commit( + anchor=anchor, + short=short, + author=author, + subject=subject, + body=body.rstrip("\n"), + files=file_views(parse_patch(patch), anchor, top, limit), + ) + + +def collect_review( + cwd: str, max_file_lines: int, base: str | None, notes: list[str] +) -> Review: + """ + Args: + cwd: Directory inside the git working copy to review. + max_file_lines: Diff lines shown per file before the rest is summarised. + base: Branch, under refs/heads/ or refs/remotes/, whose ``REF..HEAD`` + commits are listed, or None for no Commits section. + notes: Regular text files shown verbatim at the top, relative to + ``cwd``; a symbolic link anywhere in the path is refused. + + Returns: + The collected Review model. + """ + top = top_level(cwd) + base_name, base_sha = (None, None) if base is None else base_ref(base, top) + note_items = tuple(read_note(path, cwd) for path in notes) + unsafe = unsafe_configs(top) + skipped = ( + f"Unstaged changes and index tree not computed: {'; '.join(unsafe)}." + if unsafe + else None + ) + staged = Section( + key="staged", + title="Staged", + files=file_views( + parse_patch(git_text(["diff", "--cached", *DIFF_FLAGS], top)), + "staged", + top, + max_file_lines, + ), + skipped=None, + ) + unstaged = Section( + key="unstaged", + title="Unstaged", + files=() + if skipped + else file_views( + parse_patch(git_text(["diff", *DIFF_FLAGS], top)), + "unstaged", + top, + max_file_lines, + ), + skipped=skipped, + ) + untracked = Section( + key="untracked", + title="Untracked", + files=untracked_views(top, max_file_lines), + skipped=None, + ) + commits: tuple[Commit, ...] = () + if base_sha is not None: + shas = git_text(["rev-list", END_OF_OPTIONS, f"{base_sha}..HEAD"], top) + commits = tuple( + _commit(sha, f"commit-{index}", top, max_file_lines) + for index, sha in enumerate(shas.split(), 1) + ) + tracked = [ + *staged.files, + *unstaged.files, + *(view for commit in commits for view in commit.files), + ] + return Review( + top_level=top, + name=os.path.basename(top), + branch=git_optional( + ["symbolic-ref", "--short", "-q", END_OF_OPTIONS, "HEAD"], top + ), + head=git_optional( + ["rev-parse", "--verify", "--quiet", "--short", END_OF_OPTIONS, "HEAD"], + top, + ), + index_tree="(not computed, see the warnings)" + if skipped + else git_optional(["write-tree"], top) or "(index has unmerged entries)", + base=base_name, + base_short=None + if base_sha is None + else git_text(["rev-parse", "--short", END_OF_OPTIONS, base_sha], top).strip(), + warnings=tuple( + ([skipped] if skipped else []) + + hidden_entries(top) + + attribute_binary(tracked, top) + ), + notes=note_items, + sections=(staged, unstaged, untracked), + commits=commits, + ) diff --git a/src/pkgmgr/actions/review/diff.py b/src/pkgmgr/actions/review/diff.py new file mode 100644 index 0000000..b58f262 --- /dev/null +++ b/src/pkgmgr/actions/review/diff.py @@ -0,0 +1,154 @@ +from __future__ import annotations + +import re + +from .model import FileDiff, Hunk, Line + +HUNK_HEADER = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") +GIT_HEADER = "diff --git " +ESCAPES = { + "a": 7, + "b": 8, + "t": 9, + "n": 10, + "v": 11, + "f": 12, + "r": 13, + '"': 34, + "\\": 92, +} +HEADER_FIELDS = ( + "rename from ", + "rename to ", + "copy from ", + "copy to ", + "new file mode ", + "deleted file mode ", + "old mode ", + "new mode ", + "--- ", + "+++ ", +) + + +def unquote(text: str) -> str: + if len(text) < 2 or not (text.startswith('"') and text.endswith('"')): + return text + body = text[1:-1] + raw = bytearray() + index = 0 + while index < len(body): + char = body[index] + escaped = body[index + 1 : index + 2] + octal = body[index + 1 : index + 4] + if char == "\\" and escaped in ESCAPES: + raw.append(ESCAPES[escaped]) + index += 2 + elif char == "\\" and len(octal) == 3 and set(octal) <= set("01234567"): + raw.append(int(octal, 8) & 0xFF) + index += 4 + else: + raw.extend(char.encode("utf-8")) + index += 1 + return raw.decode("utf-8", "replace") + + +def _side(value: str, prefix: str) -> str | None: + value = value.removesuffix("\t") + if value == "/dev/null": + return None + return unquote(value).removeprefix(prefix) + + +def _file( + git_line: str, headers: list[str], pending: list[tuple[str, int, list[Line]]] +) -> FileDiff: + fields: dict[str, str] = {} + for header in headers: + for key in HEADER_FIELDS: + if header.startswith(key): + fields[key.strip()] = header[len(key) :] + break + half = (len(git_line) - 1) // 2 + old_path = _side(git_line[:half], "a/") + new_path = _side(git_line[half + 1 :], "b/") + if "---" in fields: + old_path = _side(fields["---"], "a/") + if "+++" in fields: + new_path = _side(fields["+++"], "b/") + status = "modified" + for kind, moved in (("rename", "renamed"), ("copy", "copied")): + if f"{kind} from" in fields and f"{kind} to" in fields: + old_path = unquote(fields[f"{kind} from"]) + new_path = unquote(fields[f"{kind} to"]) + status = moved + if "new file mode" in fields: + old_path, status = None, "added" + if "deleted file mode" in fields: + new_path, status = None, "deleted" + mode_change = None + if "old mode" in fields and "new mode" in fields: + mode_change = f"{fields['old mode']} → {fields['new mode']}" + return FileDiff( + old_path=old_path, + new_path=new_path, + status=status, + binary=any(header.startswith("Binary files ") for header in headers), + mode_change=mode_change, + hunks=tuple( + Hunk(header, start, tuple(lines)) for header, start, lines in pending + ), + ) + + +def parse_patch(text: str) -> list[FileDiff]: + """ + Args: + text: Output of ``git diff`` or ``git show`` produced with ``a/`` and + ``b/`` prefixes and decoded without newline translation. + + Returns: + One FileDiff per ``diff --git`` block, in git's order. + """ + files: list[FileDiff] = [] + git_line: str | None = None + headers: list[str] = [] + pending: list[tuple[str, int, list[Line]]] = [] + old_no = new_no = old_left = new_left = 0 + for raw in text.split("\n"): + if pending and (old_left > 0 or new_left > 0): + lines = pending[-1][2] + sign, body = raw[:1], raw[1:] + if sign == "+": + lines.append(Line("add", None, new_no, body)) + new_no, new_left = new_no + 1, new_left - 1 + elif sign == "-": + lines.append(Line("del", old_no, None, body)) + old_no, old_left = old_no + 1, old_left - 1 + elif sign == "\\": + lines.append(Line("meta", None, None, raw)) + else: + lines.append(Line("ctx", old_no, new_no, body)) + old_no, new_no = old_no + 1, new_no + 1 + old_left, new_left = old_left - 1, new_left - 1 + continue + if raw.startswith(GIT_HEADER): + if git_line is not None: + files.append(_file(git_line, headers, pending)) + git_line, headers, pending = raw[len(GIT_HEADER) :], [], [] + continue + if git_line is None: + continue + match = HUNK_HEADER.match(raw) + if match: + old_no, new_no = int(match[1]), int(match[3]) + old_left = 1 if match[2] is None else int(match[2]) + new_left = 1 if match[4] is None else int(match[4]) + pending.append((raw, new_no, [])) + elif pending and raw.startswith("\\"): + pending[-1][2].append(Line("meta", None, None, raw)) + elif not pending: + headers.append(raw) + if git_line is not None: + files.append(_file(git_line, headers, pending)) + return files diff --git a/src/pkgmgr/actions/review/files.py b/src/pkgmgr/actions/review/files.py new file mode 100644 index 0000000..77b0704 --- /dev/null +++ b/src/pkgmgr/actions/review/files.py @@ -0,0 +1,115 @@ +from __future__ import annotations + +import errno +import os +import stat + +from .model import FileDiff, FileView, Hunk, Line, Note, ReviewError +from .repo import git_text +from .views import BINARY_DETAIL, file_view + +READ_MAX_BYTES = 1024 * 1024 +BINARY_SNIFF_BYTES = 8000 + + +def _listed(path: str, binary: bool) -> FileDiff: + return FileDiff( + old_path=None, + new_path=path, + status="untracked", + binary=binary, + mode_change=None, + hunks=(), + ) + + +def _read_regular(path: str, limit: int) -> tuple[bytes, int] | None: + """ + Args: + path: Absolute path, opened one component at a time without following + a symbolic link anywhere along it. + limit: Most bytes to read. + + Returns: + The bytes read and the file size, or None when a component is a + symbolic link or the target is not a regular file. + """ + names = [name for name in path.split(os.sep) if name not in ("", ".")] + fd = os.open(os.sep, os.O_RDONLY | os.O_DIRECTORY) + try: + for index, name in enumerate(names, 1): + kind = os.O_NONBLOCK if index == len(names) else os.O_DIRECTORY + child = os.open(name, os.O_RDONLY | os.O_NOFOLLOW | kind, dir_fd=fd) + os.close(fd) + fd = child + info = os.fstat(fd) + if not stat.S_ISREG(info.st_mode): + return None + with os.fdopen(os.dup(fd), "rb") as handle: + return handle.read(limit), info.st_size + except OSError as exc: + if exc.errno in (errno.ELOOP, errno.ENOTDIR): + return None + raise + finally: + os.close(fd) + + +def _untracked_file(path: str, top: str) -> tuple[FileDiff, str | None]: + try: + read = _read_regular(os.path.join(top, path), READ_MAX_BYTES + 1) + except OSError as exc: + return _listed(path, False), f"Unreadable: {exc.strerror}." + if read is None: + return _listed(path, False), "Not a regular file, contents not shown." + data, size = read + if len(data) > READ_MAX_BYTES: + return _listed(path, False), f"Large file ({size} bytes), contents not shown." + if b"\0" in data[:BINARY_SNIFF_BYTES]: + return _listed(path, True), BINARY_DETAIL + lines = data.decode("utf-8", "replace").split("\n") + if lines[-1] == "": + lines.pop() + added = tuple( + Line("add", None, number, text) for number, text in enumerate(lines, 1) + ) + hunks = (Hunk(f"@@ -0,0 +1,{len(added)} @@", 1, added),) if added else () + return ( + FileDiff( + old_path=None, + new_path=path, + status="untracked", + binary=False, + mode_change=None, + hunks=hunks, + ), + None, + ) + + +def untracked_views(top: str, limit: int) -> tuple[FileView, ...]: + listing = git_text(["ls-files", "--others", "--exclude-standard", "-z"], top) + views = [] + for index, path in enumerate(sorted(p for p in listing.split("\0") if p), 1): + diff, detail = _untracked_file(path, top) + views.append(file_view(diff, f"untracked-{index}", top, limit, detail)) + return tuple(views) + + +def read_note(path: str, cwd: str) -> Note: + try: + read = _read_regular( + os.path.abspath(os.path.join(cwd, path)), READ_MAX_BYTES + 1 + ) + except OSError as exc: + raise ReviewError(f"cannot read note {path!r}: {exc.strerror}") from None + if read is None: + raise ReviewError( + f"cannot read note {path!r}: a symbolic link or not a regular file" + ) + data, size = read + if len(data) > READ_MAX_BYTES: + raise ReviewError( + f"cannot read note {path!r}: {size} bytes, more than {READ_MAX_BYTES}" + ) + return Note(title=os.path.basename(path), text=data.decode("utf-8", "replace")) diff --git a/src/pkgmgr/actions/review/model.py b/src/pkgmgr/actions/review/model.py new file mode 100644 index 0000000..cbe4aa3 --- /dev/null +++ b/src/pkgmgr/actions/review/model.py @@ -0,0 +1,136 @@ +from __future__ import annotations + +from dataclasses import dataclass + + +class ReviewError(RuntimeError): + pass + + +@dataclass(frozen=True) +class Line: + kind: str + old: int | None + new: int | None + text: str + + +@dataclass(frozen=True) +class Hunk: + header: str + new_start: int + lines: tuple[Line, ...] + + +@dataclass(frozen=True) +class FileDiff: + old_path: str | None + new_path: str | None + status: str + binary: bool + mode_change: str | None + hunks: tuple[Hunk, ...] + + @property + def path(self) -> str: + return self.new_path if self.new_path is not None else str(self.old_path) + + @property + def moved(self) -> bool: + return self.status in ("renamed", "copied") + + @property + def added(self) -> int: + return sum( + 1 for hunk in self.hunks for line in hunk.lines if line.kind == "add" + ) + + @property + def removed(self) -> int: + return sum( + 1 for hunk in self.hunks for line in hunk.lines if line.kind == "del" + ) + + +@dataclass(frozen=True) +class FileView: + anchor: str + diff: FileDiff + detail: str | None + link_base: str | None + link: str | None + hunks: tuple[Hunk, ...] + hidden: int + + @property + def search(self) -> str: + if self.diff.moved: + return f"{self.diff.old_path} {self.diff.new_path}" + return self.diff.path + + +@dataclass(frozen=True) +class Section: + key: str + title: str + files: tuple[FileView, ...] + skipped: str | None + + @property + def added(self) -> int: + return sum(view.diff.added for view in self.files) + + @property + def removed(self) -> int: + return sum(view.diff.removed for view in self.files) + + +@dataclass(frozen=True) +class Commit: + anchor: str + short: str + author: str + subject: str + body: str + files: tuple[FileView, ...] + + @property + def added(self) -> int: + return sum(view.diff.added for view in self.files) + + @property + def removed(self) -> int: + return sum(view.diff.removed for view in self.files) + + +@dataclass(frozen=True) +class Note: + title: str + text: str + + +@dataclass(frozen=True) +class Review: + top_level: str + name: str + branch: str | None + head: str | None + index_tree: str + base: str | None + base_short: str | None + warnings: tuple[str, ...] + notes: tuple[Note, ...] + sections: tuple[Section, ...] + commits: tuple[Commit, ...] + + @property + def commit_files(self) -> int: + return sum(len(commit.files) for commit in self.commits) + + @property + def commit_added(self) -> int: + return sum(commit.added for commit in self.commits) + + @property + def commit_removed(self) -> int: + return sum(commit.removed for commit in self.commits) diff --git a/src/pkgmgr/actions/review/render.py b/src/pkgmgr/actions/review/render.py new file mode 100644 index 0000000..9c4ee70 --- /dev/null +++ b/src/pkgmgr/actions/review/render.py @@ -0,0 +1,86 @@ +from __future__ import annotations + +import base64 +import hashlib +import os +import tempfile + +from jinja2 import Environment, FileSystemLoader, StrictUndefined + +from .collect import collect_review +from .model import Review, ReviewError + +TEMPLATE = "review.html.j2" +INVISIBLE = { + code: f"" + for code in ( + *range(0x200B, 0x2010), + *range(0x202A, 0x202F), + *range(0x2066, 0x206A), + 0x061C, + 0xFEFF, + ) +} + + +def _csp_hash(text: str) -> str: + digest = hashlib.sha256(str(text).encode("utf-8")).digest() + return "sha256-" + base64.b64encode(digest).decode("ascii") + + +def _visible(value: object) -> object: + # not isinstance: Markup.translate() would splice the marks in unescaped, + # and macro output is Markup whose text was already marked when built + return value.translate(INVISIBLE) if type(value) is str else value + + +def render_review(review: Review) -> str: + environment = Environment( + loader=FileSystemLoader(os.path.dirname(os.path.abspath(__file__))), + autoescape=True, + undefined=StrictUndefined, + trim_blocks=True, + lstrip_blocks=True, + keep_trailing_newline=True, + finalize=_visible, + ) + environment.filters["csp_hash"] = _csp_hash + return environment.get_template(TEMPLATE).render(review=review) + + +def write_review( + cwd: str, out: str, max_file_lines: int, base: str | None, notes: list[str] +) -> str: + """ + Args: + cwd: Directory inside the git working copy to review. + out: Output HTML path, relative to ``cwd`` unless absolute; written as + a new mode 0600 file renamed onto it, so a symbolic link there is + replaced, never followed. + max_file_lines: Diff lines shown per file before the rest is summarised. + base: Branch whose ``REF..HEAD`` commits are listed, or None. + notes: Text files shown verbatim at the top of the page. + + Returns: + The absolute path of the written page. + """ + html = render_review( + collect_review(cwd=cwd, max_file_lines=max_file_lines, base=base, notes=notes) + ) + path = os.path.abspath(os.path.join(cwd, out)) + directory = os.path.dirname(path) + try: + os.makedirs(directory, exist_ok=True) + fd, temp = tempfile.mkstemp(prefix=".review-", suffix=".tmp", dir=directory) + try: + with os.fdopen(fd, "w", encoding="utf-8", newline="\n") as handle: + handle.write(html) + handle.flush() + os.fsync(handle.fileno()) + os.replace(temp, path) + except OSError: + os.unlink(temp) + raise + except OSError as exc: + raise ReviewError(f"cannot write {path!r}: {exc.strerror}") from None + return path diff --git a/src/pkgmgr/actions/review/repo.py b/src/pkgmgr/actions/review/repo.py new file mode 100644 index 0000000..2ae53ff --- /dev/null +++ b/src/pkgmgr/actions/review/repo.py @@ -0,0 +1,74 @@ +from __future__ import annotations + +import subprocess + +from pkgmgr.actions.branch.gc import END_OF_OPTIONS +from pkgmgr.core.git.errors import GitBaseError +from pkgmgr.core.git.run import SCAN_FLAGS, SCAN_TIMEOUT, _scan_env, run + +from .model import ReviewError + +BRANCH_NAMESPACES = ("refs/heads/", "refs/remotes/") + + +def git_text(args: list[str], cwd: str) -> str: + try: + result = subprocess.run( + ["git", *SCAN_FLAGS, "-c", "core.quotepath=false", *args], + cwd=cwd, + env=_scan_env(), + stdin=subprocess.DEVNULL, + capture_output=True, + timeout=SCAN_TIMEOUT, + check=False, + ) + except subprocess.TimeoutExpired: + raise ReviewError(f"git {args[0]} timed out") from None + if result.returncode != 0: + stderr = result.stderr.decode("utf-8", "replace").strip().splitlines() + reason = stderr[0] if stderr else f"exit code {result.returncode}" + raise ReviewError(f"git {args[0]} failed: {reason}") + return result.stdout.decode("utf-8", "replace") + + +def git_optional(args: list[str], cwd: str) -> str | None: + try: + return run(args, cwd=cwd, scan=True) or None + except GitBaseError: + return None + + +def top_level(cwd: str) -> str: + top = git_optional(["rev-parse", "--show-toplevel"], cwd) + if top is None: + raise ReviewError(f"not inside a git working copy: {cwd!r}") + return top + + +def base_ref(base: str, top: str) -> tuple[str, str]: + names = ( + [base] + if base.startswith(BRANCH_NAMESPACES) + else [namespace + base for namespace in BRANCH_NAMESPACES] + ) + refs: dict[str, str] = {} + for name in names: + sha = git_optional( + ["show-ref", "--verify", "--hash", END_OF_OPTIONS, name], top + ) + if sha is not None: + refs[name] = sha + if len(refs) > 1: + raise ReviewError(f"ambiguous base ref: {base!r} names {' and '.join(refs)}") + if not refs: + raise ReviewError( + f"unknown base ref: {base!r}, expected a branch under " + f"{' or '.join(BRANCH_NAMESPACES)}" + ) + [(name, sha)] = refs.items() + commit = git_optional( + ["rev-parse", "--verify", "--quiet", END_OF_OPTIONS, f"{sha}^{{commit}}"], top + ) + if commit is None: + raise ReviewError(f"base ref {name!r} does not point to a commit") + return name, commit diff --git a/src/pkgmgr/actions/review/review.html.j2 b/src/pkgmgr/actions/review/review.html.j2 new file mode 100644 index 0000000..7026ec2 --- /dev/null +++ b/src/pkgmgr/actions/review/review.html.j2 @@ -0,0 +1,347 @@ +{% set status_letters = {"added": "A", "deleted": "D", "modified": "M", "renamed": "R", "copied": "C", "untracked": "U"} %} +{% set signs = {"add": "+", "del": "-", "ctx": "", "meta": ""} %} +{% set style %} +:root { + color-scheme: light dark; + --bg: #f6f8fa; + --panel: #ffffff; + --subtle: #f6f8fa; + --border: #d1d9e0; + --border-muted: #e1e6eb; + --text: #1f2328; + --muted: #59636e; + --accent: #0969da; + --accent-bg: #ddf4ff; + --add-fg: #1a7f37; + --add-bg: #dafbe1; + --add-line: #e6ffec; + --add-num: #ccffd8; + --del-fg: #cf222e; + --del-bg: #ffebe9; + --del-line: #ffebe9; + --del-num: #ffd7d5; + --hunk-bg: #ddf4ff; + --hunk-fg: #59636e; + --warn-fg: #9a6700; + --warn-bg: #fff8c5; + --shadow: 0 1px 2px rgba(31, 35, 40, 0.06); + --mono: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, "Liberation Mono", monospace; + --sans: system-ui, -apple-system, "Segoe UI", "Noto Sans", Helvetica, Arial, sans-serif; +} +@media (prefers-color-scheme: dark) { + :root { + --bg: #0d1117; + --panel: #151b23; + --subtle: #1b222c; + --border: #3d444d; + --border-muted: #2a313c; + --text: #e6edf3; + --muted: #9198a1; + --accent: #4493f8; + --accent-bg: rgba(56, 139, 253, 0.15); + --add-fg: #3fb950; + --add-bg: rgba(46, 160, 67, 0.2); + --add-line: rgba(46, 160, 67, 0.15); + --add-num: rgba(63, 185, 80, 0.3); + --del-fg: #f85149; + --del-bg: rgba(248, 81, 73, 0.2); + --del-line: rgba(248, 81, 73, 0.1); + --del-num: rgba(248, 81, 73, 0.3); + --hunk-bg: rgba(56, 139, 253, 0.1); + --hunk-fg: #9198a1; + --warn-fg: #d29922; + --warn-bg: rgba(187, 128, 9, 0.15); + --shadow: none; + } +} +*, *::before, *::after { box-sizing: border-box; } +[hidden] { display: none !important; } +html { scroll-padding-top: 64px; } +body { margin: 0; background: var(--bg); color: var(--text); font: 14px/1.5 var(--sans); } +a { color: var(--accent); text-decoration: none; } +a:hover { text-decoration: underline; } +pre, code { font-family: var(--mono); } +.muted { color: var(--muted); font-weight: 400; } +.masthead { background: var(--panel); border-bottom: 1px solid var(--border); } +.masthead-inner, .toolbar-inner, main { max-width: 1280px; margin: 0 auto; padding-left: 32px; padding-right: 32px; } +.masthead-inner { padding-top: 24px; padding-bottom: 20px; } +.eyebrow { margin: 0 0 4px; color: var(--muted); font-size: 12px; font-weight: 600; letter-spacing: 0.06em; text-transform: uppercase; } +h1 { display: flex; flex-wrap: wrap; align-items: center; gap: 10px; margin: 0; font-size: 22px; font-weight: 600; } +.pill { padding: 2px 10px; border-radius: 999px; background: var(--accent-bg); color: var(--accent); font: 500 12px/20px var(--mono); } +.facts { display: grid; grid-template-columns: max-content 1fr; gap: 4px 20px; margin: 16px 0 0; font-size: 13px; } +.facts dt { color: var(--muted); } +.facts dd { margin: 0; font-family: var(--mono); overflow-wrap: anywhere; } +.warnings, .skipped { border: 1px solid var(--warn-fg); border-radius: 8px; background: var(--warn-bg); color: var(--warn-fg); font-size: 13px; overflow-wrap: anywhere; } +.warnings { margin: 16px 0 0; padding: 10px 14px 10px 34px; } +.skipped { margin: 0; padding: 14px 16px; border-style: dashed; } +.totals { display: flex; flex-wrap: wrap; gap: 10px; margin-top: 18px; } +.total { display: flex; flex-direction: column; gap: 2px; min-width: 160px; padding: 10px 14px; border: 1px solid var(--border); border-radius: 8px; background: var(--subtle); color: var(--text); } +.total:hover { border-color: var(--accent); text-decoration: none; } +.total .label { color: var(--muted); font-size: 12px; font-weight: 600; letter-spacing: 0.04em; text-transform: uppercase; } +.total .files { font-size: 15px; font-weight: 600; } +.stat { display: inline-flex; gap: 6px; font: 600 12px var(--mono); white-space: nowrap; } +.stat .add { color: var(--add-fg); } +.stat .del { color: var(--del-fg); } +.toolbar { position: sticky; top: 0; z-index: 10; border-bottom: 1px solid var(--border); background: var(--panel); background: color-mix(in srgb, var(--panel) 86%, transparent); backdrop-filter: saturate(180%) blur(10px); box-shadow: var(--shadow); } +.toolbar-inner { display: flex; flex-wrap: wrap; align-items: center; gap: 8px; padding-top: 8px; padding-bottom: 8px; } +.jumps { display: flex; flex-wrap: wrap; gap: 2px; } +.jumps a { display: inline-flex; align-items: center; gap: 6px; padding: 4px 10px; border-radius: 6px; color: var(--text); font-weight: 500; } +.jumps a:hover { background: var(--accent-bg); text-decoration: none; } +.count { min-width: 20px; padding: 0 6px; border: 1px solid var(--border-muted); border-radius: 999px; background: var(--subtle); color: var(--muted); font-size: 12px; line-height: 18px; text-align: center; } +.spacer { flex: 1; } +input[type="search"] { width: 260px; max-width: 100%; padding: 5px 10px; border: 1px solid var(--border); border-radius: 6px; background: var(--bg); color: var(--text); font: 13px var(--mono); } +input[type="search"]:focus { outline: 2px solid var(--accent); outline-offset: -1px; } +button { padding: 5px 12px; border: 1px solid var(--border); border-radius: 6px; background: var(--subtle); color: var(--text); font: 500 13px var(--sans); cursor: pointer; } +button:hover { border-color: var(--muted); } +main { padding-top: 24px; padding-bottom: 80px; } +.group { margin-bottom: 36px; } +h2 { display: flex; flex-wrap: wrap; align-items: baseline; gap: 12px; margin: 0 0 12px; font-size: 17px; font-weight: 600; } +.note { margin: 0 0 14px; border: 1px solid var(--border); border-radius: 8px; background: var(--panel); overflow: hidden; } +.note figcaption { padding: 8px 14px; border-bottom: 1px solid var(--border); background: var(--subtle); font: 600 13px var(--mono); } +.note pre, .commit-body { margin: 0; padding: 12px 14px; font-size: 13px; line-height: 1.55; white-space: pre-wrap; overflow-wrap: anywhere; } +details.file, details.commit { margin-bottom: 12px; border: 1px solid var(--border); border-radius: 8px; background: var(--panel); box-shadow: var(--shadow); overflow: hidden; } +summary { display: flex; align-items: center; gap: 10px; padding: 8px 12px; cursor: pointer; list-style: none; } +summary::-webkit-details-marker { display: none; } +summary::before { content: ""; flex: none; width: 6px; height: 6px; margin: 0 4px 0 2px; border-right: 2px solid var(--muted); border-bottom: 2px solid var(--muted); transform: rotate(-45deg); transition: transform 0.12s ease; } +details[open] > summary::before { transform: rotate(45deg); } +details.file > summary { background: var(--subtle); font: 13px var(--mono); } +details.file[open] > summary, details.commit[open] > summary { border-bottom: 1px solid var(--border); } +summary:hover { background: var(--accent-bg); } +.badge { flex: none; width: 20px; height: 20px; border-radius: 5px; font: 700 11px/20px var(--sans); text-align: center; } +.badge.added, .badge.untracked { background: var(--add-bg); color: var(--add-fg); } +.badge.deleted { background: var(--del-bg); color: var(--del-fg); } +.badge.modified { background: var(--accent-bg); color: var(--accent); } +.badge.renamed, .badge.copied { background: var(--warn-bg); color: var(--warn-fg); } +.path { flex: 1; min-width: 0; font-weight: 600; overflow-wrap: anywhere; } +.path .old, .path .arrow { color: var(--muted); font-weight: 400; } +.tag { flex: none; padding: 0 8px; border: 1px solid var(--border); border-radius: 999px; color: var(--muted); font: 500 11px/18px var(--sans); } +.open { flex: none; font: 500 12px var(--sans); } +.placeholder, .empty { margin: 0; padding: 14px 16px; color: var(--muted); font-style: italic; } +.empty { border: 1px dashed var(--border); border-radius: 8px; } +table.diff { width: 100%; border-collapse: collapse; font: 12.5px/20px var(--mono); tab-size: 4; } +.diff td { height: 20px; padding: 0; vertical-align: top; } +td.num { width: 1%; min-width: 50px; padding: 0 10px; color: var(--muted); text-align: right; white-space: nowrap; user-select: none; } +td.num a { color: inherit; } +td.num a:hover { color: var(--accent); } +td.sign { width: 20px; padding-left: 10px; color: var(--muted); user-select: none; } +td.code { padding-right: 16px; white-space: pre-wrap; overflow-wrap: anywhere; } +tr.add td { background: var(--add-line); } +tr.add td.num { background: var(--add-num); } +tr.add td.sign { color: var(--add-fg); } +tr.del td { background: var(--del-line); } +tr.del td.num { background: var(--del-num); } +tr.del td.sign { color: var(--del-fg); } +tr.hunk td { padding-top: 4px; padding-bottom: 4px; background: var(--hunk-bg); color: var(--hunk-fg); } +tr.meta td.code { color: var(--muted); font-style: italic; } +tr.more td { padding: 8px 16px; border-top: 1px dashed var(--border); background: var(--subtle); color: var(--muted); font: italic 12px var(--sans); text-align: center; } +details.commit > summary { padding: 10px 14px; } +.sha { flex: none; padding: 1px 8px; border: 1px solid var(--border-muted); border-radius: 6px; background: var(--subtle); font-size: 12px; } +.subject { flex: 1; min-width: 0; font-weight: 600; overflow-wrap: anywhere; } +.commit-meta { padding: 10px 14px 0; color: var(--muted); font-size: 13px; } +.commit-files { padding: 12px 14px 2px; } +@media (max-width: 720px) { + .masthead-inner, .toolbar-inner, main { padding-left: 14px; padding-right: 14px; } + input[type="search"] { width: 100%; } + .spacer { display: none; } +} +{% endset %} +{% set script %} +(function () { + "use strict"; + var files = Array.prototype.slice.call(document.querySelectorAll("details.file")); + var filter = document.getElementById("filter"); + var counter = document.getElementById("filter-count"); + function setOpen(open) { + document.querySelectorAll("details").forEach(function (item) { + item.open = open; + }); + } + function applyFilter() { + var query = filter.value.trim().toLowerCase(); + var shown = 0; + files.forEach(function (item) { + var match = query === "" || item.dataset.path.toLowerCase().indexOf(query) !== -1; + item.hidden = !match; + shown += match ? 1 : 0; + }); + counter.textContent = query === "" ? "" : shown + " of " + files.length + " files"; + } + document.getElementById("expand-all").addEventListener("click", function () { + setOpen(true); + }); + document.getElementById("collapse-all").addEventListener("click", function () { + setOpen(false); + }); + filter.addEventListener("input", applyFilter); + document.addEventListener("keydown", function (event) { + if (event.key === "/" && document.activeElement !== filter) { + event.preventDefault(); + filter.focus(); + } else if (event.key === "Escape" && document.activeElement === filter) { + filter.value = ""; + applyFilter(); + filter.blur(); + } + }); + applyFilter(); +})(); +{% endset %} +{% macro stat(added, removed) %}+{{ added }}−{{ removed }}{% endmacro %} +{% macro plural(count, word) %}{{ count }} {{ word }}{{ "" if count == 1 else "s" }}{% endmacro %} +{% macro file_block(file, open) %} +
+ +{{ status_letters[file.diff.status] }} +{% if file.diff.moved %}{{ file.diff.old_path }} → {% endif %}{{ file.diff.path }} +{% if file.diff.mode_change %} +mode {{ file.diff.mode_change }} +{% endif %} +{% if file.diff.binary %} +binary +{% endif %} +{{ stat(file.diff.added, file.diff.removed) }} +{% if file.link %} +Open ↗ +{% endif %} + +{% if file.detail %} +

{{ file.detail }}

+{% elif not file.hunks %} +

No content changes.

+{% else %} + +{% for hunk in file.hunks %} + + +{% for line in hunk.lines %} + +{% endfor %} + +{% endfor %} +{% if file.hidden %} + +{% endif %} +
{{ hunk.header }}
{{ "" if line.old is none else line.old }}{% if line.new is not none and file.link_base %}{{ line.new }}{% elif line.new is not none %}{{ line.new }}{% endif %}{{ signs[line.kind] }}{{ line.text }}
… {{ plural(file.hidden, "more line") }} not shown
+{% endif %} +
+{% endmacro %} + + + + + + + +Review: {{ review.name }}{% if review.branch %} ({{ review.branch }}){% endif %} + + + +
+
+

Working copy review

+

{{ review.name }} {{ review.branch or "detached HEAD" }}

+
+
Top level
{{ review.top_level }}
+
Branch
{{ review.branch or "(detached HEAD)" }}
+
HEAD
{{ review.head or "(no commits yet)" }}
+
Index tree
{{ review.index_tree }}
+{% if review.base is not none %} +
Base
{{ review.base }} @ {{ review.base_short }}
+{% endif %} +
+{% if review.warnings %} +
    +{% for warning in review.warnings %} +
  • {{ warning }}
    • +{% endfor %} +
+{% endif %} +
+{% for section in review.sections %} +{{ section.title }}{{ plural(section.files|length, "file") }}{{ stat(section.added, section.removed) }} +{% endfor %} +{% if review.base is not none %} +Commits{{ plural(review.commits|length, "commit") }} · {{ plural(review.commit_files, "file") }}{{ stat(review.commit_added, review.commit_removed) }} +{% endif %} +
+
+
+ +
+{% if review.notes %} +
+

Notes

+{% for note in review.notes %} +
+
{{ note.title }}
+
+{{ note.text }}
+
+{% endfor %} +
+{% endif %} +{% for section in review.sections %} +
+

{{ section.title }} {{ plural(section.files|length, "file") }}{{ stat(section.added, section.removed) }}

+{% if section.skipped %} +

{{ section.skipped }}

+{% else %} +{% for file in section.files %} +{{ file_block(file, true) }} +{% else %} +

No changes.

+{% endfor %} +{% endif %} +
+{% endfor %} +{% if review.base is not none %} +
+

Commits {{ review.base }}..HEAD · {{ plural(review.commits|length, "commit") }}{{ stat(review.commit_added, review.commit_removed) }}

+{% for commit in review.commits %} +
+ +{{ commit.short }} +{{ commit.subject }} +{{ stat(commit.added, commit.removed) }} + +
{{ commit.author }}
+{% if commit.body %} +
+{{ commit.body }}
+{% endif %} +
+{% for file in commit.files %} +{{ file_block(file, false) }} +{% else %} +

No file changes.

+{% endfor %} +
+
+{% else %} +

No commits between {{ review.base }} and HEAD.

+{% endfor %} +
+{% endif %} +
+ + + diff --git a/src/pkgmgr/actions/review/views.py b/src/pkgmgr/actions/review/views.py new file mode 100644 index 0000000..ff4de92 --- /dev/null +++ b/src/pkgmgr/actions/review/views.py @@ -0,0 +1,67 @@ +from __future__ import annotations + +import os +from urllib.parse import quote + +from .model import FileDiff, FileView, Hunk + +BINARY_DETAIL = "Binary file, contents not shown." + + +def _first_changed_line(hunks: tuple[Hunk, ...]) -> int: + for hunk in hunks: + position = hunk.new_start + for line in hunk.lines: + if line.kind in ("add", "del"): + return max(position, 1) + if line.new is not None: + position = line.new + 1 + return 1 + + +def _truncate(hunks: tuple[Hunk, ...], limit: int) -> tuple[tuple[Hunk, ...], int]: + shown: list[Hunk] = [] + budget = limit + for hunk in hunks: + if budget <= 0: + break + lines = hunk.lines[:budget] + shown.append(Hunk(hunk.header, hunk.new_start, lines)) + budget -= len(lines) + total = sum(len(hunk.lines) for hunk in hunks) + return tuple(shown), total - (limit - budget) + + +def file_view( + diff: FileDiff, anchor: str, top: str, limit: int, detail: str | None +) -> FileView: + link_base = None + if diff.new_path is not None: + link_base = "vscode://file" + quote(os.path.join(top, diff.new_path)) + hunks, hidden = _truncate(diff.hunks, limit) + return FileView( + anchor=anchor, + diff=diff, + detail=detail, + link_base=link_base, + link=None + if link_base is None + else f"{link_base}:{_first_changed_line(diff.hunks)}", + hunks=hunks, + hidden=hidden, + ) + + +def file_views( + diffs: list[FileDiff], prefix: str, top: str, limit: int +) -> tuple[FileView, ...]: + return tuple( + file_view( + diff, + f"{prefix}-{index}", + top, + limit, + BINARY_DETAIL if diff.binary else None, + ) + for index, diff in enumerate(diffs, 1) + ) diff --git a/src/pkgmgr/cli/commands/__init__.py b/src/pkgmgr/cli/commands/__init__.py index ce6fb90..51357cf 100644 --- a/src/pkgmgr/cli/commands/__init__.py +++ b/src/pkgmgr/cli/commands/__init__.py @@ -8,6 +8,7 @@ from .mirror import handle_mirror_command from .publish import handle_publish from .release import handle_release from .repos import handle_repos_command +from .review import handle_review from .tools import handle_tools_command from .version import handle_version @@ -22,6 +23,7 @@ __all__ = [ "handle_publish", "handle_release", "handle_repos_command", + "handle_review", "handle_tools_command", "handle_version", ] diff --git a/src/pkgmgr/cli/commands/review.py b/src/pkgmgr/cli/commands/review.py new file mode 100644 index 0000000..ee14d05 --- /dev/null +++ b/src/pkgmgr/cli/commands/review.py @@ -0,0 +1,23 @@ +from __future__ import annotations + +import os +import sys +from pathlib import Path + +from pkgmgr.actions.review import ReviewError, write_review +from pkgmgr.cli.context import CLIContext + + +def handle_review(args, ctx: CLIContext) -> None: + try: + path = write_review( + cwd=os.getcwd(), + out=args.out, + max_file_lines=args.max_file_lines, + base=args.base, + notes=args.note, + ) + except ReviewError as exc: + print(f"[ERROR] {exc}", file=sys.stderr) + sys.exit(2) + print(Path(path).as_uri()) diff --git a/src/pkgmgr/cli/dispatch.py b/src/pkgmgr/cli/dispatch.py index 654e896..55914c3 100644 --- a/src/pkgmgr/cli/dispatch.py +++ b/src/pkgmgr/cli/dispatch.py @@ -15,6 +15,7 @@ from pkgmgr.cli.commands import ( handle_publish, handle_release, handle_repos_command, + handle_review, handle_tools_command, handle_version, ) @@ -69,6 +70,10 @@ def dispatch_command(args, ctx: CLIContext) -> None: handle_code_scanning(args, ctx) return + if args.command == "review": + handle_review(args, ctx) + return + commands_with_selection = { "install", "update", diff --git a/src/pkgmgr/cli/parser/__init__.py b/src/pkgmgr/cli/parser/__init__.py index 4f366ee..93f627f 100644 --- a/src/pkgmgr/cli/parser/__init__.py +++ b/src/pkgmgr/cli/parser/__init__.py @@ -17,6 +17,7 @@ from .mirror_cmd import add_mirror_subparsers from .navigation_cmd import add_navigation_subparsers from .publish_cmd import add_publish_subparser from .release_cmd import add_release_subparser +from .review_cmd import add_review_subparser from .version_cmd import add_version_subparser @@ -58,6 +59,7 @@ def create_parser(description_text: str) -> argparse.ArgumentParser: add_navigation_subparsers(subparsers) add_branch_subparsers(subparsers) + add_review_subparser(subparsers) add_release_subparser(subparsers) add_publish_subparser(subparsers) diff --git a/src/pkgmgr/cli/parser/review_cmd.py b/src/pkgmgr/cli/parser/review_cmd.py new file mode 100644 index 0000000..4500ae2 --- /dev/null +++ b/src/pkgmgr/cli/parser/review_cmd.py @@ -0,0 +1,47 @@ +from __future__ import annotations + +import argparse + + +def _positive_int(value: str) -> int: + number = int(value) + if number < 1: + raise argparse.ArgumentTypeError(f"must be a positive integer, got {value}") + return number + + +def add_review_subparser(subparsers: argparse._SubParsersAction) -> None: + parser = subparsers.add_parser( + "review", + help="Render the working copy's changes as a local HTML review page.", + description=( + "Write a self-contained HTML page showing the staged, unstaged and " + "untracked changes of the current git working copy (plus the commits " + "of REF..HEAD with --base) and print its file:// URL." + ), + ) + parser.add_argument( + "--out", + required=True, + metavar="FILE", + help="Write the HTML page to FILE; parent directories are created.", + ) + parser.add_argument( + "--max-file-lines", + required=True, + type=_positive_int, + metavar="N", + help="Show at most N diff lines per file and summarise the rest.", + ) + parser.add_argument( + "--base", + metavar="REF", + help="Add a Commits section for REF..HEAD; omitted means no Commits section.", + ) + parser.add_argument( + "--note", + action="append", + default=[], + metavar="FILE", + help="Show FILE verbatim in a panel at the top; repeatable.", + ) diff --git a/src/pkgmgr/core/git/run.py b/src/pkgmgr/core/git/run.py index 9fef59d..c700c4d 100644 --- a/src/pkgmgr/core/git/run.py +++ b/src/pkgmgr/core/git/run.py @@ -22,6 +22,7 @@ SCAN_FLAGS = ( SCAN_ENV = { "GIT_CONFIG_NOSYSTEM": "1", "GIT_NO_LAZY_FETCH": "1", + "GIT_NO_REPLACE_OBJECTS": "1", "GIT_OPTIONAL_LOCKS": "0", "GIT_TERMINAL_PROMPT": "0", "GIT_ASKPASS": "/bin/false", @@ -63,7 +64,8 @@ def run( cwd: directory to run in. preview: print the command instead of running it. scan: read a repository an agent may have written: no fsmonitor, hook, - pager, lazy fetch, system config or prompt, stdin closed, a timeout. + pager, lazy fetch, replace object, system config or prompt, stdin + closed, a timeout. Raises GitRunError (or a subclass) if execution fails or times out. """ diff --git a/tests/integration/test_review.py b/tests/integration/test_review.py new file mode 100644 index 0000000..2d436ff --- /dev/null +++ b/tests/integration/test_review.py @@ -0,0 +1,237 @@ +from __future__ import annotations + +import io +import os +import subprocess +import tempfile +import unittest +from contextlib import redirect_stderr, redirect_stdout +from pathlib import Path +from unittest.mock import patch +from urllib.parse import quote + +from pkgmgr.actions.review import ReviewError, write_review +from pkgmgr.cli.commands.review import handle_review +from pkgmgr.cli.parser import create_parser + +GIT_ENV = { + **os.environ, + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@example.org", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@example.org", +} +COMMIT_MESSAGE = ( + 'feat: \n\nBody ' +) + + +def git(cwd: str, *args: str) -> str: + return subprocess.run( + ["git", "-c", "commit.gpgsign=false", *args], + cwd=cwd, + env=GIT_ENV, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +def write(path: str, content: bytes) -> None: + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "wb") as handle: + handle.write(content) + + +class TestReview(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.root = os.path.realpath(self._tmp.name) + self.repo = os.path.join(self.root, "my repo") + git(self.root, "init", "-b", "main", self.repo) + app = "".join(f"line {n}\n" for n in range(1, 21)) + write(self._path("app.py"), app.encode()) + write(self._path("notes.md"), b"# Notes\n") + git(self.repo, "add", "-A") + git(self.repo, "commit", "-m", "init") + + git(self.repo, "checkout", "-b", "feature") + write(self._path("notes.md"), b"# Notes\nfeature\n") + git(self.repo, "commit", "-am", COMMIT_MESSAGE) + + write(self._path("app.py"), app.replace("line 5\n", "line five\n").encode()) + big = "".join(f"big {n}\n" for n in range(1, 31)) + write(self._path("dir with space", "big.txt"), big.encode()) + write(self._path("image.png"), b"\x89PNG\x00\x01\x02") + git(self.repo, "mv", "notes.md", "renamed.md") + git(self.repo, "add", "-A") + + unstaged = app.replace("line 5\n", "line five\n") + write( + self._path("app.py"), unstaged.replace("line 15\n", "line 15!\n").encode() + ) + + write( + self._path("todo.txt"), + b"first\n\nlast\n", + ) + write(self._path("blob.bin"), b"\x00\xffdata") + + self.note = os.path.join(self.root, "score.txt") + write(self.note, b"score 90/5\n") + + def tearDown(self) -> None: + self._tmp.cleanup() + + def _path(self, *parts: str) -> str: + return os.path.join(self.repo, *parts) + + def _render(self, out: str, base: str | None, notes: list[str]) -> str: + path = write_review( + cwd=self.repo, + out=os.path.join(self.root, "out", out), + max_file_lines=10, + base=base, + notes=notes, + ) + return Path(path).read_text(encoding="utf-8") + + def _link(self, rel: str, line: int) -> str: + return f"vscode://file{quote(os.path.join(self.repo, rel))}:{line}" + + def test_sections_and_counts(self) -> None: + html = self._render("page.html", None, []) + + self.assertIn( + 'id="staged" data-files="4" data-added="31" data-removed="1"', html + ) + self.assertIn( + 'id="unstaged" data-files="1" data-added="1" data-removed="1"', html + ) + self.assertIn( + 'id="untracked" data-files="2" data-added="3" data-removed="0"', html + ) + self.assertNotIn('id="commits"', html) + self.assertIn( + 'notes.md → renamed.md', html + ) + self.assertIn('binary', html) + self.assertIn('data-path="blob.bin"', html) + self.assertIn("Binary file, contents not shown.", html) + self.assertIn(git(self.repo, "write-tree"), html) + self.assertIn(git(self.repo, "rev-parse", "--short", "HEAD"), html) + + def test_base_adds_commits_section(self) -> None: + html = self._render("page.html", "main", []) + + self.assertIn( + 'id="commits" data-commits="1" data-files="1" data-added="1" ' + 'data-removed="0"', + html, + ) + self.assertIn("t <t@example.org>", html) + self.assertIn(git(self.repo, "rev-parse", "--short", "feature"), html) + + def test_untrusted_text_is_escaped(self) -> None: + html = self._render("page.html", "main", [self.note]) + + self.assertEqual(html.count("alert", html) + self.assertIn("<script>alert('file')</script>", html) + self.assertIn("feat: <script>alert("subject")", html) + self.assertIn("Body <script>alert("body")", html) + self.assertIn("
score.txt
", html) + self.assertIn("<b>score</b> 90/5", html) + + def test_rendering_is_deterministic(self) -> None: + first = self._render("one.html", "main", [self.note]) + second = self._render("two.html", "main", [self.note]) + + self.assertEqual(first.encode("utf-8"), second.encode("utf-8")) + + def test_vscode_links_and_truncation(self) -> None: + html = self._render("page.html", None, []) + + self.assertIn(f'class="open" href="{self._link("app.py", 5)}"', html) + self.assertIn(f'class="open" href="{self._link("app.py", 15)}"', html) + self.assertIn(f'5', html) + self.assertIn("my%20repo/dir%20with%20space/big.txt:1", html) + self.assertIn("big 10", html) + self.assertNotIn("big 11", html) + self.assertIn("20 more lines not shown", html) + + def test_errors_exit_two(self) -> None: + plain = os.path.join(self.root, "plain") + os.makedirs(plain) + with ( + patch.dict(os.environ, {"GIT_CEILING_DIRECTORIES": self.root}), + self.assertRaisesRegex(ReviewError, "not inside a git working copy"), + ): + write_review(cwd=plain, out="x.html", max_file_lines=5, base=None, notes=[]) + with self.assertRaisesRegex(ReviewError, "unknown base ref"): + write_review( + cwd=self.repo, + out="x.html", + max_file_lines=5, + base="no-such-ref", + notes=[], + ) + args = create_parser("test").parse_args( + ["review", "--out", "x.html", "--max-file-lines", "5", "--base", "nope"] + ) + stderr = io.StringIO() + with ( + patch("pkgmgr.cli.commands.review.os.getcwd", return_value=self.repo), + redirect_stderr(stderr), + self.assertRaises(SystemExit) as exit_ctx, + ): + handle_review(args, ctx=None) + self.assertEqual(exit_ctx.exception.code, 2) + self.assertEqual(len(stderr.getvalue().splitlines()), 1) + self.assertFalse(os.path.exists(self._path("x.html"))) + + def test_cli_forwards_every_option(self) -> None: + args = create_parser("test").parse_args( + [ + "review", + "--out", + "page.html", + "--max-file-lines", + "50", + "--base", + "origin/main", + "--note", + "score.txt", + "--note", + "message.txt", + ] + ) + target = os.path.join(self.root, "page.html") + stdout = io.StringIO() + with ( + patch("pkgmgr.cli.commands.review.os.getcwd", return_value=self.repo), + patch( + "pkgmgr.cli.commands.review.write_review", return_value=target + ) as run_review, + redirect_stdout(stdout), + ): + handle_review(args, ctx=None) + + run_review.assert_called_once_with( + cwd=self.repo, + out="page.html", + max_file_lines=50, + base="origin/main", + notes=["score.txt", "message.txt"], + ) + self.assertEqual(stdout.getvalue(), f"file://{target}\n") + + bare = create_parser("test").parse_args( + ["review", "--out", "p.html", "--max-file-lines", "3"] + ) + self.assertIsNone(bare.base) + self.assertEqual(bare.note, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/integration/test_review_concealment.py b/tests/integration/test_review_concealment.py new file mode 100644 index 0000000..d30a9da --- /dev/null +++ b/tests/integration/test_review_concealment.py @@ -0,0 +1,148 @@ +from __future__ import annotations + +import os +import unittest + +from pkgmgr.actions.review import ReviewError +from tests.integration.test_review import git, write +from tests.integration.test_review_hardening import MODULE, ReviewRepoCase + +INVISIBLE = "".join( + map( + chr, + ( + *range(0x202A, 0x202F), + *range(0x2066, 0x206A), + *range(0x200B, 0x2010), + 0x061C, + 0xFEFF, + ), + ) +) +RLO, LRI, ZWSP, BOM = map(chr, (0x202E, 0x2066, 0x200B, 0xFEFF)) +HIDDEN = "
  • Index entry {} is marked {}: its worktree changes are not shown.
    • " +ATTRIBUTE_BINARY = ( + "{} is shown as binary because {}, not because of its content: " + "run git diff --text to see it." +) +UNSET = "a gitattributes entry unsets its diff attribute" +DRIVER = "its gitattributes diff driver {0} sets diff.{0}.binary" + + +def marks(text: str) -> str: + return "".join(f"<U+{ord(char):04X}>" for char in text) + + +class TestReviewConcealment(ReviewRepoCase): + def test_replace_objects_cannot_mask_the_index(self) -> None: + evil = self._object("blob", "print('evil')\n") + benign = self._object("blob", "print('staged')\n") + git(self.repo, "update-index", "--cacheinfo", f"100644,{evil},app.py") + git(self.repo, "replace", evil, benign) + git(self.repo, "update-index", "--skip-worktree", "app.py") + git(self.repo, "update-index", "--assume-unchanged", "lib.py") + write(self._path("lib.py"), b"print('hidden')\n") + + html = self._render(None, []) + + self.assertIn("print('evil')", html) + self.assertNotIn("print('staged')", html) + self.assertIn(HIDDEN.format("app.py", "skip-worktree"), html) + self.assertIn(HIDDEN.format("lib.py", "assume-unchanged"), html) + + def test_ignored_submodule_change_is_shown(self) -> None: + sub = self._submodule(self.repo, "sub", "\tignore = all\n") + git(sub, "commit", "--allow-empty", "-m", "two") + new = git(sub, "rev-parse", "HEAD") + git(self.repo, "update-index", "--cacheinfo", f"160000,{new},sub") + + gitmodules = self._render(None, []) + write(self._path(".gitmodules"), MODULE.format("sub", "").encode()) + git(self.repo, "config", "diff.ignoreSubmodules", "all") + config = self._render(None, []) + + for source, html in (("gitmodules", gitmodules), ("config", config)): + with self.subTest(source=source): + self.assertIn(f"Subproject commit {new}", html) + + def test_attribute_binary_is_warned(self) -> None: + write(self._path(".gitattributes"), b"*.txt -diff\n") + write(self._path(".git", "info", "attributes"), b"*.md binary\n") + write(self._path("plain.txt"), b"plain text\n") + write(self._path("readme.md"), b"# readme\n") + write(self._path("image.bin"), b"\x00\x01binary\n") + git(self.repo, "add", ".gitattributes", "plain.txt", "readme.md", "image.bin") + + html = self._render(None, []) + + self.assertIn('data-path="image.bin"', html) + for path in ("plain.txt", "readme.md"): + self.assertIn(f"
  • {ATTRIBUTE_BINARY.format(path, UNSET)}
    • ", html) + for path in ("image.bin", "new.txt"): + self.assertNotIn(f"{path} is shown as binary", html) + + def test_binary_diff_driver_is_warned(self) -> None: + write(self._path(".gitattributes"), b"*.dat diff=opaque\n*.bin diff=plain\n") + git(self.repo, "config", "diff.opaque.binary", "yes") + write(self._path("data.dat"), b"text data\n") + write(self._path("image.bin"), b"\x00\x01binary\n") + git(self.repo, "add", ".gitattributes", "data.dat", "image.bin") + + html = self._render(None, []) + + self.assertIn('data-path="image.bin"', html) + warning = ATTRIBUTE_BINARY.format("data.dat", DRIVER.format("opaque")) + self.assertIn(f"
  • {warning}
    • ", html) + self.assertNotIn("image.bin is shown as binary", html) + + def test_base_resolves_the_branch_not_a_shadow(self) -> None: + head = git(self.repo, "rev-parse", "HEAD") + git(self.repo, "update-ref", "refs/tags/main", head) + tag_shadow = self._render("main", []) + write(self._path(".git", "main"), f"{head}\n".encode()) + dir_shadow = self._render("main", []) + + for html in (tag_shadow, dir_shadow): + self.assertIn('id="commits" data-commits="1"', html) + self.assertIn("
    Base
    refs/heads/main @ ", html) + + def test_base_refuses_ambiguous_and_other_refs(self) -> None: + head = git(self.repo, "rev-parse", "HEAD") + git(self.repo, "update-ref", "refs/remotes/main", head) + git(self.repo, "update-ref", "refs/tags/v1", head) + cases = { + "main": "ambiguous base ref", + "refs/tags/v1": "unknown base ref", + "HEAD~1": "unknown base ref", + head: "unknown base ref", + } + + for base, message in cases.items(): + with ( + self.subTest(base=base), + self.assertRaisesRegex(ReviewError, message), + ): + self._render(base, []) + + def test_invisible_characters_are_marked(self) -> None: + tree = git(self.repo, "rev-parse", "HEAD^{tree}") + message = f"fix{RLO}: subject\n\nbody{LRI}text" + commit = git(self.repo, "commit-tree", tree, "-p", "HEAD", "-m", message) + git(self.repo, "update-ref", "HEAD", commit) + write(self._path(f"evil{RLO}txt.py"), f"code{ZWSP}line{BOM}\n".encode()) + note = os.path.join(self.root, "note.txt") + write(note, f"note{INVISIBLE}\n".encode()) + + html = self._render("main", [note]) + + for char in INVISIBLE: + self.assertNotIn(char, html, f"U+{ord(char):04X}") + self.assertIn(f"fix{marks(RLO)}: subject", html) + self.assertIn(f"body{marks(LRI)}text", html) + self.assertIn(f"evil{marks(RLO)}txt.py", html) + self.assertIn(f"code{marks(ZWSP)}line{marks(BOM)}", html) + self.assertIn(f"note{marks(INVISIBLE)}", html) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/integration/test_review_hardening.py b/tests/integration/test_review_hardening.py new file mode 100644 index 0000000..0cdbf3e --- /dev/null +++ b/tests/integration/test_review_hardening.py @@ -0,0 +1,229 @@ +from __future__ import annotations + +import os +import stat +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +from pkgmgr.actions.review import ReviewError, write_review +from tests.integration.test_review import GIT_ENV, git, write + +SECRET = b"SECRET-KEY-MATERIAL\n" +SIGNATURE = ( + "gpgsig -----BEGIN PGP SIGNATURE-----\n \n iQEzBAABCAAdFiEE\n" + " -----END PGP SIGNATURE-----" +) +MODULE = '[submodule "{0}"]\n\tpath = {0}\n\turl = ./{0}\n{1}' + + +class ReviewRepoCase(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.root = os.path.realpath(self._tmp.name) + self.repo = os.path.join(self.root, "repo") + self.marker = os.path.join(self.root, "marker") + self.secret = os.path.join(self.root, "secret", "id_ed25519") + write(self.secret, SECRET) + git(self.root, "init", "-b", "main", self.repo) + write(self._path("app.py"), b"print('base')\n") + write(self._path("lib.py"), b"print('lib')\n") + git(self.repo, "add", "-A") + git(self.repo, "commit", "-m", "init") + git(self.repo, "checkout", "-b", "feature") + write(self._path("app.py"), b"print('feature')\n") + git(self.repo, "commit", "-am", "feature") + write(self._path("app.py"), b"print('staged')\n") + git(self.repo, "add", "app.py") + write(self._path("app.py"), b"print('unstaged')\n") + write(self._path("new.txt"), b"untracked\n") + + def tearDown(self) -> None: + self._tmp.cleanup() + + def _path(self, *parts: str) -> str: + return os.path.join(self.repo, *parts) + + def _render(self, base: str | None, notes: list[str]) -> str: + path = write_review( + cwd=self.repo, + out=os.path.join(self.root, "out", "page.html"), + max_file_lines=50, + base=base, + notes=notes, + ) + return Path(path).read_text(encoding="utf-8") + + def _trap(self, path: str, tail: str) -> str: + write(path, f"#!/bin/sh\ntouch '{self.marker}'\n{tail}\n".encode()) + os.chmod(path, 0o755) + return path + + def _assert_fires(self, *args: str) -> None: + subprocess.run( + ["git", *args], + cwd=self.repo, + env=GIT_ENV, + stdin=subprocess.DEVNULL, + capture_output=True, + check=False, + ) + self.assertTrue(os.path.exists(self.marker), f"plain git {args[0]} fires") + os.remove(self.marker) + + def _submodule(self, parent: str, name: str, extra: str = "") -> str: + path = os.path.join(parent, name) + git(parent, "init", "-b", "main", path) + write(os.path.join(path, "f.txt"), b"one\n") + git(path, "add", "-A") + git(path, "commit", "-m", "one") + head = git(path, "rev-parse", "HEAD") + write(os.path.join(parent, ".gitmodules"), MODULE.format(name, extra).encode()) + git(parent, "update-index", "--add", "--cacheinfo", f"160000,{head},{name}") + git(parent, "add", ".gitmodules") + git(parent, "commit", "-m", name) + return path + + def _object(self, kind: str, content: str) -> str: + return subprocess.run( + ["git", "hash-object", "-t", kind, "-w", "--stdin"], + cwd=self.repo, + env=GIT_ENV, + input=content, + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + +class TestReviewHardening(ReviewRepoCase): + def test_fsmonitor_never_runs(self) -> None: + monitor = self._trap(os.path.join(self.root, "fsmonitor"), "exit 1") + git(self.repo, "config", "core.fsmonitor", monitor) + self._assert_fires("diff") + + self._render("main", []) + + self.assertFalse(os.path.exists(self.marker)) + + def test_repository_filter_never_runs(self) -> None: + write(self._path(".git", "info", "attributes"), b"* filter=trap\n") + clean = self._trap(os.path.join(self.root, "clean"), "exec cat") + git(self.repo, "config", "filter.trap.clean", clean) + self._assert_fires("diff") + + html = self._render("main", []) + + self.assertFalse(os.path.exists(self.marker)) + self.assertIn('id="unstaged" data-files="0"', html) + self.assertIn("filter.trap.clean", html) + self.assertNotIn("print('unstaged')", html) + self.assertIn("
    (not computed, see the warnings)
    ", html) + + def test_submodule_filter_never_runs(self) -> None: + sub = self._submodule(self.repo, "sub") + inner = self._submodule(sub, "inner") + clean = self._trap(os.path.join(self.root, "clean"), "exec cat") + for path in (sub, inner): + write(os.path.join(path, ".git", "info", "attributes"), b"* filter=x\n") + write(os.path.join(path, "f.txt"), b"two\n") + git(path, "config", "filter.x.clean", clean) + self._assert_fires("diff") + + html = self._render(None, []) + + self.assertFalse(os.path.exists(self.marker)) + self.assertIn('id="unstaged" data-files="0"', html) + self.assertIn("the config of submodule sub defines filter.x.clean", html) + self.assertIn("the config of submodule sub/inner defines filter.x.clean", html) + self.assertIn("
    (not computed, see the warnings)
    ", html) + + def test_signature_program_never_runs(self) -> None: + header, message = git(self.repo, "cat-file", "commit", "HEAD").split("\n\n", 1) + signed = self._object("commit", f"{header}\n{SIGNATURE}\n\n{message}\n") + git(self.repo, "update-ref", "HEAD", signed) + program = self._trap(os.path.join(self.root, "gpg"), "exit 1") + git(self.repo, "config", "gpg.program", program) + self._assert_fires("log", "-1", "--format=%G?") + + html = self._render("main", []) + + self.assertFalse(os.path.exists(self.marker)) + self.assertIn('id="commits" data-commits="1"', html) + self.assertNotIn(' None: + link = os.path.join(self.root, "score.txt") + os.symlink(self.secret, link) + parent = os.path.join(self.root, "notes") + os.symlink(os.path.dirname(self.secret), parent) + + for note in (link, os.path.join(parent, "id_ed25519")): + with ( + self.subTest(note=note), + self.assertRaisesRegex(ReviewError, "cannot read note"), + ): + self._render(None, [note]) + + def test_untracked_parent_swap_is_not_followed(self) -> None: + write(self._path("sub", "id_ed25519"), b"harmless\n") + real_run = subprocess.run + swapped: list[list[str]] = [] + + def swap(cmd, *args, **kwargs): + result = real_run(cmd, *args, **kwargs) + if "--others" in cmd and not swapped: + os.rename(self._path("sub"), os.path.join(self.root, "sub-real")) + os.symlink(os.path.dirname(self.secret), self._path("sub")) + swapped.append(cmd) + return result + + with patch("subprocess.run", swap): + html = self._render(None, []) + + self.assertTrue(swapped) + self.assertNotIn(SECRET.decode().strip(), html) + self.assertIn("Not a regular file, contents not shown.", html) + + def test_out_symlink_is_replaced_not_followed(self) -> None: + keys = b"ssh-ed25519 AAAA operator\n" + victim = os.path.join(self.root, "authorized_keys") + write(victim, keys) + missing = os.path.join(self.root, "missing_keys") + out = os.path.join(self.root, "out") + os.makedirs(out) + os.symlink(victim, os.path.join(out, "page.html")) + os.symlink(missing, os.path.join(out, "dangling.html")) + + for name in ("page.html", "dangling.html"): + with self.subTest(out=name): + path = write_review( + cwd=self.repo, + out=os.path.join(out, name), + max_file_lines=50, + base=None, + notes=[], + ) + info = os.lstat(path) + self.assertTrue(stat.S_ISREG(info.st_mode)) + self.assertEqual(stat.S_IMODE(info.st_mode), 0o600) + + self.assertEqual(Path(victim).read_bytes(), keys) + self.assertFalse(os.path.lexists(missing)) + self.assertEqual(sorted(os.listdir(out)), ["dangling.html", "page.html"]) + + +if __name__ == "__main__": + unittest.main()