From 7a427fe3d2ba00785205dad1e908c534d8c9da58 Mon Sep 17 00:00:00 2001 From: melbinjp Date: Wed, 19 Aug 2026 19:12:07 +0530 Subject: [PATCH] A deleted re-export is not a deleted file `moved_to` asks what the deleting commit renamed, and the commonest large refactor there defeats it. A split copies a file to its new home and leaves a one-line re-export at the old path; a later tidy-up deletes the re-export. Git records that second commit as a plain `D`, correctly - 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`: 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 the old path among the files 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 that one commit pair. Lineage is proved, never guessed. Of 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` -> a vendored copy under `third-party/`. Basename finds candidates; only `log --follow --find-copies` accepts one. And the source must not have grown after the copy. Ten had proven lineage and one was plainly wrong - bytebase's `feat: implement oracle generate migration` copied the Postgres generator to start the Oracle one, and both went on living. Taking only `R` would also lose desktop-cc-gui, which git records as `C099`; size at deletion does not separate them either. What does is the source after the copy: frozen for a move, developed for a fork. Recall is 9 of 160, 5.6%, six of them one commit pair - concentrated rather than common, which is also why it matters. The growth rule is fitted against a single negative case and the docstring says so. `getsentry/sentry-react-native` made this exact edit unprompted in `fd677570` (#6594, 2026-08-18), arriving at the destination this computes. The verdict never changes. This only decides whether the reader is told where to look. --- src/docproof/vcs.py | 146 +++++++++++++++++++++++++ src/docproof/verifiers/paths.py | 12 ++ tests/test_shim_move.py | 188 ++++++++++++++++++++++++++++++++ 3 files changed, 346 insertions(+) create mode 100644 tests/test_shim_move.py 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"