From 0023ddef4371c1a1ab06e9fb3821adc40c2c3f5b Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 00:59:24 +0000 Subject: [PATCH 1/5] adopt-project.py: a `.gitmodules` git's submodule reader cannot read is a `check` finding and an `execute` refusal when the tip holds no submodule but the history of the source's branches and tags does, and so is `ignore`, `update` or `branch` with no value beside any submodule, so `execute` no longer dies in `git fast-export` after both leg repositories exist (#195) `git filter-repo` runs `git fast-export --all` in a clone of the source, and git's submodule reader reads the clone's checked-out `.gitmodules` for every gitlink an exported commit adds, changes or removes. #166 asked the tip tree alone, so a source whose history held a submodule and whose tip held none, with a tip `.gitmodules` holding a key with no value, passed `check` as `plan ok`, and `execute` died in `git fast-export` after both leg repositories were made: a corrected re-run then met `leg-remote-exists`. A line git cannot parse was passed the same way when the plan dropped the file, and was said to fail in the clone when the plan kept it. The rule, measured with git 2.43 and `git filter-repo` 2.47.0 on main a5b09c3, as on 20e13f0 and e4decfb before it: - Where the tip `.gitmodules` cannot be read for its submodules and the tip holds no gitlink, `_exported_gitlink` asks ONE streamed `git log` for the diffs fast-export makes -- the plan's commit and every branch and tag of the source, which is what `git clone` copies; root and first-parent merge diffs; no renames; `--ignore-submodules=none`, `--no-replace-objects`, `--no-show-signature` -- and stops at the first gitlink. If there is one and the plan extracts a leg, `plan-gitmodules-unreadable` names the commit and what it does to the gitlink, wherever the plan sends `.gitmodules`, and `execute` refuses it before any leg exists. Measured failing on main: a gitlink on the branch adopted, on a side branch merged, on a branch never merged (added, or added and moved), on a commit only a tag reaches, in the root commit of an orphan branch, added and removed by merges alone, and hidden by a replace ref. - Where neither the tip nor that history holds a gitlink, a key with no value still registers nothing (#166's S3), and a plan that extracts no leg is still not refused. A branch is a `refs/heads/` ref of the source: an `org/repo` clone's other branches and a commit only a detached HEAD reaches are in neither the extraction nor the scan, and adopt as on main. - `ignore`, `update` and `branch` with no value are read as `path` and `url` were: git's submodule reader dies on each (`branch` crashes git 2.43, exit 139, with nothing said). On main a submodule beside one, at the tip or in the history, passed `check`. `fetchRecurseSubmodules` and `shallow` with no value are read as true and still adopt. Unchanged, diffed over 37 measured sources: `plan`'s output and the plan it writes are byte-identical to main's in every one, and so is `check`'s output on the same plan in the 18 whose verdict does not change. `.gitmodules` values git rejects (`update=bogus`, `update=`, `update=!cmd`, `shallow=maybe`, `fetchRecurseSubmodules=bogus`) still pass `check`; that follow-up is #215. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 245 ++++++++++-- tests/test_adopt_history_gitlinks.py | 568 +++++++++++++++++++++++++++ 2 files changed, 783 insertions(+), 30 deletions(-) create mode 100644 tests/test_adopt_history_gitlinks.py diff --git a/adopt-project.py b/adopt-project.py index c96d9a07..4322110c 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -341,23 +341,165 @@ def _move(self, name: str, path: str) -> None: self._name_at[path] = name +#: The keys git's submodule reader dies on when one has no value at all, +#: under a name it does not ignore (submodule-config.c, `parse_config`). +#: Measured for #195 with git 2.43, a `git fast-export` over a gitlink: +#: `path`, `url`, `ignore` and `update` each end it with `missing value for +#: ''`, and `branch` with a crash (SIGSEGV, exit 139) and nothing said; +#: `fetchRecurseSubmodules` and `shallow` with no value are read as true, +#: and nothing dies. +SUBMODULE_VALUED_KEY_RE = re.compile( + r"submodule\.(?P.*)\.(?:path|url|ignore|update|branch)") + + def _registrations(listed: bytes) -> Registrations | str: """`git config -z --list` output, read by `Registrations`; or, for a - `path` or `url` with no value at all, what git's submodule reader dies - on. `-z` prints such a key with no newline, and git looks for its value - after it has filtered the name and before it filters the value.""" + key SUBMODULE_VALUED_KEY_RE names with no value at all, what git's + submodule reader dies on. `-z` prints such a key with no newline, and + git looks for its value after it has filtered the name and before it + filters the value.""" registrations = Registrations() for record in listed.split(b"\x00"): key, newline, value = _git_path(record).partition("\n") + valued = SUBMODULE_VALUED_KEY_RE.fullmatch(key) + if valued and not newline and not _a_name_git_ignores(valued["name"]): + return f"missing value for '{key}'" match = SUBMODULE_KEY_RE.fullmatch(key) if not match: continue - if not newline and not _a_name_git_ignores(match["name"]): - return f"missing value for '{key}'" registrations.read(match["name"], match["key"], value) return registrations +class InTheHistory(str): + """What git says of a `.gitmodules` its submodule reader cannot read, as + `Source.registered_submodules` answers for a source whose TIP holds no + gitlink and whose history does (#195): a `str`, so that every reader of + that answer takes it as git's words, and `gitlink`, where the history + holds one, as `_exported_gitlink` names it.""" + + gitlink: str + + def __new__(cls, words: str, gitlink: str) -> "InTheHistory": + said = super().__new__(cls, words) + said.gitlink = gitlink + return said + + +#: `git log` asked for the diffs `git fast-export` makes, a NUL after each +#: field; `_exported_gitlink` says why each flag is there. +EXPORTED_DIFFS = ["--no-replace-objects", "log", "-z", "--raw", "--root", + "--no-renames", "--diff-merges=first-parent", + "--ignore-submodules=none", "--no-show-signature", + "--no-abbrev", "--no-color", "--format=%H"] + + +def _exported_gitlink(source: "Source") -> str | None: + """Where the history `execute`'s extraction rewrites holds a gitlink, as + a finding names it -- "commit 3feb96f2c1a0 removes `vendor/b`" -- or + None where it holds none (#195). + + `_extract_leg` clones the source and runs `git filter-repo`, which runs + `git fast-export --all` in the clone: every branch and tag of the source, + which is what `git clone` copies, and the commit the plan names. It + diffs each commit against its first parent, a root against nothing, and + for every gitlink one of those diffs adds, changes or removes, git's + submodule reader reads the clone's CHECKED-OUT `.gitmodules`: the plan's + commit's, whatever leg the plan gives it. Measured with git 2.43 and + `git filter-repo` 2.47.0, a tip `.gitmodules` that reader cannot read + failed the extraction, after both leg repositories were made, for a + gitlink in the history of the branch being adopted, on a side branch + merged into it, on a branch never merged, on a commit only a tag + reaches, in the root commit of a branch of its own, and one that two + merges alone added and removed. + + A BRANCH here is a `refs/heads/` ref of the source, as the clone takes + it: an `org/repo` source, which `Source.open` clones, has its default + branch alone, and its other branches, its remote-tracking refs, are no + more in the extraction's clone than in this scan; neither is a commit + that only the source's detached HEAD reaches, which `_extract_leg`'s + checkout of the plan's commit leaves behind. Measured, each of those + adopted on main beside the same `.gitmodules`. + + So `git log` is asked for those diffs, over those commits, and read + until the first gitlink in one: a `160000` mode on either side. + `--root` and `--diff-merges=first-parent` are fast-export's root and + merge diffs, which `log.showRoot=false` and a plain `git log` leave out; + `--no-renames`, its diffs, one path to a change. `--ignore-submodules= + none`, because `git log` runs the same submodule reader, on the SOURCE's + checked-out `.gitmodules`, and died on the very file this is asked about + (exit 128), and because `diff.ignoreSubmodules=all` hides every gitlink. + `--no-replace-objects`, because a clone copies no `refs/replace/`, and a + replacement can hide the commit that holds one. `--no-show-signature`, + because `log.showSignature` runs gpg on every signed commit and writes + what it says into this output, ahead of the commit it names. + + The output is read as it comes and the scan stopped at the first gitlink, + so a source that holds one is not read to its root; a source that holds + none is read once, end to end (5010 commits in 0.4 s here). A scan git + cannot finish is refused as `git-failed`, as `git_out` refuses: a history + that could not be read has not been shown to hold no gitlink. + """ + command = ["git", *EXPORTED_DIFFS, source.commit, "--branches", "--tags", + "--"] + with tempfile.TemporaryFile() as said, subprocess.Popen( + command, cwd=source.path, stdout=subprocess.PIPE, + stderr=said) as log: + found = _first_gitlink(log.stdout) + if found is not None: + log.kill() + elif log.wait(): + said.seek(0) + raise Refusal("git-failed", "`{}` in {} exited {}: {}".format( + " ".join(command[1:]), source.path, log.returncode, + said.read().decode("utf-8", "replace").strip())) + return found + + +#: How a finding says what a commit does to the gitlink it names: `git log +#: --raw`'s status letter, a type change or a new commit being a change. +GITLINK_CHANGES = {"A": "adds", "D": "removes"} + + +def _first_gitlink(stream) -> str | None: + """The first gitlink `git log` writes, as `_exported_gitlink` names it. + + With `-z`, `--format=%H` and `--raw`, each field ends in a NUL: a + commit, then for each path it changes `: + ` and the path, raw bytes. A newline before a commit or a + change is not part of it. So a field after a change is a path, whatever + it spells, and any other is a commit or a change.""" + commit, change, rest = "", None, b"" + for chunk in iter(lambda: stream.read(1 << 16), b""): + *fields, rest = (rest + chunk).split(b"\x00") + for field in fields: + if change is not None: + if GITLINK_MODE in change[:2]: + return (f"commit {commit[:12]} " + f"{GITLINK_CHANGES.get(change[-1], 'changes')} " + f"{_spelled(_git_path(field))}") + change = None + elif field.lstrip(b"\n").startswith(b":"): + change = field.lstrip(b"\n")[1:].decode().split() + else: + commit = field.strip().decode() + return None + + +def _read_for_the_history(source: "Source", tree: list, + words: str) -> InTheHistory | None: + """`words`, what git says of the source's `.gitmodules`, as an + `InTheHistory` naming the gitlink the extraction reads that file for; + None when the history holds none, or when the file at the tip is not a + file the clone checks out as one, which `_where_git_fails` leaves to + the clone.""" + if not any(path == GITMODULES and mode in FILE_MODES + for path, mode, _, _ in tree): + return None + gitlink = _exported_gitlink(source) + return None if gitlink is None else InTheHistory(words, gitlink) + + class Source: """A repository being read, and nothing more. Nothing here writes to it.""" @@ -451,12 +593,21 @@ def registered_submodules(self, tree: list) -> Registrations | str: (#166), and the person told so is told why. git calls the file it read through `--file -` "standard input"; they are told `.gitmodules`. - A KEY WITH NO VALUE -- `path` or `url` with nothing after it -- is - fatal to git's submodule reader alone, and git runs that reader only - for a gitlink. So in a tree that holds none, nothing ever reads the - file that way, the assembly's clone and `execute` pass over it as - main always did, and it registers nothing (#166's S3). A line git - cannot parse at all is another matter: every reader dies on it. + A KEY WITH NO VALUE -- one SUBMODULE_VALUED_KEY_RE names, with + nothing after it -- is fatal to git's submodule reader alone, and git + runs that reader only for a gitlink: the assembly's clone for one in + the tree it checks out, and `execute`'s extraction for one in ANY + commit `git fast-export` diffs, where the tip `.gitmodules` is the + file it reads (#195). So in a tree that holds none, the file is read + that way only when the history of the source's branches and tags + holds one (`_exported_gitlink`), and then the answer is git's words, + an `InTheHistory` naming where: on main, `check` passed such a file + as registering nothing and `execute` died in `git fast-export` with + `missing value for 'submodule.b.path'`, after both leg repositories + were made. Where the history holds none either, nothing ever reads + the file that way, and it registers nothing (#166's S3). A line git + cannot parse at all is another matter: every reader dies on it, and + the history says only which reader dies first. """ if not any(path == GITMODULES for path, _, _, _ in tree): return Registrations() @@ -470,13 +621,15 @@ def registered_submodules(self, tree: list) -> Registrations | str: "--list"], cwd=self.path, input=blob.stdout, capture_output=True, check=False) if listed.returncode: - return _one_line(listed.stderr.decode(errors="replace")).replace( + read = _one_line(listed.stderr.decode(errors="replace")).replace( "standard input", GITMODULES) - read = _registrations(listed.stdout) - if isinstance(read, str) and not any( + else: + read = _registrations(listed.stdout) + if not isinstance(read, str) or any( mode == GITLINK_MODE for _, mode, _, _ in tree): - return Registrations() - return read + return read + return _read_for_the_history(self, tree, read) or ( + read if listed.returncode else Registrations()) def commit_count(self) -> int: return int(git_out(["rev-list", "--count", self.commit], cwd=self.path)) @@ -1657,8 +1810,9 @@ def _leg_findings(plan: Plan, tree: list | None) -> list[str]: "assembly's clone fails on it inside the leg. `drop` it, or register it " "in the source (that entry, committed) and re-run `plan`; a " "`.gitmodules` git cannot read for its submodules is repaired in the " - "source and re-planned the same way (in a source with no submodule, " - "dropping it is enough). " + "source and re-planned the same way (dropping it is enough in a source " + "with no submodule at its tip or in the history of its branches and " + "tags). " "To answer a submodule apart from the rest of the directory entry that " "holds it, REPLACE that entry with an entry for each of its children, " "the submodule's own path among them, as that entry's `question:` lists " @@ -1684,6 +1838,13 @@ def _leg_findings(plan: Plan, tree: list | None) -> list[str]: FAILS_IN_THE_CLONE = ( "so which submodules it registers is unknown, and the plan keeps it or a " "submodule in a leg, where the assembly's recursive clone fails on it") +#: The first end again, for a source whose tip holds no submodule and whose +#: history does (`_where_the_history_fails`, #195); `{}` is where. +FAILS_IN_THE_HISTORY = ( + "and the source's tip holds no submodule but the history `git " + "filter-repo` rewrites does -- every branch and tag of the source, where " + "{} -- so it fails on that file when `execute` extracts a leg, wherever " + "the plan sends it") def _answered_legs(entries: list, paths: list[str]) -> dict: @@ -1747,8 +1908,10 @@ def submodule_plan_problems(entries: list, tree: list, register nothing (`plan-gitmodules-unreadable`, saying what git said, and where git fails on it: `_where_git_fails`). Beside a submodule it fails the extraction of either leg, wherever the plan - sends the two; otherwise it is a problem where the plan keeps it, or - a gitlink, in a leg the assembly's clone then recurses into. + sends the two, and so it does in a source whose history alone holds + one (`_where_the_history_fails`, #195); otherwise it is a problem + where the plan keeps it, or a gitlink, in a leg the assembly's clone + then recurses into. * Every gitlink the plan KEEPS, in the spec or the code leg, must be REGISTERED (`plan-submodule-unregistered`). A leg is WORSE than the source for one that is not: a top-level `git clone @@ -1793,7 +1956,8 @@ def submodule_plan_problems(entries: list, tree: list, if not isinstance(registered, str): return problems + _kept_submodule_problems(kept, registered, registry_leg) - failure = _where_git_fails(entries, tree, registry_leg, kept) + failure = (_where_the_history_fails(entries, registered) + or _where_git_fails(entries, tree, registry_leg, kept)) if failure: problems.append(( "plan-gitmodules-unreadable", @@ -1810,21 +1974,25 @@ def _where_git_fails(entries: list, tree: list, registry_leg: str | None, Measured end to end with git 2.43 (#166's S3), the first failure first: * `git filter-repo` runs `git fast-export`, which reads the CHECKED-OUT - `.gitmodules` for each gitlink in the history it exports. So such a - FILE in a source that holds a submodule fails the extraction of - either leg, wherever the plan sends the two -- after both leg - repositories were made. A plan that extracts no leg, every path - rooted or dropped, is the only one that escapes it. + `.gitmodules` for each gitlink in the history it exports: every + commit of every branch and tag of the source, not the tip alone. So + such a FILE fails the extraction of either leg, wherever the plan + sends it -- after both leg repositories were made -- in a source + whose tree holds a submodule (here), and in one whose tree holds + none and whose history does (`_where_the_history_fails`, asked + first; #195, where `check` used to pass it). A plan that extracts no + leg, every path rooted or dropped, is the only one that escapes it. * Otherwise the assembly's recursive clone fails in a leg the plan keeps the file in, or a gitlink: on a line git cannot parse, even with no gitlink in the leg, and on a `.gitmodules` that is itself a gitlink, which the clone checks out as a directory and which registers nothing. - A key with no value is fatal to git's submodule reader alone, which - git runs only for a gitlink, so `Source.registered_submodules` reads it - as registering nothing in a tree with none, and it never reaches here - without one. + A key with no value is fatal to git's submodule reader alone, which git + runs only for a gitlink, so `Source.registered_submodules` reads it as + registering nothing where neither the tree nor the history `git + fast-export` diffs holds one, and it reaches here only where one of them + does. """ a_file = any(path == GITMODULES and mode in FILE_MODES for path, mode, _, _ in tree) @@ -1837,6 +2005,23 @@ def _where_git_fails(entries: list, tree: list, registry_leg: str | None, return None +def _where_the_history_fails(entries: list, + registered: Registrations | str) -> str | None: + """`_where_git_fails`' first end, for a source whose tip holds no + gitlink: FAILS_IN_THE_HISTORY, naming where the history holds one, when + `registered` is an `InTheHistory` and the plan extracts a leg; None + otherwise, and `_where_git_fails` is asked as before. + + The plan's answers cannot help here. `_extract_leg` checks the plan's + commit out whole before `git filter-repo` runs, so the file it reads is + there whether the plan sends `.gitmodules` to a leg or drops it, and a + gitlink in the history is not a path the plan can answer (#195).""" + if isinstance(registered, InTheHistory) and any( + str(e.get("leg")) in EXTRACTED_LEGS for e in entries): + return FAILS_IN_THE_HISTORY.format(registered.gitlink) + return None + + def _kept_in_a_leg(answered: dict, gitlinks: list[str]) -> list[tuple]: """`(path, leg, entry)` for each gitlink `answered` keeps in a leg.""" return [(path, *answered[path]) for path in gitlinks diff --git a/tests/test_adopt_history_gitlinks.py b/tests/test_adopt_history_gitlinks.py new file mode 100644 index 00000000..d2bf5e60 --- /dev/null +++ b/tests/test_adopt_history_gitlinks.py @@ -0,0 +1,568 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A submodule the HISTORY holds and the tip does not still fails the +extraction (#195). + +`git filter-repo` runs `git fast-export --all` in a clone of the source, and +git's submodule reader reads the clone's CHECKED-OUT `.gitmodules` for every +gitlink a commit of any branch or tag adds, changes or removes. #166 asked +the tip tree alone, so a source whose history once held a submodule, whose +tip holds none, and whose tip `.gitmodules` has a key with no value +(`[submodule "b"]` and a bare `path`) passed `check` as `plan ok`, and +`execute` died in `git fast-export` after both leg repositories were made: +a corrected re-run then met `leg-remote-exists`. + +All repositories here are local, and only the one end-to-end test at the +bottom needs `git filter-repo`. The rest read the history with +`_exported_gitlink`, run `check`, and call `cmd_execute` in this process +with its probe for the tool stubbed, because the refusal under test comes +before the tool would be used. +""" + +from __future__ import annotations + +import importlib.util +import os +import re +import shutil +import stat +import subprocess +from argparse import Namespace +from pathlib import Path + +import pytest + +from conftest import ADOPT, git, make_source_repo, resolve, run_script, \ + write_plan + +needs_filter_repo = pytest.mark.skipif( + shutil.which("git-filter-repo") is None, + reason="git filter-repo is not installed: `pip install git-filter-repo`") + +PROJECT = "Northwind" +GITMODULES = ".gitmodules" +UNREADABLE = "plan-gitmodules-unreadable" +REMOTES = "remotes" +#: The submodule the history holds, registered under the name `b`. +GITLINK = "vendor/b" +REGISTRATION = '[submodule "b"]\n\tpath = vendor/b\n\turl = ../b\n' +#: The issue's tip `.gitmodules`: a key with no value. +BARE_PATH = '[submodule "b"]\n\tpath\n' +#: A line no git reader can parse. +BAD_LINE = '[submodule "b"\n\tpath = vendor/b\n' +#: A source whose plan asks one question, `.gitmodules`': `specs/` and `src/` +#: classify themselves and `README.md` stays in the root. +SMALL_TREE = {"README.md": "# Thing\n", "src/app/util.py": "VALUE = 1\n", + "specs/001-feature/tasks.md": "- [ ] one\n"} + + +def named(verb: str = "(adds|removes|changes)") -> str: + """How `_exported_gitlink` names a commit's change to the gitlink, as a + pattern: `verb` is what that commit does to it.""" + return rf"commit [0-9a-f]{{12}} {verb} `vendor/b`" + + +@pytest.fixture(scope="module") +def adopter(): + spec = importlib.util.spec_from_file_location("adopt_history_gitlinks", + ADOPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def as_human(*args: str, cwd: Path): + """`git ` with an identity, for the commits and merges below.""" + return git("-c", "user.name=Source Human", + "-c", "user.email=source@invalid.example", *args, cwd=cwd) + + +def put_gitmodules(source: Path, text: str) -> None: + """`text` as the source's `.gitmodules`, staged: BYTES, so it is LF on + every platform, as a hand edit would leave it.""" + (source / GITMODULES).write_bytes(text.encode()) + git("add", "--", GITMODULES, cwd=source) + + +def link(source: Path) -> None: + """A gitlink at GITLINK, staged straight into the index, as `git add` of + an embedded clone records one; no checkout of it is needed. It names + `main`'s commit, which an orphan branch's first commit has no HEAD to + give.""" + head = git("rev-parse", "main", cwd=source).stdout.strip() + git("update-index", "--add", "--cacheinfo", f"160000,{head},{GITLINK}", + cwd=source) + + +def unlink(source: Path) -> None: + git("rm", "-q", "--cached", "--", GITLINK, cwd=source) + + +def add_then_remove(source: Path) -> None: + """Two commits on the branch checked out: the submodule and its + registration, then the submodule gone.""" + put_gitmodules(source, REGISTRATION) + link(source) + as_human("commit", "-qm", "Add vendor/b", cwd=source) + unlink(source) + as_human("commit", "-qm", "Remove vendor/b", cwd=source) + + +def on_a_side_branch_merged(source: Path) -> None: + as_human("checkout", "-q", "-b", "side", cwd=source) + add_then_remove(source) + as_human("checkout", "-q", "main", cwd=source) + as_human("merge", "-q", "--no-ff", "-m", "Merge side", "side", cwd=source) + as_human("branch", "-q", "-D", "side", cwd=source) + + +def on_a_branch_never_merged(source: Path) -> None: + as_human("checkout", "-q", "-b", "other", cwd=source) + link(source) + as_human("commit", "-qm", "Add vendor/b on other", cwd=source) + as_human("checkout", "-q", "main", cwd=source) + + +def moved_on_a_branch_never_merged(source: Path) -> None: + """A branch never merged adds it and then moves it to another commit: + the first gitlink `git log` meets is a change, neither an add nor a + removal.""" + on_a_branch_never_merged(source) + added = git("rev-parse", "other", cwd=source).stdout.strip() + as_human("checkout", "-q", "other", cwd=source) + git("update-index", "--cacheinfo", f"160000,{added},{GITLINK}", + cwd=source) + as_human("commit", "-qm", "Move vendor/b on other", cwd=source) + as_human("checkout", "-q", "main", cwd=source) + + +def on_a_commit_only_a_tag_reaches(source: Path) -> None: + as_human("checkout", "-q", "--detach", cwd=source) + link(source) + as_human("commit", "-qm", "Add vendor/b, tagged", cwd=source) + as_human("tag", "only-a-tag", cwd=source) + as_human("checkout", "-q", "main", cwd=source) + + +def in_a_root_commit_of_its_own(source: Path) -> None: + """The ONE commit of an unrelated branch adds it: only a root diff, + which `log.showRoot=false` leaves out, shows it.""" + as_human("checkout", "-q", "--orphan", "pages", cwd=source) + git("rm", "-r", "-q", "--cached", "--", ".", cwd=source) + link(source) + as_human("commit", "-qm", "pages root with vendor/b", cwd=source) + as_human("checkout", "-q", "-f", "main", cwd=source) + + +def by_merges_alone(source: Path) -> None: + """One merge adds it and another removes it, so no commit with one + parent touches it: only a merge's own diff, which a plain `git log` + does not show, finds it.""" + for branch, change in (("s1", link), ("s2", unlink)): + as_human("checkout", "-q", "-b", branch, cwd=source) + (source / f"{branch}.txt").write_bytes(b"x\n") + git("add", "--", f"{branch}.txt", cwd=source) + as_human("commit", "-qm", branch, cwd=source) + as_human("checkout", "-q", "main", cwd=source) + as_human("merge", "-q", "--no-ff", "--no-commit", branch, cwd=source) + change(source) + as_human("commit", "-qm", f"merge {branch}", cwd=source) + as_human("branch", "-q", "-D", branch, cwd=source) + + +def hidden_by_a_replacement(source: Path) -> None: + """The commit that adds it, replaced by its parent: `git log` honours + the replacement and a clone, which copies no `refs/replace/`, does + not.""" + add_then_remove(source) + added = git("rev-parse", "HEAD~1", cwd=source).stdout.strip() + git("replace", added, f"{added}~1", cwd=source) + + +#: Where the history holds the submodule, and what the commit `git log` +#: meets first does to it; the tip holds none in every one. +HISTORIES = { + "on-the-branch-adopted": (add_then_remove, "removes"), + "on-a-side-branch-merged": (on_a_side_branch_merged, "removes"), + "on-a-branch-never-merged": (on_a_branch_never_merged, "adds"), + "moved-on-a-branch-never-merged": (moved_on_a_branch_never_merged, + "changes"), + "on-a-commit-only-a-tag-reaches": (on_a_commit_only_a_tag_reaches, + "adds"), + "in-a-root-commit-of-its-own": (in_a_root_commit_of_its_own, "adds"), + "by-merges-alone": (by_merges_alone, "removes"), + "hidden-by-a-replacement": (hidden_by_a_replacement, "removes"), +} + + +def history_source(base: Path, history=add_then_remove, + tip: str | None = BARE_PATH) -> Path: + """A source whose `history` holds GITLINK and whose tip holds no + submodule, with `tip` as its `.gitmodules` (none when None), committed + last on `main` and checked out.""" + source = make_source_repo(base / "Thing", tree=SMALL_TREE, edits=()) + if history is not None: + history(source) + if tip is None: + git("rm", "-q", "--ignore-unmatch", "--", GITMODULES, cwd=source) + else: + put_gitmodules(source, tip) + as_human("commit", "-q", "--allow-empty", "-m", "The tip .gitmodules", + cwd=source) + tip_tree = git("ls-tree", "-r", "HEAD", cwd=source).stdout + assert "160000" not in tip_tree, tip_tree + return source + + +def opened(adopter, source: Path, tmp_path: Path): + return adopter.Source.open(str(source), tmp_path / "read") + + +# --- where the history holds a gitlink ------------------------------------- + +@pytest.mark.parametrize("history, verb", HISTORIES.values(), + ids=list(HISTORIES)) +def test_the_history_is_read_where_the_extraction_reads_it( + adopter, tmp_path, history, verb): + """Each of these failed `execute`'s extraction on main (measured with + git 2.43 and `git filter-repo` 2.47.0). The source's own configuration + hides a gitlink from a plain `git log` twice over, and its checked-out + `.gitmodules` is the file git's submodule reader dies on, which a `git + log` that ran that reader would die on too.""" + source = history_source(tmp_path, history) + git("config", "diff.ignoreSubmodules", "all", cwd=source) + git("config", "log.showRoot", "false", cwd=source) + assert (source / GITMODULES).read_bytes() == BARE_PATH.encode() + found = adopter._exported_gitlink(opened(adopter, source, tmp_path)) + assert re.fullmatch(named(verb), found), found + + +def test_a_history_with_no_gitlink_is_read_to_its_root(adopter, tmp_path): + source = history_source(tmp_path, None) + assert adopter._exported_gitlink(opened(adopter, source, tmp_path)) \ + is None + + +def test_a_history_git_cannot_read_is_refused_not_taken_to_hold_none( + adopter, tmp_path): + """A tree of the first commit, gone from the object store: the scan + cannot finish, and a history it could not read has not been shown to + hold no gitlink.""" + source = history_source(tmp_path, None) + tree = git("rev-parse", "HEAD~1^{tree}", cwd=source).stdout.strip() + loose = source / ".git" / "objects" / tree[:2] / tree[2:] + os.chmod(loose, stat.S_IWRITE) # git writes objects read-only + loose.unlink() + with pytest.raises(adopter.Refusal) as caught: + adopter._exported_gitlink(opened(adopter, source, tmp_path)) + assert caught.value.code == "git-failed" + assert tree in caught.value.detail + + +def test_a_signature_the_source_would_show_does_not_reach_the_scan( + adopter, tmp_path, monkeypatch): + """`log.showSignature` has `git log` run gpg on each signed commit and + write what gpg says into the output the scan reads, ahead of the + commit. Here the commit that adds the gitlink is signed, and the scan + still names it. gpg gets a home of its own, so that a scan which did run + it would not write into the person's.""" + monkeypatch.setenv("GNUPGHOME", str(tmp_path / "gnupg")) + source = history_source(tmp_path, None) + git("config", "log.showSignature", "true", cwd=source) + tip = git("rev-parse", "HEAD", cwd=source).stdout.strip() + link(source) + tree = git("write-tree", cwd=source).stdout.strip() + git("reset", "-q", cwd=source) + signed = subprocess.run( + ["git", "hash-object", "-t", "commit", "-w", "--stdin"], cwd=source, + input=(f"tree {tree}\nparent {tip}\n" + "author Source Human 1700000000 +0000" + "\ncommitter Source Human 1700000000 " + "+0000\ngpgsig -----BEGIN PGP SIGNATURE-----\n \n" + " iQEzBAABCAAdFiEE\n -----END PGP SIGNATURE-----\n\n" + "Add vendor/b, signed\n").encode(), + capture_output=True, check=True).stdout.decode().strip() + git("branch", "signed", signed, cwd=source) + assert adopter._exported_gitlink(opened(adopter, source, tmp_path)) \ + == f"commit {signed[:12]} adds `vendor/b`" + + +# --- which commits the scan reads ------------------------------------------- + +@pytest.mark.parametrize("tagged, verb", [(False, None), (True, "adds")], + ids=["on-a-branch-alone", "tagged"]) +def test_an_org_repo_source_is_read_as_the_extraction_clones_it( + adopter, tmp_path, monkeypatch, tagged, verb): + """`Source.open` clones an `org/repo` source, and that clone's one + branch is the default: a branch never merged is a remote-tracking ref + there, which the extraction's clone of it does not take, and a tag is a + tag in both. Measured on main, from a clone of this source, the first + adopted and the second failed the extraction after both legs were made. + `git clone` of the local source stands in for `gh repo clone`.""" + source = history_source(tmp_path, on_a_branch_never_merged) + if tagged: + as_human("tag", "on-other", "other", cwd=source) + run = adopter.run + + def cloned(args, cwd=None, capture=True): + if args[:3] == ["gh", "repo", "clone"]: + return run(["git", "clone", "--quiet", str(source), args[4]]) + return run(args, cwd, capture) + + monkeypatch.setattr(adopter, "run", cloned) + clone = adopter.Source.open("testorg/Thing", tmp_path / "work") + git("rev-parse", "--verify", "-q", "refs/remotes/origin/other", + cwd=clone.path) + found = adopter._exported_gitlink(clone) + if verb is None: + assert found is None, found + else: + assert re.fullmatch(named(verb), found), found + + +def test_the_scan_starts_at_the_plans_commit_not_the_source_head(adopter, + tmp_path): + """`Source.open` reads the default branch, not HEAD. A commit only the + source's detached HEAD reaches is left behind by `_extract_leg`'s + checkout of the plan's commit, and beside this `.gitmodules` it adopted + on main: the scan passes it by. The commit the scan starts at is the + one the plan names, which a branch need not reach.""" + source = history_source(tmp_path, None) + as_human("checkout", "-q", "--detach", cwd=source) + link(source) + as_human("commit", "-qm", "Add vendor/b, detached", cwd=source) + detached = git("rev-parse", "HEAD", cwd=source).stdout.strip() + read = opened(adopter, source, tmp_path) + assert read.commit == git("rev-parse", "main", cwd=source).stdout.strip() + assert adopter._exported_gitlink(read) is None + there = adopter.Source(read.path, read.repository, read.branch, detached) + assert re.fullmatch(named("adds"), adopter._exported_gitlink(there)) + + +# --- what `check` finds ----------------------------------------------------- + +def planned(source: Path, base: Path, answers: dict) -> Path: + """`plan` for `source`, its `.gitmodules` question answered and any + other entry's `leg:` set as `answers` says.""" + plan = base / "plan" / "adoption-plan.yaml" + plan.parent.mkdir() + written = write_plan(source, plan, project=PROJECT) + assert written.returncode == 0, written.stderr + written.stdout + text = plan.read_bytes().decode() + for path, leg in answers.items(): + entry = re.search(rf" - path: {re.escape(path)}\n leg: (\S+)\n", + text) + assert entry, f"no entry {path} in the plan" + if entry[1] == "null": + resolve(plan, path, leg) + text = plan.read_bytes().decode() + else: + text = text.replace(entry[0], + f" - path: {path}\n leg: {leg}\n", 1) + plan.write_bytes(text.encode()) + return plan + + +def findings(plan: Path) -> list[tuple[str, str]]: + result = run_script(ADOPT, "check", "--plan", str(plan)) + found = [tuple(line.split(" ", 1)[1].split(": ", 1)) + for line in result.stderr.splitlines() + if line.startswith("FINDING ")] + assert result.returncode == (1 if found else 0), result.stderr + return found + + +def in_the_history(adopter, verb: str = "removes") -> re.Pattern: + """The end of the finding for a gitlink only the history holds.""" + return re.compile(re.escape(adopter.FAILS_IN_THE_HISTORY).replace( + re.escape("{}"), named(verb)) + "$") + + +#: Where the plan sends `.gitmodules`; the two legs are extracted either way. +EITHER_WAY = pytest.mark.parametrize("modules_leg", ["code", "drop"]) + + +@EITHER_WAY +@pytest.mark.parametrize("tip, said", [ + (BARE_PATH, "missing value for 'submodule.b.path'"), + ('[submodule "b"]\n\tpath = vendor/b\n\turl\n', + "missing value for 'submodule.b.url'"), + # git's submodule reader dies on these three with no value as well; + # `branch` crashes git 2.43 outright, with nothing said. + (REGISTRATION + "\tignore\n", "missing value for 'submodule.b.ignore'"), + (REGISTRATION + "\tupdate\n", "missing value for 'submodule.b.update'"), + (REGISTRATION + "\tbranch\n", "missing value for 'submodule.b.branch'"), + (BAD_LINE, "fatal: bad config line 2 in .gitmodules"), +], ids=["path", "url", "ignore", "update", "branch", "bad-line"]) +def test_check_finds_a_gitmodules_the_extraction_cannot_read( + adopter, tmp_path, modules_leg, tip, said): + """On main each of these passed `check`, but for a bad line kept in a + leg, which it said fails in the CLONE: the extraction fails first.""" + plan = planned(history_source(tmp_path, tip=tip), tmp_path, + {GITMODULES: modules_leg}) + found = findings(plan) + assert [code for code, _ in found] == [UNREADABLE], found + detail = found[0][1] + assert detail.startswith("git cannot read the source's .gitmodules for " + f"its submodules ({said}), "), detail + assert in_the_history(adopter).search(detail), detail + + +def test_check_names_a_commit_no_branch_of_the_adoption_holds(adopter, + tmp_path): + """The submodule is on a branch never merged: the person reading the + finding finds no such commit on `main`, so it is named.""" + plan = planned(history_source(tmp_path, on_a_branch_never_merged), + tmp_path, {GITMODULES: "code"}) + found = findings(plan) + assert [code for code, _ in found] == [UNREADABLE], found + assert in_the_history(adopter, "adds").search(found[0][1]), found + + +@pytest.mark.parametrize("key", ["ignore", "update", "branch"]) +def test_a_key_with_no_value_beside_a_submodule_at_the_tip_is_found( + adopter, tmp_path, key): + """`ignore`, `update` and `branch` with no value are read as `path` and + `url` were: git's submodule reader dies on each. On main, beside a + submodule at the tip, `check` passed all three, and `execute` died in + `git fast-export` after both leg repositories were made -- on `branch` + with a crash and nothing said.""" + source = make_source_repo(tmp_path / "Thing", tree=SMALL_TREE, edits=()) + put_gitmodules(source, REGISTRATION + f"\t{key}\n") + link(source) + as_human("commit", "-qm", "Add vendor/b", cwd=source) + plan = planned(source, tmp_path, {GITMODULES: "code", "vendor/": "code"}) + assert findings(plan) == [( + UNREADABLE, "git cannot read the source's .gitmodules for its " + f"submodules (missing value for 'submodule.b.{key}'), " + + adopter.FAILS_IN_THE_EXTRACTION)] + + +def test_a_gitmodules_that_is_no_file_at_the_tip_is_read_as_on_main( + adopter, tmp_path): + """A `.gitmodules` the tip holds as a symlink is no file the clone + checks out for git to read: it is answered as on main, with what git + says of its blob, and the history is not asked.""" + source = history_source(tmp_path) + target = subprocess.run(["git", "hash-object", "-w", "--stdin"], + cwd=source, input=b"../elsewhere/.gitmodules", + capture_output=True, check=True) + entries = [line if not line.endswith(f"\t{GITMODULES}") else + f"120000 blob {target.stdout.decode().strip()}\t{GITMODULES}" + for line in git("ls-tree", "HEAD", cwd=source + ).stdout.splitlines()] + tree = subprocess.run(["git", "mktree"], cwd=source, + input="".join(f"{line}\n" for line in entries + ).encode(), + capture_output=True, check=True) + commit = as_human("commit-tree", tree.stdout.decode().strip(), "-p", + "HEAD", "-m", "A symlink .gitmodules", cwd=source) + git("update-ref", "refs/heads/main", commit.stdout.strip(), cwd=source) + read = opened(adopter, source, tmp_path) + tree = read.tree() + assert (GITMODULES, "120000") in [(path, mode) for path, mode, _, _ + in tree] + answer = read.registered_submodules(tree) + assert type(answer) is str, repr(answer) + assert answer == "fatal: bad config line 1 in .gitmodules" + + +@pytest.mark.parametrize("tip", [ + # read as true, and git's submodule reader goes on + REGISTRATION + "\tfetchRecurseSubmodules\n", + REGISTRATION + "\tshallow\n", + # a well-formed file registering a submodule the tree no longer holds + REGISTRATION, + # no `.gitmodules` at the tip at all: nothing for the reader to read + None, +], ids=["fetchRecurseSubmodules", "shallow", "stale-registration", + "no-gitmodules"]) +def test_check_passes_a_gitmodules_the_extraction_reads(tmp_path, tip): + """Each of these adopts on main with the history's submodule, and + `check` still passes it.""" + answers = {GITMODULES: "code"} if tip is not None else {} + plan = planned(history_source(tmp_path, tip=tip), tmp_path, answers) + assert findings(plan) == [] + + +def test_a_plan_that_extracts_no_leg_is_not_refused(tmp_path): + """Every path rooted or dropped: `git filter-repo` never runs, so + nothing reads the file, and on main this adopts and verifies.""" + plan = planned(history_source(tmp_path), tmp_path, + {GITMODULES: "drop", "src/": "root", "specs/": "root"}) + assert findings(plan) == [] + + +def test_a_key_with_no_value_and_no_gitlink_anywhere_still_passes(tmp_path): + """#166's S3, unchanged: with no gitlink in the tree or its history, + nothing reads the file with git's submodule reader.""" + plan = planned(history_source(tmp_path, None), tmp_path, + {GITMODULES: "code"}) + assert findings(plan) == [] + + +# --- what `execute` refuses ------------------------------------------------- + +@EITHER_WAY +def test_execute_refuses_it_before_any_leg_exists(adopter, tmp_path, + monkeypatch, modules_leg): + """The refusal is `check`'s finding, raised by `_refuse_what_check_finds` + before `_create_leg_remotes`: the directory the legs go in is never + made.""" + monkeypatch.setattr(adopter, "_require_filter_repo", lambda: None) + plan = planned(history_source(tmp_path), tmp_path, + {GITMODULES: modules_leg}) + found = findings(plan) + remotes = tmp_path / REMOTES + with pytest.raises(adopter.Refusal) as caught: + adopter.cmd_execute(Namespace( + plan=str(plan), source=None, local_remote_dir=remotes, + allow_empty_leg=[], yes=True, work_dir=tmp_path / "work")) + refusal = caught.value + assert refusal.code == UNREADABLE + assert refusal.detail.splitlines()[1:] == [ + f" FINDING {code}: {detail}" for code, detail in found] + assert "no leg repository was created" in refusal.remediation + assert refusal.remediation.endswith("\n" + adopter.SUBMODULE_REMEDIATION) + assert not remotes.exists() + + +# --- end to end ------------------------------------------------------------- + +@needs_filter_repo +def test_the_issue_shape_is_refused_and_the_repaired_source_then_adopts( + tmp_path): + """#195 as the issue measured it: `check` 0 and `execute` 2 after both + leg remotes existed, so that the corrected re-run met + `leg-remote-exists`. Now `check` finds it, `execute` refuses it with no + leg made, and once the source's `.gitmodules` is repaired the same + `--local-remote-dir` takes the adoption.""" + source = history_source(tmp_path) + plan = planned(source, tmp_path, {GITMODULES: "code"}) + checked = run_script(ADOPT, "check", "--plan", str(plan)) + assert checked.returncode == 1, checked.stderr + checked.stdout + assert (f"FINDING {UNREADABLE}: git cannot read the source's .gitmodules " + "for its submodules (missing value for 'submodule.b.path'), and " + "the source's tip holds no submodule but the history `git " + "filter-repo` rewrites does") in checked.stderr + remotes, work = tmp_path / REMOTES, tmp_path / "work" + flags = ("--yes", "--local-remote-dir", str(remotes)) + refused = run_script(ADOPT, "execute", "--plan", str(plan), *flags, + "--work-dir", str(work)) + assert refused.returncode == 2, refused.stderr + refused.stdout + assert f"REFUSED {UNREADABLE}: 1 finding(s) in " in refused.stderr + assert "creating the leg repositories" not in refused.stdout + assert not remotes.exists() or not any(remotes.iterdir()), \ + "a refused plan left a leg repository behind" + + put_gitmodules(source, REGISTRATION) + as_human("commit", "-qm", "Repair .gitmodules", cwd=source) + repaired = tmp_path / "repaired" + repaired.mkdir() + plan = planned(source, repaired, {GITMODULES: "code"}) + assert run_script(ADOPT, "check", "--plan", str(plan)).returncode == 0 + adopted = run_script(ADOPT, "execute", "--plan", str(plan), *flags, + "--work-dir", str(repaired / "work")) + assert adopted.returncode == 0, adopted.stderr + adopted.stdout + assert "adoption verified" in adopted.stdout + for role in ("spec", "code"): + assert (remotes / f"{PROJECT}-{role}.git").is_dir() From c800efd463be4bf040f7c96f0bb8e26bc2c55180 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:27:15 +0000 Subject: [PATCH 2/5] adopt-project.py: a `.gitmodules` value git's submodule reader refuses (`update = bogus`, `update =`, `update = !cmd`, `shallow = maybe`, `fetchRecurseSubmodules = bogus`) is a `check` finding and an `execute` refusal, so `execute` no longer dies in `git fast-export` after both leg repositories exist, whether the tip or only the history holds the gitlink (#215) git's submodule reader (submodule-config.c, `parse_config`) types three keys of a `.gitmodules` and dies on a value it refuses for one of them. `git filter-repo` runs `git fast-export --all`, which runs that reader on the clone's checked-out `.gitmodules` for every gitlink an exported commit adds, changes or removes (#195). `git config --list` reads every such value and `_registrations` checked none, so `check` said `plan ok`, and `execute` died in the extraction, with nothing on stderr (#171), after both leg repositories were made: a corrected re-run then met `leg-remote-exists`. Measured on main a5b09c3 and on 0023dde (#226), the tip holding the gitlink or only an earlier commit. The rule follows git's source. For these three keys it is the same in git 2.39.5 through 2.51.0, but for the one bound named below, and it was checked against git 2.43 over 252 `.gitmodules` cases with `git fast-export`: - git reads the checked-out file through `gitmodules_cb`, which sets `overwrite`, so every value is read, a key given twice included, and git dies on the first key it refuses in the file. The finding names that key, whether it is one of #195's keys with no value or one of these. A name git ignores (`check_submodule_name`) is never read. - `update` (`parse_submodule_update_strategy`, submodule.c) is `checkout`, `rebase`, `merge` or `none`, exactly. A `!command` is refused in a `.gitmodules`, and so is anything else, an empty value included: `invalid value for 'submodule..update'`. - `shallow` (`git_config_bool`) is a boolean `git_parse_maybe_bool` reads: empty, `true`/`yes`/`on`/`false`/`no`/`off` in any ASCII case, or an integer `git_parse_int` takes, with a `k`, `m` or `g` unit. Anything else: `bad boolean config value '' for ''`. - `fetchRecurseSubmodules` (`parse_fetch_recurse`, with `die_on_error` for the checked-out file) is such a boolean or exactly `on-demand`. Anything else: `bad argument: `. - The finding is #195's `plan-gitmodules-unreadable`, ending where git fails: the extraction beside a gitlink at the tip, and the history, naming the commit, where only the history holds one. `execute` refuses it through #168's shared list before any leg exists. Its words are git's, with the key and the value spelled as `plan` spells a path, and it says which values `update` and `fetchRecurseSubmodules` take. - Not refused, because git reads them: `ignore` with a value it does not know (`ignore = bogus` warns and goes on), and any value of `path`, `url` or `branch` (`branch =`). With no gitlink at the tip or in the history, a refused value still registers nothing and adopts, as #166's S3 does for a key with no value: nothing runs the reader on it. - Where git versions or C libraries differ, `check` takes what any of them reads. git 2.50 moved `git_parse_signed`'s bound below zero by one (`(-max - 1) / factor`), so -2147483648, -2097152k, -2048m and -2g pass `check` though git 2.43 refuses them. A `0b` number passes too: a git built on glibc 2.38 or later reads it (git 2.43 here does), and others refuse it. Those few are left to the extraction, as on main. Measured on main a5b09c3, on 0023dde and on this head, with git 2.43.0 and git filter-repo 2.47.0. Each of the five refused lines, at the tip or only in the history, was `check` 0 and `execute` 2 with both leg repositories left on main and on 0023dde. It is now `check` 1 and `execute` 2, refused with no remotes directory made. With no gitlink anywhere each is `check` 0 and `execute` 0 on all three heads, and `ignore = bogus` and `branch =` are `check` 0 and `execute` 0, `adoption verified`, in every position on all three. Unchanged, diffed byte for byte over 23 sources on the three heads: `plan`'s exit, output and plan file in all 23, and `check`'s exit and output in the 13 whose verdict does not change. Left out: #195's `missing value for ''` still prints the key in git's quotes, not spelled, so a submodule name holding a control character reaches that finding raw. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 131 +++++++- tests/test_adopt_gitmodules_values.py | 464 ++++++++++++++++++++++++++ 2 files changed, 582 insertions(+), 13 deletions(-) create mode 100644 tests/test_adopt_gitmodules_values.py diff --git a/adopt-project.py b/adopt-project.py index 4322110c..73985992 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -350,20 +350,120 @@ def _move(self, name: str, path: str) -> None: #: and nothing dies. SUBMODULE_VALUED_KEY_RE = re.compile( r"submodule\.(?P.*)\.(?:path|url|ignore|update|branch)") +#: The keys git's submodule reader dies on when one has a VALUE it refuses, +#: under a name it does not ignore (#215); `_refused_value` says which +#: values. `ignore` with a value git does not know only warns, and `path`, +#: `url` and `branch` take any value, so none of them is here. +SUBMODULE_TYPED_KEY_RE = re.compile( + r"submodule\.(?P.*)\.(?Pupdate|shallow|fetchrecursesubmodules)") +#: The values `update` may take in a `.gitmodules` (submodule.c, +#: `parse_submodule_update_type`), as git spells them, case and all. +UPDATE_STRATEGIES = ("checkout", "rebase", "merge", "none") +#: The words git reads as a boolean, in any ASCII case (config.c / parse.c, +#: `git_parse_maybe_bool_text`). +BOOLEAN_WORDS = ("true", "yes", "on", "false", "no", "off") +#: An integer as `git_parse_signed` reads one: `strtoimax(value, &end, 0)`, +#: which skips C whitespace and takes a sign and a hex, octal or decimal +#: number -- and, built against glibc 2.38 or later, a `0b` binary one -- and +#: then a unit, which is all that may follow. +GIT_INTEGER_RE = re.compile( + r"[ \t\n\v\f\r]*(?P[+-]?)(?P0[xX][0-9a-fA-F]+|0[bB][01]+" + r"|0[0-7]*|[1-9][0-9]*)(?P[kKmMgG]?)") +#: `get_unit_factor`, by the unit's lower case. +UNIT_FACTORS = {"": 1, "k": 1 << 10, "m": 1 << 20, "g": 1 << 30} +#: The `int` git reads a boolean's number into: 2**31 - 1 is its largest. +GIT_INT_BITS = 31 + + +def _git_reads_as_a_boolean(value: str) -> bool: + """Whether git takes `value` as a boolean (`git_parse_maybe_bool`): + empty, which is false; one of BOOLEAN_WORDS; or an integer + `git_parse_int` takes, which is true unless it is zero. + + The integer must fit an `int` once its unit is applied. git 2.50 moved + the bound below zero by one (`(-max - 1) / factor`, where 2.49 and + before had `-max / factor`), and the wider range is the one taken here, + so that nothing a newer git reads is refused. An older git refuses the + four values that move let in, -2147483648, -2097152k, -2048m and -2g, + however spelled; those are left to the extraction, as on main, and so + is a `0b` number, which git reads only where its C library does.""" + if not value or (value.isascii() and value.lower() in BOOLEAN_WORDS): + return True + number = GIT_INTEGER_RE.fullmatch(value) + if number is None: + return False + digits = number["digits"] + if digits[1:2] in ("x", "X"): + size = int(digits[2:], 16) + elif digits[1:2] in ("b", "B"): + size = int(digits[2:], 2) + else: + size = int(digits, 8 if digits.startswith("0") else 10) + factor = UNIT_FACTORS[number["unit"].lower()] + if number["sign"] == "-": + return size <= (1 << GIT_INT_BITS) // factor + return size <= ((1 << GIT_INT_BITS) - 1) // factor + + +def _refused_value(key: str, value: str) -> str | None: + """What git's submodule reader dies on, in its words with the key and + the value spelled, when `key` is one SUBMODULE_TYPED_KEY_RE names and + `value` one git refuses for it; None otherwise (#215). + + git reads the CHECKED-OUT `.gitmodules` through `gitmodules_cb` + (submodule-config.c), which sets `overwrite`, so `parse_config` reads + every value, a key given twice included, and dies on the first it + refuses. Read in the source code of git 2.39.5 through 2.51.0, the + same in each but for the bound `_git_reads_as_a_boolean` names, and + measured with git 2.43 over `git fast-export`: + + * `update`, `parse_submodule_update_strategy` (submodule.c): one of + UPDATE_STRATEGIES, or `!` and a command, which `parse_config` + refuses in a `.gitmodules`; anything else, an empty value too, ends + it with `invalid value for 'submodule..update'`. + * `shallow`, `git_config_bool`: a value `git_parse_maybe_bool` does not + read ends it with `bad boolean config value '' for ''`. + * `fetchrecursesubmodules`, `parse_fetch_recurse`, which dies on an + error for the checked-out file (`die_on_error`): a boolean or + `on-demand`, exactly; anything else ends it with `bad + argument: `.""" + typed = SUBMODULE_TYPED_KEY_RE.fullmatch(key) + if typed is None or _a_name_git_ignores(typed["name"]): + return None + said = _spelled(value) if value else "an empty value" + if typed["key"] == "update": + if value in UPDATE_STRATEGIES: + return None + why = (f"a command, which git never takes from a {GITMODULES}" + if value.startswith("!") else + f"which is not one of {_listed(list(UPDATE_STRATEGIES))}") + return f"invalid value for {_spelled(key)}: {said}, {why}" + if _git_reads_as_a_boolean(value): + return None + if typed["key"] == "shallow": + return f"bad boolean config value {said} for {_spelled(key)}" + if value == "on-demand": + return None + return (f"bad {_spelled(key)} argument: {said}, which is neither a " + "boolean nor `on-demand`") def _registrations(listed: bytes) -> Registrations | str: - """`git config -z --list` output, read by `Registrations`; or, for a - key SUBMODULE_VALUED_KEY_RE names with no value at all, what git's - submodule reader dies on. `-z` prints such a key with no newline, and - git looks for its value after it has filtered the name and before it - filters the value.""" + """`git config -z --list` output, read by `Registrations`; or what git's + submodule reader dies on, at the first key in the file it dies on: one + SUBMODULE_VALUED_KEY_RE names with no value at all, or one whose value + it refuses (`_refused_value`, #215). `-z` prints a key with no value + with no newline, and git looks for its value after it has filtered the + name and before it filters the value.""" registrations = Registrations() for record in listed.split(b"\x00"): key, newline, value = _git_path(record).partition("\n") valued = SUBMODULE_VALUED_KEY_RE.fullmatch(key) if valued and not newline and not _a_name_git_ignores(valued["name"]): return f"missing value for '{key}'" + refused = _refused_value(key, value) if newline else None + if refused: + return refused match = SUBMODULE_KEY_RE.fullmatch(key) if not match: continue @@ -605,9 +705,14 @@ def registered_submodules(self, tree: list) -> Registrations | str: as registering nothing and `execute` died in `git fast-export` with `missing value for 'submodule.b.path'`, after both leg repositories were made. Where the history holds none either, nothing ever reads - the file that way, and it registers nothing (#166's S3). A line git - cannot parse at all is another matter: every reader dies on it, and - the history says only which reader dies first. + the file that way, and it registers nothing (#166's S3). A VALUE + git's submodule reader refuses -- `update = bogus`, `shallow = + maybe`, `_refused_value` -- is read the same way, wherever + the gitlink is (#215): `git config --list` exits 0 on it, and on + main `check` passed it while `execute` died in `git fast-export` + after both leg repositories were made. A line git cannot parse at + all is another matter: every reader dies on it, and the history says + only which reader dies first. """ if not any(path == GITMODULES for path, _, _, _ in tree): return Registrations() @@ -1988,11 +2093,11 @@ def _where_git_fails(entries: list, tree: list, registry_leg: str | None, gitlink, which the clone checks out as a directory and which registers nothing. - A key with no value is fatal to git's submodule reader alone, which git - runs only for a gitlink, so `Source.registered_submodules` reads it as - registering nothing where neither the tree nor the history `git - fast-export` diffs holds one, and it reaches here only where one of them - does. + A key with no value, or a value git refuses (#215), is fatal to git's + submodule reader alone, which git runs only for a gitlink, so + `Source.registered_submodules` reads it as registering nothing where + neither the tree nor the history `git fast-export` diffs holds one, and + it reaches here only where one of them does. """ a_file = any(path == GITMODULES and mode in FILE_MODES for path, mode, _, _ in tree) diff --git a/tests/test_adopt_gitmodules_values.py b/tests/test_adopt_gitmodules_values.py new file mode 100644 index 00000000..27fa7f32 --- /dev/null +++ b/tests/test_adopt_gitmodules_values.py @@ -0,0 +1,464 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A `.gitmodules` VALUE git's submodule reader refuses is a finding (#215). + +git's submodule reader (submodule-config.c, `parse_config`) types three of +the keys it reads, and dies on a value it refuses: an `update` that is not +`checkout`, `rebase`, `merge` or `none` (a `!command` among them), a +`shallow` that is not a boolean, and a `fetchRecurseSubmodules` that is +neither a boolean nor `on-demand`. `git config --list` reads every one of +them, so on main `check` said `plan ok`, and `execute` died in the `git +fast-export` that `git filter-repo` runs, after both leg repositories were +made, whether the tip held the gitlink or only the history did (#195): a +corrected re-run then met `leg-remote-exists`. + +All repositories here are local. Only the end-to-end tests at the bottom +need `git filter-repo`. The rest run `check`, call `cmd_execute` in this +process with its probe for the tool stubbed, because the refusal under test +comes before the tool would be used, or ask the installed git's own +submodule reader, through `git fast-export`, what it makes of a value. +""" + +from __future__ import annotations + +import importlib.util +import re +import shutil +import subprocess +from argparse import Namespace +from pathlib import Path + +import pytest + +from conftest import ADOPT, FILE_PROTOCOL, git, make_source_repo, resolve, \ + run_script, write_plan + +needs_filter_repo = pytest.mark.skipif( + shutil.which("git-filter-repo") is None, + reason="git filter-repo is not installed: `pip install git-filter-repo`") + +PROJECT = "Northwind" +GITMODULES = ".gitmodules" +UNREADABLE = "plan-gitmodules-unreadable" +REMOTES = "remotes" +#: The submodule, registered under the name `b`. +GITLINK = "vendor/b" +REGISTRATION = '[submodule "b"]\n\tpath = vendor/b\n\turl = ../b\n' +#: A source whose plan asks about `.gitmodules`, and about `vendor/` where +#: the tip holds the submodule: `specs/` and `src/` classify themselves and +#: `README.md` stays in the root. +SMALL_TREE = {"README.md": "# Thing\n", "src/app/util.py": "VALUE = 1\n", + "specs/001-feature/tasks.md": "- [ ] one\n"} +#: What every value of `update` that git refuses is told, after the value. +NOT_A_STRATEGY = "which is not one of `checkout`, `rebase`, `merge`, `none`" + +#: The issue's table, the lines git refuses: each one more line under +#: `[submodule "b"]`, and what the finding says git dies on. Measured with +#: git 2.43 on main a5b09c3: `check` 0 and `execute` 2 with both leg +#: repositories left behind, the tip holding the gitlink or the history. +REFUSED = { + "update = bogus": + f"invalid value for `submodule.b.update`: `bogus`, {NOT_A_STRATEGY}", + "update =": "invalid value for `submodule.b.update`: an empty value, " + + NOT_A_STRATEGY, + "update = !cmd": "invalid value for `submodule.b.update`: `!cmd`, a " + "command, which git never takes from a .gitmodules", + "shallow = maybe": + "bad boolean config value `maybe` for `submodule.b.shallow`", + "fetchRecurseSubmodules = bogus": + "bad `submodule.b.fetchrecursesubmodules` argument: `bogus`, which " + "is neither a boolean nor `on-demand`", +} +#: The issue's table, the lines git reads, and three more: `ignore = bogus` +#: only warns, `branch` takes any value, and the others are values git takes. +#: Each adopted on main and still does. +ACCEPTED = ["ignore = bogus", "branch =", "update = none", "shallow = 0x10", + "fetchRecurseSubmodules = on-demand"] + + +@pytest.fixture(scope="module") +def adopter(): + spec = importlib.util.spec_from_file_location("adopt_gitmodules_values", + ADOPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def as_human(*args: str, cwd: Path): + """`git ` with an identity, for the commits below.""" + return git("-c", "user.name=Source Human", + "-c", "user.email=source@invalid.example", *args, cwd=cwd) + + +def put_gitmodules(source: Path, text: str | bytes) -> None: + """`text` as the source's `.gitmodules`, staged: BYTES, so it is LF on + every platform, as a hand edit would leave it.""" + (source / GITMODULES).write_bytes( + text if isinstance(text, bytes) else text.encode()) + git("add", "--", GITMODULES, cwd=source) + + +def link(source: Path) -> None: + """A gitlink at GITLINK, staged straight into the index, as `git add` of + an embedded clone records one; no checkout of it is needed.""" + head = git("rev-parse", "main", cwd=source).stdout.strip() + git("update-index", "--add", "--cacheinfo", f"160000,{head},{GITLINK}", + cwd=source) + + +def source_with(base: Path, gitmodules: str | bytes, where: str) -> Path: + """A source whose `.gitmodules` at the tip is `gitmodules`, and whose + submodule is `where`: at the `tip`; only in the `history`, added and + removed by two earlier commits under a well-formed `.gitmodules`; or + `nowhere`, in no commit at all.""" + source = make_source_repo(base / "Thing", tree=SMALL_TREE, edits=()) + if where == "history": + put_gitmodules(source, REGISTRATION) + link(source) + as_human("commit", "-qm", "Add vendor/b", cwd=source) + git("rm", "-q", "--cached", "--", GITLINK, cwd=source) + as_human("commit", "-qm", "Remove vendor/b", cwd=source) + put_gitmodules(source, gitmodules) + if where == "tip": + link(source) + as_human("commit", "-qm", "The tip .gitmodules", cwd=source) + held = git("ls-tree", "-r", "HEAD", cwd=source).stdout + assert ("160000" in held) == (where == "tip"), held + return source + + +def with_line(line: str) -> str: + """REGISTRATION and one more line under it, as the issue wrote each.""" + return REGISTRATION + f"\t{line}\n" + + +#: Where the plan sends `.gitmodules`, and the submodule's entry where the +#: tip holds one: to the code leg, as the issue's plan did. +ANSWERS = {"tip": {GITMODULES: "code", "vendor/": "code"}, + "history": {GITMODULES: "code"}, "nowhere": {GITMODULES: "code"}} + + +def planned(source: Path, base: Path, answers: dict) -> Path: + """`plan` for `source`, each entry `answers` names given its leg.""" + plan = base / "plan" / "adoption-plan.yaml" + plan.parent.mkdir() + written = write_plan(source, plan, project=PROJECT) + assert written.returncode == 0, written.stderr + written.stdout + text = plan.read_bytes().decode() + for path, leg in answers.items(): + entry = re.search(rf" - path: {re.escape(path)}\n leg: (\S+)\n", + text) + assert entry, f"no entry {path} in the plan" + if entry[1] == "null": + resolve(plan, path, leg) + text = plan.read_bytes().decode() + else: + text = text.replace(entry[0], + f" - path: {path}\n leg: {leg}\n", 1) + plan.write_bytes(text.encode()) + return plan + + +def findings(plan: Path) -> list[tuple[str, str]]: + result = run_script(ADOPT, "check", "--plan", str(plan)) + found = [tuple(line.split(" ", 1)[1].split(": ", 1)) + for line in result.stderr.splitlines() + if line.startswith("FINDING ")] + assert result.returncode == (1 if found else 0), result.stderr + return found + + +def unreadable(said: str) -> str: + """How the finding begins: what git's submodule reader dies on.""" + return (f"git cannot read the source's .gitmodules for its submodules " + f"({said}), ") + + +def in_the_history(adopter) -> re.Pattern: + """How the finding ends for a submodule only the history holds: the + commit that removed it, which `git log` meets first.""" + return re.compile(re.escape(adopter.FAILS_IN_THE_HISTORY).replace( + re.escape("{}"), r"commit [0-9a-f]{12} removes `vendor/b`") + "$") + + +# --- what `check` finds ----------------------------------------------------- + +@pytest.mark.parametrize("where", ["tip", "history"]) +@pytest.mark.parametrize("line, said", REFUSED.items(), ids=list(REFUSED)) +def test_check_finds_a_value_git_refuses(adopter, tmp_path, line, said, + where): + """Each row of the issue's table that git refuses, in both of its + positions. On main each passed `check` and failed `execute` in `git + fast-export` after both leg repositories were made.""" + plan = planned(source_with(tmp_path, with_line(line), where), tmp_path, + ANSWERS[where]) + found = findings(plan) + if where == "tip": + assert found == [(UNREADABLE, unreadable(said) + + adopter.FAILS_IN_THE_EXTRACTION)] + return + assert [code for code, _ in found] == [UNREADABLE], found + assert found[0][1].startswith(unreadable(said)), found + assert in_the_history(adopter).search(found[0][1]), found + + +@pytest.mark.parametrize("where", ["tip", "history", "nowhere"]) +@pytest.mark.parametrize("line", ACCEPTED) +def test_check_passes_a_value_git_reads(tmp_path, line, where): + """A value git only warns about, or reads, is no finding wherever the + submodule is: `ignore = bogus` says `warning: Invalid parameter` and + goes on, and `git fast-export` exits 0.""" + plan = planned(source_with(tmp_path, with_line(line), where), tmp_path, + ANSWERS[where]) + assert findings(plan) == [] + + +@pytest.mark.parametrize("line", REFUSED) +def test_a_refused_value_beside_no_gitlink_anywhere_still_passes(tmp_path, + line): + """#166's S3, for a value: with no gitlink in the tree or its history, + git never runs its submodule reader on the file, and main adopted and + cloned each of these.""" + plan = planned(source_with(tmp_path, with_line(line), "nowhere"), + tmp_path, ANSWERS["nowhere"]) + assert findings(plan) == [] + + +@pytest.mark.parametrize("gitmodules, said", [ + # git reads the file in order and dies on the first key it refuses. + (with_line("shallow = maybe") + "\tpath\n", + "bad boolean config value `maybe` for `submodule.b.shallow`"), + (with_line("update") + "\tshallow = maybe\n", + "missing value for 'submodule.b.update'"), + # A key given twice: git reads the checked-out file with `overwrite` + # set, so every value is read, and a good one first does not help. + (with_line("update = checkout") + "\tupdate = bogus\n", + f"invalid value for `submodule.b.update`: `bogus`, {NOT_A_STRATEGY}"), + # Under a name that registers nothing: git reads the whole file. + (REGISTRATION + '[submodule "c"]\n\tupdate = bogus\n', + f"invalid value for `submodule.c.update`: `bogus`, {NOT_A_STRATEGY}"), + # git lower-cases the key, and so does the finding. + (with_line("FetchRecurseSubmodules = maybe"), + "bad `submodule.b.fetchrecursesubmodules` argument: `maybe`, which is " + "neither a boolean nor `on-demand`"), + # Under a name git ignores, nothing is read, and nothing dies. + (REGISTRATION + '[submodule "../x"]\n\tupdate = bogus\n', None), + (REGISTRATION + '[submodule ""]\n\tshallow = maybe\n', None), +], ids=["first-of-two-is-a-value", "first-of-two-has-no-value", + "a-key-given-twice", "under-another-name", "upper-case-key", + "under-a-suspicious-name", "under-an-empty-name"]) +def test_the_finding_names_the_key_git_dies_on(adopter, tmp_path, gitmodules, + said): + plan = planned(source_with(tmp_path, gitmodules, "tip"), tmp_path, + ANSWERS["tip"]) + expected = [] if said is None else [ + (UNREADABLE, unreadable(said) + adopter.FAILS_IN_THE_EXTRACTION)] + assert findings(plan) == expected + + +@pytest.mark.parametrize("gitmodules, said", [ + (with_line(r'update = "bo\ngus"'), + f"invalid value for `submodule.b.update`: `bo\\ngus`, {NOT_A_STRATEGY}"), + (with_line("shallow = caf").encode()[:-1] + b"\xe9\n", + "bad boolean config value `caf\\xe9` for `submodule.b.shallow`"), + (b'[submodule "b\x1bc"]\n\tpath = vendor/b\n\turl = ../b\n' + b"\tfetchRecurseSubmodules = `x`\n", + "bad `submodule.b\\x1bc.fetchrecursesubmodules` argument: `\\`x\\``, " + "which is neither a boolean nor `on-demand`"), +], ids=["a-newline-in-the-value", "a-byte-that-is-not-utf-8", + "an-escape-in-the-name-and-backticks-in-the-value"]) +def test_the_key_and_the_value_are_spelled_on_one_line(adopter, tmp_path, + gitmodules, said): + """The key holds the submodule's name and the value is whatever the + file says: both are spelled as `plan` spells a path, so a finding is + one line of printable text.""" + plan = planned(source_with(tmp_path, gitmodules, "tip"), tmp_path, + ANSWERS["tip"]) + found = findings(plan) + assert found == [(UNREADABLE, unreadable(said) + + adopter.FAILS_IN_THE_EXTRACTION)] + assert found[0][1].isprintable() + + +# --- what `execute` refuses ------------------------------------------------- + +@pytest.mark.parametrize("where", ["tip", "history"]) +@pytest.mark.parametrize("line", REFUSED) +def test_execute_refuses_it_before_any_leg_exists(adopter, tmp_path, + monkeypatch, line, where): + """The refusal is `check`'s finding, raised by `_refuse_what_check_finds` + before `_create_leg_remotes`: the directory the legs go in is never + made.""" + monkeypatch.setattr(adopter, "_require_filter_repo", lambda: None) + plan = planned(source_with(tmp_path, with_line(line), where), tmp_path, + ANSWERS[where]) + found = findings(plan) + remotes = tmp_path / REMOTES + with pytest.raises(adopter.Refusal) as caught: + adopter.cmd_execute(Namespace( + plan=str(plan), source=None, local_remote_dir=remotes, + allow_empty_leg=[], yes=True, work_dir=tmp_path / "work")) + refusal = caught.value + assert refusal.code == UNREADABLE + assert refusal.detail.splitlines()[1:] == [ + f" FINDING {code}: {detail}" for code, detail in found] + assert "no leg repository was created" in refusal.remediation + assert not remotes.exists() + + +# --- read as git reads it --------------------------------------------------- + +#: Values whose reading is the same in git 2.39.5 through 2.51 and in every +#: C library CI runs on. Left out: the four bounds git 2.50 moved below +#: zero, which an older git refuses, and a `0b` number, which only glibc +#: 2.38 and later reads; `test_the_numbers_check_reads_as_a_boolean` has those. +BOOLEAN_VALUES = ["", "true", "Off", "YES", "maybe", "t", "1", "-1", "+1", + "007", "08", "0x10", "0x", "1k", "1G", "2g", "-1g", + "2097151k", "2097152k", "2147483647", "2147483648", + "-2147483647", "1e3", "1.0", "1_000", "k", "-", '" 1"', + '"1 "', '" true"', "１", "trüe", "on-demand", + "On-Demand"] +READ_AS_GIT_READS = ( + [f"shallow = {value}" for value in BOOLEAN_VALUES] + + [f"fetchRecurseSubmodules = {value}" for value in BOOLEAN_VALUES] + + [f"update = {value}" for value in + ["none", "checkout", "rebase", "merge", "Checkout", "", "!cmd", "!", + "bogus", '" checkout"']] + + ["ignore = bogus", "ignore =", "branch =", "path =", "url = -x", + "shallow", "fetchRecurseSubmodules"]) + + +@pytest.fixture(scope="module") +def reader(tmp_path_factory) -> Path: + """A repository whose second commit adds a gitlink, so that `git + fast-export --all` there runs git's submodule reader on whatever + `.gitmodules` the worktree holds -- as it does in the clone `git + filter-repo` rewrites.""" + repository = tmp_path_factory.mktemp("reader") + git("init", "-q", "-b", "main", ".", cwd=repository) + (repository / "README.md").write_bytes(b"# Reader\n") + git("add", "README.md", cwd=repository) + as_human("commit", "-qm", "one", cwd=repository) + link(repository) + as_human("commit", "-qm", "Add vendor/b", cwd=repository) + return repository + + +@pytest.mark.parametrize("line", READ_AS_GIT_READS) +def test_check_reads_a_value_as_the_installed_git_reads_it(adopter, reader, + line): + """Whatever git runs this test: `_registrations` refuses the line + exactly where git's own submodule reader dies on it, in the `git + fast-export` `git filter-repo` runs.""" + gitmodules = with_line(line).encode() + (reader / GITMODULES).write_bytes(gitmodules) + exported = subprocess.run(["git", "fast-export", "--all"], cwd=reader, + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.PIPE, check=False) + listed = subprocess.run( + ["git", "config", "-z", "--no-includes", "--file", "-", "--list"], + input=gitmodules, capture_output=True, check=True) + read = adopter._registrations(listed.stdout) + assert isinstance(read, str) == bool(exported.returncode), ( + read, exported.stderr.decode(errors="replace")) + + +@pytest.mark.parametrize("value, read", [ + # git 2.50 lets in one more number below zero, for each unit; an older + # git refuses it, and `check` leaves it to the extraction, as on main. + ("-2147483648", True), ("-2147483649", False), + ("-2097152k", True), ("-2097153k", False), + ("-2048m", True), ("-2049m", False), ("-2g", True), ("-3g", False), + ("-0x80000000", True), + # Above zero every version agrees. + ("2147483647", True), ("2147483648", False), ("1g", True), + ("2g", False), + # Read as binary by a git built against glibc 2.38 or later (git 2.43 + # here does), and refused elsewhere: not refused, so as not to refuse + # what git reads. + ("0b1", True), ("0B0", True), ("0b", False), ("0b2", False), +]) +def test_the_numbers_check_reads_as_a_boolean(adopter, value, read): + assert adopter._git_reads_as_a_boolean(value) is read + + +# --- end to end ------------------------------------------------------------- + +def execute_in(plan: Path, base: Path): + return run_script(ADOPT, "execute", "--plan", str(plan), "--yes", + "--local-remote-dir", str(base / REMOTES), + "--work-dir", str(base / "work")) + + +@needs_filter_repo +def test_the_issue_shape_is_refused_and_the_repaired_source_then_adopts( + tmp_path): + """#215 as the issue measured it, step by step (the tip, `update = + bogus`): `check` 0, `execute` 2 with both leg repositories left, and the + corrected re-run refused as `leg-remote-exists`. Now `check` finds it, + `execute` refuses it with no leg made, and once the source's + `.gitmodules` is repaired the same `--local-remote-dir` takes the + adoption.""" + source = source_with(tmp_path, with_line("update = bogus"), "tip") + plan = planned(source, tmp_path, ANSWERS["tip"]) + checked = run_script(ADOPT, "check", "--plan", str(plan)) + assert checked.returncode == 1, checked.stderr + checked.stdout + assert (f"FINDING {UNREADABLE}: " + + unreadable(REFUSED["update = bogus"])) in checked.stderr + refused = execute_in(plan, tmp_path) + assert refused.returncode == 2, refused.stderr + refused.stdout + assert f"REFUSED {UNREADABLE}: 1 finding(s) in " in refused.stderr + assert "creating the leg repositories" not in refused.stdout + remotes = tmp_path / REMOTES + assert not remotes.exists() or not any(remotes.iterdir()), \ + "a refused plan left a leg repository behind" + + put_gitmodules(source, with_line("update = checkout")) + as_human("commit", "-qm", "Repair .gitmodules", cwd=source) + repaired = tmp_path / "repaired" + repaired.mkdir() + plan = planned(source, repaired, ANSWERS["tip"]) + assert run_script(ADOPT, "check", "--plan", str(plan)).returncode == 0 + adopted = run_script(ADOPT, "execute", "--plan", str(plan), "--yes", + "--local-remote-dir", str(remotes), "--work-dir", + str(repaired / "work")) + assert adopted.returncode == 0, adopted.stderr + adopted.stdout + assert "adoption verified" in adopted.stdout + + +@needs_filter_repo +@pytest.mark.parametrize("where", ["tip", "history"]) +@pytest.mark.parametrize("line", ["ignore = bogus", "branch ="]) +def test_a_value_git_reads_still_adopts(tmp_path, line, where): + """The issue's two rows git reads, end to end: `check` passes them and + `execute` verifies, as on main.""" + plan = planned(source_with(tmp_path, with_line(line), where), tmp_path, + ANSWERS[where]) + checked = run_script(ADOPT, "check", "--plan", str(plan)) + assert checked.returncode == 0, checked.stderr + checked.stdout + adopted = execute_in(plan, tmp_path) + assert adopted.returncode == 0, adopted.stderr + adopted.stdout + assert "adoption verified" in adopted.stdout + + +@needs_filter_repo +def test_a_refused_value_beside_no_gitlink_anywhere_adopts_and_clones( + tmp_path): + """#166's S3, for a value, end to end: `update = bogus` in a source no + commit of which ever held a gitlink, kept in the code leg. Nothing runs + git's submodule reader on it: `execute` verifies, and the assembly's + `git clone --recurse-submodules` exits 0.""" + source = source_with(tmp_path, with_line("update = bogus"), "nowhere") + plan = planned(source, tmp_path, ANSWERS["nowhere"]) + assert run_script(ADOPT, "check", "--plan", str(plan)).returncode == 0 + adopted = execute_in(plan, tmp_path) + assert adopted.returncode == 0, adopted.stderr + adopted.stdout + assert "adoption verified" in adopted.stdout + clone = tmp_path / "clone" + cloned = git(*FILE_PROTOCOL, "clone", "-q", "--recurse-submodules", "-b", + "adopt/three-repo-shape", str(source), str(clone), + cwd=tmp_path, check=False) + assert cloned.returncode == 0, cloned.stderr + assert (clone / "code" / GITMODULES).read_bytes() \ + == with_line("update = bogus").encode() From f48c669a205fbbb468d1da2e4f5a0ed810e1320b Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:36:36 +0000 Subject: [PATCH 3/5] adopt-project.py: the #195 history scan reads the source's history without its `info/grafts` and passes a type change by, so a graft no longer hides a gitlink the extraction exports, and a gitlink only type changes move is no longer refused (#195) Fix round 1 on PR #226, from the independent review: - A graft in the source's `.git/info/grafts` hid the commit that added a gitlink from the scan's `git log`, and `--no-replace-objects` leaves grafts on. A clone copies no grafts file, so `git fast-export` in the extraction's clone exported that commit: `check` said 0 and `execute` died after both leg repositories were made, as on main. The scan now runs with `GIT_GRAFT_FILE` naming a file that does not exist, and names the commit (`removes vendor/b`). - git's diff asks the submodule reader about a changed path only when both of its sides are gitlinks, so a type change (a file or symlink becoming a gitlink, or the reverse) never reads `.gitmodules`. A gitlink that only ever entered and left by type changes adopted on main and was refused here; `_first_gitlink` now passes `T` by. One that enters by a type change and leaves by a removal is still found at the removal. - The docstring's cost claim is measured again: 100,001 commits in 4.9 to 5.2 s against 4.2 to 4.5 s for `git fast-export --no-data` over the same history, 5010 commits in 0.15 s. Tests: two `HISTORIES` shapes (a graft, entering by a type change), a graft test that checks its premise with `git fast-export` in a clone, and a type-change control for a file and a symlink that checks `git fast-export` in a clone exits 0. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 112 +++++++++++++++++---------- tests/test_adopt_history_gitlinks.py | 107 ++++++++++++++++++++++++- 2 files changed, 175 insertions(+), 44 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 4322110c..4b770151 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -79,6 +79,7 @@ import argparse import datetime as _dt +import os import re import shutil import subprocess @@ -403,15 +404,24 @@ def _exported_gitlink(source: "Source") -> str | None: `git fast-export --all` in the clone: every branch and tag of the source, which is what `git clone` copies, and the commit the plan names. It diffs each commit against its first parent, a root against nothing, and - for every gitlink one of those diffs adds, changes or removes, git's - submodule reader reads the clone's CHECKED-OUT `.gitmodules`: the plan's - commit's, whatever leg the plan gives it. Measured with git 2.43 and - `git filter-repo` 2.47.0, a tip `.gitmodules` that reader cannot read - failed the extraction, after both leg repositories were made, for a - gitlink in the history of the branch being adopted, on a side branch - merged into it, on a branch never merged, on a commit only a tag - reaches, in the root commit of a branch of its own, and one that two - merges alone added and removed. + for every gitlink one of those diffs adds, removes or moves to another + commit, git's submodule reader reads the clone's CHECKED-OUT + `.gitmodules`: the plan's commit's, whatever leg the plan gives it. + Measured with git 2.43 and `git filter-repo` 2.47.0, a tip `.gitmodules` + that reader cannot read failed the extraction, after both leg + repositories were made, for a gitlink in the history of the branch being + adopted, on a side branch merged into it, on a branch never merged, on a + commit only a tag reaches, in the root commit of a branch of its own, + one that two merges alone added and removed, and one the source's own + `git log` does not see for a replacement or a graft. + + A TYPE CHANGE -- a file or a symlink becoming a gitlink, or a gitlink + becoming one -- is not read that way: git's diff asks the submodule + reader about a changed path only when both of its sides are gitlinks. + Measured, a gitlink that only ever entered and left by a type change + adopted on main beside the same `.gitmodules`, so `_first_gitlink` + passes a type change by; one that enters by a type change and leaves by + a removal is found at the removal. A BRANCH here is a `refs/heads/` ref of the source, as the clone takes it: an `org/repo` source, which `Source.open` clones, has its default @@ -422,7 +432,8 @@ def _exported_gitlink(source: "Source") -> str | None: adopted on main beside the same `.gitmodules`. So `git log` is asked for those diffs, over those commits, and read - until the first gitlink in one: a `160000` mode on either side. + until the first gitlink in one: a `160000` mode on either side of a + change that is no type change. `--root` and `--diff-merges=first-parent` are fast-export's root and merge diffs, which `log.showRoot=false` and a plain `git log` leave out; `--no-renames`, its diffs, one path to a change. `--ignore-submodules= @@ -430,35 +441,47 @@ def _exported_gitlink(source: "Source") -> str | None: checked-out `.gitmodules`, and died on the very file this is asked about (exit 128), and because `diff.ignoreSubmodules=all` hides every gitlink. `--no-replace-objects`, because a clone copies no `refs/replace/`, and a - replacement can hide the commit that holds one. `--no-show-signature`, - because `log.showSignature` runs gpg on every signed commit and writes - what it says into this output, ahead of the commit it names. + replacement can hide the commit that holds one. `GIT_GRAFT_FILE` naming + a file that does not exist, because a clone copies no `info/grafts` + either, a graft can hide that commit too, and `--no-replace-objects` + leaves grafts on. `--no-show-signature`, because `log.showSignature` + runs gpg on every signed commit and writes what it says into this + output, ahead of the commit it names. The output is read as it comes and the scan stopped at the first gitlink, - so a source that holds one is not read to its root; a source that holds - none is read once, end to end (5010 commits in 0.4 s here). A scan git - cannot finish is refused as `git-failed`, as `git_out` refuses: a history - that could not be read has not been shown to hold no gitlink. + so a source that holds one is not read to its root. A source that holds + none is read once, end to end, at about the cost of one `git fast-export + --no-data` over the same history, which the extraction runs and then + rewrites: measured here, 100,001 commits in 4.9 to 5.2 s against 4.2 to + 4.5 s for that export, and 5010 commits in 0.15 s. A scan git cannot + finish is refused as `git-failed`, as `git_out` refuses: a history that + could not be read has not been shown to hold no gitlink. """ command = ["git", *EXPORTED_DIFFS, source.commit, "--branches", "--tags", "--"] - with tempfile.TemporaryFile() as said, subprocess.Popen( - command, cwd=source.path, stdout=subprocess.PIPE, - stderr=said) as log: - found = _first_gitlink(log.stdout) - if found is not None: - log.kill() - elif log.wait(): - said.seek(0) - raise Refusal("git-failed", "`{}` in {} exited {}: {}".format( - " ".join(command[1:]), source.path, log.returncode, - said.read().decode("utf-8", "replace").strip())) + with tempfile.TemporaryDirectory() as empty: + no_grafts = {**os.environ, + "GIT_GRAFT_FILE": os.path.join(empty, "grafts")} + with tempfile.TemporaryFile() as said, subprocess.Popen( + command, cwd=source.path, stdout=subprocess.PIPE, + stderr=said, env=no_grafts) as log: + found = _first_gitlink(log.stdout) + if found is not None: + log.kill() + elif log.wait(): + said.seek(0) + raise Refusal("git-failed", "`{}` in {} exited {}: {}".format( + " ".join(command[1:]), source.path, log.returncode, + said.read().decode("utf-8", "replace").strip())) return found #: How a finding says what a commit does to the gitlink it names: `git log -#: --raw`'s status letter, a type change or a new commit being a change. +#: --raw`'s status letter, a new commit being a change. A type change, `T`, +#: is never named: `_first_gitlink` passes it by (`_exported_gitlink`). GITLINK_CHANGES = {"A": "adds", "D": "removes"} +#: `git log --raw`'s status letter for a type change. +TYPE_CHANGE = "T" def _first_gitlink(stream) -> str | None: @@ -468,13 +491,15 @@ def _first_gitlink(stream) -> str | None: commit, then for each path it changes `: ` and the path, raw bytes. A newline before a commit or a change is not part of it. So a field after a change is a path, whatever - it spells, and any other is a commit or a change.""" + it spells, and any other is a commit or a change. A type change to or + from a gitlink is no gitlink here: git's submodule reader never reads + `.gitmodules` for one.""" commit, change, rest = "", None, b"" for chunk in iter(lambda: stream.read(1 << 16), b""): *fields, rest = (rest + chunk).split(b"\x00") for field in fields: if change is not None: - if GITLINK_MODE in change[:2]: + if GITLINK_MODE in change[:2] and change[-1] != TYPE_CHANGE: return (f"commit {commit[:12]} " f"{GITLINK_CHANGES.get(change[-1], 'changes')} " f"{_spelled(_git_path(field))}") @@ -596,18 +621,19 @@ def registered_submodules(self, tree: list) -> Registrations | str: A KEY WITH NO VALUE -- one SUBMODULE_VALUED_KEY_RE names, with nothing after it -- is fatal to git's submodule reader alone, and git runs that reader only for a gitlink: the assembly's clone for one in - the tree it checks out, and `execute`'s extraction for one in ANY - commit `git fast-export` diffs, where the tip `.gitmodules` is the - file it reads (#195). So in a tree that holds none, the file is read - that way only when the history of the source's branches and tags - holds one (`_exported_gitlink`), and then the answer is git's words, - an `InTheHistory` naming where: on main, `check` passed such a file - as registering nothing and `execute` died in `git fast-export` with - `missing value for 'submodule.b.path'`, after both leg repositories - were made. Where the history holds none either, nothing ever reads - the file that way, and it registers nothing (#166's S3). A line git - cannot parse at all is another matter: every reader dies on it, and - the history says only which reader dies first. + the tree it checks out, and `execute`'s extraction for one that ANY + commit `git fast-export` diffs adds, removes or moves, where the tip + `.gitmodules` is the file it reads (#195). So in a tree that holds + none, the file is read that way only when the history of the + source's branches and tags holds one (`_exported_gitlink`), and then + the answer is git's words, an `InTheHistory` naming where: on main, + `check` passed such a file as registering nothing and `execute` died + in `git fast-export` with `missing value for 'submodule.b.path'`, + after both leg repositories were made. Where the history holds none + either, nothing ever reads the file that way, and it registers + nothing (#166's S3). A line git cannot parse at all is another + matter: every reader dies on it, and the history says only which + reader dies first. """ if not any(path == GITMODULES for path, _, _, _ in tree): return Registrations() diff --git a/tests/test_adopt_history_gitlinks.py b/tests/test_adopt_history_gitlinks.py index d2bf5e60..c4bf1289 100644 --- a/tests/test_adopt_history_gitlinks.py +++ b/tests/test_adopt_history_gitlinks.py @@ -4,7 +4,7 @@ `git filter-repo` runs `git fast-export --all` in a clone of the source, and git's submodule reader reads the clone's CHECKED-OUT `.gitmodules` for every -gitlink a commit of any branch or tag adds, changes or removes. #166 asked +gitlink a commit of any branch or tag adds, removes or moves. #166 asked the tip tree alone, so a source whose history once held a submodule, whose tip holds none, and whose tip `.gitmodules` has a key with no value (`[submodule "b"]` and a bare `path`) passed `check` as `plan ok`, and @@ -178,6 +178,61 @@ def hidden_by_a_replacement(source: Path) -> None: git("replace", added, f"{added}~1", cwd=source) +def hidden_by_a_graft(source: Path) -> None: + """The commit that removes it, grafted onto the root commit by + `info/grafts`, so that the commit that adds it is no ancestor: `git log` + honours the graft, `--no-replace-objects` or not, and a clone, which + copies no grafts file, does not.""" + add_then_remove(source) + removed = git("rev-parse", "HEAD", cwd=source).stdout.strip() + root = git("rev-list", "--max-parents=0", "HEAD", cwd=source + ).stdout.strip() + grafts = source / ".git" / "info" / "grafts" + grafts.parent.mkdir(exist_ok=True) + grafts.write_bytes(f"{removed} {root}\n".encode()) + + +#: What GITLINK is before it is a gitlink, and after: a file, or a symlink. +#: Each is staged straight into the index, so no symlink is made on disk. +NOT_A_GITLINK = {"file": ("100644", b"a file\n"), + "symlink": ("120000", b"../elsewhere")} + + +def not_a_gitlink(source: Path, kind: str) -> None: + """GITLINK, staged as NOT_A_GITLINK's `kind`, in place of whatever the + index holds there.""" + mode, body = NOT_A_GITLINK[kind] + blob = subprocess.run(["git", "hash-object", "-w", "--stdin"], + cwd=source, input=body, capture_output=True, + check=True).stdout.decode().strip() + git("update-index", "--add", "--cacheinfo", f"{mode},{blob},{GITLINK}", + cwd=source) + + +def turned_gitlink(source: Path, kind: str = "file") -> None: + """GITLINK a file or a symlink, then a gitlink in its place: to `git + log --raw` a type change, `T`, and no add.""" + not_a_gitlink(source, kind) + as_human("commit", "-qm", f"vendor/b, a {kind}", cwd=source) + link(source) + as_human("commit", "-qm", "vendor/b, a gitlink", cwd=source) + + +def entering_by_a_type_change(source: Path) -> None: + """It enters by a type change, which git's submodule reader reads no + `.gitmodules` for, and leaves by a removal, which it does.""" + turned_gitlink(source) + unlink(source) + as_human("commit", "-qm", "Remove vendor/b", cwd=source) + + +def by_type_changes_alone(source: Path, kind: str) -> None: + """It enters and leaves by type changes alone: a `kind` again after.""" + turned_gitlink(source, kind) + not_a_gitlink(source, kind) + as_human("commit", "-qm", f"vendor/b, a {kind} again", cwd=source) + + #: Where the history holds the submodule, and what the commit `git log` #: meets first does to it; the tip holds none in every one. HISTORIES = { @@ -191,6 +246,8 @@ def hidden_by_a_replacement(source: Path) -> None: "in-a-root-commit-of-its-own": (in_a_root_commit_of_its_own, "adds"), "by-merges-alone": (by_merges_alone, "removes"), "hidden-by-a-replacement": (hidden_by_a_replacement, "removes"), + "hidden-by-a-graft": (hidden_by_a_graft, "removes"), + "entering-by-a-type-change": (entering_by_a_type_change, "removes"), } @@ -217,6 +274,17 @@ def opened(adopter, source: Path, tmp_path: Path): return adopter.Source.open(str(source), tmp_path / "read") +def exported_in_a_clone(source: Path, base: Path): + """`git fast-export --all` in a clone of `source`, as `git filter-repo` + runs it in the extraction's clone: the run whose failure the scan + foretells. It needs git alone, so it runs where `git filter-repo` is + not installed; bytes, because it exports the source's own content.""" + clone = base / "export-clone" + git("clone", "-q", "--", str(source), str(clone), cwd=base) + return subprocess.run(["git", "fast-export", "--all"], cwd=clone, + capture_output=True, check=False) + + # --- where the history holds a gitlink ------------------------------------- @pytest.mark.parametrize("history, verb", HISTORIES.values(), @@ -236,6 +304,43 @@ def test_the_history_is_read_where_the_extraction_reads_it( assert re.fullmatch(named(verb), found), found +def test_a_graft_hides_from_the_source_what_the_clone_exports(adopter, + tmp_path): + """`--no-replace-objects` leaves grafts on: the source's own `git log` + with it walks past the commit that adds the gitlink, and `git + fast-export` in a clone of the source, which copies no grafts file, + exports that commit and dies on the tip `.gitmodules`. On main `check` + passed this, and `execute` failed after both leg repositories were + made. The scan names the commit the clone exports.""" + source = history_source(tmp_path, hidden_by_a_graft) + seen = git("--no-replace-objects", "log", "--format=%s", "main", + cwd=source).stdout.splitlines() + assert "Add vendor/b" not in seen, seen + exported = exported_in_a_clone(source, tmp_path) + assert exported.returncode != 0 + assert b"missing value for 'submodule.b.path'" in exported.stderr + found = adopter._exported_gitlink(opened(adopter, source, tmp_path)) + assert re.fullmatch(named("removes"), found), found + + +@pytest.mark.parametrize("kind", list(NOT_A_GITLINK)) +def test_a_gitlink_only_type_changes_move_is_not_read(adopter, tmp_path, + kind): + """A file or a symlink becomes a gitlink and then what it was again. + git's diff asks the submodule reader about a changed path only when + both of its sides are gitlinks, so `git fast-export` in a clone of the + source never reads the tip `.gitmodules` for this one, and on main the + file case adopted: the scan finds no gitlink, and `check` passes it.""" + source = history_source( + tmp_path, lambda source: by_type_changes_alone(source, kind)) + exported = exported_in_a_clone(source, tmp_path) + assert exported.returncode == 0, exported.stderr + assert adopter._exported_gitlink(opened(adopter, source, tmp_path)) \ + is None + plan = planned(source, tmp_path, {GITMODULES: "code", "vendor/": "code"}) + assert findings(plan) == [] + + def test_a_history_with_no_gitlink_is_read_to_its_root(adopter, tmp_path): source = history_source(tmp_path, None) assert adopter._exported_gitlink(opened(adopter, source, tmp_path)) \ From ddb141916a1a10cbb598b532401f0b677c8770cc Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 21:26:44 +0000 Subject: [PATCH 4/5] fix round 1 (#215): refuse the numbers only some gits read, share the .gitmodules test helpers through conftest.py, cover a gitlink only type changes move Fix round 1 on PR #240, after merging #226's fix round 1 (f48c669) in the merge commit before this one: - `check` refuses what ANY git `execute` may run refuses (lane 2's nit at adopt-project.py:370 and :404). It took git 2.50's wider bound below zero and glibc 2.38's `0b` numbers, so on git 2.49 and older `shallow = -2147483648` (or -2097152k, -2048m, -2g, however spelled) passed `check` and `execute` died in the extraction with both leg repositories left; measured here with git 2.43 on c800efd and on the merge, for `shallow` and `fetchRecurseSubmodules`, at the tip and in the history. A refusal at `check` costs an edit to an absurd value; a death at `execute` costs two leg repositories. `_who_refuses_a_boolean` replaces `_git_reads_as_a_boolean`: None where every git reads the value, `EVERY_GIT` where none does, or why only some do, which the finding then says: "a number git 2.49 and older refuse, and 2.50 and later read", or "a `0b` number, which only a git built on glibc 2.38 or later reads". Every other value reads exactly as before; the in-suite differential against the installed git leaves these values out and still passes. - SonarCloud's duplication (6.3% on new code against 3%): `as_human`, `put_gitmodules`, `link`, `named`, `exported_in_a_clone`, `planned`, `findings` and `in_the_history`, and the `execute` refusal block both files repeated, now `refused_before_any_leg`, live in tests/conftest.py as plain helpers beside `write_plan` and `resolve`, with GITMODULES and GITLINK. tests/test_adopt_history_gitlinks.py and tests/test_adopt_gitmodules_values.py import them; no assertion either file made changes. - python:S5778: the helper builds the `Namespace` before `pytest.raises`, so only `cmd_execute` is inside the block (in both files' test). - A gitlink that only type changes move, beside a value git refuses: on c800efd `check` refused it, though `git fast-export` in a clone exits 0; with f48c669 merged it passes and adopts. A `type-changes` position covers it: `check` passes each refused line there, the clone's export exits 0, and `update = bogus` adopts and clones end to end. - `test_who_refuses_a_number_as_a_boolean` states each bound for each unit, the values one above, and the `0b` cases; the three numbers are refused at the tip and in the history by `check` and by `execute` before any leg exists, and pass beside no gitlink at all. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 77 ++++++---- tests/conftest.py | 120 +++++++++++++++ tests/test_adopt_gitmodules_values.py | 211 ++++++++++++-------------- tests/test_adopt_history_gitlinks.py | 102 +------------ 4 files changed, 278 insertions(+), 232 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 3c51b524..6769d209 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -374,36 +374,55 @@ def _move(self, name: str, path: str) -> None: UNIT_FACTORS = {"": 1, "k": 1 << 10, "m": 1 << 20, "g": 1 << 30} #: The `int` git reads a boolean's number into: 2**31 - 1 is its largest. GIT_INT_BITS = 31 - - -def _git_reads_as_a_boolean(value: str) -> bool: - """Whether git takes `value` as a boolean (`git_parse_maybe_bool`): - empty, which is false; one of BOOLEAN_WORDS; or an integer - `git_parse_int` takes, which is true unless it is zero. - - The integer must fit an `int` once its unit is applied. git 2.50 moved - the bound below zero by one (`(-max - 1) / factor`, where 2.49 and - before had `-max / factor`), and the wider range is the one taken here, - so that nothing a newer git reads is refused. An older git refuses the - four values that move let in, -2147483648, -2097152k, -2048m and -2g, - however spelled; those are left to the extraction, as on main, and so - is a `0b` number, which git reads only where its C library does.""" +#: Who refuses a value as a boolean (`_who_refuses_a_boolean`): no git +#: reads it, or only some of the gits `execute` may run do, and why. +EVERY_GIT = "every git" +OLDER_GITS_REFUSE = ("a number git 2.49 and older refuse, and 2.50 and later " + "read") +SOME_LIBRARIES_REFUSE = ("a `0b` number, which only a git built on glibc " + "2.38 or later reads") + + +def _who_refuses_a_boolean(value: str) -> str | None: + """None where every git takes `value` as a boolean + (`git_parse_maybe_bool`): empty, which is false; one of BOOLEAN_WORDS; + or an integer `git_parse_int` takes, which is true unless it is zero. + Otherwise EVERY_GIT where no git takes it, or why only some do. + + The integer must fit an `int` once its unit is applied. Where gits + differ on one, `check` refuses it with the gits that refuse it: a + refusal there costs an edit to an absurd value, and a git that refuses + it in the extraction costs both leg repositories. git 2.50 moved the + bound below zero by one (`(-max - 1) / factor`, where 2.49 and before + have `-max / factor`), so -2147483648, -2097152k, -2048m and -2g, + however spelled, are OLDER_GITS_REFUSE: measured with git 2.43, each + passed `check` and `execute` died after both leg repositories were + made. A `0b` number, which git reads only where its C library's + `strtoimax` does, is SOME_LIBRARIES_REFUSE.""" if not value or (value.isascii() and value.lower() in BOOLEAN_WORDS): - return True + return None number = GIT_INTEGER_RE.fullmatch(value) if number is None: - return False + return EVERY_GIT digits = number["digits"] + binary = digits[1:2] in ("b", "B") if digits[1:2] in ("x", "X"): size = int(digits[2:], 16) - elif digits[1:2] in ("b", "B"): + elif binary: size = int(digits[2:], 2) else: size = int(digits, 8 if digits.startswith("0") else 10) factor = UNIT_FACTORS[number["unit"].lower()] - if number["sign"] == "-": - return size <= (1 << GIT_INT_BITS) // factor - return size <= ((1 << GIT_INT_BITS) - 1) // factor + every_git_reads = ((1 << GIT_INT_BITS) - 1) // factor + newer_gits_read = ((1 << GIT_INT_BITS) // factor if number["sign"] == "-" + else every_git_reads) + if size > newer_gits_read: + return EVERY_GIT + if binary: + return SOME_LIBRARIES_REFUSE + if size > every_git_reads: + return OLDER_GITS_REFUSE + return None def _refused_value(key: str, value: str) -> str | None: @@ -415,7 +434,7 @@ def _refused_value(key: str, value: str) -> str | None: (submodule-config.c), which sets `overwrite`, so `parse_config` reads every value, a key given twice included, and dies on the first it refuses. Read in the source code of git 2.39.5 through 2.51.0, the - same in each but for the bound `_git_reads_as_a_boolean` names, and + same in each but for the bound `_who_refuses_a_boolean` names, and measured with git 2.43 over `git fast-export`: * `update`, `parse_submodule_update_strategy` (submodule.c): one of @@ -427,7 +446,10 @@ def _refused_value(key: str, value: str) -> str | None: * `fetchrecursesubmodules`, `parse_fetch_recurse`, which dies on an error for the checked-out file (`die_on_error`): a boolean or `on-demand`, exactly; anything else ends it with `bad - argument: `.""" + argument: `. + + A number only some gits read as a boolean is refused as the gits that + refuse it say, and the finding says which gits those are.""" typed = SUBMODULE_TYPED_KEY_RE.fullmatch(key) if typed is None or _a_name_git_ignores(typed["name"]): return None @@ -439,14 +461,17 @@ def _refused_value(key: str, value: str) -> str | None: if value.startswith("!") else f"which is not one of {_listed(list(UPDATE_STRATEGIES))}") return f"invalid value for {_spelled(key)}: {said}, {why}" - if _git_reads_as_a_boolean(value): + refusers = _who_refuses_a_boolean(value) + if refusers is None: return None if typed["key"] == "shallow": - return f"bad boolean config value {said} for {_spelled(key)}" + return (f"bad boolean config value {said} for {_spelled(key)}" + + ("" if refusers == EVERY_GIT else f", {refusers}")) if value == "on-demand": return None - return (f"bad {_spelled(key)} argument: {said}, which is neither a " - "boolean nor `on-demand`") + return (f"bad {_spelled(key)} argument: {said}, " + + ("which is neither a boolean nor `on-demand`" + if refusers == EVERY_GIT else refusers)) def _registrations(listed: bytes) -> Registrations | str: diff --git a/tests/conftest.py b/tests/conftest.py index 4e136610..c1876cb3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,10 +17,12 @@ from __future__ import annotations import os +import re import shutil import stat import subprocess import sys +from argparse import Namespace from pathlib import Path import pytest @@ -380,3 +382,121 @@ def resolve(plan_path: Path, path: str, leg: str, handle.write(text.replace( needle, f" - path: {path}\n leg: {leg}\n" f" resolution: \"{why}\"\n", 1)) + + +# --- `.gitmodules` sources, and what `check` and `execute` say of them ----- +# +# Shared by `test_adopt_history_gitlinks.py` (#195) and +# `test_adopt_gitmodules_values.py` (#215), which build sources that hold a +# submodule at GITLINK, at the tip or only in the history, beside a +# `.gitmodules` git's submodule reader may die on. + +GITMODULES = ".gitmodules" +#: The submodule those sources hold, registered under the name `b`. +GITLINK = "vendor/b" + + +def as_human(*args: str, cwd: Path) -> subprocess.CompletedProcess: + """`git ` with an identity, for the commits and merges a test + makes in a source.""" + return git("-c", "user.name=Source Human", + "-c", "user.email=source@invalid.example", *args, cwd=cwd) + + +def put_gitmodules(source: Path, text: str | bytes) -> None: + """`text` as the source's `.gitmodules`, staged: BYTES, so it is LF on + every platform, as a hand edit would leave it.""" + (source / GITMODULES).write_bytes( + text if isinstance(text, bytes) else text.encode()) + git("add", "--", GITMODULES, cwd=source) + + +def link(source: Path) -> None: + """A gitlink at GITLINK, staged straight into the index, as `git add` of + an embedded clone records one; no checkout of it is needed. It names + `main`'s commit, which an orphan branch's first commit has no HEAD to + give.""" + head = git("rev-parse", "main", cwd=source).stdout.strip() + git("update-index", "--add", "--cacheinfo", f"160000,{head},{GITLINK}", + cwd=source) + + +def named(verb: str = "(adds|removes|changes)") -> str: + """How `_exported_gitlink` names a commit's change to the gitlink, as a + pattern: `verb` is what that commit does to it.""" + return rf"commit [0-9a-f]{{12}} {verb} `vendor/b`" + + +def exported_in_a_clone(source: Path, + base: Path) -> subprocess.CompletedProcess: + """`git fast-export --all` in a clone of `source`, as `git filter-repo` + runs it in the extraction's clone: the run whose failure the scan + foretells. It needs git alone, so it runs where `git filter-repo` is + not installed; bytes, because it exports the source's own content.""" + clone = base / "export-clone" + git("clone", "-q", "--", str(source), str(clone), cwd=base) + return subprocess.run(["git", "fast-export", "--all"], cwd=clone, + capture_output=True, check=False) + + +def planned(source: Path, base: Path, answers: dict) -> Path: + """`plan` for `source` into `base/plan/` (project Northwind), each entry + `answers` names given its leg: an entry `plan` asks about is resolved, + and any other has its `leg:` set.""" + plan = base / "plan" / "adoption-plan.yaml" + plan.parent.mkdir() + written = write_plan(source, plan) + assert written.returncode == 0, written.stderr + written.stdout + text = plan.read_bytes().decode() + for path, leg in answers.items(): + entry = re.search(rf" - path: {re.escape(path)}\n leg: (\S+)\n", + text) + assert entry, f"no entry {path} in the plan" + if entry[1] == "null": + resolve(plan, path, leg) + text = plan.read_bytes().decode() + else: + text = text.replace(entry[0], + f" - path: {path}\n leg: {leg}\n", 1) + plan.write_bytes(text.encode()) + return plan + + +def findings(plan: Path) -> list[tuple[str, str]]: + """`check`'s findings on `plan`, as `(code, detail)`; its exit is 1 when + there is one and 0 when there is none.""" + result = run_script(ADOPT, "check", "--plan", str(plan)) + found = [tuple(line.split(" ", 1)[1].split(": ", 1)) + for line in result.stderr.splitlines() + if line.startswith("FINDING ")] + assert result.returncode == (1 if found else 0), result.stderr + return found + + +def in_the_history(adopter, verb: str = "removes") -> re.Pattern: + """How the finding ends for a gitlink only the history holds: the + commit `git log` meets first, which `verb`s it.""" + return re.compile(re.escape(adopter.FAILS_IN_THE_HISTORY).replace( + re.escape("{}"), named(verb)) + "$") + + +def refused_before_any_leg(adopter, monkeypatch: pytest.MonkeyPatch, + plan: Path, base: Path): + """`execute --yes` on `plan`, in this process, its probe for `git + filter-repo` stubbed, because the refusal under test comes before the + tool would be used. The refusal must be `check`'s findings, raised by + `_refuse_what_check_finds` before `_create_leg_remotes`, so the + directory the legs go in is never made; it is returned.""" + monkeypatch.setattr(adopter, "_require_filter_repo", lambda: None) + found = findings(plan) + remotes = base / "remotes" + args = Namespace(plan=str(plan), source=None, local_remote_dir=remotes, + allow_empty_leg=[], yes=True, work_dir=base / "work") + with pytest.raises(adopter.Refusal) as caught: + adopter.cmd_execute(args) + refusal = caught.value + assert refusal.detail.splitlines()[1:] == [ + f" FINDING {code}: {detail}" for code, detail in found] + assert "no leg repository was created" in refusal.remediation + assert not remotes.exists() + return refusal diff --git a/tests/test_adopt_gitmodules_values.py b/tests/test_adopt_gitmodules_values.py index 27fa7f32..9d0b3ab9 100644 --- a/tests/test_adopt_gitmodules_values.py +++ b/tests/test_adopt_gitmodules_values.py @@ -21,27 +21,23 @@ from __future__ import annotations import importlib.util -import re import shutil import subprocess -from argparse import Namespace from pathlib import Path import pytest -from conftest import ADOPT, FILE_PROTOCOL, git, make_source_repo, resolve, \ - run_script, write_plan +from conftest import ADOPT, FILE_PROTOCOL, GITLINK, GITMODULES, as_human, \ + exported_in_a_clone, findings, git, in_the_history, link, \ + make_source_repo, planned, put_gitmodules, refused_before_any_leg, \ + run_script needs_filter_repo = pytest.mark.skipif( shutil.which("git-filter-repo") is None, reason="git filter-repo is not installed: `pip install git-filter-repo`") -PROJECT = "Northwind" -GITMODULES = ".gitmodules" UNREADABLE = "plan-gitmodules-unreadable" REMOTES = "remotes" -#: The submodule, registered under the name `b`. -GITLINK = "vendor/b" REGISTRATION = '[submodule "b"]\n\tpath = vendor/b\n\turl = ../b\n' #: A source whose plan asks about `.gitmodules`, and about `vendor/` where #: the tip holds the submodule: `specs/` and `src/` classify themselves and @@ -68,6 +64,25 @@ "bad `submodule.b.fetchrecursesubmodules` argument: `bogus`, which " "is neither a boolean nor `on-demand`", } +#: Numbers only some of the gits `execute` may run read as a boolean, and +#: what the finding says of each: git 2.50 and later read the first two, +#: and git 2.43 here refuses them (measured on c800efd, `check` 0 and +#: `execute` 2 with both leg repositories left); a git built on glibc 2.38 +#: or later, as git 2.43 here is, reads the third. `check` refuses each +#: with the gits that refuse it. +ONLY_SOME_GITS_READ = { + "shallow = -2147483648": + "bad boolean config value `-2147483648` for `submodule.b.shallow`, " + "a number git 2.49 and older refuse, and 2.50 and later read", + "fetchRecurseSubmodules = -2g": + "bad `submodule.b.fetchrecursesubmodules` argument: `-2g`, a number " + "git 2.49 and older refuse, and 2.50 and later read", + "shallow = 0b1": + "bad boolean config value `0b1` for `submodule.b.shallow`, a `0b` " + "number, which only a git built on glibc 2.38 or later reads", +} +#: Every line `check` refuses beside a gitlink. +CHECK_REFUSES = {**REFUSED, **ONLY_SOME_GITS_READ} #: The issue's table, the lines git reads, and three more: `ignore = bogus` #: only warns, `branch` takes any value, and the others are values git takes. #: Each adopted on main and still does. @@ -84,33 +99,27 @@ def adopter(): return module -def as_human(*args: str, cwd: Path): - """`git ` with an identity, for the commits below.""" - return git("-c", "user.name=Source Human", - "-c", "user.email=source@invalid.example", *args, cwd=cwd) - - -def put_gitmodules(source: Path, text: str | bytes) -> None: - """`text` as the source's `.gitmodules`, staged: BYTES, so it is LF on - every platform, as a hand edit would leave it.""" - (source / GITMODULES).write_bytes( - text if isinstance(text, bytes) else text.encode()) - git("add", "--", GITMODULES, cwd=source) - - -def link(source: Path) -> None: - """A gitlink at GITLINK, staged straight into the index, as `git add` of - an embedded clone records one; no checkout of it is needed.""" - head = git("rev-parse", "main", cwd=source).stdout.strip() - git("update-index", "--add", "--cacheinfo", f"160000,{head},{GITLINK}", - cwd=source) +def type_changes(source: Path) -> None: + """A file at GITLINK, then a gitlink in its place, then the file again: + two commits `git log --raw` shows as type changes, `T`, and no commit + that adds or removes a gitlink.""" + (source / GITLINK).parent.mkdir() + (source / GITLINK).write_bytes(b"a file\n") + git("add", "--", GITLINK, cwd=source) + as_human("commit", "-qm", "vendor/b, a file", cwd=source) + git("rm", "-q", "--cached", "--", GITLINK, cwd=source) + link(source) + as_human("commit", "-qm", "vendor/b, a gitlink", cwd=source) + git("add", "--", GITLINK, cwd=source) + as_human("commit", "-qm", "vendor/b, a file again", cwd=source) def source_with(base: Path, gitmodules: str | bytes, where: str) -> Path: """A source whose `.gitmodules` at the tip is `gitmodules`, and whose submodule is `where`: at the `tip`; only in the `history`, added and - removed by two earlier commits under a well-formed `.gitmodules`; or - `nowhere`, in no commit at all.""" + removed by two earlier commits under a well-formed `.gitmodules`; + entered and left by `type-changes` alone, which git's diff never asks + the submodule reader about; or `nowhere`, in no commit at all.""" source = make_source_repo(base / "Thing", tree=SMALL_TREE, edits=()) if where == "history": put_gitmodules(source, REGISTRATION) @@ -118,6 +127,8 @@ def source_with(base: Path, gitmodules: str | bytes, where: str) -> Path: as_human("commit", "-qm", "Add vendor/b", cwd=source) git("rm", "-q", "--cached", "--", GITLINK, cwd=source) as_human("commit", "-qm", "Remove vendor/b", cwd=source) + elif where == "type-changes": + type_changes(source) put_gitmodules(source, gitmodules) if where == "tip": link(source) @@ -132,40 +143,12 @@ def with_line(line: str) -> str: return REGISTRATION + f"\t{line}\n" -#: Where the plan sends `.gitmodules`, and the submodule's entry where the -#: tip holds one: to the code leg, as the issue's plan did. +#: Where the plan sends `.gitmodules`, and the entry for `vendor/` where +#: the tip holds something there: to the code leg, as the issue's plan did. ANSWERS = {"tip": {GITMODULES: "code", "vendor/": "code"}, - "history": {GITMODULES: "code"}, "nowhere": {GITMODULES: "code"}} - - -def planned(source: Path, base: Path, answers: dict) -> Path: - """`plan` for `source`, each entry `answers` names given its leg.""" - plan = base / "plan" / "adoption-plan.yaml" - plan.parent.mkdir() - written = write_plan(source, plan, project=PROJECT) - assert written.returncode == 0, written.stderr + written.stdout - text = plan.read_bytes().decode() - for path, leg in answers.items(): - entry = re.search(rf" - path: {re.escape(path)}\n leg: (\S+)\n", - text) - assert entry, f"no entry {path} in the plan" - if entry[1] == "null": - resolve(plan, path, leg) - text = plan.read_bytes().decode() - else: - text = text.replace(entry[0], - f" - path: {path}\n leg: {leg}\n", 1) - plan.write_bytes(text.encode()) - return plan - - -def findings(plan: Path) -> list[tuple[str, str]]: - result = run_script(ADOPT, "check", "--plan", str(plan)) - found = [tuple(line.split(" ", 1)[1].split(": ", 1)) - for line in result.stderr.splitlines() - if line.startswith("FINDING ")] - assert result.returncode == (1 if found else 0), result.stderr - return found + "history": {GITMODULES: "code"}, + "type-changes": {GITMODULES: "code", "vendor/": "code"}, + "nowhere": {GITMODULES: "code"}} def unreadable(said: str) -> str: @@ -174,22 +157,17 @@ def unreadable(said: str) -> str: f"({said}), ") -def in_the_history(adopter) -> re.Pattern: - """How the finding ends for a submodule only the history holds: the - commit that removed it, which `git log` meets first.""" - return re.compile(re.escape(adopter.FAILS_IN_THE_HISTORY).replace( - re.escape("{}"), r"commit [0-9a-f]{12} removes `vendor/b`") + "$") - - # --- what `check` finds ----------------------------------------------------- @pytest.mark.parametrize("where", ["tip", "history"]) -@pytest.mark.parametrize("line, said", REFUSED.items(), ids=list(REFUSED)) +@pytest.mark.parametrize("line, said", CHECK_REFUSES.items(), + ids=list(CHECK_REFUSES)) def test_check_finds_a_value_git_refuses(adopter, tmp_path, line, said, where): """Each row of the issue's table that git refuses, in both of its positions. On main each passed `check` and failed `execute` in `git - fast-export` after both leg repositories were made.""" + fast-export` after both leg repositories were made. So do the numbers + only some gits read, refused with the gits that refuse them.""" plan = planned(source_with(tmp_path, with_line(line), where), tmp_path, ANSWERS[where]) found = findings(plan) @@ -213,7 +191,7 @@ def test_check_passes_a_value_git_reads(tmp_path, line, where): assert findings(plan) == [] -@pytest.mark.parametrize("line", REFUSED) +@pytest.mark.parametrize("line", CHECK_REFUSES) def test_a_refused_value_beside_no_gitlink_anywhere_still_passes(tmp_path, line): """#166's S3, for a value: with no gitlink in the tree or its history, @@ -224,6 +202,21 @@ def test_a_refused_value_beside_no_gitlink_anywhere_still_passes(tmp_path, assert findings(plan) == [] +@pytest.mark.parametrize("line", CHECK_REFUSES) +def test_a_refused_value_beside_a_gitlink_only_type_changes_move_passes( + tmp_path, line): + """#195's type change, for a value: git's diff asks the submodule + reader about a changed path only when both of its sides are gitlinks, + so `git fast-export` in a clone of the source never reads this + `.gitmodules` and exits 0, and `check` passes it. Before #226's fix + round (f48c669) was merged here, `check` refused it.""" + source = source_with(tmp_path, with_line(line), "type-changes") + exported = exported_in_a_clone(source, tmp_path) + assert exported.returncode == 0, exported.stderr + plan = planned(source, tmp_path, ANSWERS["type-changes"]) + assert findings(plan) == [] + + @pytest.mark.parametrize("gitmodules, said", [ # git reads the file in order and dies on the first key it refuses. (with_line("shallow = maybe") + "\tpath\n", @@ -283,27 +276,16 @@ def test_the_key_and_the_value_are_spelled_on_one_line(adopter, tmp_path, # --- what `execute` refuses ------------------------------------------------- @pytest.mark.parametrize("where", ["tip", "history"]) -@pytest.mark.parametrize("line", REFUSED) +@pytest.mark.parametrize("line", CHECK_REFUSES) def test_execute_refuses_it_before_any_leg_exists(adopter, tmp_path, monkeypatch, line, where): """The refusal is `check`'s finding, raised by `_refuse_what_check_finds` before `_create_leg_remotes`: the directory the legs go in is never - made.""" - monkeypatch.setattr(adopter, "_require_filter_repo", lambda: None) + made (`refused_before_any_leg`).""" plan = planned(source_with(tmp_path, with_line(line), where), tmp_path, ANSWERS[where]) - found = findings(plan) - remotes = tmp_path / REMOTES - with pytest.raises(adopter.Refusal) as caught: - adopter.cmd_execute(Namespace( - plan=str(plan), source=None, local_remote_dir=remotes, - allow_empty_leg=[], yes=True, work_dir=tmp_path / "work")) - refusal = caught.value + refusal = refused_before_any_leg(adopter, monkeypatch, plan, tmp_path) assert refusal.code == UNREADABLE - assert refusal.detail.splitlines()[1:] == [ - f" FINDING {code}: {detail}" for code, detail in found] - assert "no leg repository was created" in refusal.remediation - assert not remotes.exists() # --- read as git reads it --------------------------------------------------- @@ -311,7 +293,8 @@ def test_execute_refuses_it_before_any_leg_exists(adopter, tmp_path, #: Values whose reading is the same in git 2.39.5 through 2.51 and in every #: C library CI runs on. Left out: the four bounds git 2.50 moved below #: zero, which an older git refuses, and a `0b` number, which only glibc -#: 2.38 and later reads; `test_the_numbers_check_reads_as_a_boolean` has those. +#: 2.38 and later reads: `check` refuses those with the gits that refuse +#: them, and `test_who_refuses_a_number_as_a_boolean` has them. BOOLEAN_VALUES = ["", "true", "Off", "YES", "maybe", "t", "1", "-1", "+1", "007", "08", "0x10", "0x", "1k", "1G", "2g", "-1g", "2097151k", "2097152k", "2147483647", "2147483648", @@ -364,23 +347,30 @@ def test_check_reads_a_value_as_the_installed_git_reads_it(adopter, reader, read, exported.stderr.decode(errors="replace")) -@pytest.mark.parametrize("value, read", [ - # git 2.50 lets in one more number below zero, for each unit; an older - # git refuses it, and `check` leaves it to the extraction, as on main. - ("-2147483648", True), ("-2147483649", False), - ("-2097152k", True), ("-2097153k", False), - ("-2048m", True), ("-2049m", False), ("-2g", True), ("-3g", False), - ("-0x80000000", True), +@pytest.mark.parametrize("value, refusers", [ + # git 2.50 lets in one more number below zero, for each unit, and an + # older git refuses it: `check` refuses it with the older git, so that + # `execute` cannot die on it after both leg repositories exist. + ("-2147483648", "OLDER_GITS_REFUSE"), ("-2147483649", "EVERY_GIT"), + ("-2097152k", "OLDER_GITS_REFUSE"), ("-2097153k", "EVERY_GIT"), + ("-2048m", "OLDER_GITS_REFUSE"), ("-2049m", "EVERY_GIT"), + ("-2g", "OLDER_GITS_REFUSE"), ("-3g", "EVERY_GIT"), + ("-0x80000000", "OLDER_GITS_REFUSE"), + # One above each, every version reads. + ("-2147483647", None), ("-2097151k", None), ("-2047m", None), + ("-1g", None), # Above zero every version agrees. - ("2147483647", True), ("2147483648", False), ("1g", True), - ("2g", False), + ("2147483647", None), ("2147483648", "EVERY_GIT"), ("1g", None), + ("2g", "EVERY_GIT"), # Read as binary by a git built against glibc 2.38 or later (git 2.43 - # here does), and refused elsewhere: not refused, so as not to refuse - # what git reads. - ("0b1", True), ("0B0", True), ("0b", False), ("0b2", False), + # here is), and refused by every other: `check` refuses it too. + ("0b1", "SOME_LIBRARIES_REFUSE"), ("0B0", "SOME_LIBRARIES_REFUSE"), + ("-0b1", "SOME_LIBRARIES_REFUSE"), ("0b", "EVERY_GIT"), + ("0b2", "EVERY_GIT"), ]) -def test_the_numbers_check_reads_as_a_boolean(adopter, value, read): - assert adopter._git_reads_as_a_boolean(value) is read +def test_who_refuses_a_number_as_a_boolean(adopter, value, refusers): + expected = None if refusers is None else getattr(adopter, refusers) + assert adopter._who_refuses_a_boolean(value) == expected # --- end to end ------------------------------------------------------------- @@ -443,14 +433,15 @@ def test_a_value_git_reads_still_adopts(tmp_path, line, where): @needs_filter_repo -def test_a_refused_value_beside_no_gitlink_anywhere_adopts_and_clones( - tmp_path): - """#166's S3, for a value, end to end: `update = bogus` in a source no - commit of which ever held a gitlink, kept in the code leg. Nothing runs - git's submodule reader on it: `execute` verifies, and the assembly's - `git clone --recurse-submodules` exits 0.""" - source = source_with(tmp_path, with_line("update = bogus"), "nowhere") - plan = planned(source, tmp_path, ANSWERS["nowhere"]) +@pytest.mark.parametrize("where", ["nowhere", "type-changes"]) +def test_a_refused_value_git_never_reads_adopts_and_clones(tmp_path, where): + """#166's S3, for a value, end to end: `update = bogus`, kept in the + code leg, in a source no commit of which ever held a gitlink, or whose + gitlink only type changes moved (#195's fix round). Nothing runs git's + submodule reader on it: `execute` verifies, and the assembly's `git + clone --recurse-submodules` exits 0.""" + source = source_with(tmp_path, with_line("update = bogus"), where) + plan = planned(source, tmp_path, ANSWERS[where]) assert run_script(ADOPT, "check", "--plan", str(plan)).returncode == 0 adopted = execute_in(plan, tmp_path) assert adopted.returncode == 0, adopted.stderr + adopted.stdout diff --git a/tests/test_adopt_history_gitlinks.py b/tests/test_adopt_history_gitlinks.py index c4bf1289..f6b07356 100644 --- a/tests/test_adopt_history_gitlinks.py +++ b/tests/test_adopt_history_gitlinks.py @@ -26,24 +26,22 @@ import shutil import stat import subprocess -from argparse import Namespace from pathlib import Path import pytest -from conftest import ADOPT, git, make_source_repo, resolve, run_script, \ - write_plan +from conftest import ADOPT, GITLINK, GITMODULES, as_human, \ + exported_in_a_clone, findings, git, in_the_history, link, \ + make_source_repo, named, planned, put_gitmodules, \ + refused_before_any_leg, run_script needs_filter_repo = pytest.mark.skipif( shutil.which("git-filter-repo") is None, reason="git filter-repo is not installed: `pip install git-filter-repo`") PROJECT = "Northwind" -GITMODULES = ".gitmodules" UNREADABLE = "plan-gitmodules-unreadable" REMOTES = "remotes" -#: The submodule the history holds, registered under the name `b`. -GITLINK = "vendor/b" REGISTRATION = '[submodule "b"]\n\tpath = vendor/b\n\turl = ../b\n' #: The issue's tip `.gitmodules`: a key with no value. BARE_PATH = '[submodule "b"]\n\tpath\n' @@ -55,12 +53,6 @@ "specs/001-feature/tasks.md": "- [ ] one\n"} -def named(verb: str = "(adds|removes|changes)") -> str: - """How `_exported_gitlink` names a commit's change to the gitlink, as a - pattern: `verb` is what that commit does to it.""" - return rf"commit [0-9a-f]{{12}} {verb} `vendor/b`" - - @pytest.fixture(scope="module") def adopter(): spec = importlib.util.spec_from_file_location("adopt_history_gitlinks", @@ -70,29 +62,6 @@ def adopter(): return module -def as_human(*args: str, cwd: Path): - """`git ` with an identity, for the commits and merges below.""" - return git("-c", "user.name=Source Human", - "-c", "user.email=source@invalid.example", *args, cwd=cwd) - - -def put_gitmodules(source: Path, text: str) -> None: - """`text` as the source's `.gitmodules`, staged: BYTES, so it is LF on - every platform, as a hand edit would leave it.""" - (source / GITMODULES).write_bytes(text.encode()) - git("add", "--", GITMODULES, cwd=source) - - -def link(source: Path) -> None: - """A gitlink at GITLINK, staged straight into the index, as `git add` of - an embedded clone records one; no checkout of it is needed. It names - `main`'s commit, which an orphan branch's first commit has no HEAD to - give.""" - head = git("rev-parse", "main", cwd=source).stdout.strip() - git("update-index", "--add", "--cacheinfo", f"160000,{head},{GITLINK}", - cwd=source) - - def unlink(source: Path) -> None: git("rm", "-q", "--cached", "--", GITLINK, cwd=source) @@ -274,17 +243,6 @@ def opened(adopter, source: Path, tmp_path: Path): return adopter.Source.open(str(source), tmp_path / "read") -def exported_in_a_clone(source: Path, base: Path): - """`git fast-export --all` in a clone of `source`, as `git filter-repo` - runs it in the extraction's clone: the run whose failure the scan - foretells. It needs git alone, so it runs where `git filter-repo` is - not installed; bytes, because it exports the source's own content.""" - clone = base / "export-clone" - git("clone", "-q", "--", str(source), str(clone), cwd=base) - return subprocess.run(["git", "fast-export", "--all"], cwd=clone, - capture_output=True, check=False) - - # --- where the history holds a gitlink ------------------------------------- @pytest.mark.parametrize("history, verb", HISTORIES.values(), @@ -445,43 +403,6 @@ def test_the_scan_starts_at_the_plans_commit_not_the_source_head(adopter, # --- what `check` finds ----------------------------------------------------- -def planned(source: Path, base: Path, answers: dict) -> Path: - """`plan` for `source`, its `.gitmodules` question answered and any - other entry's `leg:` set as `answers` says.""" - plan = base / "plan" / "adoption-plan.yaml" - plan.parent.mkdir() - written = write_plan(source, plan, project=PROJECT) - assert written.returncode == 0, written.stderr + written.stdout - text = plan.read_bytes().decode() - for path, leg in answers.items(): - entry = re.search(rf" - path: {re.escape(path)}\n leg: (\S+)\n", - text) - assert entry, f"no entry {path} in the plan" - if entry[1] == "null": - resolve(plan, path, leg) - text = plan.read_bytes().decode() - else: - text = text.replace(entry[0], - f" - path: {path}\n leg: {leg}\n", 1) - plan.write_bytes(text.encode()) - return plan - - -def findings(plan: Path) -> list[tuple[str, str]]: - result = run_script(ADOPT, "check", "--plan", str(plan)) - found = [tuple(line.split(" ", 1)[1].split(": ", 1)) - for line in result.stderr.splitlines() - if line.startswith("FINDING ")] - assert result.returncode == (1 if found else 0), result.stderr - return found - - -def in_the_history(adopter, verb: str = "removes") -> re.Pattern: - """The end of the finding for a gitlink only the history holds.""" - return re.compile(re.escape(adopter.FAILS_IN_THE_HISTORY).replace( - re.escape("{}"), named(verb)) + "$") - - #: Where the plan sends `.gitmodules`; the two legs are extracted either way. EITHER_WAY = pytest.mark.parametrize("modules_leg", ["code", "drop"]) @@ -612,23 +533,12 @@ def test_execute_refuses_it_before_any_leg_exists(adopter, tmp_path, monkeypatch, modules_leg): """The refusal is `check`'s finding, raised by `_refuse_what_check_finds` before `_create_leg_remotes`: the directory the legs go in is never - made.""" - monkeypatch.setattr(adopter, "_require_filter_repo", lambda: None) + made (`refused_before_any_leg`).""" plan = planned(history_source(tmp_path), tmp_path, {GITMODULES: modules_leg}) - found = findings(plan) - remotes = tmp_path / REMOTES - with pytest.raises(adopter.Refusal) as caught: - adopter.cmd_execute(Namespace( - plan=str(plan), source=None, local_remote_dir=remotes, - allow_empty_leg=[], yes=True, work_dir=tmp_path / "work")) - refusal = caught.value + refusal = refused_before_any_leg(adopter, monkeypatch, plan, tmp_path) assert refusal.code == UNREADABLE - assert refusal.detail.splitlines()[1:] == [ - f" FINDING {code}: {detail}" for code, detail in found] - assert "no leg repository was created" in refusal.remediation assert refusal.remediation.endswith("\n" + adopter.SUBMODULE_REMEDIATION) - assert not remotes.exists() # --- end to end ------------------------------------------------------------- From 5521e8281eded4f008f80a3bb89b978ca8dcb18c Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 23:08:21 +0000 Subject: [PATCH 5/5] fix round 2 (#215): the `0b` refusal names the C library feature, not glibc alone The verifier's optional nit 1 on fix round 1: C23 makes a base-0 `strtoimax` read `0b`/`0B`, so any C library that implements it reads a `0b` number, not glibc alone. SOME_LIBRARIES_REFUSE now says "a `0b` number, which a git reads only where its C library reads C23 binary numbers (glibc 2.38 or later, for one)", and the comments that said "only glibc" or "refused by every other" say the same. Wording only: what `check` refuses is unchanged. Nit 2 (the frame "git cannot read the source's .gitmodules" over a value the installed git reads) is left: the frame is #226's shared finding builder on main, and the parenthesis already says which gits refuse it. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 10 ++++++---- tests/test_adopt_gitmodules_values.py | 15 +++++++++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index c00d04b3..18216542 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -385,8 +385,9 @@ def _move(self, name: str, path: str) -> None: BOOLEAN_WORDS = ("true", "yes", "on", "false", "no", "off") #: An integer as `git_parse_signed` reads one: `strtoimax(value, &end, 0)`, #: which skips C whitespace and takes a sign and a hex, octal or decimal -#: number -- and, built against glibc 2.38 or later, a `0b` binary one -- and -#: then a unit, which is all that may follow. +#: number -- and, where the C library reads C23 binary numbers (glibc 2.38 +#: or later, for one), a `0b` binary one -- and then a unit, which is all +#: that may follow. GIT_INTEGER_RE = re.compile( r"[ \t\n\v\f\r]*(?P[+-]?)(?P0[xX][0-9a-fA-F]+|0[bB][01]+" r"|0[0-7]*|[1-9][0-9]*)(?P[kKmMgG]?)") @@ -399,8 +400,9 @@ def _move(self, name: str, path: str) -> None: EVERY_GIT = "every git" OLDER_GITS_REFUSE = ("a number git 2.49 and older refuse, and 2.50 and later " "read") -SOME_LIBRARIES_REFUSE = ("a `0b` number, which only a git built on glibc " - "2.38 or later reads") +SOME_LIBRARIES_REFUSE = ("a `0b` number, which a git reads only where its C " + "library reads C23 binary numbers (glibc 2.38 or " + "later, for one)") def _who_refuses_a_boolean(value: str) -> str | None: diff --git a/tests/test_adopt_gitmodules_values.py b/tests/test_adopt_gitmodules_values.py index 9d0b3ab9..255edaa3 100644 --- a/tests/test_adopt_gitmodules_values.py +++ b/tests/test_adopt_gitmodules_values.py @@ -79,7 +79,8 @@ "git 2.49 and older refuse, and 2.50 and later read", "shallow = 0b1": "bad boolean config value `0b1` for `submodule.b.shallow`, a `0b` " - "number, which only a git built on glibc 2.38 or later reads", + "number, which a git reads only where its C library reads C23 binary " + "numbers (glibc 2.38 or later, for one)", } #: Every line `check` refuses beside a gitlink. CHECK_REFUSES = {**REFUSED, **ONLY_SOME_GITS_READ} @@ -292,9 +293,10 @@ def test_execute_refuses_it_before_any_leg_exists(adopter, tmp_path, #: Values whose reading is the same in git 2.39.5 through 2.51 and in every #: C library CI runs on. Left out: the four bounds git 2.50 moved below -#: zero, which an older git refuses, and a `0b` number, which only glibc -#: 2.38 and later reads: `check` refuses those with the gits that refuse -#: them, and `test_who_refuses_a_number_as_a_boolean` has them. +#: zero, which an older git refuses, and a `0b` number, which only a C +#: library that reads C23 binary numbers (glibc 2.38 or later) reads: +#: `check` refuses those with the gits that refuse them, and +#: `test_who_refuses_a_number_as_a_boolean` has them. BOOLEAN_VALUES = ["", "true", "Off", "YES", "maybe", "t", "1", "-1", "+1", "007", "08", "0x10", "0x", "1k", "1G", "2g", "-1g", "2097151k", "2097152k", "2147483647", "2147483648", @@ -362,8 +364,9 @@ def test_check_reads_a_value_as_the_installed_git_reads_it(adopter, reader, # Above zero every version agrees. ("2147483647", None), ("2147483648", "EVERY_GIT"), ("1g", None), ("2g", "EVERY_GIT"), - # Read as binary by a git built against glibc 2.38 or later (git 2.43 - # here is), and refused by every other: `check` refuses it too. + # Read as binary by a git whose C library reads C23 binary numbers + # (glibc 2.38 or later; git 2.43 here is one), and refused by any other: + # `check` refuses it too. ("0b1", "SOME_LIBRARIES_REFUSE"), ("0B0", "SOME_LIBRARIES_REFUSE"), ("-0b1", "SOME_LIBRARIES_REFUSE"), ("0b", "EVERY_GIT"), ("0b2", "EVERY_GIT"),