From 54c30a81b55732869808841542605f0da244e4f9 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 21:31:27 +0000 Subject: [PATCH 1/3] family.py: `add`, `bump` and `remove` refuse before their first write a merge, cherry-pick, revert, rebase or `git am` in progress, a `.gitmodules`, `members/` or `family.yaml` the sparse checkout leaves out, and a file named `members`, which each failed after writing and was rolled back, or committed in the wrong place (#612) `add`, `bump` and `remove` each end in ONE `git commit -- `, made after the verb's first writes and put back by a rollback if it fails. Three states of the holder made it fail, or land in the wrong place, every time. Each is now asked before the first write and under `--dry-run` alike, so the two runs refuse in the same words. AN ACT GIT HAS UNFINISHED (571(e)). `_checked_no_operation_in_progress` looks for the holder's MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD, rebase-merge/ and rebase-apply/ where `git rev-parse --git-path` says git keeps them, which is right in a linked worktree too, and refuses `family-operation-in-progress`: the detail names the act and what the verb's commit would do in it, the remediation that act's own exit (`git commit`, `--continue` or `--abort`). It comes before the branch refusal and before every refusal that reads the holder. Measured on git 2.43: a merge and a cherry-pick fail the partial commit after the writes and the rollback puts them back, but a revert, a rebase and a `git am` do NOT fail it, so the commit was made inside the act, and the real run met a rebase as `family-holder-detached`, whose `git switch -c` is the wrong advice there, while its dry run printed a plan. A PATH THE SPARSE CHECKOUT LEAVES OUT (571(d), 574/5). `_checked_inside_sparse_rules` asks `git sparse-checkout check-rules` about the paths the verb stages: `.gitmodules`, `members/` and `family.yaml` for `add` and `remove`, the last two for `bump`. #407's `gitmodules-sparse` reads the skip-worktree MARK, which git clears for a file put back by hand, so the rules are asked instead, and `family-sparse-excluded` names `git sparse-checkout add members` (or `/.gitmodules`) and `git sparse-checkout disable`. It is asked only where `git sparse-checkout list` says sparse checkout is on: `disable` leaves the rules file behind and check-rules still reads it. In cone mode git keeps every root file inside, so only the mount path can be refused there. A git whose check-rules fails is "cannot tell": nothing is refused, and the late failure and its rollback stay. A FILE NAMED `members` (574/5). `_checked_mount_path` refuses it as `member-path-unaddable`, the code of a mount path git will not take, before the member is read, with the move that makes room (`git mv` and a commit where git tracks the file, `mv` where it does not). 48 tests in tests/test_family_refuse_up_front.py: each verb under a merge, the five acts, the linked worktree, the sparse cases in cone and non-cone mode, a disabled sparse checkout, a git that cannot tell, and the file, each with its remedy typed as printed. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 325 +++++++++++- tests/test_family_refuse_up_front.py | 768 +++++++++++++++++++++++++++ 2 files changed, 1084 insertions(+), 9 deletions(-) create mode 100644 tests/test_family_refuse_up_front.py diff --git a/scripts/family.py b/scripts/family.py index a3eb0e08..89287576 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -753,6 +753,249 @@ def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: f" {_switch_command(family.root, suggested)}"] +# --------------------------------------------------------------------------- +# What the holder is in the middle of, and what its sparse checkout leaves out +# --------------------------------------------------------------------------- +# +# `add`, `bump` and `remove` each end in ONE `git commit -- ` +# (`_commit_one`), made after the verb's first writes and put back by its +# rollback if it fails. An act git has unfinished in the holder, and a path +# its sparse checkout leaves out, make that commit fail or land in the wrong +# place EVERY time, so each verb did its writes and undid them, or committed +# where nobody wanted it: refusals made at the wrong end (#612, from #459's +# 571(e), 571(d) and 574/5; the third of 574/5, a `members` that is a file, +# is `_members_is_a_file`, beside the mount path's other refusals). Each is +# asked once, before the first write and under `--dry-run` alike, so the two +# runs refuse in the same words. + +#: What git keeps in its directory while an act is unfinished, in the order +#: `_operation_in_progress` asks it: `(the name git keeps, what it is, how a +#: person finishes it, how a person gives it up, what `git commit -- +#: ` does during it)`, the last with `{verb}` for the verb that +#: asked. A REBASE IS ASKED FIRST: it holds a HEAD detached on the commit it +#: stopped at, it may hold a merge, a cherry-pick or a revert of its own +#: beside its mark, and `git rebase --abort` puts all of it back, so it is +#: the act to name. What git does with the partial commit was measured on +#: git 2.43 in each state: it REFUSES one during a merge and a cherry-pick +#: and MAKES it during a revert, a rebase and a `git am`. +OPERATION_MARKS = ( + ("rebase-merge", "a rebase", "git rebase --continue", + "git rebase --abort", "git does not refuse that during a rebase, so " + "the commit would land on the HEAD the rebase has detached, inside its " + "sequence, where no branch can carry it to a pull request"), + ("rebase-apply", "a rebase", "git rebase --continue", + "git rebase --abort", "git does not refuse that during a rebase, so " + "the commit would land on the HEAD the rebase has detached, inside its " + "sequence, where no branch can carry it to a pull request"), + ("MERGE_HEAD", "a merge", "git commit", "git merge --abort", + "git refuses that during a merge (`fatal: cannot do a partial commit " + "during a merge.`), after `{verb}`'s first writes, which are then put " + "back"), + ("CHERRY_PICK_HEAD", "a cherry-pick", "git cherry-pick --continue", + "git cherry-pick --abort", + "git refuses that during a cherry-pick (`fatal: cannot do a partial " + "commit during a cherry-pick.`), after `{verb}`'s first writes, which " + "are then put back"), + ("REVERT_HEAD", "a revert", "git revert --continue", + "git revert --abort", "git does not refuse that during a revert, so " + "the commit would be made inside the revert and end it as the revert's " + "own"), +) + +#: `git am` keeps its state in `rebase-apply` too, and git tells it from a +#: rebase by the file `applying` there (`wt_status_check_rebase`): so that +#: its exit is `git am --abort`, which `git rebase --abort` is not. Its HEAD +#: stays on the branch, so the partial commit lands there. +AM_MARK = "rebase-apply/applying" +AM_OPERATION = ("a `git am`", "git am --continue", "git am --abort", + "git does not refuse that during a `git am`, so the commit " + "would land on the branch, in the middle of the patches " + "being applied") + + +def _git_path(root: Path, name: str) -> Path: + """Where git keeps `name` for the holder at `root`, ASKED OF GIT (`git + rev-parse --git-path`), never `.git/`: in a linked worktree, which + is where a lane opens its pull request, `.git` is a file and the + per-worktree state (`MERGE_HEAD`, `rebase-merge/`, `index.lock`) is under + `.git/worktrees//` (#365, `_module_dir`, `_checked_index_unlocked`). + git answers relative to `root` or absolute, and `root /` takes both.""" + shown = _git_bytes(["rev-parse", "--git-path", name], root).decode( + "utf-8", "surrogateescape") + return root / (shown[:-1] if shown.endswith("\n") else shown) + + +def _operation_in_progress(root: Path + ) -> tuple[str, str, Path, str, str, str] | None: + """`(what, the mark's name, where it is, how to finish it, how to give it + up, what the partial commit would do)` for the act the holder's git + directory shows unfinished, or None; the first of `OPERATION_MARKS` + that is there.""" + for mark, what, finish, abort, effect in OPERATION_MARKS: + where = _git_path(root, mark) + if not os.path.lexists(where): + continue + if mark == "rebase-apply" and os.path.lexists( + _git_path(root, AM_MARK)): + what, finish, abort, effect = AM_OPERATION + return what, mark, where, finish, abort, effect + return None + + +def _checked_no_operation_in_progress(root: Path, verb: str) -> None: + """`family-operation-in-progress`: `verb` refuses, before its first + write and under `--dry-run` alike, a holder with a merge, a cherry-pick, + a revert, a rebase or a `git am` unfinished in it (#612, 571(e)). + + THE COMMIT GOES WRONG, AFTER THE WRITES. `add`, `bump` and `remove` + commit with `git commit -- ` (`_commit_one`). git refuses a + partial commit while a merge or a cherry-pick is unfinished (`fatal: + cannot do a partial commit during a merge.`), and by then `add` had + mounted the member, `bump` had checked it out at `--to` and `remove` had + unmounted it; the rollback put each back (#203, #365) and the person got + exit 2, with the work done and undone for nothing. The commonest source + is `git merge`, which is also where #571's `--ours` and `--theirs` + restores come from: the same state, one step earlier. A merge that + stopped with `--no-commit` has no conflict at all and fails the same. + + A REVERT, A REBASE AND A `git am` ARE NOT REFUSED BY git, which is + worse (measured on git 2.43, each stopped on a conflict). The partial + commit is made: inside the revert, where it ends the revert with a + family commit as the revert's own; on the HEAD a rebase has detached, + where it sits in the rebase's sequence and no branch carries it to a + pull request; and on the branch in the middle of an `am`. The real run + met a rebase at the branch refusal (`family-holder-detached`, #487), + whose remedy, `git switch -c`, is the wrong advice in the middle of a + rebase, and the dry run printed a plan for it. This check comes before + that one, so a rebase is named as what it is. + + So the five marks are asked, and the refusal names the one it found and + that act's OWN exit: `git commit` for a merge, `--continue` for the + others, or `--abort`. Every mark is looked for WHERE GIT KEEPS IT + (`_git_path`), which in a linked worktree is not `.git/`. Asked of + the holder's own git directory, never of a member's: a merge in a + MEMBER's mount is that checkout's own business. + """ + found = _operation_in_progress(root) + if found is None: + return + what, mark, where, finish, abort, effect = found + raise Refusal( + "family-operation-in-progress", + f"{what} is unfinished in the family root: git keeps {mark} at " + f"{where.as_posix()}. `{verb}` makes its commit with `git commit -- " + f"`, and {effect.format(verb=verb)}. No git command that " + "writes was run and nothing was written.", + "Remediation: end it, whichever way suits the work in it: finish it " + f"with `{finish}` once whatever stopped it is resolved and staged, " + f"or give it up with `{abort}`. Then run the same command again.") + + +def _sparse_rules_exclude(root: Path, paths: list[str]) -> list[str]: + """The `paths` the holder's sparse-checkout rules leave out, in the order + given, ASKED OF GIT (`git sparse-checkout check-rules`), or none. + + `check-rules` reads paths on stdin and prints the ones the rules keep + INSIDE, so what it does not print is out. It is asked only where sparse + checkout is ON, and that is asked of git too, with `git sparse-checkout + list`, which dies `this worktree is not sparse` where `core.sparseCheckout` + is off. THE GATE IS NOT OPTIONAL (measured on git 2.43): `git + sparse-checkout disable` turns the setting off and leaves + `.git/info/sparse-checkout` as it was, and `check-rules` still reads that + file and says the paths it excluded are outside, so a holder that is no + longer sparse would be refused for rules git no longer applies. + + A GIT THAT CANNOT ANSWER IS "CANNOT TELL", NOT A REFUSAL. A git without + the subcommand says `unknown subcommand` and exits 129; one with it can + fail on the rules themselves (a sparse-checkout file that is not there). + Any non-zero exit is treated alike: nothing is refused, and the late + failure and its rollback stay as they were before #612, which leave the + holder as it was. So a refusal made here is always git's own word, and a + git that cannot give it costs nothing but the earliness. + + Paths go on stdin as BYTES, NUL-separated (`-z`), so no name is quoted, + split or re-encoded on the way, on Windows too. + """ + try: + _git_bytes(["sparse-checkout", "list"], root) + except CommandFailed: + return [] + names = [path.encode("utf-8", "surrogateescape") for path in paths] + try: + inside = _git_bytes(["sparse-checkout", "check-rules", "-z"], root, + b"".join(name + b"\0" for name in names)) + except CommandFailed: + return [] + kept = set(inside.split(b"\0")) + return [path for path, name in zip(paths, names) if name not in kept] + + +def _checked_inside_sparse_rules(root: Path, verb: str, + paths: list[str]) -> None: + """`family-sparse-excluded`: `verb` refuses, before its first write and + under `--dry-run` alike, a path it stages that the holder's sparse + checkout leaves out (#612, 571(d) and 574/5). + + GIT WILL NOT UPDATE AN INDEX ENTRY OUTSIDE THE RULES. `git add` and `git + rm` say so (`outside of your sparse-checkout definition`) whatever the + entry's skip-worktree mark says, and `git submodule add` says it of the + mount. #407's `gitmodules-sparse` reads the MARK, which git clears for a + file put back on disk by hand, so a `.gitmodules` the rules leave out + but a person restored passed it: `remove` then did `deinit` and `git + rm`, and failed at `git add` in its commit. A `members/` the rules + leave out failed `add` at `git submodule add`, after the member was + read, and `remove` and `bump` at `git rm` and `git add`, after the + unmount and the checkout. Each rolled back. Here the rules are asked, + not the marks: the paths each verb stages, named by its caller, are + `.gitmodules`, `members/` and `family.yaml` for `add` and + `remove` (the gitlink of the second is staged by `git rm`), and + `members/` and `family.yaml` for `bump`. + + THE EXIT NAMES THE COMMAND THAT BRINGS THE PATH BACK. `git + sparse-checkout add members` for the mount path, which works in cone + mode and in non-cone mode. For `.gitmodules` or `family.yaml`, only + non-cone rules can leave a root file out: in CONE mode git keeps every + file at the root inside (measured on git 2.43 with `set --cone`: + `.gitmodules` and `family.yaml` are inside, and a directory that is not + in the cone is out), so a root file is never refused there, and a root + file `check-rules` does call outside has non-cone rules. Its pattern is + `git sparse-checkout add /.gitmodules`, with the leading slash that makes + it the root's file alone, appended after the pattern that left it out, + which is the one that wins. `git sparse-checkout disable` is named beside + it for any path: it brings back every path and leaves a copy already on + disk as it is, as `gitmodules-sparse` says, and it needs no pattern + typed: a leading slash is a path to a shell that converts one (Git Bash + on Windows can), so the second command is the one that never meets that. + + `git sparse-checkout check-rules` is a recent subcommand (the git here, + 2.43, has it); a git without it is "cannot tell" and refuses nothing + (`_sparse_rules_exclude`). + """ + out = _sparse_rules_exclude(root, paths) + if not out: + return + # ONE PATTERN PER PATH, the mount path's `members` once. A root file is + # only ever out of NON-cone rules, so its pattern is the non-cone one. + patterns = list(dict.fromkeys( + MEMBERS_DIR if path.startswith(MEMBERS_DIR + "/") else "/" + path + for path in out)) + names = ", ".join(f"`{path}`" for path in out) + one = len(out) == 1 + raise Refusal( + "family-sparse-excluded", + f"the holder's sparse checkout leaves out {names}, which `{verb}` " + "stages, and git will not update an index entry outside the " + "sparse-checkout definition: `git sparse-checkout check-rules` says " + f"{'it is' if one else 'they are'} outside, so `{verb}` would fail " + "after its first writes. No git command that writes was run and " + "nothing was written.", + f"Remediation: bring {'it' if one else 'them'} back into the rules: " + f"`git sparse-checkout add {' '.join(patterns)}`, or `git " + "sparse-checkout disable`, which brings back every path the sparse " + "checkout leaves out and leaves a copy already on disk as it is. " + "Then run the same command again.") + + def _print_next(family: "Family", branch: str) -> None: """The NEXT an act that committed ends with (#487): ONE pasteable line that pushes the branch the commit is on, every word quoted for the @@ -2470,6 +2713,32 @@ def _submodule_paths( for name, keys in found.items() if "path" in keys} +def _members_is_a_file(root: Path, path: str) -> Refusal: + """`member-path-unaddable` for a `members` that is a FILE (#612, 574/5): + what it says, and the move that makes room. A file git tracks is moved + with `git mv` and the move committed first, since the add's own commit + names three paths and would leave the rename staged and uncommitted + beside it; one it does not track is moved with `mv`.""" + tracked = any(name == MEMBERS_DIR + for name, _ in _index_entries(root, MEMBERS_DIR)) + move = ("git mv" if tracked else "mv") + return Refusal( + "member-path-unaddable", + f"git will not add the mount path {path}: `{MEMBERS_DIR}` is " + f"{'a tracked' if tracked else 'an untracked'} file in the family " + "root, not a directory, so nothing can be mounted under it, and " + "`git submodule add` would fail at its mount (`Not a directory`) " + "after the member was read. No git command that writes was run and " + "nothing was written.", + f"Remediation: every member is mounted under {MEMBERS_DIR}/, which " + "must be a plain directory of the holder's own. Move the file " + f"aside, `{move} -- {MEMBERS_DIR} `" + + (", and commit the move on its own first, since the add's own " + f"commit names {GITMODULES}, {path} and {MANIFEST} alone" + if tracked else "") + + ", and run the same command again.") + + def _checked_mount_path(root: Path, project: str, path: str) -> None: """Every refusal about the MOUNT PATH, made before the member is read or anything is mounted (#242). @@ -2509,6 +2778,17 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: renames it first. `member-path-taken` where a path is a case variant of the mount path, `family-members-dir-variant` where it is under a spelling of `members/` alone. + - A FILE NAMED `members` (#612, 574/5). Git's own dry run, below, does + not refuse it (measured on git 2.43), and the mount's `git submodule + add --force` then dies cloning into it (`could not create leading + directories ... Not a directory`), after the member was read, and the + unwind restores the holder. It is asked of the listing already made + (`_members_on_disk`): `members` itself, spelled so and a file. It is + refused as `member-path-unaddable`, the code of a mount path git will + not take, whose remediation says already that `members/` must be a + plain directory of the holder's own (a link and a submodule are the + other two ways it is not). A file under another spelling of `members` + is `_members_spellings`'s, below. - GIT'S OWN DRY RUN, `git add --dry-run --ignore-missing` of the path — the check `--force` skips — made here with `-f`, so an ignore rule passes it and nothing else does. It refuses a path beyond a symbolic @@ -2544,6 +2824,8 @@ def _checked_mount_path(root: Path, project: str, path: str) -> None: "directory. Remove it (`git rm -r` if it is tracked), then re-run.") on_disk = _members_on_disk(root) canon = _members_dir_on_disk(root, on_disk) + if on_disk.get(MEMBERS_DIR, ("",))[0] == "a file": + raise _members_is_a_file(root, path) held = _index_entries(root, path) # Each spelling of the mount path the index holds other than its own, # and whether git tracks a directory there. @@ -3542,14 +3824,17 @@ def cmd_add(args) -> int: holder exactly as it was (#203); or with `--dry-run`, print what it would write and change nothing in the holder (#326, `_add_dry_run`). - IN THIS ORDER, AND ONLY THIS ORDER: the holder's refusals - (`_checked_add_target`), then the branch the commit lands on, which the - real run refuses on the tracking branch or none (`_checked_branch`, - #487) and a dry run only reports, then the member's (`_probe_member`, of - a throwaway clone), then the member's id against the rows - (`_checked_member_id`, #330), and only then the mount, the pin and the - commit (`_mount_and_pin`), inside a `MountRollback` that puts the holder - back if anything still fails and says what it put back. + IN THIS ORDER, AND ONLY THIS ORDER: an act git has unfinished in the + holder (`_checked_no_operation_in_progress`, #612), the holder's + refusals (`_checked_add_target`, which includes a `members` that is a + file, `_checked_mount_path`, #612), the paths the sparse rules leave out + (`_checked_inside_sparse_rules`, #612), then the branch the commit lands + on, which the real run refuses on the tracking branch or none + (`_checked_branch`, #487) and a dry run only reports, then the member's + (`_probe_member`, of a throwaway clone), then the member's id against the + rows (`_checked_member_id`, #330), and only then the mount, the pin and + the commit (`_mount_and_pin`), inside a `MountRollback` that puts the + holder back if anything still fails and says what it put back. WHEN TWO APPLY, THE EARLIER ONE IS SAID. The holder's refusals need nothing cloned, so they come first: a section under another name that @@ -3561,7 +3846,14 @@ def cmd_add(args) -> int: family = Family.open(args.family_root) repository, project = _checked_member(args.member) path = _member_path(project) + # A merge, cherry-pick, revert or rebase in progress is asked first: it + # fails the commit however the rest is (#612, 571(e)). + _checked_no_operation_in_progress(family.root, "add") at = _checked_add_target(family, repository, project, path, args.at) + # What the sparse rules leave out, after the refusals that read the same + # paths by their marks (`gitmodules-sparse`), before the member is read. + _checked_inside_sparse_rules(family.root, "add", + [GITMODULES, path, MANIFEST]) suggested = _suggested_branch("add", project) branch = None if args.dry_run else _checked_branch(family, "add", suggested) @@ -4971,6 +5263,10 @@ def cmd_bump(args) -> int: # fetch and checkout in it, and every pathspec after them are this path. path = _member_path(project, row) member = family.root / path + # BEFORE `member-uninitialized`, whose remedy a merge in progress or a + # sparse checkout that leaves the mount out would make wrong (#612). + _checked_no_operation_in_progress(family.root, "bump") + _checked_inside_sparse_rules(family.root, "bump", [path, MANIFEST]) if not (member / ".git").exists(): raise Refusal( "member-uninitialized", @@ -5200,7 +5496,10 @@ def _checked_gitmodules_as_staged(root: Path, verb: str) -> None: sparse-checkout disable` is the restore that works as typed. `add` refuses it on disk or not, since `git submodule add` cannot stage it; `remove` refuses it where it is missing or is not the index's copy, - and commits a clean one on disk rightly, as it did before #407. + and commits a clean one on disk rightly, as it did before #407. The + MARK is all this reads: a file the rules leave out, put back on disk, + has none, and is `_checked_inside_sparse_rules`'s (`family-sparse- + excluded`, #612), which asks git's rules. "Modified" is git's own answer, `git ls-files -m`, which writes nothing: the comparison `git status` lists a file as modified by, and the one @@ -5586,6 +5885,9 @@ def cmd_remove(args) -> int: # call: `path: members/*` unmounted every member under one commit naming # one, and `path: .` removed the holder's own files. path = _member_path(project, row) + # A merge, cherry-pick, revert or rebase in progress is asked before the + # holder's files are read: its commit fails however they are (#612). + _checked_no_operation_in_progress(family.root, "remove") # A `.gitmodules` the work tree lacks, or has not as the index has it, # is refused first (#407): the commit would take it out of HEAD, or # carry a change nobody staged. Where the index holds a copy, every @@ -5595,6 +5897,11 @@ def cmd_remove(args) -> int: # here, before any git command that writes (#360), so the dry run makes # the same refusal, as it makes every one above. _checked_one_section(family.root, project, path) + # The paths the commit stages, which the sparse rules may leave out + # though the file is on disk and its mark clear (#612, 571(d)). The + # gitlink is staged by `git rm`, which refuses it the same way. + _checked_inside_sparse_rules(family.root, "remove", + [GITMODULES, path, MANIFEST]) # Every refusal above is the same under `--dry-run`, which leaves here, # before the unmount, the real run's first write (#326), and reports the # branch the real run refuses (#487) rather than refusing it. diff --git a/tests/test_family_refuse_up_front.py b/tests/test_family_refuse_up_front.py new file mode 100644 index 00000000..f648b5a4 --- /dev/null +++ b/tests/test_family_refuse_up_front.py @@ -0,0 +1,768 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`add`, `bump` and `remove` refuse, BEFORE their first write, a holder they +cannot commit in (#612, from #459's 571(e), 571(d) and 574/5). + +Each of the three ends in ONE `git commit -- `, made after the verb's +first writes (`add` mounts the member, `bump` checks it out at `--to`, +`remove` unmounts it), and a rollback puts the holder back when that commit +or anything else after the first write fails. Three states made it fail, or +land where nobody wanted it, every time: + +- **An act git has unfinished in the holder** (571(e)). A merge or a + cherry-pick: `fatal: cannot do a partial commit during a merge.`, after + the writes, rolled back. A revert, a rebase and a `git am`: git does NOT + refuse the partial commit (measured on git 2.43), so it was made inside + the act, and for a rebase the real run met the branch refusal first + (`family-holder-detached`, whose `git switch -c` is the wrong advice + there) while the dry run printed a plan. One refusal now, `family- + operation-in-progress`, under both runs, naming the act and its own exit. +- **A path the sparse checkout leaves out** (571(d), 574/5). `.gitmodules` + put back on disk by hand, its skip-worktree mark cleared by git, passed + #407's check by mark and failed `remove` at `git add`; a `members/` the + rules leave out failed `add` at `git submodule add` and `bump` and + `remove` at `git add` and `git rm`. Asked of git's own rules (`git + sparse-checkout check-rules`), `family-sparse-excluded`, and only where + sparse checkout is on and git can answer. +- **A FILE named `members`** (574/5): `add` read the member, then failed at + the mount (`Not a directory`). `member-path-unaddable` now, before the + member is read. + +THE PROOF IS THE SAME FOR EACH: exit 2 and one REFUSED paragraph, nothing on +stdout (so no member was read: `add` says `checking ... before mounting it` +before it reads), the holder byte for byte as it was (HEAD, index, +`family.yaml`, work tree, git config) under the real run and `--dry-run` +alike; then the remedy typed as printed, and the same command goes through. + +THE SAME FAMILY AS `test_family.py`, this module's own instance of its +fixture, in a private copy of the holder that stands on the work branch. Each +file written is written as bytes, every git command runs under the offline, +identity-carrying environment the dry-run tests use, and a rebase or a merge +is made with git itself, never by writing its state files. +""" + +from __future__ import annotations + +import os +import re +import shlex +import shutil +import stat +import subprocess +import sys +import tempfile +from pathlib import Path + +import pytest + +#: The family fixture and its names, imported from the module that owns them. +from test_family import SPARE, family # noqa: F401 +from test_family_add_excludes import empty_holder # noqa: F401 +from test_family_add_refusals import holder, offline, refusal # noqa: F401 +#: What a verb must leave as it found it, and the three verbs, as the dry-run +#: tests spell them: the order of each comparison names the first fact that +#: moved, and `add`'s differs from `bump`'s and `remove`'s. +from test_family_dry_run import ( + BUMP_ORDER, REMOVE_ORDER, add, add_state, assert_add_as_it_was, + assert_as_it_was, bump, isolated, remove, state, +) +from test_family_member_path import MEMBER, OTHER, older, raw + +GITMODULES = ".gitmodules" +NOTES = "NOTES.txt" + + +def check_rules_available() -> bool: + """Does this machine's git have `git sparse-checkout check-rules`? Asked + of git, in a repository of its own that has sparse checkout on, so an + unknown subcommand and a git that cannot read the rules both say no.""" + with tempfile.TemporaryDirectory() as scratch: + env = {**os.environ, "GIT_CONFIG_NOSYSTEM": "1"} + for args in (["init", "-q", "."], + ["-c", "user.name=t", "-c", "user.email=t@t.invalid", + "commit", "-q", "--allow-empty", "-m", "c"], + ["sparse-checkout", "set", "--no-cone", "/*"]): + if subprocess.run(["git", *args], cwd=scratch, env=env, + capture_output=True, check=False + ).returncode != 0: + return False + asked = subprocess.run( + ["git", "sparse-checkout", "check-rules", "-z"], cwd=scratch, + env=env, input=b"a\0", capture_output=True, check=False) + return asked.returncode == 0 and asked.stdout == b"a\0" + + +needs_check_rules = pytest.mark.skipif( + not check_rules_available(), + reason="this git has no `git sparse-checkout check-rules`, which the " + "refusal asks; without it the verb cannot tell and refuses " + "nothing, as `test_a_git_that_cannot_tell_refuses_nothing` shows " + "where it can run") + + +# --- the three verbs -------------------------------------------------------- + +VERBS = ("add", "bump", "remove") + + +def run_verb(verb: str, family, root: Path, env: dict, *extra: str): + """`verb` against `root`, as a person types it: `add` mounts SPARE, + `bump` moves MEMBER to an older commit, `remove` takes MEMBER out.""" + if verb == "add": + return add(root, env, family["remotes"], *extra) + if verb == "bump": + return bump(root, env, older(family), *extra) + return remove(root, env, *extra) + + +def snapshot(verb: str, root: Path, env: dict) -> dict: + """Everything the verb's refusal must leave as it found it.""" + return add_state(root, env) if verb == "add" else state(root, env) + + +def assert_untouched(verb: str, root: Path, env: dict, before: dict) -> None: + if verb == "add": + assert_add_as_it_was(root, env, before) + else: + assert_as_it_was(root, env, before, + BUMP_ORDER if verb == "bump" else REMOVE_ORDER) + + +def head_of(root: Path, env: dict) -> str: + return raw(root, env, "rev-parse", "HEAD").stdout.decode().strip() + + +def assert_refused_alike(verb: str, family, root: Path, env: dict, + expected_stderr) -> None: + """THE PROOF OF A REFUSAL BEFORE THE FIRST WRITE, under both runs: exit 2, + the one paragraph `expected_stderr` (a compiled pattern for the REFUSED + line, the remediation exactly) and nothing on stdout, so the member was + not read; the holder as it was; and the dry run's words the real run's.""" + before = snapshot(verb, root, env) + said = [] + for extra in ((), ("--dry-run",)): + ran = run_verb(verb, family, root, env, *extra) + assert ran.returncode == 2, (extra, ran.stdout, ran.stderr) + assert ran.stdout == "", (extra, ran.stdout) + line, remedy = refusal(ran.stderr) + said.append((line, remedy)) + expected_line, expected_remedy = expected_stderr + assert expected_line.fullmatch(line), (extra, line) + assert remedy == expected_remedy, (extra, remedy) + assert_untouched(verb, root, env, before) + assert said[0] == said[1], "the two runs refuse in different words" + + +# --- an act git has unfinished --------------------------------------------- + +#: `{kind: (what, the mark git keeps, how it is finished, how it is given +#: up, what the commit would do)}`, as the refusal says each. `{verb}` is the +#: verb that asked. +OPERATIONS = { + "merge": ( + "a merge", "MERGE_HEAD", "git commit", "git merge --abort", + "git refuses that during a merge (`fatal: cannot do a partial " + "commit during a merge.`), after `{verb}`'s first writes, which are " + "then put back"), + "cherry-pick": ( + "a cherry-pick", "CHERRY_PICK_HEAD", "git cherry-pick --continue", + "git cherry-pick --abort", + "git refuses that during a cherry-pick (`fatal: cannot do a partial " + "commit during a cherry-pick.`), after `{verb}`'s first writes, " + "which are then put back"), + "rebase": ( + "a rebase", "rebase-merge", "git rebase --continue", + "git rebase --abort", + "git does not refuse that during a rebase, so the commit would land " + "on the HEAD the rebase has detached, inside its sequence, where no " + "branch can carry it to a pull request"), + "revert": ( + "a revert", "REVERT_HEAD", "git revert --continue", + "git revert --abort", + "git does not refuse that during a revert, so the commit would be " + "made inside the revert and end it as the revert's own"), + "am": ( + "a `git am`", "rebase-apply", "git am --continue", "git am --abort", + "git does not refuse that during a `git am`, so the commit would " + "land on the branch, in the middle of the patches being applied"), +} + + +def operation_said(kind: str, verb: str) -> tuple: + """`(the REFUSED line as a pattern, the remediation)` for `kind`, asked by + `verb`: the path git keeps the mark at is the one part that varies.""" + what, mark, finish, abort, effect = OPERATIONS[kind] + line = re.compile( + re.escape(f"REFUSED family-operation-in-progress: {what} is " + f"unfinished in the family root: git keeps {mark} at ") + + r".+/" + re.escape(mark) + + re.escape(f". `{verb}` makes its commit with `git commit -- " + f"`, and {effect.format(verb=verb)}. No git " + "command that writes was run and nothing was written.")) + remedy = ("Remediation: end it, whichever way suits the work in it: " + f"finish it with `{finish}` once whatever stopped it is " + f"resolved and staged, or give it up with `{abort}`. Then run " + "the same command again.") + return line, remedy + + +def commit_file(root: Path, env: dict, name: str, data: bytes, + message: str) -> None: + (root / name).write_bytes(data) + raw(root, env, "add", "--", name) + raw(root, env, "commit", "-q", "-m", message) + + +def work_branch(root: Path, env: dict) -> str: + return raw(root, env, "symbolic-ref", "--short", "HEAD" + ).stdout.decode().strip() + + +def conflicting_history(root: Path, env: dict) -> str: + """`NOTES.txt` as `base` on the work branch, `side` on a branch made from + it, and `main` on the work branch again: a cherry-pick of `side`, a rebase + of it, an `am` of its patch all stop on that file. Returns the work + branch, which the holder is left on.""" + work = work_branch(root, env) + commit_file(root, env, NOTES, b"base\n", "notes") + raw(root, env, "switch", "-q", "-c", "side") + commit_file(root, env, NOTES, b"side\n", "side") + raw(root, env, "switch", "-q", work) + commit_file(root, env, NOTES, b"main\n", "main") + return work + + +def mark_of(root: Path, env: dict, name: str) -> Path: + shown = raw(root, env, "rev-parse", "--git-path", name + ).stdout.decode().strip() + return root / shown + + +def begin(kind: str, root: Path, env: dict, tmp_path: Path) -> None: + """Leave `root` in the middle of `kind`, with git, and check the mark git + keeps for it is there: the premise of every test that calls this.""" + if kind == "merge": + # `--no-commit`: no conflict at all, the state the issue's probe made. + work = work_branch(root, env) + raw(root, env, "switch", "-q", "-c", "side") + commit_file(root, env, "other.txt", b"other\n", "other") + raw(root, env, "switch", "-q", work) + raw(root, env, "merge", "--no-commit", "--no-ff", "side") + elif kind == "cherry-pick": + conflicting_history(root, env) + stopped = raw(root, env, "cherry-pick", "side", check=False) + assert stopped.returncode != 0, "premise: the cherry-pick conflicts" + elif kind == "rebase": + work = conflicting_history(root, env) + raw(root, env, "switch", "-q", "side") + stopped = raw(root, env, "rebase", work, check=False) + assert stopped.returncode != 0, "premise: the rebase conflicts" + elif kind == "revert": + conflicting_history(root, env) + commit_file(root, env, NOTES, b"later\n", "later") + stopped = raw(root, env, "revert", "--no-edit", "HEAD~1", check=False) + assert stopped.returncode != 0, "premise: the revert conflicts" + else: + work = conflicting_history(root, env) + patch = tmp_path / "side.patch" + patch.write_bytes(raw(root, env, "format-patch", "-1", "side", + "--stdout").stdout) + stopped = raw(root, env, "am", str(patch), check=False) + assert stopped.returncode != 0, "premise: the am conflicts" + assert work_branch(root, env) == work + mark = OPERATIONS[kind][1] + assert os.path.lexists(mark_of(root, env, mark)), f"premise: {mark}" + + +@pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) +@pytest.mark.parametrize("verb", VERBS) +def test_each_verb_refuses_a_merge_in_progress_before_it_writes( + family, holder, offline, tmp_path, verb, dry_run): + """THE ISSUE'S PROBE (p07), for each verb: during a `--no-commit` merge + every verb exited 2 at `git commit`, `fatal: cannot do a partial commit + during a merge.`, after its first writes. Now `family-operation-in- + progress`, before them: HEAD, the index, `family.yaml`, the work tree and + the git config as they were, no member read, and the dry run's words the + real run's. The real run and the dry run are each asked of the holder + the merge left, which neither changes.""" + env = isolated(offline, tmp_path) + begin("merge", holder, env, tmp_path) + before = snapshot(verb, holder, env) + manifest = (holder / "family.yaml").read_bytes() + extra = ("--dry-run",) if dry_run else () + ran = run_verb(verb, family, holder, env, *extra) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert ran.stdout == "", ran.stdout + line, remedy = refusal(ran.stderr) + expected_line, expected_remedy = operation_said("merge", verb) + assert expected_line.fullmatch(line), line + assert remedy == expected_remedy + assert "partial commit" in line and "after" in line + assert (holder / "family.yaml").read_bytes() == manifest + assert_untouched(verb, holder, env, before) + assert os.path.lexists(mark_of(holder, env, "MERGE_HEAD")) + + +@pytest.mark.parametrize("kind, verb", [ + ("merge", "add"), ("merge", "bump"), ("merge", "remove"), + ("cherry-pick", "add"), ("cherry-pick", "remove"), + ("rebase", "bump"), ("rebase", "add"), ("revert", "remove"), + ("revert", "bump"), ("am", "add"), ("am", "remove"), +]) +def test_every_unfinished_act_is_named_and_refused_alike_in_both_runs( + family, holder, offline, tmp_path, kind, verb): + """A merge, a cherry-pick, a rebase, a revert and a `git am`, each + stopped on a conflict by git: each named, with its own finish and its + own give-up, by the real run and the dry run in the same words, the + holder untouched. A rebase and an `am` were no failure at the commit at + all (git does not refuse a partial commit there); the real run met a + rebase as `family-holder-detached`, the dry run printed a plan.""" + env = isolated(offline, tmp_path) + begin(kind, holder, env, tmp_path) + assert_refused_alike(verb, family, holder, env, operation_said(kind, verb)) + + +#: `{id: (kind, verb, the command the remediation names, what is added to it +#: to run without an editor)}`: `git commit` finishes a merge that stopped +#: with no conflict, once it has a message. +EXITS = { + "merge, finished": ("merge", "add", "git commit", ("-q", "-m", "merged")), + "merge, given up": ("merge", "remove", "git merge --abort", ()), + "cherry-pick, given up": ("cherry-pick", "bump", "git cherry-pick --abort", + ()), + "rebase, given up": ("rebase", "add", "git rebase --abort", ()), + "revert, given up": ("revert", "remove", "git revert --abort", ()), + "am, given up": ("am", "bump", "git am --abort", ()), +} + + +@pytest.mark.parametrize("kind, verb, printed, added", EXITS.values(), + ids=EXITS) +def test_the_remedy_typed_as_printed_lets_the_same_command_run( + family, holder, offline, tmp_path, kind, verb, printed, added): + """THE REMEDY IS THE ACT'S OWN EXIT, and it works: the command it prints + is read back out of the refusal and run, and the same verb then makes its + ONE commit, a child of the HEAD it started from.""" + env = isolated(offline, tmp_path) + begin(kind, holder, env, tmp_path) + ran = run_verb(verb, family, holder, env) + assert ran.returncode == 2, ran.stdout + ran.stderr + _, remedy = refusal(ran.stderr) + assert printed in re.findall(r"`(git [^`]+)`", remedy), remedy + raw(holder, env, *shlex.split(printed)[1:], *added) + assert not os.path.lexists(mark_of(holder, env, OPERATIONS[kind][1])) + was = head_of(holder, env) + done = run_verb(verb, family, holder, env) + assert done.returncode == 0, done.stdout + done.stderr + assert raw(holder, env, "rev-parse", "HEAD~1").stdout.decode().strip() \ + == was + assert raw(holder, env, "status", "--porcelain").stdout == b"" + + +def test_a_merge_in_a_linked_worktree_is_found_where_git_keeps_it( + family, holder, offline, tmp_path): + """`.git` is a FILE in a linked worktree, which is where a lane opens its + pull request, and the merge's mark is under `.git/worktrees//`, + never `.git/MERGE_HEAD`: asked of `git rev-parse --git-path`, the mark is + found, and the refusal names the path git keeps it at.""" + env = isolated(offline, tmp_path) + linked = tmp_path / "linked" + raw(holder, env, "worktree", "add", "-q", "-b", "linked-branch", + str(linked)) + begin("merge", linked, env, tmp_path) + assert (linked / ".git").is_file() + for extra in ((), ("--dry-run",)): + ran = add(linked, env, family["remotes"], *extra) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert ran.stdout == "" + line, _ = refusal(ran.stderr) + assert re.search(r"git keeps MERGE_HEAD at \S+/worktrees/[^/]+/" + r"MERGE_HEAD\. ", line), line + + +def test_with_no_act_and_no_sparse_checkout_each_verb_runs_as_it_always_did( + family, holder, offline, tmp_path): + """THE CONTROL: no act in progress, no sparse checkout; each verb commits + as it did, in a holder of its own.""" + env = isolated(offline, tmp_path) + for verb in VERBS: + copy = tmp_path / f"control-{verb}" + shutil.copytree(holder, copy, symlinks=True) + was = head_of(copy, env) + ran = run_verb(verb, family, copy, env) + assert ran.returncode == 0, (verb, ran.stdout, ran.stderr) + assert raw(copy, env, "rev-parse", "HEAD~1").stdout.decode().strip() \ + == was + + +# --- a path the sparse checkout leaves out ---------------------------------- + +def sparse_set(root: Path, env: dict, *rules: str, cone: bool = False) -> None: + raw(root, env, "sparse-checkout", "set", + "--cone" if cone else "--no-cone", *rules) + + +def restored_by_hand(root: Path, env: dict) -> None: + """THE ISSUE'S STATE (571(d)): the rules leave `.gitmodules` out, the file + is back on disk because a person put it there, and git's skip-worktree + mark is CLEAR, so #407's check, which reads the mark, passes. Built with + git's own `sparse-checkout set`, then the file written back as bytes and + the mark taken off.""" + sparse_set(root, env, "/*", f"!/{GITMODULES}") + put_back(root, env, GITMODULES) + + +def put_back(root: Path, env: dict, name: str) -> None: + """`name`, a file the rules leave out, written back by a hand with the + index's own bytes, and git's mark taken off it: the file is on disk, the + mark is clear and the rules still leave it out. Each is a premise.""" + if not (root / name).exists(): + (root / name).write_bytes(raw(root, env, "show", f":{name}").stdout) + raw(root, env, "update-index", "--no-skip-worktree", "--", name) + listed = raw(root, env, "ls-files", "-v", "--", name).stdout + assert listed.startswith(b"H "), f"premise: the mark is clear: {listed!r}" + assert outside(root, env, name), "premise: the rules leave it out" + + +def outside(root: Path, env: dict, path: str) -> bool: + """Do the holder's sparse rules leave `path` out? Asked of git as the + tool asks it.""" + kept = raw(root, env, "sparse-checkout", "check-rules", "-z", + stdin=path.encode() + b"\0").stdout + return kept == b"" + + +SPARSE_LINE = ( + "REFUSED family-sparse-excluded: the holder's sparse checkout leaves out " + "{names}, which `{verb}` stages, and git will not update an index entry " + "outside the sparse-checkout definition: `git sparse-checkout " + "check-rules` says {are} outside, so `{verb}` would fail after its first " + "writes. No git command that writes was run and nothing was written.") + + +def sparse_remedy(*patterns: str, one: bool = True) -> str: + adding = ("" if not patterns else + f"`git sparse-checkout add {' '.join(patterns)}`, or ") + return (f"Remediation: bring {'it' if one else 'them'} back into the " + f"rules: {adding}`git sparse-checkout disable`, which brings " + "back every path the sparse checkout leaves out and leaves a " + "copy already on disk as it is. Then run the same command " + "again.") + + +def sparse_said(verb: str, paths: list[str], *patterns: str) -> tuple: + names = ", ".join(f"`{path}`" for path in paths) + line = SPARSE_LINE.format(names=names, verb=verb, + are="it is" if len(paths) == 1 else "they are") + return re.compile(re.escape(line)), sparse_remedy( + *patterns, one=len(paths) == 1) + + +def type_the_remedy(root: Path, env: dict, stderr: str) -> None: + """Run the `git sparse-checkout add ...` the refusal prints, as printed.""" + typed = re.search(r"`git sparse-checkout add ([^`]+)`", stderr) + assert typed, stderr + raw(root, env, "sparse-checkout", "add", *typed.group(1).split()) + + +@needs_check_rules +@pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) +def test_remove_refuses_a_gitmodules_the_rules_leave_out_before_deinit( + family, holder, offline, tmp_path, dry_run): + """THE ISSUE'S p06: sparse rules leave `.gitmodules` out, the file is put + back by hand, git clears the mark, #407's check passes, and `remove` did + `deinit` and `git rm` and failed at `git add` (`outside of your + sparse-checkout definition`), then rolled back. Now refused before + `deinit`: the mount, the registration in the git config and the index are + untouched, which `REMOVE_ORDER` reads key by key. The pattern the + remediation names is typed as printed, and `remove` then goes through.""" + env = isolated(offline, tmp_path) + restored_by_hand(holder, env) + before = state(holder, env) + extra = ("--dry-run",) if dry_run else () + ran = remove(holder, env, *extra) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert ran.stdout == "" + line, remedy = refusal(ran.stderr) + expected_line, expected_remedy = sparse_said( + "remove", [GITMODULES], f"/{GITMODULES}") + assert expected_line.fullmatch(line), line + assert remedy == expected_remedy + assert_as_it_was(holder, env, before, REMOVE_ORDER) + assert (holder / "members" / MEMBER / "project.yaml").is_file() + + type_the_remedy(holder, env, ran.stderr) + done = remove(holder, env) + assert done.returncode == 0, done.stdout + done.stderr + assert raw(holder, env, "rev-parse", "HEAD~1").stdout.decode().strip() \ + == before["HEAD"].decode().strip() + assert not (holder / "members" / MEMBER).exists() + + +@needs_check_rules +@pytest.mark.parametrize("cone", [False, True], ids=["non-cone", "cone"]) +@pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) +def test_add_refuses_a_members_the_rules_leave_out_before_it_reads_the_member( + family, holder, offline, tmp_path, cone, dry_run): + """574/5 (p12b): `add` printed `mounting ...`, then `git submodule add` + refused the mount as outside the sparse-checkout definition, and the + unwind restored the holder. Now refused first, naming `git + sparse-checkout add members`, which works in cone mode and in non-cone + mode: it is typed as printed, and `add` then mounts the member. Nothing + on stdout: not even the line that says the member is being read.""" + env = isolated(offline, tmp_path) + if cone: + sparse_set(holder, env, "contracts", cone=True) + else: + sparse_set(holder, env, "/*", "!/members/") + assert outside(holder, env, f"members/{SPARE}"), "premise: out" + before = add_state(holder, env) + extra = ("--dry-run",) if dry_run else () + ran = add(holder, env, family["remotes"], *extra) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert ran.stdout == "" + line, remedy = refusal(ran.stderr) + expected_line, expected_remedy = sparse_said( + "add", [f"members/{SPARE}"], "members") + assert expected_line.fullmatch(line), line + assert remedy == expected_remedy + assert_add_as_it_was(holder, env, before) + + type_the_remedy(holder, env, ran.stderr) + done = add(holder, env, family["remotes"]) + assert done.returncode == 0, done.stdout + done.stderr + assert (holder / "members" / SPARE / "project.yaml").is_file() + + +@needs_check_rules +@pytest.mark.parametrize("verb", ["bump", "remove"]) +def test_bump_and_remove_refuse_a_members_the_rules_leave_out_too( + family, holder, offline, tmp_path, verb): + """The same helper covers `bump`'s staged paths, `members/

` and + `family.yaml`, and `remove`'s: before it, `bump` checked the member out + at `--to` and failed at `git add` of the gitlink, `remove` ran `deinit` + and failed at `git rm`, each rolled back. Refused first, both runs.""" + env = isolated(offline, tmp_path) + sparse_set(holder, env, "/*", "!/members/") + assert outside(holder, env, f"members/{MEMBER}"), "premise: out" + # `remove` stages `.gitmodules`, the gitlink and `family.yaml`; only the + # gitlink is out here, so it alone is named, as for `bump`. + assert_refused_alike(verb, family, holder, env, + sparse_said(verb, [f"members/{MEMBER}"], "members")) + + +@needs_check_rules +def test_the_cone_keeps_every_root_file_so_only_the_mount_is_refused( + family, holder, offline, tmp_path): + """MEASURED, as the docstring says: in cone mode git keeps every file at + the root inside (`.gitmodules` and `family.yaml`), so a cone that leaves + out `members/` is refused for the mount path ALONE and never for the two + files; and a root file is never offered a pattern there.""" + env = isolated(offline, tmp_path) + sparse_set(holder, env, "contracts", cone=True) + assert not outside(holder, env, GITMODULES) + assert not outside(holder, env, "family.yaml") + assert outside(holder, env, f"members/{OTHER}") + ran = remove(holder, env, "--dry-run") + line, _ = refusal(ran.stderr) + assert ran.returncode == 2 + assert f"leaves out `members/{MEMBER}`, which" in line, line + assert GITMODULES not in line and "family.yaml" not in line, line + + +@needs_check_rules +def test_every_excluded_path_is_named_and_one_command_brings_them_all_back( + family, holder, offline, tmp_path): + """Non-cone rules can leave any of the three out: all three are named, in + the order the verb stages them, and the one `git sparse-checkout add` the + refusal prints, typed as printed, brings every one back.""" + env = isolated(offline, tmp_path) + sparse_set(holder, env, "/*", f"!/{GITMODULES}", "!/members/", + "!/family.yaml") + # `family.yaml` is what the tool opens first: a sparse checkout that + # removed it from disk is `family-root-missing`, so, as for `.gitmodules`, + # a hand put it back. + put_back(holder, env, GITMODULES) + put_back(holder, env, "family.yaml") + before = add_state(holder, env) + ran = add(holder, env, family["remotes"]) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert ran.stdout == "" + line, remedy = refusal(ran.stderr) + expected_line, expected_remedy = sparse_said( + "add", [GITMODULES, f"members/{SPARE}", "family.yaml"], + f"/{GITMODULES}", "members", "/family.yaml") + assert expected_line.fullmatch(line), line + assert remedy == expected_remedy + assert_add_as_it_was(holder, env, before) + type_the_remedy(holder, env, ran.stderr) + assert not outside(holder, env, GITMODULES) + assert not outside(holder, env, f"members/{SPARE}") + assert not outside(holder, env, "family.yaml") + done = add(holder, env, family["remotes"]) + assert done.returncode == 0, done.stdout + done.stderr + + +@needs_check_rules +@pytest.mark.parametrize("verb", VERBS) +def test_rules_that_keep_every_path_refuse_nothing( + family, holder, offline, tmp_path, verb): + """THE CONTROL for the sparse check: sparse checkout is ON, and keeps + everything the verb stages, so the verb runs as it did.""" + env = isolated(offline, tmp_path) + sparse_set(holder, env, "/*") + was = head_of(holder, env) + ran = run_verb(verb, family, holder, env) + assert ran.returncode == 0, ran.stdout + ran.stderr + assert raw(holder, env, "rev-parse", "HEAD~1").stdout.decode().strip() \ + == was + + +@needs_check_rules +@pytest.mark.parametrize("verb", VERBS) +def test_a_sparse_checkout_that_was_disabled_refuses_nothing( + family, holder, offline, tmp_path, verb): + """THE GATE: `git sparse-checkout disable` turns `core.sparseCheckout` off + and LEAVES `.git/info/sparse-checkout` as it was, and `check-rules`, asked + bare, still reads it and says `members/

` is outside (the premise, + measured). A holder that is no longer sparse must not be refused for + rules git no longer applies: the verb runs.""" + env = isolated(offline, tmp_path) + sparse_set(holder, env, "/*", "!/members/") + raw(holder, env, "sparse-checkout", "disable") + assert raw(holder, env, "config", "--bool", "core.sparseCheckout" + ).stdout.strip() == b"false" + assert (holder / ".git" / "info" / "sparse-checkout").is_file() + assert outside(holder, env, f"members/{MEMBER}"), ( + "premise: check-rules still reads the file the disable left") + was = head_of(holder, env) + ran = run_verb(verb, family, holder, env) + assert ran.returncode == 0, ran.stdout + ran.stderr + assert raw(holder, env, "rev-parse", "HEAD~1").stdout.decode().strip() \ + == was + + +def test_rules_git_cannot_read_are_cannot_tell_and_refuse_nothing( + family, holder, offline, tmp_path): + """`core.sparseCheckout` on and no rules file: `git sparse-checkout + check-rules` exits non-zero (`unable to load existing sparse-checkout + patterns`), which is "cannot tell", not a refusal: the verb runs. The + same road a git with NO `check-rules` takes (an unknown subcommand, exit + 129), asked for here without needing such a git.""" + env = isolated(offline, tmp_path) + raw(holder, env, "config", "core.sparseCheckout", "true") + assert not (holder / ".git" / "info" / "sparse-checkout").exists() + ran = add(holder, env, family["remotes"]) + assert ran.returncode == 0, ran.stdout + ran.stderr + + +@pytest.mark.skipif(sys.platform == "win32", + reason="the git that lacks `check-rules` is a `sh` " + "script put first on PATH, which Windows does not " + "run as `git`") +@needs_check_rules +def test_a_git_that_cannot_tell_refuses_nothing_and_the_late_failure_stays( + family, holder, offline, tmp_path): + """A GIT WITHOUT `check-rules`, made with a `git` first on PATH that says + `unknown subcommand` (exit 129) to that one subcommand and runs the real + git for everything else. `remove` in the issue's 571(d) state is NOT + refused: it fails where it always failed, at its commit, and its + rollback leaves the holder as it was. So a git that cannot tell costs + nothing but the earliness.""" + env = isolated(offline, tmp_path) + restored_by_hand(holder, env) + real = shutil.which("git") + assert real + shim = tmp_path / "oldgit" + shim.mkdir() + (shim / "git").write_text( + "#!/bin/sh\n" + 'if [ "$1" = "sparse-checkout" ] && [ "$2" = "check-rules" ]; then\n' + " echo \"error: unknown subcommand: \\`check-rules'\" >&2\n" + " exit 129\n" + "fi\n" + f"exec '{real}' \"$@\"\n", encoding="utf-8") + (shim / "git").chmod((shim / "git").stat().st_mode | stat.S_IXUSR) + old = {**env, "PATH": str(shim) + os.pathsep + env["PATH"]} + before = state(holder, env) + config = sorted(raw(holder, env, "config", "--local", "--list" + ).stdout.splitlines()) + ran = remove(holder, old) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert "family-sparse-excluded" not in ran.stderr + assert "outside of your sparse-checkout definition" in ran.stderr + # EVERY FACT BUT TWO. The config's keys and values are the same and their + # order is not: the rollback puts the member's section back written LAST + # (`PinRollback`); and the failed commit had already written the blobs it + # staged, loose objects no rollback takes out of the store and nothing + # refers to. The rest is as it was: HEAD, the index, `.gitmodules`, + # `family.yaml`, the work tree and the member's checkout. + kept = tuple(key for key in REMOVE_ORDER + if key not in (".git/config", "objects")) + assert_as_it_was(holder, env, before, kept) + assert sorted(raw(holder, env, "config", "--local", "--list" + ).stdout.splitlines()) == config + + +# --- a FILE named `members` ------------------------------------------------- + +FILE_LINE = ( + "REFUSED member-path-unaddable: git will not add the mount path " + f"members/{SPARE}: `members` is {{kind}} file in the family root, not a " + "directory, so nothing can be mounted under it, and `git submodule add` " + "would fail at its mount (`Not a directory`) after the member was read. " + "No git command that writes was run and nothing was written.") +FILE_REMEDY = ( + "Remediation: every member is mounted under members/, which must be a " + "plain directory of the holder's own. Move the file aside, `{move} -- " + "members `{commit}, and run the same command again.") + + +@pytest.mark.parametrize("tracked", [False, True], + ids=["untracked", "tracked"]) +@pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) +def test_add_refuses_a_file_named_members_before_it_reads_the_member( + family, empty_holder, offline, tmp_path, tracked, dry_run): + """574/5 (p12a): with a FILE named `members`, `add` printed `mounting + ...`, then `git submodule add` died cloning into it (`Not a directory`), + after the member was read. Now refused first, with nothing on stdout, as + `member-path-unaddable`, the code of a mount path git will not take. The + move the remediation names is made as printed, a tracked file with `git + mv` and the move committed, and `add` then mounts the member.""" + holder = empty_holder + env = isolated(offline, tmp_path) + (holder / "members").write_bytes(b"a file where the directory goes\n") + if tracked: + raw(holder, env, "add", "-f", "--", "members") + raw(holder, env, "commit", "-q", "-m", "a file named members") + before = add_state(holder, env) + extra = ("--dry-run",) if dry_run else () + ran = add(holder, env, family["remotes"], *extra) + assert ran.returncode == 2, ran.stdout + ran.stderr + assert ran.stdout == "" + line, remedy = refusal(ran.stderr) + assert line == FILE_LINE.format( + kind="a tracked" if tracked else "an untracked") + assert remedy == FILE_REMEDY.format( + move="git mv" if tracked else "mv", + commit=(", and commit the move on its own first, since the add's own " + f"commit names .gitmodules, members/{SPARE} and family.yaml " + "alone" if tracked else "")) + assert_add_as_it_was(holder, env, before) + + if tracked: + raw(holder, env, "mv", "--", "members", "members-file") + raw(holder, env, "commit", "-q", "-m", "move the file aside") + else: + (holder / "members").rename(holder / "members-file") + done = add(holder, env, family["remotes"]) + assert done.returncode == 0, done.stdout + done.stderr + assert (holder / "members" / SPARE / "project.yaml").is_file() + + +def test_a_members_directory_is_not_refused_as_a_file( + family, holder, offline, tmp_path): + """THE CONTROL: `members` is the directory every holder has, and `add` + mounts beside what is there.""" + env = isolated(offline, tmp_path) + ran = add(holder, env, family["remotes"]) + assert ran.returncode == 0, ran.stdout + ran.stderr From 9ef002b98ab35668d6518fd11e55c3a7c2ab29da Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 22:52:38 +0000 Subject: [PATCH 2/3] family.py: #626 fix round 1: the revert and rebase sentences say what git does, a cherry-pick or revert series with only `sequencer/` left is refused, the mount pattern brings the path back under non-cone rules that name it, and the root-file patterns are unanchored (#612) R612's review of 54c30a81 asked for four fixes, and the lane took most of its nits. F1. Two refusal sentences were false as measured on git 2.43. In a revert the verb's commit lands on the branch as the verb's own and git takes it as the END of the revert (`REVERT_HEAD` goes, `--continue` and `--abort` then say `no cherry-pick or revert in progress`, the revert's changes are left half done). In a rebase it lands on the detached HEAD, in the middle of the sequence: `--continue` replays the rest on top of it into the rebased branch and `--abort` drops it. `test_what_each_refusal_says_the_commit_would_do_is_ what_git_does` ties every sentence to git by making the partial commit with git itself. F2. `git cherry-pick A..B` and `git revert A..B` keep `sequencer/` for the steps to come. Once the stopped step is committed by hand no `CHERRY_PICK_HEAD` or `REVERT_HEAD` is left, `git status` still says the act is in progress, and the verb's commit landed in the middle of the series. `sequencer` is now asked after `REVERT_HEAD`, named from the first word of `sequencer/todo` as `git status` names it (`pick` or `p`, a cherry-pick; `revert`, a revert), with that act's `--continue` and `--abort`. A `todo` that is missing, empty or no series reads as nothing in progress to `git status` and `--continue` fails on it, while `--abort` and `--quit` exit 0: it is refused generically, naming only what works. F3. Under non-cone rules that name the member (`!/members/IRQS`, `!/members/*`) the printed `git sparse-checkout add members` exited 0 and left the path out, so the verb was refused again with the same remedy. The mode is now read as a listing, `git config -z --list`, key `core.sparsecheckoutcone`, and the pattern is `members/` under non-cone rules and `members` in cone mode, where `members/` is refused for a gitlink git tracks. N7: `.gitmodules` and `family.yaml` are named unanchored, which wins over `!/.gitmodules`, `!.gitmodules` and `!/.*` (measured) and meets no Git Bash conversion of a leading slash. F4. Two S9073 asserts: one went with the test it sat in, one is split. N1 folds the merge-only test into `assert_refused_alike` and drops a control the control test covers. N2 pins that a rebase is asked first (a merge begun at a rebase's stop leaves `MERGE_HEAD` and `rebase-merge/`, and `git rebase --abort` clears both). N3 corrects `_members_is_a_file`'s account of why the move is committed first. N8 asks every mark in ONE `git rev-parse`. N4 is NOT taken. Asking about a merge before `Family.open` parses `family.yaml` would make git run before the refusal of a hand-edited `path:`, which `test_family_member_path.py::test_the_refusal_is_made_before_ any_git_call` holds to no git command at all. The comment that said the check "is asked first" now says what is true, and a merge conflicted in `family.yaml` still reads `yaml-unparsable`, left for #459. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 288 +++++++++++++++------ tests/test_family_refuse_up_front.py | 362 ++++++++++++++++++++------- 2 files changed, 488 insertions(+), 162 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 89287576..dfaab724 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -774,19 +774,22 @@ def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: #: ` does during it)`, the last with `{verb}` for the verb that #: asked. A REBASE IS ASKED FIRST: it holds a HEAD detached on the commit it #: stopped at, it may hold a merge, a cherry-pick or a revert of its own -#: beside its mark, and `git rebase --abort` puts all of it back, so it is -#: the act to name. What git does with the partial commit was measured on -#: git 2.43 in each state: it REFUSES one during a merge and a cherry-pick -#: and MAKES it during a revert, a rebase and a `git am`. +#: beside its mark (a merge begun at a rebase's `edit` stop leaves both +#: `MERGE_HEAD` and `rebase-merge/`), and `git rebase --abort` puts all of it +#: back, where `git merge --abort` would leave `rebase-merge/` and a second +#: refusal. What git does with the partial commit was measured on git 2.43 in +#: each state: it REFUSES one during a merge and a cherry-pick and MAKES it +#: during a revert, a rebase and a `git am`. +REBASE_EFFECT = ( + "git does not refuse that during a rebase: the commit would land on the " + "HEAD the rebase has detached, in the middle of its sequence, where " + "`git rebase --continue` replays the rest on top of it into the rebased " + "branch and `git rebase --abort` drops it") OPERATION_MARKS = ( ("rebase-merge", "a rebase", "git rebase --continue", - "git rebase --abort", "git does not refuse that during a rebase, so " - "the commit would land on the HEAD the rebase has detached, inside its " - "sequence, where no branch can carry it to a pull request"), + "git rebase --abort", REBASE_EFFECT), ("rebase-apply", "a rebase", "git rebase --continue", - "git rebase --abort", "git does not refuse that during a rebase, so " - "the commit would land on the HEAD the rebase has detached, inside its " - "sequence, where no branch can carry it to a pull request"), + "git rebase --abort", REBASE_EFFECT), ("MERGE_HEAD", "a merge", "git commit", "git merge --abort", "git refuses that during a merge (`fatal: cannot do a partial commit " "during a merge.`), after `{verb}`'s first writes, which are then put " @@ -797,9 +800,12 @@ def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: "commit during a cherry-pick.`), after `{verb}`'s first writes, which " "are then put back"), ("REVERT_HEAD", "a revert", "git revert --continue", - "git revert --abort", "git does not refuse that during a revert, so " - "the commit would be made inside the revert and end it as the revert's " - "own"), + "git revert --abort", + "git does not refuse that during a revert: the commit is made on the " + "branch as `{verb}`'s own, and git takes it as the end of the revert, " + "so `git revert --continue` and `--abort` then say `no cherry-pick or " + "revert in progress` and the revert's changes are left half done in " + "the index and the work tree"), ) #: `git am` keeps its state in `rebase-apply` too, and git tells it from a @@ -812,33 +818,90 @@ def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: "would land on the branch, in the middle of the patches " "being applied") - -def _git_path(root: Path, name: str) -> Path: - """Where git keeps `name` for the holder at `root`, ASKED OF GIT (`git - rev-parse --git-path`), never `.git/`: in a linked worktree, which - is where a lane opens its pull request, `.git` is a file and the - per-worktree state (`MERGE_HEAD`, `rebase-merge/`, `index.lock`) is under - `.git/worktrees//` (#365, `_module_dir`, `_checked_index_unlocked`). - git answers relative to `root` or absolute, and `root /` takes both.""" - shown = _git_bytes(["rev-parse", "--git-path", name], root).decode( - "utf-8", "surrogateescape") - return root / (shown[:-1] if shown.endswith("\n") else shown) - - -def _operation_in_progress(root: Path - ) -> tuple[str, str, Path, str, str, str] | None: +#: A cherry-pick or a revert of SEVERAL commits keeps `sequencer/` for the +#: steps still to take, and once its stopped step is committed by hand +#: neither `CHERRY_PICK_HEAD` nor `REVERT_HEAD` is left, though `git status` +#: still says `Cherry-pick currently in progress.` (#612's review, F2). It +#: is named from the first word of `sequencer/todo`, as `git status` names +#: it: `pick` or `p`, a cherry-pick; `revert`, a revert. +SEQUENCER = "sequencer" +SEQUENCER_TODO = "sequencer/todo" +SEQUENCE_COMMANDS = {"pick": "git cherry-pick", "p": "git cherry-pick", + "revert": "git revert"} + +#: A `sequencer/` whose `todo` is missing, empty or not a series git reads. +#: Measured on git 2.43: `git status` says nothing is in progress, +#: `--continue` fails (`fatal: cherry-pick failed`, exit 128), and `--abort` +#: and `--quit` of either command exit 0 and take the directory away. So it +#: is named for what it is and has no `finish`. +SEQUENCE_UNREADABLE = ( + "a cherry-pick or revert series", None, "git cherry-pick --abort", + "git does not refuse that, and the commit would land beside a " + "`sequencer/` directory git can read no series from, which `--continue` " + "fails on") + + +def _git_paths(root: Path, names: list[str]) -> list[Path]: + """Where git keeps each of `names` for the holder at `root`, ASKED OF GIT + in ONE `git rev-parse --git-path --git-path ...` (one line per + name; a process per name matters on Windows), never `.git/`: in a + linked worktree, which is where a lane opens its pull request, `.git` is + a file and the per-worktree state (`MERGE_HEAD`, `rebase-merge/`, + `index.lock`) is under `.git/worktrees//` (#365, `_module_dir`, + `_checked_index_unlocked`). git answers relative to `root` or absolute, + and `root /` takes both. A holder whose path holds a newline would make + the lines and the names disagree: each name is then asked alone.""" + def asked(*words: str) -> list[str]: + shown = _git_bytes(["rev-parse", *words], root).decode( + "utf-8", "surrogateescape") + return shown[:-1].split("\n") if shown.endswith("\n") else [shown] + + lines = asked(*[word for name in names for word in ("--git-path", name)]) + if len(lines) != len(names): + lines = [asked("--git-path", name)[0] for name in names] + return [root / line for line in lines] + + +def _sequence_act(todo: Path) -> tuple: + """`(what, finish, abort, effect)` for the series `todo` holds, named from + its first word as `git status` names it, or `SEQUENCE_UNREADABLE`.""" + try: + first = todo.read_bytes().split(b"\n", 1)[0].split(None, 1) + except OSError: + first = [] + command = SEQUENCE_COMMANDS.get( + first[0].decode("utf-8", "replace") if first else "") + if command is None: + return SEQUENCE_UNREADABLE + what = "a cherry-pick" if command == "git cherry-pick" else "a revert" + return (f"{what} series", f"{command} --continue", f"{command} --abort", + "git does not refuse that between the steps of a series, so the " + f"commit would land in the middle of it: `{command} --continue` " + f"replays the rest on top of it, and `{command} --abort` stops " + "and leaves it where it is (`You seem to have moved HEAD. Not " + "rewinding`)") + + +def _operation_in_progress( + root: Path) -> tuple[str, str, Path, str | None, str, str] | None: """`(what, the mark's name, where it is, how to finish it, how to give it up, what the partial commit would do)` for the act the holder's git directory shows unfinished, or None; the first of `OPERATION_MARKS` - that is there.""" + that is there, then a `sequencer/`. `finish` is None where git can + continue nothing.""" + names = [*(mark for mark, *_ in OPERATION_MARKS), AM_MARK, SEQUENCER, + SEQUENCER_TODO] + marks = dict(zip(names, _git_paths(root, names))) for mark, what, finish, abort, effect in OPERATION_MARKS: - where = _git_path(root, mark) + where = marks[mark] if not os.path.lexists(where): continue - if mark == "rebase-apply" and os.path.lexists( - _git_path(root, AM_MARK)): + if mark == "rebase-apply" and os.path.lexists(marks[AM_MARK]): what, finish, abort, effect = AM_OPERATION return what, mark, where, finish, abort, effect + if os.path.lexists(marks[SEQUENCER]): + what, finish, abort, effect = _sequence_act(marks[SEQUENCER_TODO]) + return what, SEQUENCER, marks[SEQUENCER], finish, abort, effect return None @@ -860,35 +923,70 @@ def _checked_no_operation_in_progress(root: Path, verb: str) -> None: A REVERT, A REBASE AND A `git am` ARE NOT REFUSED BY git, which is worse (measured on git 2.43, each stopped on a conflict). The partial - commit is made: inside the revert, where it ends the revert with a - family commit as the revert's own; on the HEAD a rebase has detached, - where it sits in the rebase's sequence and no branch carries it to a - pull request; and on the branch in the middle of an `am`. The real run - met a rebase at the branch refusal (`family-holder-detached`, #487), - whose remedy, `git switch -c`, is the wrong advice in the middle of a - rebase, and the dry run printed a plan for it. This check comes before - that one, so a rebase is named as what it is. - - So the five marks are asked, and the refusal names the one it found and - that act's OWN exit: `git commit` for a merge, `--continue` for the - others, or `--abort`. Every mark is looked for WHERE GIT KEEPS IT - (`_git_path`), which in a linked worktree is not `.git/`. Asked of - the holder's own git directory, never of a member's: a merge in a - MEMBER's mount is that checkout's own business. + commit is made: in a revert, on the branch as the verb's own, and git + takes it as the END of the revert (`REVERT_HEAD` goes, `--continue` and + `--abort` then say `no cherry-pick or revert in progress`, and the + revert's changes are left half done in the index and the work tree); on + the HEAD a rebase has detached, in the middle of its sequence, where + `git rebase --continue` replays the rest on top of it into the rebased + branch and `--abort` drops it; and on the branch in the middle of an + `am`. The real run met a rebase at the branch refusal + (`family-holder-detached`, #487), which committed nothing but whose + remedy, `git switch -c`, is the wrong advice in the middle of a rebase, + and the dry run printed a plan for it. This check comes before that one, + so a rebase is named as what it is. + + A SERIES IS ASKED TOO (#612's review, F2). `git cherry-pick A..B` and + `git revert A..B` keep `sequencer/` for the steps to come; once a + stopped step is committed by hand no `CHERRY_PICK_HEAD` or `REVERT_HEAD` + is left, yet `git status` still says the act is in progress, and the + verb's commit lands in the middle of the series (measured: `--continue` + then replays the rest on top of it, and `--abort` exits 0 with `You seem + to have moved HEAD. Not rewinding`, keeping it). The act is named from + the first word of `sequencer/todo`. Where that file is missing, empty or + no series, `git status` says nothing and `--continue` fails, and + `--abort` and `--quit` both exit 0: the refusal names it generically and + offers only what works. + + So the marks are asked, and the refusal names the one it found and that + act's OWN exit: `git commit` for a merge, `--continue` for the others, + or `--abort`. Every mark is looked for WHERE GIT KEEPS IT (`_git_paths`), + which in a linked worktree is not `.git/`. Asked of the holder's + own git directory, never of a member's: a merge in a MEMBER's mount is + that checkout's own business. + + WHEN IT IS ASKED, AND WHAT IT DOES NOT NAME (#612's review, N4). Each + verb asks it right after `Family.open` and the checks of its own + arguments, so after `family.yaml` is parsed. The commonest merge conflict + in a holder is in `family.yaml` itself, whose `<<<<<<<` `load_yaml` reads + as a YAML merge key: `yaml-unparsable`, with no remediation, and no word + of a merge. That predates this check and is left for #459: asking before + `load_yaml` makes git run before the refusal of a hand-edited `path:`, + which this test holds to no git command at all: + `test_family_member_path.py::test_the_refusal_is_made_before_any_git_call`. """ found = _operation_in_progress(root) if found is None: return what, mark, where, finish, abort, effect = found + if finish is None: + end = ("Remediation: `sequencer/` is what a cherry-pick or revert " + "series left behind, and git can read no step from it, so " + "there is nothing to continue: give it up with " + f"`{abort}` (`git revert --abort` is the same), or clear it " + "without moving HEAD with `git cherry-pick --quit`. Then run " + "the same command again.") + else: + end = ("Remediation: end it, whichever way suits the work in it: " + f"finish it with `{finish}` once whatever stopped it is " + f"resolved and staged, or give it up with `{abort}`. Then " + "run the same command again.") raise Refusal( "family-operation-in-progress", f"{what} is unfinished in the family root: git keeps {mark} at " f"{where.as_posix()}. `{verb}` makes its commit with `git commit -- " f"`, and {effect.format(verb=verb)}. No git command that " - "writes was run and nothing was written.", - "Remediation: end it, whichever way suits the work in it: finish it " - f"with `{finish}` once whatever stopped it is resolved and staged, " - f"or give it up with `{abort}`. Then run the same command again.") + "writes was run and nothing was written.", end) def _sparse_rules_exclude(root: Path, paths: list[str]) -> list[str]: @@ -930,6 +1028,28 @@ def _sparse_rules_exclude(root: Path, paths: list[str]) -> list[str]: return [path for path, name in zip(paths, names) if name not in kept] +def _sparse_is_cone(root: Path) -> bool: + """Are the holder's sparse rules in cone mode? Read as a LISTING, `git + config -z --list`, the way `_registered` reads config, and never `git + config --get`: `test_family_dry_run.py` counts any `git config` that is + not a listing as a write. Git writes section and variable names in lower + case, so the key is `core.sparsecheckoutcone`; the last one listed wins, + one with no value (`sparseCheckoutCone` alone) is true, and one that is + unset is false, which is non-cone.""" + try: + listing = _git_bytes(["config", "-z", "--list"], root) + except CommandFailed: + return False + cone = False + for record in listing.split(b"\0"): + key, has_value, value = record.partition(b"\n") + if key.lower() == b"core.sparsecheckoutcone": + word = value.strip().lower() + cone = not has_value or word in (b"true", b"yes", b"on") or ( + word.isdigit() and int(word) != 0) + return cone + + def _checked_inside_sparse_rules(root: Path, verb: str, paths: list[str]) -> None: """`family-sparse-excluded`: `verb` refuses, before its first write and @@ -951,21 +1071,34 @@ def _checked_inside_sparse_rules(root: Path, verb: str, `remove` (the gitlink of the second is staged by `git rm`), and `members/` and `family.yaml` for `bump`. - THE EXIT NAMES THE COMMAND THAT BRINGS THE PATH BACK. `git - sparse-checkout add members` for the mount path, which works in cone - mode and in non-cone mode. For `.gitmodules` or `family.yaml`, only - non-cone rules can leave a root file out: in CONE mode git keeps every - file at the root inside (measured on git 2.43 with `set --cone`: - `.gitmodules` and `family.yaml` are inside, and a directory that is not - in the cone is out), so a root file is never refused there, and a root - file `check-rules` does call outside has non-cone rules. Its pattern is - `git sparse-checkout add /.gitmodules`, with the leading slash that makes - it the root's file alone, appended after the pattern that left it out, - which is the one that wins. `git sparse-checkout disable` is named beside - it for any path: it brings back every path and leaves a copy already on - disk as it is, as `gitmodules-sparse` says, and it needs no pattern - typed: a leading slash is a path to a shell that converts one (Git Bash - on Windows can), so the second command is the one that never meets that. + THE EXIT NAMES THE COMMAND THAT BRINGS THE PATH BACK, by the mode of the + rules (`_sparse_is_cone`). In CONE mode, `git sparse-checkout add + members`: it takes a directory, and `members/` is refused there + for a gitlink git already tracks (`fatal: 'members/IRSS' is not a + directory`). In NON-cone mode, the mount path itself, `git + sparse-checkout add members/`, which its inner slash anchors at + the root. `members` alone is NOT enough there: appended after a rule that + names the member (`!/members/IRQS`, `!/members/*`, `!IRQS`, + `!/members/I*`) it exits 0 and leaves the path out (measured on git + 2.43, #612's review F3), and the verb was refused again with the same + remedy, a loop that only `disable` got out of. + + A ROOT FILE (`.gitmodules`, `family.yaml`) can only be out of non-cone + rules: in CONE mode git keeps every file at the root inside (measured on + git 2.43 with `set --cone`: both are inside, and a directory that is not + in the cone is out). It is named UNANCHORED, `git sparse-checkout add + .gitmodules`, which wins over `!/.gitmodules`, `!.gitmodules` and `!/.*` + appended before it (measured here, and over `!/*` by the review), and + which meets no shell that converts a leading slash into a path (Git Bash + on Windows can). git + prints a warning for such a pattern (`pass a leading slash before paths + such as 'family.yaml' if you want a single file`); it is about how far a + slashless pattern reaches, and nothing else in a holder's root, or in + what `members/` holds that git tracks, is named so. + + `git sparse-checkout disable` is named beside it for any path: it brings + back every path and leaves a copy already on disk as it is, as + `gitmodules-sparse` says. `git sparse-checkout check-rules` is a recent subcommand (the git here, 2.43, has it); a git without it is "cannot tell" and refuses nothing @@ -974,10 +1107,11 @@ def _checked_inside_sparse_rules(root: Path, verb: str, out = _sparse_rules_exclude(root, paths) if not out: return - # ONE PATTERN PER PATH, the mount path's `members` once. A root file is - # only ever out of NON-cone rules, so its pattern is the non-cone one. + # ONE PATTERN PER PATH: the mount path itself, or `members` once in cone + # mode; a root file as it is named, unanchored. + cone = _sparse_is_cone(root) patterns = list(dict.fromkeys( - MEMBERS_DIR if path.startswith(MEMBERS_DIR + "/") else "/" + path + MEMBERS_DIR if cone and path.startswith(MEMBERS_DIR + "/") else path for path in out)) names = ", ".join(f"`{path}`" for path in out) one = len(out) == 1 @@ -2716,9 +2850,13 @@ def _submodule_paths( def _members_is_a_file(root: Path, path: str) -> Refusal: """`member-path-unaddable` for a `members` that is a FILE (#612, 574/5): what it says, and the move that makes room. A file git tracks is moved - with `git mv` and the move committed first, since the add's own commit - names three paths and would leave the rename staged and uncommitted - beside it; one it does not track is moved with `mv`.""" + with `git mv` and the move committed first. Left staged, the move is not + left alone by the add: its commit names three paths, and git, putting a + directory where the file was, takes the deletion half of the move into + that commit (`A .gitmodules`, `M family.yaml`, `D members`, `A + members/`) and leaves the new name staged alone (`A + members-file`), which is half a move in the holder's history (#612's + review, N3). One git does not track is moved with `mv`.""" tracked = any(name == MEMBERS_DIR for name, _ in _index_entries(root, MEMBERS_DIR)) move = ("git mv" if tracked else "mv") @@ -3846,8 +3984,10 @@ def cmd_add(args) -> int: family = Family.open(args.family_root) repository, project = _checked_member(args.member) path = _member_path(project) - # A merge, cherry-pick, revert or rebase in progress is asked first: it - # fails the commit however the rest is (#612, 571(e)). + # A merge, cherry-pick, revert, rebase or `am` unfinished in the holder + # is asked before any refusal that reads the holder, since its commit + # fails however the rest is; `family.yaml` is read before it, so a merge + # conflicted there reads as `yaml-unparsable` (#612, 571(e), N4). _checked_no_operation_in_progress(family.root, "add") at = _checked_add_target(family, repository, project, path, args.at) # What the sparse rules leave out, after the refusals that read the same diff --git a/tests/test_family_refuse_up_front.py b/tests/test_family_refuse_up_front.py index f648b5a4..7d264b56 100644 --- a/tests/test_family_refuse_up_front.py +++ b/tests/test_family_refuse_up_front.py @@ -55,7 +55,7 @@ import pytest #: The family fixture and its names, imported from the module that owns them. -from test_family import SPARE, family # noqa: F401 +from test_family import SPARE, family, family_module # noqa: F401 from test_family_add_excludes import empty_holder # noqa: F401 from test_family_add_refusals import holder, offline, refusal # noqa: F401 #: What a verb must leave as it found it, and the three verbs, as the dry-run @@ -65,7 +65,7 @@ BUMP_ORDER, REMOVE_ORDER, add, add_state, assert_add_as_it_was, assert_as_it_was, bump, isolated, remove, state, ) -from test_family_member_path import MEMBER, OTHER, older, raw +from test_family_member_path import MEMBER, OTHER, older, raw, recorded GITMODULES = ".gitmodules" NOTES = "NOTES.txt" @@ -132,11 +132,13 @@ def head_of(root: Path, env: dict) -> str: def assert_refused_alike(verb: str, family, root: Path, env: dict, - expected_stderr) -> None: + expected_stderr, mark: str | None = None): """THE PROOF OF A REFUSAL BEFORE THE FIRST WRITE, under both runs: exit 2, the one paragraph `expected_stderr` (a compiled pattern for the REFUSED line, the remediation exactly) and nothing on stdout, so the member was - not read; the holder as it was; and the dry run's words the real run's.""" + not read; the holder as it was, and `mark`, the act's own mark in git's + directory, still there; and the dry run's words the real run's. Returns + the last run.""" before = snapshot(verb, root, env) said = [] for extra in ((), ("--dry-run",)): @@ -149,14 +151,32 @@ def assert_refused_alike(verb: str, family, root: Path, env: dict, assert expected_line.fullmatch(line), (extra, line) assert remedy == expected_remedy, (extra, remedy) assert_untouched(verb, root, env, before) + if mark is not None: + assert os.path.lexists(mark_of(root, env, mark)), mark assert said[0] == said[1], "the two runs refuse in different words" + return ran # --- an act git has unfinished --------------------------------------------- #: `{kind: (what, the mark git keeps, how it is finished, how it is given #: up, what the commit would do)}`, as the refusal says each. `{verb}` is the -#: verb that asked. +#: verb that asked. `finish` is None where git can continue nothing. +REBASE_EFFECT = ( + "git does not refuse that during a rebase: the commit would land on the " + "HEAD the rebase has detached, in the middle of its sequence, where " + "`git rebase --continue` replays the rest on top of it into the rebased " + "branch and `git rebase --abort` drops it") + + +def series_effect(command: str) -> str: + return ("git does not refuse that between the steps of a series, so the " + f"commit would land in the middle of it: `{command} --continue` " + f"replays the rest on top of it, and `{command} --abort` stops " + "and leaves it where it is (`You seem to have moved HEAD. Not " + "rewinding`)") + + OPERATIONS = { "merge": ( "a merge", "MERGE_HEAD", "git commit", "git merge --abort", @@ -171,19 +191,31 @@ def assert_refused_alike(verb: str, family, root: Path, env: dict, "which are then put back"), "rebase": ( "a rebase", "rebase-merge", "git rebase --continue", - "git rebase --abort", - "git does not refuse that during a rebase, so the commit would land " - "on the HEAD the rebase has detached, inside its sequence, where no " - "branch can carry it to a pull request"), + "git rebase --abort", REBASE_EFFECT), "revert": ( "a revert", "REVERT_HEAD", "git revert --continue", "git revert --abort", - "git does not refuse that during a revert, so the commit would be " - "made inside the revert and end it as the revert's own"), + "git does not refuse that during a revert: the commit is made on " + "the branch as `{verb}`'s own, and git takes it as the end of the " + "revert, so `git revert --continue` and `--abort` then say `no " + "cherry-pick or revert in progress` and the revert's changes are " + "left half done in the index and the work tree"), "am": ( "a `git am`", "rebase-apply", "git am --continue", "git am --abort", "git does not refuse that during a `git am`, so the commit would " "land on the branch, in the middle of the patches being applied"), + "cherry-pick series": ( + "a cherry-pick series", "sequencer", "git cherry-pick --continue", + "git cherry-pick --abort", series_effect("git cherry-pick")), + "revert series": ( + "a revert series", "sequencer", "git revert --continue", + "git revert --abort", series_effect("git revert")), + "unreadable series": ( + "a cherry-pick or revert series", "sequencer", None, + "git cherry-pick --abort", + "git does not refuse that, and the commit would land beside a " + "`sequencer/` directory git can read no series from, which " + "`--continue` fails on"), } @@ -198,10 +230,18 @@ def operation_said(kind: str, verb: str) -> tuple: + re.escape(f". `{verb}` makes its commit with `git commit -- " f"`, and {effect.format(verb=verb)}. No git " "command that writes was run and nothing was written.")) - remedy = ("Remediation: end it, whichever way suits the work in it: " - f"finish it with `{finish}` once whatever stopped it is " - f"resolved and staged, or give it up with `{abort}`. Then run " - "the same command again.") + if finish is None: + remedy = ("Remediation: `sequencer/` is what a cherry-pick or revert " + "series left behind, and git can read no step from it, so " + "there is nothing to continue: give it up with " + f"`{abort}` (`git revert --abort` is the same), or clear " + "it without moving HEAD with `git cherry-pick --quit`. " + "Then run the same command again.") + else: + remedy = ("Remediation: end it, whichever way suits the work in it: " + f"finish it with `{finish}` once whatever stopped it is " + f"resolved and staged, or give it up with `{abort}`. Then " + "run the same command again.") return line, remedy @@ -237,10 +277,61 @@ def mark_of(root: Path, env: dict, name: str) -> Path: return root / shown +def series(kind: str, root: Path, env: dict) -> None: + """A cherry-pick or a revert of SEVERAL commits, stopped on its first step + and that step committed by hand, so no `CHERRY_PICK_HEAD` or + `REVERT_HEAD` is left and `sequencer/` holds the steps still to take + (#612's review, F2). `unreadable series` has its `todo` taken away.""" + work = work_branch(root, env) + if kind == "revert series": + commit_file(root, env, NOTES, b"zero\n", "zero") + base = head_of(root, env) + commit_file(root, env, "x1.txt", b"1\n", "x1") + commit_file(root, env, "x2.txt", b"2\n", "x2") + commit_file(root, env, NOTES, b"mid\n", "y") + commit_file(root, env, NOTES, b"late\n", "z") + stopped = raw(root, env, "revert", "--no-edit", f"{base}..HEAD~1", + check=False) + else: + commit_file(root, env, NOTES, b"base\n", "base") + base = head_of(root, env) + raw(root, env, "switch", "-q", "-c", "side") + commit_file(root, env, NOTES, b"side\n", "side1") + commit_file(root, env, "side2.txt", b"2\n", "side2") + commit_file(root, env, "side3.txt", b"3\n", "side3") + raw(root, env, "switch", "-q", work) + commit_file(root, env, NOTES, b"main\n", "main") + stopped = raw(root, env, "cherry-pick", f"{base}..side", check=False) + assert stopped.returncode != 0, "premise: the first step conflicts" + commit_file(root, env, NOTES, b"resolved\n", "the stopped step, by hand") + for gone in ("CHERRY_PICK_HEAD", "REVERT_HEAD"): + assert not os.path.lexists(mark_of(root, env, gone)), ( + f"premise: only `sequencer/` is left, not {gone}") + if kind == "unreadable series": + mark_of(root, env, "sequencer/todo").unlink() + + def begin(kind: str, root: Path, env: dict, tmp_path: Path) -> None: """Leave `root` in the middle of `kind`, with git, and check the mark git keeps for it is there: the premise of every test that calls this.""" - if kind == "merge": + if kind.endswith(" series"): + series(kind, root, env) + elif kind == "rebase and merge": + # A merge begun at a rebase's stop leaves `MERGE_HEAD` and + # `rebase-merge/` both. `-x "exit 1"` stops the rebase after a step. + work = work_branch(root, env) + base = head_of(root, env) + raw(root, env, "switch", "-q", "-c", "other") + commit_file(root, env, "other.txt", b"other\n", "other") + raw(root, env, "switch", "-q", work) + raw(root, env, "switch", "-q", "-c", "topic") + commit_file(root, env, "topic.txt", b"topic\n", "topic") + stopped = raw(root, env, "rebase", "-x", "exit 1", base, check=False) + assert stopped.returncode != 0, "premise: the rebase stops" + raw(root, env, "merge", "--no-commit", "--no-ff", "other") + assert os.path.lexists(mark_of(root, env, "MERGE_HEAD")), ( + "premise: the merge is in progress too") + elif kind == "merge": # `--no-commit`: no conflict at all, the state the issue's probe made. work = work_branch(root, env) raw(root, env, "switch", "-q", "-c", "side") @@ -269,56 +360,78 @@ def begin(kind: str, root: Path, env: dict, tmp_path: Path) -> None: stopped = raw(root, env, "am", str(patch), check=False) assert stopped.returncode != 0, "premise: the am conflicts" assert work_branch(root, env) == work - mark = OPERATIONS[kind][1] + mark = OPERATIONS.get(kind, OPERATIONS["rebase"])[1] assert os.path.lexists(mark_of(root, env, mark)), f"premise: {mark}" -@pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) -@pytest.mark.parametrize("verb", VERBS) -def test_each_verb_refuses_a_merge_in_progress_before_it_writes( - family, holder, offline, tmp_path, verb, dry_run): - """THE ISSUE'S PROBE (p07), for each verb: during a `--no-commit` merge - every verb exited 2 at `git commit`, `fatal: cannot do a partial commit - during a merge.`, after its first writes. Now `family-operation-in- - progress`, before them: HEAD, the index, `family.yaml`, the work tree and - the git config as they were, no member read, and the dry run's words the - real run's. The real run and the dry run are each asked of the holder - the merge left, which neither changes.""" - env = isolated(offline, tmp_path) - begin("merge", holder, env, tmp_path) - before = snapshot(verb, holder, env) - manifest = (holder / "family.yaml").read_bytes() - extra = ("--dry-run",) if dry_run else () - ran = run_verb(verb, family, holder, env, *extra) - assert ran.returncode == 2, ran.stdout + ran.stderr - assert ran.stdout == "", ran.stdout - line, remedy = refusal(ran.stderr) - expected_line, expected_remedy = operation_said("merge", verb) - assert expected_line.fullmatch(line), line - assert remedy == expected_remedy - assert "partial commit" in line and "after" in line - assert (holder / "family.yaml").read_bytes() == manifest - assert_untouched(verb, holder, env, before) - assert os.path.lexists(mark_of(holder, env, "MERGE_HEAD")) - - @pytest.mark.parametrize("kind, verb", [ ("merge", "add"), ("merge", "bump"), ("merge", "remove"), ("cherry-pick", "add"), ("cherry-pick", "remove"), ("rebase", "bump"), ("rebase", "add"), ("revert", "remove"), ("revert", "bump"), ("am", "add"), ("am", "remove"), + ("cherry-pick series", "add"), ("cherry-pick series", "bump"), + ("revert series", "remove"), ("revert series", "add"), + ("unreadable series", "remove"), ("unreadable series", "bump"), ]) def test_every_unfinished_act_is_named_and_refused_alike_in_both_runs( family, holder, offline, tmp_path, kind, verb): - """A merge, a cherry-pick, a rebase, a revert and a `git am`, each - stopped on a conflict by git: each named, with its own finish and its - own give-up, by the real run and the dry run in the same words, the - holder untouched. A rebase and an `am` were no failure at the commit at - all (git does not refuse a partial commit there); the real run met a - rebase as `family-holder-detached`, the dry run printed a plan.""" + """THE ISSUE'S PROBE (p07) for a merge, and the other acts: a merge + (stopped with `--no-commit`, no conflict at all), a cherry-pick, a + rebase, a revert and a `git am`, each stopped on a conflict by git, and a + cherry-pick or revert SERIES with only `sequencer/` left, and one whose + `todo` git cannot read. Each is named, with its own finish and its own + give-up, by the real run and the dry run in the same words, the holder + untouched and the act's mark still there. A revert, a rebase and an `am` + were no failure at the commit (git does not refuse a partial commit + there); the real run met a rebase as `family-holder-detached`, which + committed nothing, the dry run printed a plan.""" env = isolated(offline, tmp_path) begin(kind, holder, env, tmp_path) - assert_refused_alike(verb, family, holder, env, operation_said(kind, verb)) + assert_refused_alike(verb, family, holder, env, operation_said(kind, verb), + mark=OPERATIONS[kind][1]) + + +@pytest.mark.parametrize("kind, refused", [ + ("merge", "a merge"), ("cherry-pick", "a cherry-pick"), + ("revert", None), ("rebase", None), ("am", None), + ("cherry-pick series", None), ("revert series", None)]) +def test_what_each_refusal_says_the_commit_would_do_is_what_git_does( + family, holder, offline, tmp_path, kind, refused): + """THE EFFECT SENTENCES ARE TIED TO GIT, so they cannot drift from it + (#612's review, F1). The partial commit the verbs make is made here with + git itself, `git commit -- :(literal)family.yaml`: a merge and a + cherry-pick are refused (`cannot do a partial commit during ...`); a + revert takes it as the END of the revert (`REVERT_HEAD` goes, and `git + revert --continue` says `no cherry-pick or revert in progress`); a + rebase and an `am` keep their mark; and in a series git commits it, and + `--continue` then replays the rest on top of it.""" + env = isolated(offline, tmp_path) + begin(kind, holder, env, tmp_path) + manifest = holder / "family.yaml" + manifest.write_bytes(manifest.read_bytes() + b"# a change\n") + made = raw(holder, env, "commit", "-q", "-m", "THE VERB'S COMMIT", "--", + ":(literal)family.yaml", check=False) + mark = OPERATIONS[kind][1] + if refused: + assert made.returncode == 128, made.stderr + assert (f"cannot do a partial commit during {refused}".encode() + in made.stderr) + return + assert made.returncode == 0, made.stderr + if kind == "revert": + assert not os.path.lexists(mark_of(holder, env, mark)) + stuck = raw(holder, env, "revert", "--continue", check=False) + assert stuck.returncode == 128 + assert b"no cherry-pick or revert in progress" in stuck.stderr + elif kind.endswith(" series"): + command = kind.split()[0] + raw(holder, env, command, "--continue") + subjects = raw(holder, env, "log", "--format=%s", "-6" + ).stdout.decode().split("\n") + assert "THE VERB'S COMMIT" in subjects[1:], subjects + assert subjects[0] != "THE VERB'S COMMIT", subjects + else: + assert os.path.lexists(mark_of(holder, env, mark)) #: `{id: (kind, verb, the command the remediation names, what is added to it @@ -332,6 +445,14 @@ def test_every_unfinished_act_is_named_and_refused_alike_in_both_runs( "rebase, given up": ("rebase", "add", "git rebase --abort", ()), "revert, given up": ("revert", "remove", "git revert --abort", ()), "am, given up": ("am", "bump", "git am --abort", ()), + "cherry-pick series, continued": ( + "cherry-pick series", "add", "git cherry-pick --continue", ()), + "cherry-pick series, given up": ( + "cherry-pick series", "bump", "git cherry-pick --abort", ()), + "revert series, continued": ( + "revert series", "remove", "git revert --continue", ()), + "unreadable series, given up": ( + "unreadable series", "add", "git cherry-pick --abort", ()), } @@ -358,6 +479,41 @@ def test_the_remedy_typed_as_printed_lets_the_same_command_run( assert raw(holder, env, "status", "--porcelain").stdout == b"" +@pytest.mark.parametrize("verb", VERBS) +def test_a_rebase_is_asked_before_a_merge_begun_inside_it( + family, holder, offline, tmp_path, verb): + """"A REBASE IS ASKED FIRST": a merge begun at a rebase's stop leaves + `MERGE_HEAD` and `rebase-merge/` both, and the refusal names the rebase, + whose `git rebase --abort` clears both in one step, where `git merge + --abort` would leave `rebase-merge/` and a second refusal (#612's + review, N2). The exit is typed as printed, and the verb then runs.""" + env = isolated(offline, tmp_path) + begin("rebase and merge", holder, env, tmp_path) + ran = assert_refused_alike(verb, family, holder, env, + operation_said("rebase", verb), + mark="rebase-merge") + assert "git rebase --abort" in ran.stderr + raw(holder, env, "rebase", "--abort") + assert not os.path.lexists(mark_of(holder, env, "MERGE_HEAD")) + assert not os.path.lexists(mark_of(holder, env, "rebase-merge")) + done = run_verb(verb, family, holder, env) + assert done.returncode == 0, done.stdout + done.stderr + + +def test_every_mark_is_asked_in_one_git_call( + family, holder, offline, tmp_path, monkeypatch): + """ONE `git rev-parse` answers every mark (#612's review, N8): up to eight + `--git-path` arguments, one line each, where a process per mark cost + eight on every verb, and more on Windows.""" + for key, value in isolated(offline, tmp_path).items(): + monkeypatch.setenv(key, value) + calls = recorded(monkeypatch) + assert family_module()._operation_in_progress(holder) is None + asked = [argv for argv in calls if "--git-path" in argv] + assert len(asked) == 1, asked + assert asked[0].count("--git-path") >= 8, asked + + def test_a_merge_in_a_linked_worktree_is_found_where_git_keeps_it( family, holder, offline, tmp_path): """`.git` is a FILE in a linked worktree, which is where a lane opens its @@ -401,13 +557,20 @@ def sparse_set(root: Path, env: dict, *rules: str, cone: bool = False) -> None: "--cone" if cone else "--no-cone", *rules) -def restored_by_hand(root: Path, env: dict) -> None: +#: The rules that leave `.gitmodules` out, by what names it: the file +#: anchored, the file anywhere, and every dotfile at the root. +GITMODULES_OUT = {"anchored": ("/*", f"!/{GITMODULES}"), + "unanchored": ("/*", f"!{GITMODULES}"), + "every dotfile": ("/*", "!/.*")} + + +def restored_by_hand(root: Path, env: dict, shape: str = "anchored") -> None: """THE ISSUE'S STATE (571(d)): the rules leave `.gitmodules` out, the file is back on disk because a person put it there, and git's skip-worktree mark is CLEAR, so #407's check, which reads the mark, passes. Built with git's own `sparse-checkout set`, then the file written back as bytes and the mark taken off.""" - sparse_set(root, env, "/*", f"!/{GITMODULES}") + sparse_set(root, env, *GITMODULES_OUT[shape]) put_back(root, env, GITMODULES) @@ -465,18 +628,22 @@ def type_the_remedy(root: Path, env: dict, stderr: str) -> None: @needs_check_rules +@pytest.mark.parametrize("shape", GITMODULES_OUT) @pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) def test_remove_refuses_a_gitmodules_the_rules_leave_out_before_deinit( - family, holder, offline, tmp_path, dry_run): + family, holder, offline, tmp_path, dry_run, shape): """THE ISSUE'S p06: sparse rules leave `.gitmodules` out, the file is put back by hand, git clears the mark, #407's check passes, and `remove` did `deinit` and `git rm` and failed at `git add` (`outside of your sparse-checkout definition`), then rolled back. Now refused before `deinit`: the mount, the registration in the git config and the index are untouched, which `REMOVE_ORDER` reads key by key. The pattern the - remediation names is typed as printed, and `remove` then goes through.""" + remediation names is typed as printed, and `remove` then goes through. + The pattern is the file UNANCHORED, `.gitmodules`: appended after each + of the rules that leave it out (the file named with a slash, without + one, and every dotfile) it brings the file back (#612's review, N7).""" env = isolated(offline, tmp_path) - restored_by_hand(holder, env) + restored_by_hand(holder, env, shape) before = state(holder, env) extra = ("--dry-run",) if dry_run else () ran = remove(holder, env, *extra) @@ -484,7 +651,7 @@ def test_remove_refuses_a_gitmodules_the_rules_leave_out_before_deinit( assert ran.stdout == "" line, remedy = refusal(ran.stderr) expected_line, expected_remedy = sparse_said( - "remove", [GITMODULES], f"/{GITMODULES}") + "remove", [GITMODULES], GITMODULES) assert expected_line.fullmatch(line), line assert remedy == expected_remedy assert_as_it_was(holder, env, before, REMOVE_ORDER) @@ -498,23 +665,45 @@ def test_remove_refuses_a_gitmodules_the_rules_leave_out_before_deinit( assert not (holder / "members" / MEMBER).exists() +#: The rules that leave a member's mount out: cone mode, which keeps only +#: what it names, and non-cone rules that leave out the whole of `members/`, +#: or name the member, or glob under it. `{p}` is the member's name. +MOUNT_OUT = { + "cone": (True, ("contracts",)), + "non-cone, members/": (False, ("/*", "!/members/")), + "non-cone, the member named": (False, ("/*", "!/members/{p}")), + "non-cone, members/*": (False, ("/*", "!/members/*")), +} + + +def mount_out(root: Path, env: dict, rows: str, member: str) -> str: + """Put the rules `rows` in force, and name the pattern the refusal must + print: `members` in cone mode, the mount path itself otherwise (#612's + review, F3).""" + cone, rules = MOUNT_OUT[rows] + sparse_set(root, env, *[rule.format(p=member) for rule in rules], + cone=cone) + assert outside(root, env, f"members/{member}"), "premise: out" + return "members" if cone else f"members/{member}" + + @needs_check_rules -@pytest.mark.parametrize("cone", [False, True], ids=["non-cone", "cone"]) +@pytest.mark.parametrize("rows", MOUNT_OUT) @pytest.mark.parametrize("dry_run", [False, True], ids=["real", "dry-run"]) def test_add_refuses_a_members_the_rules_leave_out_before_it_reads_the_member( - family, holder, offline, tmp_path, cone, dry_run): + family, holder, offline, tmp_path, rows, dry_run): """574/5 (p12b): `add` printed `mounting ...`, then `git submodule add` refused the mount as outside the sparse-checkout definition, and the - unwind restored the holder. Now refused first, naming `git - sparse-checkout add members`, which works in cone mode and in non-cone - mode: it is typed as printed, and `add` then mounts the member. Nothing - on stdout: not even the line that says the member is being read.""" + unwind restored the holder. Now refused first, naming the pattern that + brings the mount back: `git sparse-checkout add members` in cone mode, + and the mount path itself, `members/`, under non-cone rules. A + bare `members` does not win over a rule that names the member (`!/members/ +

`, `!/members/*`: it exits 0 and the verb was refused again with the + same remedy, #612's review, F3). The printed command is typed as printed, + and `add` then mounts the member. Nothing on stdout: not even the line + that says the member is being read.""" env = isolated(offline, tmp_path) - if cone: - sparse_set(holder, env, "contracts", cone=True) - else: - sparse_set(holder, env, "/*", "!/members/") - assert outside(holder, env, f"members/{SPARE}"), "premise: out" + pattern = mount_out(holder, env, rows, SPARE) before = add_state(holder, env) extra = ("--dry-run",) if dry_run else () ran = add(holder, env, family["remotes"], *extra) @@ -522,7 +711,7 @@ def test_add_refuses_a_members_the_rules_leave_out_before_it_reads_the_member( assert ran.stdout == "" line, remedy = refusal(ran.stderr) expected_line, expected_remedy = sparse_said( - "add", [f"members/{SPARE}"], "members") + "add", [f"members/{SPARE}"], pattern) assert expected_line.fullmatch(line), line assert remedy == expected_remedy assert_add_as_it_was(holder, env, before) @@ -534,20 +723,25 @@ def test_add_refuses_a_members_the_rules_leave_out_before_it_reads_the_member( @needs_check_rules +@pytest.mark.parametrize("rows", MOUNT_OUT) @pytest.mark.parametrize("verb", ["bump", "remove"]) def test_bump_and_remove_refuse_a_members_the_rules_leave_out_too( - family, holder, offline, tmp_path, verb): + family, holder, offline, tmp_path, verb, rows): """The same helper covers `bump`'s staged paths, `members/

` and `family.yaml`, and `remove`'s: before it, `bump` checked the member out at `--to` and failed at `git add` of the gitlink, `remove` ran `deinit` - and failed at `git rm`, each rolled back. Refused first, both runs.""" + and failed at `git rm`, each rolled back. Refused first, both runs, with + the pattern of `mount_out`; typed as printed, the verb then runs.""" env = isolated(offline, tmp_path) - sparse_set(holder, env, "/*", "!/members/") - assert outside(holder, env, f"members/{MEMBER}"), "premise: out" + pattern = mount_out(holder, env, rows, MEMBER) # `remove` stages `.gitmodules`, the gitlink and `family.yaml`; only the # gitlink is out here, so it alone is named, as for `bump`. - assert_refused_alike(verb, family, holder, env, - sparse_said(verb, [f"members/{MEMBER}"], "members")) + ran = assert_refused_alike( + verb, family, holder, env, + sparse_said(verb, [f"members/{MEMBER}"], pattern)) + type_the_remedy(holder, env, ran.stderr) + done = run_verb(verb, family, holder, env) + assert done.returncode == 0, done.stdout + done.stderr @needs_check_rules @@ -566,7 +760,8 @@ def test_the_cone_keeps_every_root_file_so_only_the_mount_is_refused( line, _ = refusal(ran.stderr) assert ran.returncode == 2 assert f"leaves out `members/{MEMBER}`, which" in line, line - assert GITMODULES not in line and "family.yaml" not in line, line + assert GITMODULES not in line, line + assert "family.yaml" not in line, line @needs_check_rules @@ -590,7 +785,7 @@ def test_every_excluded_path_is_named_and_one_command_brings_them_all_back( line, remedy = refusal(ran.stderr) expected_line, expected_remedy = sparse_said( "add", [GITMODULES, f"members/{SPARE}", "family.yaml"], - f"/{GITMODULES}", "members", "/family.yaml") + GITMODULES, f"members/{SPARE}", "family.yaml") assert expected_line.fullmatch(line), line assert remedy == expected_remedy assert_add_as_it_was(holder, env, before) @@ -757,12 +952,3 @@ def test_add_refuses_a_file_named_members_before_it_reads_the_member( done = add(holder, env, family["remotes"]) assert done.returncode == 0, done.stdout + done.stderr assert (holder / "members" / SPARE / "project.yaml").is_file() - - -def test_a_members_directory_is_not_refused_as_a_file( - family, holder, offline, tmp_path): - """THE CONTROL: `members` is the directory every holder has, and `add` - mounts beside what is there.""" - env = isolated(offline, tmp_path) - ran = add(holder, env, family["remotes"]) - assert ran.returncode == 0, ran.stdout + ran.stderr From 04ad53afcafb8a6200a633ea247021fe925a3870 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 16 Oct 2026 09:57:17 +0000 Subject: [PATCH 3/3] family.py: #626 fix round 2: `_git_paths` keeps a holder path that holds a newline whole, so a merge in a linked worktree under it is found again, `git cherry-pick` is one constant, and two comments say what is true (#612) R612 verified 207d4e1d and asked for three small things. V-F1. `_git_paths` asks every mark in ONE `git rev-parse`; where the lines and the names disagree it asks each name alone. That fallback still split the one answer on a newline and kept the first line, which cut the very path it was written for: a linked worktree's answer is absolute, through the holder's own `.git`, so a holder in a directory whose name holds a newline gave `.../Ink`, which does not exist. No mark was found, `add --dry-run` printed a plan, and the real `add` mounted and failed at the commit, #612's own defect back for that path. The fallback now keeps the whole answer less git's final newline, as round 0's `_git_path` did. A POSIX-only test (Windows refuses a newline in a name) puts the holder in `Ink\nRouter`, begins a `--no-commit` merge in a linked worktree outside it, and requires `add`, in both runs, to refuse `family-operation-in-progress` naming `MERGE_HEAD`. The mutant that keeps only the first line fails it. V-F2. SonarCloud S1192: `"git cherry-pick"` stood three times in the sequencer's code. It is one constant, `CHERRY_PICK`. V-F3. The comments in `cmd_add` and `cmd_remove` said the check is asked before the holder is read and that its commit "fails". It is asked after `Family.open`, `member-unknown` and the row's `path:` check, which read the manifest, and the commit goes wrong however the rest is: refused after the writes, or made inside the act. The docstring that names git's leading-slash warning now says it shows for an existing member's `members/` too, and never in the verb's own output. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- scripts/family.py | 49 ++++++++++++++++++---------- tests/test_family_refuse_up_front.py | 33 +++++++++++++++++++ 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/scripts/family.py b/scripts/family.py index 1864304c..709e8a92 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -826,7 +826,8 @@ def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: #: it: `pick` or `p`, a cherry-pick; `revert`, a revert. SEQUENCER = "sequencer" SEQUENCER_TODO = "sequencer/todo" -SEQUENCE_COMMANDS = {"pick": "git cherry-pick", "p": "git cherry-pick", +CHERRY_PICK = "git cherry-pick" +SEQUENCE_COMMANDS = {"pick": CHERRY_PICK, "p": CHERRY_PICK, "revert": "git revert"} #: A `sequencer/` whose `todo` is missing, empty or not a series git reads. @@ -849,16 +850,21 @@ def _git_paths(root: Path, names: list[str]) -> list[Path]: a file and the per-worktree state (`MERGE_HEAD`, `rebase-merge/`, `index.lock`) is under `.git/worktrees//` (#365, `_module_dir`, `_checked_index_unlocked`). git answers relative to `root` or absolute, - and `root /` takes both. A holder whose path holds a newline would make - the lines and the names disagree: each name is then asked alone.""" - def asked(*words: str) -> list[str]: + and `root /` takes both. A holder whose path holds a newline (a linked + worktree's answer is absolute, through the holder's own `.git`) would make + the lines and the names disagree: each name is then asked alone, and its + WHOLE answer less git's final newline is the path, never its first line + (#612's verification, V-F1: a path cut at its newline named nothing, and + a merge in that worktree was missed, which is #612's own defect).""" + def asked(*words: str) -> str: shown = _git_bytes(["rev-parse", *words], root).decode( "utf-8", "surrogateescape") - return shown[:-1].split("\n") if shown.endswith("\n") else [shown] + return shown[:-1] if shown.endswith("\n") else shown - lines = asked(*[word for name in names for word in ("--git-path", name)]) + lines = asked(*[word for name in names + for word in ("--git-path", name)]).split("\n") if len(lines) != len(names): - lines = [asked("--git-path", name)[0] for name in names] + lines = [asked("--git-path", name) for name in names] return [root / line for line in lines] @@ -873,7 +879,7 @@ def _sequence_act(todo: Path) -> tuple: first[0].decode("utf-8", "replace") if first else "") if command is None: return SEQUENCE_UNREADABLE - what = "a cherry-pick" if command == "git cherry-pick" else "a revert" + what = "a cherry-pick" if command == CHERRY_PICK else "a revert" return (f"{what} series", f"{command} --continue", f"{command} --abort", "git does not refuse that between the steps of a series, so the " f"commit would land in the middle of it: `{command} --continue` " @@ -1090,11 +1096,13 @@ def _checked_inside_sparse_rules(root: Path, verb: str, .gitmodules`, which wins over `!/.gitmodules`, `!.gitmodules` and `!/.*` appended before it (measured here, and over `!/*` by the review), and which meets no shell that converts a leading slash into a path (Git Bash - on Windows can). git - prints a warning for such a pattern (`pass a leading slash before paths - such as 'family.yaml' if you want a single file`); it is about how far a - slashless pattern reaches, and nothing else in a holder's root, or in - what `members/` holds that git tracks, is named so. + on Windows can). git prints a warning when the PERSON types such a + pattern, and also for `members/` of a member git already tracks + (`pass a leading slash before paths such as 'family.yaml' if you want a + single file`); it never appears in the verb's own output, and the command + brings the path back all the same. It is about how far a slashless + pattern reaches, and nothing else in a holder's root, or in what + `members/` holds that git tracks, is named so. `git sparse-checkout disable` is named beside it for any path: it brings back every path and leaves a copy already on disk as it is, as @@ -3985,9 +3993,11 @@ def cmd_add(args) -> int: repository, project = _checked_member(args.member) path = _member_path(project) # A merge, cherry-pick, revert, rebase or `am` unfinished in the holder - # is asked before any refusal that reads the holder, since its commit - # fails however the rest is; `family.yaml` is read before it, so a merge - # conflicted there reads as `yaml-unparsable` (#612, 571(e), N4). + # is asked after `Family.open`, which parses `family.yaml` (so a merge + # conflicted there reads as `yaml-unparsable`, #612 N4), and before every + # refusal about the holder's own state: the commit goes wrong however + # the rest is, refused after the writes or made inside the act (#612, + # 571(e)). _checked_no_operation_in_progress(family.root, "add") at = _checked_add_target(family, repository, project, path, args.at) # What the sparse rules leave out, after the refusals that read the same @@ -6124,8 +6134,11 @@ def cmd_remove(args) -> int: # call: `path: members/*` unmounted every member under one commit naming # one, and `path: .` removed the holder's own files. path = _member_path(project, row) - # A merge, cherry-pick, revert or rebase in progress is asked before the - # holder's files are read: its commit fails however they are (#612). + # A merge, cherry-pick, revert, rebase or `am` unfinished in the holder + # is asked after `Family.open`, `member-unknown` and the row's `path:` + # check, which read the manifest, and before the refusals about the + # holder's own files: the commit goes wrong however they are, refused + # after the writes or made inside the act (#612). _checked_no_operation_in_progress(family.root, "remove") # A `.gitmodules` the work tree lacks, or has not as the index has it, # is refused first (#407): the commit would take it out of HEAD, or diff --git a/tests/test_family_refuse_up_front.py b/tests/test_family_refuse_up_front.py index 7d264b56..8258af96 100644 --- a/tests/test_family_refuse_up_front.py +++ b/tests/test_family_refuse_up_front.py @@ -535,6 +535,39 @@ def test_a_merge_in_a_linked_worktree_is_found_where_git_keeps_it( r"MERGE_HEAD\. ", line), line +@pytest.mark.skipif(sys.platform == "win32", + reason="Windows refuses a newline in a file name; Linux " + "and APFS take it") +def test_a_holder_path_with_a_newline_still_shows_its_merge( + family, holder, offline, tmp_path): + """V-F1 of #612's verification. A linked worktree's `git rev-parse + --git-path` answers with an ABSOLUTE path through the holder's own + `.git`, so a holder in a directory whose name holds a newline makes every + answer two lines, and `_git_paths` asks each name alone. It once kept + only that answer's FIRST line, which named a directory that does not + exist: no mark was found, `add --dry-run` printed a plan, and the real + `add` mounted and failed at the commit, #612's own defect back for that + path. The whole answer, less git's final newline, is the path.""" + env = isolated(offline, tmp_path) + broken = tmp_path / "Ink\nRouter" + shutil.copytree(holder, broken, symlinks=True) + linked = tmp_path / "linked" + raw(broken, env, "worktree", "add", "-q", "-b", "linked-branch", + str(linked)) + begin("merge", linked, env, tmp_path) + shown = raw(linked, env, "rev-parse", "--git-path", "MERGE_HEAD" + ).stdout.decode() + assert shown.count("\n") == 2, f"premise: a newline in the path: {shown!r}" + for extra in ((), ("--dry-run",)): + ran = add(linked, env, family["remotes"], *extra) + assert ran.returncode == 2, (extra, ran.stdout, ran.stderr) + assert ran.stdout == "" + assert ran.stderr.startswith( + "REFUSED family-operation-in-progress: a merge is unfinished in " + "the family root: git keeps MERGE_HEAD at "), ran.stderr + assert "Ink\nRouter/.git/worktrees/" in ran.stderr, ran.stderr + + def test_with_no_act_and_no_sparse_checkout_each_verb_runs_as_it_always_did( family, holder, offline, tmp_path): """THE CONTROL: no act in progress, no sparse checkout; each verb commits