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 774ff488..1abd0d0d 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 @@ -877,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 99bac4e9..6d0ee8b1 100644 --- a/tests/test_openreposhape_command.py +++ b/tests/test_openreposhape_command.py @@ -1715,32 +1715,166 @@ 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, + 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: 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` + here looks at no repository.""" + fake = where / "fake-path" + fake.mkdir() + log = where / "git-asked" + 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" + '[ "$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 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; " \ + "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.glob("*")) == [] + + +@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") == [] + + +@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 "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):