From 3a703d34df4cb2c427f7ad04726201bd2520572f Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 18:12:23 +0000 Subject: [PATCH 1/3] openRepoShape: `--doctor` checks `$OPENREPOSHAPE_REF` before it clones the standard, so a ref `checked_ref` refuses (`a.lock/b`, `a//b`, one over 255 characters) makes no clone and, where the clone fails, is still refused in its own words, and the test that names the order pins it with a `git` that logs what it is asked (#942) `shape_checkout` ran `git clone` and then `checked_ref "$REF"`, so a ref the rule refuses cost a whole clone first, and where the clone failed (offline, a mirror that is down) the run ended in "could not clone", never naming the ref. `checked_ref` reads only the value, and nothing between the two lines needs the clone, so the call now comes first, after `shape_clone_url` and before `workdir`: `[ "$REF_NAMED" -eq 0 ] || checked_ref "$REF"`. It stays in `shape_checkout`, not `refuse_bad_values`, whose check of the ref is its characters only on purpose (`--install` takes `bad..ref` with no opt-in, and `--doctor` beside a checkout never uses the ref). A refused ref now leaves no directory in `$TMPDIR` at all. The comment above the function says the order and why. `test_a_ref_the_doctor_would_check_out_is_validated` named this order in its docstring and did not pin it: it passed with the call above or below the clone, and each of its five cases made a real clone of this checkout. It runs with a `git` first on `$PATH` that logs what it is asked and fails every clone, over ten refs (its five, and `a.lock/b`, `a//b`, `a.`, `a/b/`, `topic.lock`): exit 2 in `ref_fault`'s words for the value, no clone and no checkout asked of git, nothing left in `$TMPDIR`. Two tests are new: a good ref (and no ref) still clones once and checks out once, and a bad ref with `OPENREPOSHAPE_REPO=/nonexistent/...` ends in the ref's refusal, not "could not clone", while a good ref there still meets the clone's. The five-ref test no longer clones: seconds, not 188. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- openRepoShape | 12 ++- tests/test_openreposhape_command.py | 141 ++++++++++++++++++++++++---- 2 files changed, 132 insertions(+), 21 deletions(-) diff --git a/openRepoShape b/openRepoShape index 774ff488..4a4ff0a0 100755 --- a/openRepoShape +++ b/openRepoShape @@ -826,9 +826,18 @@ checked_repo() { # # $OPENREPOSHAPE_REPO HAS PASSED `checked_repo` BEFORE THIS RUNS (#866), as # every run but `--help` checks it once, and nothing between changes it. +# +# THE REF IS CHECKED BEFORE THE CLONE (#942). `checked_ref` reads the value +# and nothing else, so a ref it refuses costs no clone, and a clone that +# fails (offline, a mirror that is down) cannot hide the refusal of a ref +# that was never going to be checked out. It is asked here and not by +# `refuse_bad_values`, whose check of the ref is its characters only, on +# purpose: `--install` takes `bad..ref` with no opt-in, and `--doctor` beside +# a checkout never uses the ref at all. shape_checkout() { local url url="$(shape_clone_url)" + [ "$REF_NAMED" -eq 0 ] || checked_ref "$REF" workdir openRepoShape-doctor SHAPE_DIR="$WORKDIR/shape" git clone --quiet -- "$url" "$SHAPE_DIR" || @@ -836,12 +845,11 @@ shape_checkout() { standard to check against. \`--doctor\` needs one: run it from a checkout of openRepoShape, or make the clone work." if [ "$REF_NAMED" -eq 1 ]; then - checked_ref "$REF" # ` --`, never `-- `: a `--` BEFORE the argument makes git # read it as a PATHSPEC and try to restore a file of that name from # the index. AFTER it, it says "and no paths follow", which is the # disambiguation this wants. The leading-dash case is refused by - # `checked_ref` above, which is where it belongs. + # `checked_ref` above, before the clone, which is where it belongs. git -C "$SHAPE_DIR" checkout --quiet "$REF" -- || die "cloned $REPO but could not check out '$REF'." fi diff --git a/tests/test_openreposhape_command.py b/tests/test_openreposhape_command.py index 99bac4e9..82883af6 100644 --- a/tests/test_openreposhape_command.py +++ b/tests/test_openreposhape_command.py @@ -1715,32 +1715,135 @@ def test_two_modes_is_a_refusal_not_last_wins(args, tmp_path): assert not bin_dir.exists(), "a refused run placed a command anyway" -@pytest.mark.parametrize("ref", ["-x", "--upload-pack=touch /tmp/pwned", - "a" * 256, "refs/heads/..", "x.lock"]) +def doctor_from_stdin(where: Path, repo: str, ref: str | None = None, + clone_fails: bool = True): + """`--doctor` run from STDIN, so the clone path is the one taken, with a + `git` first on `$PATH` that logs what it is asked and clones nothing: + the clone fails, or (`clone_fails=False`) makes the empty directory it + is handed, which is a standard with no doctor in it. Returns the run, + the lines the `git` logged (each without a leading `-C `, so the + subcommand opens it) and the `$TMPDIR` the run was given. + + Each of these runs made a real clone of this checkout until #942, and + five of them took 188 s under load. `repo` is never reached: the `git` + here looks at no repository.""" + fake = where / "fake-path" + fake.mkdir() + log = where / "git-asked" + temporary = where / "tmp" + temporary.mkdir() + write_script(fake / "git", "#!/bin/sh\n" + 'case "$1" in -C) shift 2 ;; esac\n' + f"printf '%s\\n' \"$*\" >> '{bash_path(log)}'\n" + '[ "$1" = clone ] || exit 0\n' + + ("exit 1\n" if clone_fails else 'mkdir -p "$5"\n')) + (fake / "git").chmod(0o755) + env = {"OPENREPOSHAPE_REPO": repo, "TMPDIR": str(temporary), + "PATH": f"{fake}{os.pathsep}{os.environ['PATH']}"} + if ref is not None: + env["OPENREPOSHAPE_REF"] = ref + result = subprocess.run( + ["bash", "-s", "--", "--doctor", str(where)], executable=BASH, + capture_output=True, text=True, check=False, + input=COMMAND.read_text(encoding="utf-8"), cwd=str(where), + env=command_env(home=where, setup_sh=None, env=env)) + asked = log.read_text(encoding="utf-8").splitlines() \ + if log.exists() else [] + return result, asked, temporary + + +def asked_of_git(asked: list, subcommand: str) -> list: + """The logged calls of one subcommand of the `git` that + `doctor_from_stdin` put first on `$PATH`.""" + return [call for call in asked if call.split()[:1] == [subcommand]] + + +def assert_the_ref_was_refused_before_any_clone(result, asked, temporary, + ref: str) -> None: + """Exit 2 in `ref_fault`'s words for `ref`, no clone and no checkout + asked of git, and nothing in `$TMPDIR`: the ref was refused first (#942). + The over-long and control-character refusals do not echo the value back; + the shape refusal does, having passed both.""" + assert result.returncode == 2, result.stdout + result.stderr + if len(ref) > 255: + assert f"REFUSED: $OPENREPOSHAPE_REF is {len(ref)} characters long; " \ + "a branch, tag or commit this tool will pass to `git checkout` " \ + "is at most 255 characters." in result.stderr + assert ref not in result.stderr + else: + assert_the_refusal_states_the_ref_rule( + result.stderr, + f"REFUSED: $OPENREPOSHAPE_REF is '{ref}', which is not a " + "branch, tag or commit this tool will pass to `git checkout`: ") + assert "could not clone" not in result.stderr + assert asked_of_git(asked, "clone") == [], ( + "a refused ref was asked of a clone first") + assert asked_of_git(asked, "checkout") == [] + assert list(temporary.iterdir()) == [] + + +@pytest.mark.parametrize("ref", [ + "-x", "--upload-pack=touch /tmp/pwned", + pytest.param("a" * 256, id="a-x256"), "refs/heads/..", "x.lock", + "a.lock/b", "a//b", "a.", "a/b/", "topic.lock"]) def test_a_ref_the_doctor_would_check_out_is_validated(ref, tmp_path): - """$OPENREPOSHAPE_REF reaches `git checkout`, so it is checked first. + """$OPENREPOSHAPE_REF reaches `git checkout`, so it is checked first, + and before the clone (#942). `setup.sh` validates `--shape-ref` in three steps and this value lands in the same place, which the first cut of `--doctor` missed (Copilot, PR #96): a leading dash is an option to git, not a ref. Run from STDIN - so the clone path is the one taken: the command clones this checkout - first, then checks the ref, and refuses before `git checkout` is ever - handed it. + so the clone path is the one taken, with a `git` that logs what it is + asked: the command checks the ref first, and refuses before any clone, + so git is asked for no clone and no checkout, and what the person reads + is `ref_fault`'s sentence for that value. The check came after the + clone until #942, and each of these then made a whole clone first. """ - result = subprocess.run( - ["bash", "-s", "--", "--doctor", str(tmp_path)], executable=BASH, - capture_output=True, text=True, check=False, - input=COMMAND.read_text(encoding="utf-8"), cwd=str(tmp_path), - env=command_env(home=tmp_path, setup_sh=None, - env={"OPENREPOSHAPE_REPO": bash_path(REPO), - "OPENREPOSHAPE_REF": ref})) + result, asked, temporary = doctor_from_stdin(tmp_path, "acme/x", ref) + assert_the_ref_was_refused_before_any_clone(result, asked, temporary, ref) + + +@pytest.mark.parametrize("ref", [None, "main", "v1.2.0", + "release/v1.2.0-rc_1"]) +def test_a_ref_the_doctor_takes_still_clones_once_and_checks_out_once( + ref, tmp_path): + """Checking the ref first leaves a good one where it was (#942): one + clone of the standard, then one `git checkout` of the ref, and the + doctor's own refusal that this stand-in standard has no doctor, which + is as far as the run goes. No ref named is no checkout at all: the + clone's default branch is the standard.""" + result, asked, _temporary = doctor_from_stdin( + tmp_path, "acme/x", ref, clone_fails=False) assert result.returncode == 2, result.stdout + result.stderr - assert "OPENREPOSHAPE_REF" in result.stderr - assert "git checkout" in result.stderr - # The over-long and control-character refusals must not echo the value - # back; the shape refusal may, having passed both. - if len(ref) > 255: - assert ref not in result.stderr + assert "has no shape-doctor.py" in result.stderr + assert "which is not a branch, tag or commit" not in result.stderr + clones = asked_of_git(asked, "clone") + assert len(clones) == 1, asked + assert clones[0].startswith( + "clone --quiet -- https://github.com/acme/x.git "), clones + expected = [] if ref is None else [f"checkout --quiet {ref} --"] + assert asked_of_git(asked, "checkout") == expected + + +def test_a_ref_is_refused_where_the_clone_would_have_failed(tmp_path): + """OFFLINE, OR WITH A MIRROR THAT IS DOWN, a person who typed a bad ref + was told to make the clone work and told the ref was bad once it did + (#942). The ref is refused in its own words now, with no clone asked + for, and the clone's own refusal is still what a good ref meets.""" + gone = "/nonexistent/openRepoShape.git" + bad = tmp_path / "bad" + bad.mkdir() + assert_the_ref_was_refused_before_any_clone( + *doctor_from_stdin(bad, gone, "a.lock/b"), "a.lock/b") + + good = tmp_path / "good" + good.mkdir() + result, asked, _temporary = doctor_from_stdin(good, gone, "v1.2.0") + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED: could not clone {gone} into a temporary directory" \ + in result.stderr + assert len(asked_of_git(asked, "clone")) == 1, asked + assert asked_of_git(asked, "checkout") == [] def test_a_checkout_without_the_doctor_refuses_by_name(tmp_path): From d5d2152caf0b5dc260a98e488f006e0bf322052a Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 18:50:38 +0000 Subject: [PATCH 2/3] tests/test_openreposhape_command.py, openRepoShape and tests.yml: a refused `$OPENREPOSHAPE_REF` is held to be refused above `workdir`, so it is named even where `$TMPDIR` is not there; the Windows job's `fetch-depth: 0` comment no longer says a test clones this checkout; two comments say the ref is refused before the clone (#942) Review round 1 on #967 (rv967's READY, two SHOULDs and two NITs). SHOULD 1. Nothing held `checked_ref` above `workdir`: swapped below it, all 457 tests of the file still passed, because the clone count stays 0 and the EXIT trap removes the empty folder. With `$TMPDIR` missing the swap, like the old code, ends in "could not make a temporary directory" and never names the ref. `doctor_from_stdin` takes the `$TMPDIR` it hands the run, and a new test runs `a.lock/b` and 256 `a` against one that does not exist: exit 2 in `ref_fault`'s words, git asked nothing, no "could not make a temporary directory", and the folder still not made; a good ref there still ends in that refusal. It fails on the swap and on the launcher at `a4de0aae`, and passes at the head. The helper looks for what is in `$TMPDIR` with `glob`, which a missing folder does not break. The first commit's message said a refused ref "leaves no directory in `$TMPDIR` at all", which was true before too: the trap removed the clone. What changed is that a refused ref makes none. SHOULD 2. The Windows job's comment said `fetch-depth: 0` was there because the launcher's suite clones this checkout itself, in the test this change makes run a fake `git`. None of its three files (the launcher's, `test_install_sh.py`, `test_install_sh_filter_repo.py`) clones it or reads its history now. The comment says so, as the ubuntu job has the setting, and the setting stays: a shallow Windows checkout is a change of its own and wants a Windows run behind it. NIT 1. The helper's docstring named a control-character refusal it does not handle, and none reaches `shape_checkout` (`refuse_bad_values` refuses it first). NIT 2. `ref_fault`'s comment says the doctor's ref is refused before its clone, not before `git checkout`. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/tests.yml | 8 ++--- openRepoShape | 2 +- tests/test_openreposhape_command.py | 45 ++++++++++++++++++++++++----- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 9c090853..a9226d3a 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -209,10 +209,10 @@ jobs: GIT_CONFIG_KEY_1: core.longpaths GIT_CONFIG_VALUE_1: 'true' steps: - # The SAME two commits the ubuntu job pins. `fetch-depth: 0` because - # the launcher's suite clones this checkout itself - # (`OPENREPOSHAPE_REPO` in `test_a_ref_the_doctor_would_check_out_is_ - # validated`), and every Windows run of it has cloned a full one. + # The SAME two commits the ubuntu job pins. `fetch-depth: 0`, as the + # ubuntu job has it: no test in these three files has cloned this + # checkout since #942. A shallow checkout here is a change of its own, + # and wants a Windows run behind it. - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 with: fetch-depth: 0 diff --git a/openRepoShape b/openRepoShape index 4a4ff0a0..1abd0d0d 100755 --- a/openRepoShape +++ b/openRepoShape @@ -885,7 +885,7 @@ shape_checkout() { # name alone. It is asked where a ref is USED, and each use is a gate: a # record it refuses is refused before any request (`read_recorded_ref`), a # ref an install would record is refused before anything is placed, and the -# doctor's checkout is refused before `git checkout`. A ref that passes here +# doctor's ref is refused before its clone. A ref that passes here # is one `setup.sh` is handed, by name, on its way to `git clone`. ref_fault() { local ref="$1" what="$2" fault diff --git a/tests/test_openreposhape_command.py b/tests/test_openreposhape_command.py index 82883af6..0f69dd8e 100644 --- a/tests/test_openreposhape_command.py +++ b/tests/test_openreposhape_command.py @@ -1716,13 +1716,15 @@ def test_two_modes_is_a_refusal_not_last_wins(args, tmp_path): def doctor_from_stdin(where: Path, repo: str, ref: str | None = None, - clone_fails: bool = True): + clone_fails: bool = True, + temporary: Path | None = None): """`--doctor` run from STDIN, so the clone path is the one taken, with a `git` first on `$PATH` that logs what it is asked and clones nothing: the clone fails, or (`clone_fails=False`) makes the empty directory it is handed, which is a standard with no doctor in it. Returns the run, the lines the `git` logged (each without a leading `-C `, so the - subcommand opens it) and the `$TMPDIR` the run was given. + subcommand opens it) and the `$TMPDIR` the run was given: a folder made + here, or the path `temporary` names, made or not. Each of these runs made a real clone of this checkout until #942, and five of them took 188 s under load. `repo` is never reached: the `git` @@ -1730,8 +1732,9 @@ def doctor_from_stdin(where: Path, repo: str, ref: str | None = None, fake = where / "fake-path" fake.mkdir() log = where / "git-asked" - temporary = where / "tmp" - temporary.mkdir() + if temporary is None: + temporary = where / "tmp" + temporary.mkdir() write_script(fake / "git", "#!/bin/sh\n" 'case "$1" in -C) shift 2 ;; esac\n' f"printf '%s\\n' \"$*\" >> '{bash_path(log)}'\n" @@ -1762,8 +1765,8 @@ def assert_the_ref_was_refused_before_any_clone(result, asked, temporary, ref: str) -> None: """Exit 2 in `ref_fault`'s words for `ref`, no clone and no checkout asked of git, and nothing in `$TMPDIR`: the ref was refused first (#942). - The over-long and control-character refusals do not echo the value back; - the shape refusal does, having passed both.""" + The over-long refusal does not echo the value back; the shape refusal + does, having passed the length and character checks.""" assert result.returncode == 2, result.stdout + result.stderr if len(ref) > 255: assert f"REFUSED: $OPENREPOSHAPE_REF is {len(ref)} characters long; " \ @@ -1779,7 +1782,7 @@ def assert_the_ref_was_refused_before_any_clone(result, asked, temporary, assert asked_of_git(asked, "clone") == [], ( "a refused ref was asked of a clone first") assert asked_of_git(asked, "checkout") == [] - assert list(temporary.iterdir()) == [] + assert list(temporary.glob("*")) == [] @pytest.mark.parametrize("ref", [ @@ -1846,6 +1849,34 @@ def test_a_ref_is_refused_where_the_clone_would_have_failed(tmp_path): assert asked_of_git(asked, "checkout") == [] +@pytest.mark.parametrize("ref, refused", [ + pytest.param("a.lock/b", True, id="a.lock-b"), + pytest.param("a" * 256, True, id="a-x256"), + pytest.param("v1.2.0", False, id="a-good-ref")]) +def test_a_ref_is_refused_before_a_directory_is_made_for_the_clone( + ref, refused, tmp_path): + """A REFUSED REF MAKES NO DIRECTORY (#942). `workdir` makes the folder + the clone goes into, under `$TMPDIR`, and refuses where it cannot. With + the check below it, a bad ref and a `$TMPDIR` that is not there ended in + "could not make a temporary directory", and the ref was never named. + The ref is refused in its own words now, with git asked nothing, and + the `$TMPDIR` is still not there; a good ref still meets the + `$TMPDIR` refusal, with git asked nothing as well.""" + missing = tmp_path / "not-there" + result, asked, temporary = doctor_from_stdin( + tmp_path, "acme/x", ref, temporary=missing) + assert asked == [] + assert not missing.exists() + if refused: + assert_the_ref_was_refused_before_any_clone( + result, asked, temporary, ref) + assert "could not make a temporary directory" not in result.stderr + return + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED: could not make a temporary directory under {missing}." \ + in result.stderr + + def test_a_checkout_without_the_doctor_refuses_by_name(tmp_path): """A ref older than `shape-doctor.py` produced `can't open file` from python3 and a stack of nothing useful. The ref is the thing to change, From 4f83eaf53b0e77ee1f60ff69fe1f6cddb6698362 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 20:10:56 +0000 Subject: [PATCH 3/3] tests/test_openreposhape_command.py: the good-ref row of the missing-`$TMPDIR` test expects the folder as bash spells it, so it passes under Git Bash as well as on Linux (#942) `test_a_ref_is_refused_before_a_directory_is_made_for_the_clone[a-good-ref]` expected "could not make a temporary directory under {missing}." with the native path, and the launcher prints `$TMPDIR` as bash spells it: under Git Bash `/d/a/.../not-there`, where the native one is `D:\a\...`. The `tests-git-bash` job failed that one row (1 failed, 337 passed, 410 skipped at d5d2152c). The expected text goes through `bash_path`, which returns the path unchanged off Windows. No other expected text in the file's new tests holds a native path: the others name a ref, a URL, a fixed word or a `git` subcommand. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_openreposhape_command.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_openreposhape_command.py b/tests/test_openreposhape_command.py index 0f69dd8e..6d0ee8b1 100644 --- a/tests/test_openreposhape_command.py +++ b/tests/test_openreposhape_command.py @@ -1873,8 +1873,8 @@ def test_a_ref_is_refused_before_a_directory_is_made_for_the_clone( assert "could not make a temporary directory" not in result.stderr return assert result.returncode == 2, result.stdout + result.stderr - assert f"REFUSED: could not make a temporary directory under {missing}." \ - in result.stderr + assert "REFUSED: could not make a temporary directory under " \ + f"{bash_path(missing)}." in result.stderr def test_a_checkout_without_the_doctor_refuses_by_name(tmp_path):