shape-doctor.py: a family holder's .gitmodules is read as bytes and a url that is not UTF-8 is replaced, as make siblings reads it, so --root reaches a verdict where a member mounted from a legacy-code-page directory ended in a UnicodeDecodeError traceback, exit 1, with none (#432)

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(".")

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 `<remotes>/<directory>/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