diff --git a/shape-doctor.py b/shape-doctor.py index 95997b54..6d174aaa 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -6954,14 +6954,44 @@ def submodule_config(root: Path) -> dict: config syntax, it ESCAPES a backslash on the way in -- a Windows path is stored `D:\\\\a\\\\...` -- and reading it as text is how a report ends up quoting a url nobody has. + + AND IT IS READ AS `make siblings` READS IT: a byte that is not UTF-8 is + replaced, which is `siblings.py::submodule_urls`'s `errors="replace"` + over the same file (#432), and the decode `origin_url` makes of the + holder's own remote (#385). git keeps a url as the bytes it was given, + so a member mounted from a directory named in a legacy code page, or a + hand-edited `.gitmodules`, has one that is not text, and read through + `git_out`'s strict decode it raised a `UnicodeDecodeError` that the + `except` below does not name -- it is a `ValueError`, neither a + `Refusal` nor an `OSError` -- so `--root` ended in a traceback, exit 1, + which is also the code of a finding, with no verdict. The bytes are + asked for, so nothing is decoded before this function says how. + + THE URL IS KEPT, NOT DROPPED. The row exists to agree with `make + siblings` about one directory (#146): a clone whose own `origin` is the + same bytes reads back as the same string through `origin_url`, and is + the member's working clone by the mount comparison, as `verify_sibling` + calls it. An empty `{}` here would send every such member's comparison + to the `repository:` tail alone and call that clone a stranger. A url + that matches nothing is quoted with the replacement character in it, as + `origin_url` quotes one. + + `validate-family.py` reads the same file as bytes too, and decodes with + `surrogateescape`. The replacement is chosen over it because this url + is COMPARED with `origin_url`'s reading of a clone's remote, which is + the replacement (#385): the same bytes read two ways are two strings, + so a clone made from this very url would stop being the member's + working clone. It is also what the row PRINTS, and a lone surrogate is + a string no UTF-8 stream can write. """ fields: dict = {} try: - listing = git_out(["config", "-f", str(root / ".gitmodules"), - "--get-regexp", r"^submodule\..*\.(path|url)$"], - cwd=root) + raw = git_out(["config", "-f", str(root / ".gitmodules"), + "--get-regexp", r"^submodule\..*\.(path|url)$"], + cwd=root, binary=True) except (Refusal, OSError): return fields + listing = raw.decode("utf-8", "replace").strip() for line in listing.splitlines(): key, _, value = line.partition(" ") name, _, field = key[len("submodule."):].rpartition(".") diff --git a/tests/test_doctor_gitmodules_not_utf8.py b/tests/test_doctor_gitmodules_not_utf8.py new file mode 100644 index 00000000..4add9472 --- /dev/null +++ b/tests/test_doctor_gitmodules_not_utf8.py @@ -0,0 +1,241 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`shape-doctor.py`'s `members` row reads `.gitmodules` as bytes (#432). + +THE DEFECT. For a family holder the `members` row asks where each member is +mounted from, and the answer is the member's `url` in the holder's +`.gitmodules`, read by `submodule_config` through `git_out`, which decodes +strictly. git keeps a url as the bytes it was given, so a member mounted +from a directory named in a legacy code page (`caf\\xe9`), or a hand-edited +`.gitmodules`, has one that is not UTF-8, and the strict decode raised a +`UnicodeDecodeError`. It is a `ValueError`, neither a `Refusal` nor an +`OSError`, which are the only two the `except` beside it names and the only +two `run_checks` catches. So `--root` ended in a traceback, exit 1 -- also the +code of a finding, so a script could not tell this crash from `INVALID` -- +with no verdict and no rows. + +WHAT IT DOES NOW. The bytes are read and the one that is not UTF-8 is +replaced, as `siblings.py::submodule_urls` reads the same file for `make +siblings` and as `origin_url` reads a remote since #385. The row says what +it found, quoting the url with the replacement character, and the run +reaches the verdict a holder with an ASCII url gets. + +THE FIXTURES ARE `test_shape_doctor`'s, imported rather than rebuilt: a +holder made any other way would be a different one. The `.gitmodules` is +written as BYTES (a Windows command line is text), with forward slashes so +git's escaping of a backslash never changes what is compared. +""" + +from __future__ import annotations + +import json +import re +import shutil +import subprocess + +from pathlib import Path + +import pytest + +from conftest import git, run_script +from test_shape_advisory import with_origin +from test_shape_doctor import ( # noqa: F401 + DOCTOR, FAMILY_MEMBER, FAMILY_NAME, commit_all, doctor, doctor_module, + holder, member_beside, rows_of, siblings, standard, +) + +SECTION = f"members/{FAMILY_MEMBER}" +REPOSITORY = f"{FAMILY_NAME}/{FAMILY_MEMBER}" + +#: The directory the member's mount url runs through, in #432's own three +#: rows: the harness's own bare repository (the control), a directory named +#: in UTF-8 (`café`), and one named in Latin-1 (`caf\xe9`), which is not +#: UTF-8. `None` leaves `.gitmodules` as `family.py add` wrote it. +DIRECTORIES = [ + pytest.param(None, id="ascii-control"), + pytest.param("café".encode("utf-8"), id="utf-8"), + pytest.param(b"caf\xe9", id="not-utf-8"), +] + + +def holder_copy(holder: dict, tmp_path: Path) -> Path: + """A private copy of the session's holder, in a folder of its own.""" + base = tmp_path / "fam" + base.mkdir() + root = base / FAMILY_NAME + shutil.copytree(holder["root"], root, symlinks=True) + return root + + +def mount_through(holder: dict, root: Path, directory: bytes | None) -> bytes: + """`root`'s member mounted from `//IRRS.git`, and + that url as the BYTES now in `.gitmodules`. + + The file's `url` line is rewritten as bytes and committed, so the + working tree is clean as a person's would be. Skipped where git does not + hand the value back as written. No directory is made: the doctor + fetches nothing, so the url need not lead anywhere. + """ + remotes = (holder["base"] / "remotes").as_posix().encode("utf-8") + if directory is None: + return remotes + f"/{FAMILY_MEMBER}.git".encode("ascii") + url = remotes + b"/" + directory + f"/{FAMILY_MEMBER}.git".encode("ascii") + config = root / ".gitmodules" + rewritten, count = re.subn(rb"(?m)^([ \t]*url[ \t]*=[ \t]*)[^\r\n]*", + lambda found: found.group(1) + url, + config.read_bytes()) + assert count == 1, config.read_bytes() + config.write_bytes(rewritten) + read = subprocess.run(["git", "config", "-f", str(config), "--get", + f"submodule.{SECTION}.url"], + capture_output=True, check=False) + if read.stdout.rstrip(b"\r\n") != url: + pytest.skip(f"`git config` here reads the url as {read.stdout!r}, " + f"not {url!r}") + commit_all(root, "Mount the member from another directory") + return url + + +def members_line(text: str) -> str: + """The `members` row of a printed report, as one line.""" + return next(line for line in text.splitlines() + if line.split()[1:2] == ["members"]) + + +@pytest.mark.parametrize("directory", DIRECTORIES) +def test_the_run_reaches_a_verdict_whatever_bytes_the_mount_url_holds( + standard, holder, tmp_path, directory): + """#432's table, row by row, through `--json` and the printed report. + + On the base the third row ended in `UnicodeDecodeError` out of + `submodule_config`, exit 1, and no verdict. Now every row is + `COMPLIANT`, exit 0, the `members` row names the member it found, and + the url it quotes is the one in `.gitmodules` with the byte replaced + where it is not UTF-8. The other two rows are the control: this is + not a test that passes by refusing every url. + """ + root = holder_copy(holder, tmp_path) + url = mount_through(holder, root, directory) + quoted = url.decode("utf-8", "replace") + + result = doctor(standard, root, "--json") + assert "Traceback" not in result.stderr, result.stderr + payload = json.loads(result.stdout) + assert (payload["verdict"], result.returncode) == ("COMPLIANT", 0), \ + payload + row = rows_of(result)["members"] + assert row["status"] == "ok", row + entry = row["detail"]["members"][0] + assert entry["project"] == FAMILY_MEMBER, entry + assert entry["path"] == SECTION, entry + assert entry["mounted_from"].replace("\\", "/") == quoted, entry + assert FAMILY_MEMBER in row["reason"], row["reason"] + + text = doctor(standard, root) + assert "Traceback" not in text.stderr, text.stderr + assert text.returncode == 0, text.stdout + text.stderr + assert text.stdout.rstrip().endswith("COMPLIANT (exit 0)"), text.stdout + assert SECTION in members_line(text.stdout), text.stdout + + +@pytest.mark.parametrize("directory", DIRECTORIES) +def test_submodule_config_keeps_the_url_as_text_it_can_compare( + standard, holder, tmp_path, siblings, directory): + """The unit #432 names: `submodule_config(root)` returns the section. + + The url is kept, as the string `make siblings` reads out of the same + file (`siblings.submodule_urls`), so the two tools hold one answer + about where the member is mounted from, and it is text the row can + compare: a byte that is not UTF-8 is U+FFFD, never dropped, and never + a lone surrogate no UTF-8 stream can write. + """ + root = holder_copy(holder, tmp_path) + url = mount_through(holder, root, directory) + quoted = url.decode("utf-8", "replace") + + config = doctor_module(standard).submodule_config(root) + assert list(config) == [SECTION], config + assert config[SECTION]["path"] == SECTION, config + assert config[SECTION]["url"].replace("\\", "/") == quoted, config + assert ("�" in config[SECTION]["url"]) is (directory == b"caf\xe9") + assert config[SECTION]["url"] == siblings.submodule_urls(root)[SECTION] + assert config[SECTION]["url"].encode("utf-8") + + +def test_a_clone_made_from_the_same_bytes_is_the_working_clone( + standard, holder, tmp_path, siblings): + """THE ROW AGREES WITH `make siblings` ABOUT ONE DIRECTORY (#146). + + A clone beside the holder whose own `origin` is the same bytes as the + member's mount url reads back as the same string through `origin_url` + and through `submodule_config`, so the mount comparison accepts it, as + `verify_sibling` does. A reading that dropped the url and fell back to + the `repository:` guess would call this clone a stranger, and the two + tools would disagree about the directory. + """ + root, sibling = member_beside(holder, tmp_path) + url = mount_through(holder, root, b"caf\xe9") + git("remote", "remove", "origin", cwd=sibling) + with_origin(sibling, url) + + result = doctor(standard, root, "--json") + assert "Traceback" not in result.stderr, result.stderr + assert (json.loads(result.stdout)["verdict"], result.returncode) == \ + ("COMPLIANT", 0), result.stdout + entry = rows_of(result)["members"]["detail"]["members"][0] + assert entry["working_clone"] == sibling.as_posix(), entry + assert entry["other_origin"] is None, entry + + member = siblings.Sibling(FAMILY_MEMBER, sibling) + assert siblings.verify_sibling( + {"project": FAMILY_MEMBER, "id": siblings.project_id(sibling), + "repository": REPOSITORY}, + sibling, siblings.submodule_urls(root)[SECTION], REPOSITORY, + member) is True, member.finding + + +def test_a_clone_from_another_remote_is_still_a_stranger_beside_such_a_mount( + standard, holder, tmp_path, siblings): + """And the other direction, so the test above cannot pass by accepting + every clone: the member's own clone, at the harness's real remote, is + not a clone of the directory the holder now mounts it from, and `make + siblings` calls the same directory `WRONG ORIGIN`. + + The row reports it and never refuses it, as for any stranger (#146): the + verdict is the one a holder with no clone beside it gets, and the printed + row says which origin it found. + """ + root, sibling = member_beside(holder, tmp_path) + mount_through(holder, root, b"caf\xe9") + origin = subprocess.run(["git", "remote", "get-url", "origin"], + cwd=str(sibling), capture_output=True, text=True, + check=True).stdout.strip() + + result = doctor(standard, root, "--json") + assert "Traceback" not in result.stderr, result.stderr + assert (json.loads(result.stdout)["verdict"], result.returncode) == \ + ("COMPLIANT", 0), result.stdout + row = rows_of(result)["members"] + entry = row["detail"]["members"][0] + assert entry["working_clone"] is None, entry + assert entry["other_origin"] == origin, entry + assert row["status"] == "ok", row + assert f"{FAMILY_MEMBER}: {sibling} is a clone of {origin}" in \ + row["reason"], row["reason"] + + member = siblings.Sibling(FAMILY_MEMBER, sibling) + assert siblings.verify_sibling( + {"project": FAMILY_MEMBER, "id": siblings.project_id(sibling), + "repository": REPOSITORY}, + sibling, siblings.submodule_urls(root)[SECTION], REPOSITORY, + member) is False + assert member.state == "WRONG ORIGIN", member.state + + # The url the holder mounts the member from is quoted in the printed + # row, with the replacement printed as `?` (#385), on an ASCII console + # as on any other. + for env in ({}, {"PYTHONIOENCODING": "ascii"}): + text = run_script(standard / DOCTOR, "--root", str(root), env=env) + assert "Traceback" not in text.stderr, text.stderr + assert text.returncode == 0, text.stdout + text.stderr + assert "caf?/IRRS.git, so it is not counted as its working clone" \ + in members_line(text.stdout), text.stdout