diff --git a/src/docproof/vcs.py b/src/docproof/vcs.py index c08cbe7..61a3fde 100644 --- a/src/docproof/vcs.py +++ b/src/docproof/vcs.py @@ -34,6 +34,12 @@ # costs a bounded number of `git show` calls rather than a hang. _MAX_RENAME_HOPS = 10 +# How many same-basename files `succeeded_by` will test for lineage. Five, because the +# candidates are ranked by how much of the original directory chain survives and the +# lineage proof is what accepts one - the cap only bounds the cost on a monorepo with a +# hundred and fifty `index.ts`. The worst real case needed the third. +_MAX_MOVE_CANDIDATES = 5 + def _run(args: list[str], cwd: Path, stdin: str | None = None) -> tuple[int, str, str]: try: @@ -237,6 +243,146 @@ def moved_to(self, path: str, commit: str) -> str | None: destination = following return None + @cached_property + def _tracked_by_basename(self) -> dict[str, tuple[str, ...]]: + index: dict[str, list[str]] = {} + for path in self.tracked_files: + index.setdefault(path.rsplit("/", 1)[-1], []).append(path) + return {name: tuple(sorted(paths)) for name, paths in index.items()} + + def _size_at(self, revision: str, path: str) -> int | None: + code, out, _ = _run(["git", "cat-file", "-s", f"{revision}:{path}"], self.root) + if code != 0: + return None + try: + return int(out.strip()) + except ValueError: + return None + + def succeeded_by(self, path: str, commit: str) -> str | None: + """Where the content went when the DELETING commit is not the move. + + `moved_to` asks git what the deleting commit renamed, and a very common refactor + defeats it. A split copies a file to its new home and leaves a one-line re-export + behind; a later commit sweeps up the re-export. Git records that second commit as + a plain `D` and is right to - a one-line stub resembles nothing - so the finding + says "deleted and never restored", which is true of the path and false of the + content. + + `zhukunpenglinyutong/desktop-cc-gui`, the case that forced this: + + b66a616b3 "Split app-shell.tsx" C099 app-shell-parts/modelSelection.ts + -> app-shell/domains/modelSelection.ts + 772b6c681 "Clean up useless code" D app-shell-parts/modelSelection.ts (1 line) + + Its onboarding guide - `status: active`, calibrated to the current release - lists + that path as a file you must edit to add an engine. The function is at line 112 of + the new one. Six of that project's eleven findings are this same commit pair. + + THE LINEAGE IS PROVED, NOT GUESSED. The obvious version looks for a file at HEAD + with the same basename, and that is the rule `BARE-FILENAME-VERDICT.md` rejected + wearing a different hat. Measured on the 160 corpus findings that say + "never restored", 25 have a same-basename file at HEAD and no lineage, and reading + them is enough: `sdk/package.json` -> `package.json`, `pkg/registry/types.go` -> + `pkg/git/types.go`, `website/content/getting-started.md` -> + `third-party/vendor/logos-0.14.4/book/src/getting-started.md`. So a candidate is + only accepted when `log --follow --find-copies` from it names this exact path as an + ancestor. Basename is how candidates are FOUND; it is never how one is accepted. + + AND THE SOURCE MUST NOT HAVE GROWN AFTER THE COPY. Ten findings had proven lineage + and one of them was plainly wrong: + + bytebase docs/adding-new-object-to-sdl-mode.md:492 + says backend/plugin/schema/pg/generate_migration.go + C077 in 044c898364 "feat: implement oracle generate migration (#16546)" + + Somebody copied the Postgres implementation to start the Oracle one. Nothing moved: + there are four `generate_migration.go` at HEAD, one per dialect. Taking only `R` and + dropping `--find-copies` would remove it and would also remove desktop-cc-gui, which + git records as `C099`. Size at deletion does not separate it either - the true group + runs from 1 line to 252 and cherry-studio sits at 0.979 of its destination. + + What separates them is what happened to the SOURCE after the copy landed. A file on + its way out is frozen; it becomes a stub, or sits untouched while callers migrate. + A forked sibling is developed, because it is now a second thing. + + desktop-cc-gui x6 1 -> 1 bytes-equivalent, 0 commits move + bytebase directiveUtils.ts 252 -> 252, 0 commits move + cherry-studio useToolApproval.ts 143 -> 143, 0 commits move + sentry-rn build.gradle 42 -> 22, 4 commits move (shrank) + bytebase generate_migration 1583 -> 5192, 44 FORK (grew) + + So: refuse the candidate if the source was bigger when it was deleted than when the + copy landed. Nine of ten survive, and the tenth is the fork. + + HONEST LIMITS. Recall is 9 of 160 findings, 5.6%, and six of the nine are one commit + pair in one project - this pattern is concentrated, not common, which is also why it + matters: a project that does one split-with-shims gets every finding mis-explained at + once. And the growth rule is fitted against a single negative case. It is stated this + way because a rule chosen from n=1 should say so. + + VALIDATED BY SOMEONE ELSE. `getsentry/sentry-react-native` made this exact edit + unprompted in `fd677570` (2026-08-18, "docs: update CONTRIBUTING paths for the + monorepo layout", #6594), replacing `sample/android/build.gradle` with + `samples/react-native/android/build.gradle` - the destination this computes. + + The verdict never changes. The path really is gone and the document really is stale. + This only decides whether the reader is told where to look. + """ + candidates = self._tracked_by_basename.get(path.rsplit("/", 1)[-1], ()) + if not candidates: + return None + original = set(path.split("/")[:-1]) + ranked = sorted( + candidates, + key=lambda found: (-len(set(found.split("/")[:-1]) & original), len(found)), + ) + for candidate in ranked[:_MAX_MOVE_CANDIDATES]: + copied_in = self._copied_from(candidate, path) + if copied_in is None: + continue + when_copied = self._size_at(copied_in, path) + when_deleted = self._size_at(f"{commit}^", path) + if when_copied is None or when_deleted is None: + continue + if when_deleted > when_copied: + continue + return candidate + return None + + def _copied_from(self, destination: str, source: str) -> str | None: + """The commit where `source` became `destination`, as git tells it. + + Asked from the destination, because that is the direction that answers. A pathspec + on the deleted path finds nothing - it is the source of the copy, not a file the + commit changed - and the query returns empty. Tested before this was written. + """ + code, out, _ = _run( + [ + "git", + "log", + "--follow", + "--find-copies", + "--diff-filter=CR", + "--name-status", + "--format=commit %h", + "--", + destination, + ], + self.root, + ) + if code != 0: + return None + commit: str | None = None + for line in out.splitlines(): + if line.startswith("commit "): + commit = line[len("commit ") :].strip() + continue + parts = line.split("\t") + if len(parts) == 3 and parts[0][:1] in {"R", "C"} and parts[1] == source: + return commit + return None + def claim_introduced_after(self, doc: str, subject: str, commit: str) -> bool | None: """Whether this document first mentioned `subject` *after* `commit` removed it. diff --git a/src/docproof/verifiers/paths.py b/src/docproof/verifiers/paths.py index c5c503b..c1aa0de 100644 --- a/src/docproof/verifiers/paths.py +++ b/src/docproof/verifiers/paths.py @@ -673,6 +673,18 @@ def _judge(self, project: Project, claim: Claim, git: Git) -> Finding: f"and the documentation still points at the old path", ) + # And when the deleting commit is only sweeping up a re-export the split left + # behind, git records a plain `D` and `moved_to` finds nothing. The content is + # still somewhere and `succeeded_by` will name it if git can prove the lineage. + successor = git.succeeded_by(subject, commit) + if successor: + return self.broken( + claim, + f"the content is at `{successor}`; what {commit} ({date}, " + f'"{subject_line[:60]}") deleted was the path, left behind by an earlier ' + f"move, and the documentation still points at it", + ) + return self.broken( claim, f'deleted in {commit} ({date}, "{subject_line[:60]}") and never restored, ' diff --git a/tests/test_shim_move.py b/tests/test_shim_move.py new file mode 100644 index 0000000..f37fbbf --- /dev/null +++ b/tests/test_shim_move.py @@ -0,0 +1,188 @@ +"""A deleted re-export is not a deleted file. + +`moved_to` asks what the DELETING commit renamed, and the commonest large refactor there +is defeats it. A split copies a file to its new home and leaves a one-line re-export at the +old path so nothing breaks; weeks later a tidy-up deletes the re-export. Git calls that +second commit a plain `D`, correctly - a one-line stub resembles nothing - so the finding +reads "deleted and never restored", which is true of the path and false of the content. + +`zhukunpenglinyutong/desktop-cc-gui`: + + b66a616b3 "Split app-shell.tsx" C099 app-shell-parts/modelSelection.ts + -> app-shell/domains/modelSelection.ts + 772b6c681 "Clean up useless code" D app-shell-parts/modelSelection.ts (1 line) + +Its onboarding guide - `status: active` in its own frontmatter, calibrated to the current +release - lists the old path among the files you must edit to add an engine. Six of that +project's eleven findings are that one commit pair, which is the shape of this: rare across +projects, and everywhere at once inside the project that does it. + +The two negative tests are the point of the design. Accepting a same-basename file at HEAD +is the rule `BARE-FILENAME-VERDICT.md` rejected wearing a different hat, and accepting any +git-visible copy admits `bytebase`, where `feat: implement oracle generate migration` copied +the Postgres migration generator to start the Oracle one and both went on living. + +These build their own history rather than using `make_repo`: the shape under test is a copy +and a deletion in two separate commits, and a fixture that cannot produce that cannot test +this. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from docproof.vcs import Git + +BODY = "\n".join(f"export const value{n} = {n};" for n in range(60)) + "\n" +SHIM = 'export * from "../../new/home/modelSelection";\n' + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", *args], + cwd=repo, + capture_output=True, + check=True, + timeout=60, + env={ + "GIT_AUTHOR_NAME": "docproof tests", + "GIT_AUTHOR_EMAIL": "test@example.invalid", + "GIT_COMMITTER_NAME": "docproof tests", + "GIT_COMMITTER_EMAIL": "test@example.invalid", + "GIT_AUTHOR_DATE": "2026-01-01T00:00:00", + "GIT_COMMITTER_DATE": "2026-01-01T00:00:00", + "PATH": os.environ.get("PATH", ""), + "SYSTEMROOT": os.environ.get("SYSTEMROOT", ""), + }, + ) + + +def _write(repo: Path, path: str, text: str) -> None: + target = repo / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(text, encoding="utf-8") + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + root = tmp_path / "split" + root.mkdir() + _git(root, "init", "-q") + _write(root, "src/old/parts/modelSelection.ts", BODY) + _git(root, "add", "-A") + _git(root, "commit", "-qm", "the file, at its original home", "--no-gpg-sign") + return root + + +def _split_leaving_a_shim(repo: Path) -> None: + """One commit: the content lands at the new path, the old path becomes a re-export. + + Both in the same commit, because that is what lets git's copy detection pair them - + and it is what the real refactor does, so nothing breaks between the two commits. + """ + _write(repo, "src/new/home/modelSelection.ts", BODY) + _write(repo, "src/old/parts/modelSelection.ts", SHIM) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "Split app-shell.tsx", "--no-gpg-sign") + + +def _sweep_up_the_shim(repo: Path) -> None: + _git(repo, "rm", "-q", "src/old/parts/modelSelection.ts") + _git(repo, "commit", "-qm", "Clean up useless code", "--no-gpg-sign") + + +def test_the_shim_pattern_names_where_the_content_went(repo: Path) -> None: + """The case that forced this to exist.""" + _split_leaving_a_shim(repo) + _sweep_up_the_shim(repo) + + git = Git(root=repo) + receipt = git.deleted("src/old/parts/modelSelection.ts") + assert receipt is not None + + # The premise: git calls this a plain deletion, so the existing path finds nothing. + assert git.moved_to("src/old/parts/modelSelection.ts", receipt[0]) is None + + destination = git.succeeded_by("src/old/parts/modelSelection.ts", receipt[0]) + assert destination == "src/new/home/modelSelection.ts" + # Same invariant `moved_to` holds: never name a path that is not there now. + assert git.tracks(destination) + + +def test_a_forked_sibling_is_refused(repo: Path) -> None: + """`bytebase`, reduced to its bones. + + `044c898364 "feat: implement oracle generate migration"` copied the Postgres generator + to start the Oracle one. Git records `C077`, so lineage alone would accept it. The + Postgres file then grew from 1583 lines to 5192 and was deleted on its own a year later. + Nothing moved; there are four `generate_migration.go` at that HEAD, one per dialect. + + What separates the two cases is the source AFTER the copy: a file on its way out is + frozen, a forked sibling keeps being developed. + """ + _write(repo, "src/new/home/modelSelection.ts", BODY) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "start a second dialect from the first", "--no-gpg-sign") + + _write(repo, "src/old/parts/modelSelection.ts", BODY + BODY) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "keep developing the original", "--no-gpg-sign") + + _git(repo, "rm", "-q", "src/old/parts/modelSelection.ts") + _git(repo, "commit", "-qm", "retire the original, much later", "--no-gpg-sign") + + git = Git(root=repo) + receipt = git.deleted("src/old/parts/modelSelection.ts") + assert receipt is not None + assert git.succeeded_by("src/old/parts/modelSelection.ts", receipt[0]) is None + + +def test_a_namesake_with_no_lineage_is_refused(repo: Path) -> None: + """Basename finds candidates. It never accepts one. + + Of the 160 corpus findings that say "never restored", 25 have a same-basename file at + HEAD with no lineage to it, and they are `sdk/package.json` -> `package.json`, + `pkg/registry/types.go` -> `pkg/git/types.go`, and + `website/content/getting-started.md` -> a vendored copy under `third-party/`. + """ + _write(repo, "src/unrelated/modelSelection.ts", "export const different = true;\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "a different file that shares a name", "--no-gpg-sign") + + _git(repo, "rm", "-q", "src/old/parts/modelSelection.ts") + _git(repo, "commit", "-qm", "delete the original outright", "--no-gpg-sign") + + git = Git(root=repo) + receipt = git.deleted("src/old/parts/modelSelection.ts") + assert receipt is not None + assert git.succeeded_by("src/old/parts/modelSelection.ts", receipt[0]) is None + + +def test_a_file_with_no_namesake_at_all_names_nothing(repo: Path) -> None: + _git(repo, "rm", "-q", "src/old/parts/modelSelection.ts") + _git(repo, "commit", "-qm", "gone, and nothing took its place", "--no-gpg-sign") + + git = Git(root=repo) + receipt = git.deleted("src/old/parts/modelSelection.ts") + assert receipt is not None + assert git.succeeded_by("src/old/parts/modelSelection.ts", receipt[0]) is None + + +def test_a_plain_rename_is_left_to_moved_to(repo: Path) -> None: + """No regression on the path that already worked. + + A single `git mv` is what `moved_to` is for, and it must keep answering first - this + only ever runs when that one returns None. + """ + (repo / "src" / "new" / "home").mkdir(parents=True) + _git(repo, "mv", "src/old/parts/modelSelection.ts", "src/new/home/modelSelection.ts") + _git(repo, "commit", "-qm", "just move it", "--no-gpg-sign") + + git = Git(root=repo) + receipt = git.deleted("src/old/parts/modelSelection.ts") + assert receipt is not None + assert git.moved_to("src/old/parts/modelSelection.ts", receipt[0]) == "src/new/home/modelSelection.ts"