diff --git a/scripts/shape_advisory.py b/scripts/shape_advisory.py index 2486f21e..64a489e9 100644 --- a/scripts/shape_advisory.py +++ b/scripts/shape_advisory.py @@ -433,12 +433,28 @@ def repository_names(root: Path) -> list[str]: `origin` is asked only when `root` ITSELF carries a `.git`: git walks up to an ancestor otherwise, and an ancestor's name is not this directory's -- the trap `shape-doctor.py`'s `repo_local_origin_name` guards against. + + AN ORIGIN URL THAT IS NOT UTF-8 NAMES NOTHING, as no origin names nothing + (#385). git keeps a URL as the bytes it was given, so a clone of a + directory named in a legacy code page has one, and nobody did anything + wrong. Read strictly, it raised `UnicodeDecodeError` into `read()`'s net, + which answered `NOT_ANSWERED`: a single repository lost the advisory, and + a leg clone its instruction, even one whose own `AGENTS.md` says what it + is without any URL. NOT EVEN THE LAST SEGMENT IS SALVAGED. In a + double-byte code page (Shift_JIS, GBK, Big5) the byte `\\` is the second + half of many characters, and `repository_name` splits on `\\`, so a + segment read out of such a URL can be a fragment of a name. It is also + the answer `adopt-project.py`'s `_remote_repository` gives the same URL + (#292), and `shape-doctor.py`'s `origin_name` gives it too, so the + `naming` row and the `Triad` row read one identity. The directory's own + name, the leg's `AGENTS.md` and the person's `workspace.yaml` still + answer. """ names = [root.name] if (root / ".git").exists(): try: url = git_out(["remote", "get-url", "origin"], cwd=root) - except (Refusal, OSError): + except (Refusal, OSError, UnicodeDecodeError): url = "" name = repository_name(url) if url else None if name and name not in names: diff --git a/shape-doctor.py b/shape-doctor.py index d4377ea6..7bcc8109 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -4054,13 +4054,27 @@ def origin_url(path: Path) -> str: BOTH THE SIBLING AND THE HOLDER are asked this: the holder's own remote is what a relative submodule url is resolved against. + + AND IT IS READ AS `make siblings` READS IT: a byte that is not UTF-8 is + replaced, which is `siblings.py::git_text`'s `errors="replace"` (#385). + git keeps a URL as the bytes it was given, so a clone of a mirror under + a directory named in a legacy code page has one, and read strictly it + ended this report in a `UnicodeDecodeError` traceback. It is NOT the + empty string: `verify_sibling` accepts `/caf\\ufffd/Org/Repo.git` + as the member's clone by the row's `repository: Org/Repo` tail, so + `""` here would call the clone `make siblings` fetches into `(no + origin)` and no working clone -- the two tools disagreeing about one + directory, which `working_clone` exists to prevent (#146). A URL that + matches nothing is quoted with the replacement, and a row prints it as + `?`. """ if not (path / ".git").exists(): return "" try: - return git_out(["remote", "get-url", "origin"], cwd=path) + raw = git_out(["remote", "get-url", "origin"], cwd=path, binary=True) except (Refusal, OSError): return "" + return raw.decode("utf-8", "replace").strip() def submodule_config(root: Path) -> dict: @@ -4550,10 +4564,21 @@ def origin_name(root: Path) -> str | None: containing a second colon of its own: `D` in `D:/a/.../Atlas.git` is a PATH, never a host, however this function is later changed to use what comes after it. + + AND A URL THAT IS NOT UTF-8 NAMES NOTHING, as no origin names nothing + (#385). git keeps a URL as the bytes it was given -- a clone of a + directory named in a legacy code page has one -- and read strictly it + ended this report in a `UnicodeDecodeError` traceback, exit 1, from the + `naming` row, where the same repository with no origin ends NOT A SHAPE + ROOT, exit 2. No segment of it is salvaged: in a double-byte code page + the byte `\\` is the second half of many characters, so what the rules + above split off could be a fragment of a name. It is the answer + `shape_advisory.repository_names` gives the same URL, so the `naming` + row, `the way in` and the `Triad` row read one identity. """ try: url = git_out(["remote", "get-url", "origin"], cwd=root) - except (Refusal, OSError): + except (Refusal, OSError, UnicodeDecodeError): return None if _WINDOWS_PATH_RE.match(url): url = url.replace("\\", "/") diff --git a/tests/test_shape_advisory.py b/tests/test_shape_advisory.py index 64183f99..b68229b4 100644 --- a/tests/test_shape_advisory.py +++ b/tests/test_shape_advisory.py @@ -578,6 +578,142 @@ def test_a_workspace_path_no_machine_can_hold_fails_nothing(tmp_path, assert "This is a single repository." in result.stdout +#: The start of an `origin` URL holding the byte 0xE9, as a clone of a +#: directory named `café` in Latin-1 has one (#385): git keeps a URL as the +#: bytes it was given, and nobody did anything wrong. Relative, so no host +#: path goes into `.git/config`, where a Windows backslash is an escape. +NOT_UTF8 = b"../caf\xe9/" + + +def with_origin(here: Path, url: bytes) -> Path: + """`here`, its `origin` set to `url`. + + Written into `.git/config` as bytes, as #292's + `tests/test_adopt_git_reads.py` writes it, so the byte reaches git as it + is on every platform (a Windows command line is text). Skipped where + `git remote get-url origin` does not give it back as written. + """ + config = here / ".git" / "config" + config.write_bytes(config.read_bytes() + + b'[remote "origin"]\n\turl = ' + url + b"\n") + read = subprocess.run(["git", "remote", "get-url", "origin"], + cwd=str(here), capture_output=True, check=False) + if read.stdout.rstrip(b"\r\n") != url: + pytest.skip(f"`git remote get-url origin` here is {read.stdout!r}, " + f"not {url!r}") + return here + + +def doctor_text(here: Path, env: dict) -> subprocess.CompletedProcess: + """The printed report on an ASCII console, read for a traceback.""" + result = run_script(DOCTOR, "--root", str(here), + env={**env, "PYTHONIOENCODING": "ascii"}) + assert no_traceback(result), result.stdout + result.stderr + return result + + +def test_a_single_repository_whose_origin_is_not_utf8_hears_it(tmp_path, + quiet_env): + """`advise`, and every row the same repository with NO origin gets. + + On the base `repository_names` raised `UnicodeDecodeError` into + `read()`'s net, which answered `not-answered` and said nothing, and + `shape-doctor.py --root` ended in that traceback, exit 1, from the + `naming` row's `origin_name`. Such a URL names nothing, as no origin + names nothing, so the twin here is the same repository under another + parent with no `origin` at all, and the two reports must agree row for + row: the verdict, the advisory, the names classified and the project + `the way in` suggests. + """ + here = with_origin(repository(tmp_path / "a" / "Thing"), + NOT_UTF8 + b"repo.git") + twin = repository(tmp_path / "b" / "Thing") + reading = shape_advisory.read(here, env=quiet_env) + assert reading.state == shape_advisory.ADVISE, reading.as_dict() + assert shape_advisory.repository_names(here) == ["Thing"] + + code, rows, payload = doctor_json(here, quiet_env) + twin_code, twin_rows, twin_payload = doctor_json(twin, quiet_env) + assert (code, payload["verdict"]) == (2, "NOT A SHAPE ROOT"), payload + assert (twin_code, twin_payload["verdict"]) == (code, payload["verdict"]) + assert rows[ROW]["status"] == "advisory", rows[ROW] + for row in (ROW, "naming", "way-in"): + assert rows[row]["status"] == twin_rows[row]["status"], row + assert rows[row]["detail"] == twin_rows[row]["detail"], row + assert rows["naming"]["detail"]["origin"] is None, rows["naming"] + + text = doctor_text(here, quiet_env) + assert text.returncode == 2, text.stdout + text.stderr + assert "advisory Triad" in text.stdout, text.stdout + assert "NOT A SHAPE ROOT" in text.stdout, text.stdout + + +@pytest.mark.parametrize("how", ["agents", "name"]) +def test_a_leg_clone_whose_origin_is_not_utf8_is_still_sent_to_its_root( + tmp_path, quiet_env, how): + """By its own `AGENTS.md`, or by its directory's name: `leg-clone`. + + Neither route needs the URL: the leg's `AGENTS.md` is read from its own + tree and its name from its directory. On the base neither ran, because + `repository_names` raised before them, so the advisory said nothing and + the leg was not sent to its root; and `--root` ended in the traceback. + """ + if how == "agents": + here = repository(tmp_path / "renamed-clone", + {"AGENTS.md": rendered_leg_agents("spec")}) + said = "this is a clone of the spec leg of a Triad" + else: + here = repository(tmp_path / "Northwind-spec") + said = "`Northwind-spec` is named as the spec leg of `Northwind`" + with_origin(here, NOT_UTF8 + b"Northwind-spec.git") + reading = shape_advisory.read(here, env=quiet_env) + assert reading.state == shape_advisory.LEG_CLONE, reading.as_dict() + assert reading.leg["by"] == how, reading.leg + + code, rows, payload = doctor_json(here, quiet_env) + assert (code, payload["verdict"]) == (2, "NOT A SHAPE ROOT"), payload + row = rows[ROW] + assert row["status"] == "n/a", row + assert said in row["reason"], row["reason"] + assert rows["way-in"]["next"] is None, rows["way-in"] + text = doctor_text(here, quiet_env) + assert text.returncode == 2, text.stdout + text.stderr + assert "NOT A SHAPE ROOT" in text.stdout, text.stdout + + +@pytest.mark.parametrize("directory, names_it", [ + (b"cafe/", True), ("café/".encode("utf-8"), True), (NOT_UTF8[3:], False), +], ids=["ascii", "utf-8", "latin-1"]) +def test_only_an_origin_that_is_text_names_the_repository( + tmp_path, quiet_env, directory, names_it): + """A clone in a directory named for nothing, its URL ending in a leg's + name: read under an ASCII or a UTF-8 directory, and not under Latin-1. + + THE ISSUE LEFT THIS TO ITS AUTHOR, and this pins the choice: a URL that + is not UTF-8 names nothing, NOT EVEN ITS LAST SEGMENT. In a double-byte + code page (Shift_JIS, GBK, Big5) the byte `\\` is the second half of many + characters, and `repository_name` splits on `\\`, so a segment read out + of such a URL can be a fragment of a name. It is what + `adopt-project.py`'s `_remote_repository` (#292) and the doctor's + `origin_name` read too. The clone is then what one with no origin is: a + single repository, advised, and the doctor's `naming` row classifies its + directory's name alone. + """ + here = with_origin(repository(tmp_path / "clone"), + b"../" + directory + b"Northwind-spec.git") + names = ["clone", "Northwind-spec"] if names_it else ["clone"] + assert shape_advisory.repository_names(here) == names + reading = shape_advisory.read(here, env=quiet_env) + if names_it: + assert reading.state == shape_advisory.LEG_CLONE, reading.as_dict() + assert reading.leg["by"] == "name", reading.leg + else: + assert reading.state == shape_advisory.ADVISE, reading.as_dict() + code, rows, _payload = doctor_json(here, quiet_env) + assert code == 2 + assert rows["naming"]["detail"]["names"] == names, rows["naming"] + + def test_a_byte_order_mark_is_the_editors_and_not_the_persons(tmp_path, quiet_env): """A UTF-8 BOM ahead of the first key changes no answer. diff --git a/tests/test_shape_doctor.py b/tests/test_shape_doctor.py index d23c8bef..080bf5ca 100644 --- a/tests/test_shape_doctor.py +++ b/tests/test_shape_doctor.py @@ -58,6 +58,10 @@ #: rewrite, imported from the module that owns its tests for the same reason: #: a test that retyped the form would pass against a line nothing renders. from test_update_shape import rendered_shape_line +#: An `origin` written as BYTES git hands back unchanged, and the start of a +#: URL holding the byte 0xE9 (#385), from the file that owns the advisory's +#: half of that issue, so the two halves write the same URL the same way. +from test_shape_advisory import NOT_UTF8, with_origin DOCTOR = "shape-doctor.py" ORG = "testorg" @@ -2796,6 +2800,107 @@ def test_a_directory_that_is_not_a_clone_at_all_is_not_called_a_stranger( assert row["status"] == "ok", row +@pytest.mark.parametrize("tail, counted", [ + (f"{FAMILY_NAME}/{FAMILY_MEMBER}.git", True), + ("Someone/Fork.git", False), +], ids=["the-members-mirror", "a-stranger"]) +def test_a_sibling_origin_not_utf8_is_read_as_make_siblings_reads_it( + standard, holder, tmp_path, tail, counted): + """#385, through `--root`: a holder whose member's clone beside it has an + `origin` that is not UTF-8. + + On the base the `members` row's `origin_url` raised `UnicodeDecodeError` + and the report ended in a traceback, exit 1. Now the URL is read with + the byte replaced, as `make siblings` reads it: a mirror whose tail is + the row's `repository:` IS the working clone, and a stranger's is + reported with the replacement, printed as `?`, and the verdict is the + one a holder with no clone beside it gets. + """ + root, sibling = member_beside(holder, tmp_path) + git("remote", "remove", "origin", cwd=sibling) + url = NOT_UTF8 + tail.encode("ascii") + with_origin(sibling, url) + 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 + entry = rows_of(result)["members"]["detail"]["members"][0] + if counted: + assert entry["working_clone"] == sibling.as_posix(), entry + assert entry["other_origin"] is None, entry + else: + assert entry["working_clone"] is None, entry + assert entry["other_origin"] == url.decode("utf-8", "replace"), entry + text = doctor(standard, root) + assert text.returncode == 0, text.stdout + text.stderr + assert "Traceback" not in text.stderr, text.stderr + assert ("is a clone of ../caf?/Someone/Fork.git" in text.stdout) \ + is not counted, text.stdout + + +@pytest.mark.parametrize("relative", [False, True], + ids=["an-absolute-mount-url", "a-relative-mount-url"]) +def test_a_holders_own_origin_not_utf8_is_read_as_make_siblings_reads_it( + standard, holder, tmp_path, relative): + """#385, the OTHER route into `origin_url`: the HOLDER'S own `origin`. + + The issue names two callers of `origin_url`. `working_clone` asks it of + the SIBLING, which the test above pins. `mounted_from` asks it of the + HOLDER, to resolve a relative `.gitmodules` url against, and it asks + whatever the url is: the call is an argument of `resolved_remote`, which + is handed an absolute url too and gives it straight back. So a holder + cloned from a mirror under a directory named in Latin-1 reached the + traceback through `mounted_from` with an ASCII `.gitmodules` as well. + + On the base both cases raised `UnicodeDecodeError` out of + `origin_url`, called from `mounted_from` (the `members` row's own + reader, before `working_clone` asks anything). Now the holder's URL is + read with the byte replaced, as `make siblings` reads it, and: + - an absolute mount url never needed it, so the row answers exactly as + it does for an ASCII holder, and + - a relative one is resolved against it, as git does, and the clone + made from that resolved url is counted as the working clone. The + `mounted_from` the row prints carries U+FFFD. + The relative case is the one a lossy read could get WRONG: reading the + holder's URL as `""` leaves `../IRRS.git` unresolved, which matches no + sibling, so the member's clone is not counted. + """ + root, sibling = member_beside(holder, tmp_path) + git("remote", "remove", "origin", cwd=root) + with_origin(root, NOT_UTF8 + f"{FAMILY_NAME}.git".encode("ascii")) + if relative: + git("config", "-f", str(root / ".gitmodules"), + f"submodule.members/{FAMILY_MEMBER}.url", + f"../{FAMILY_MEMBER}.git", cwd=root) + commit_all(root, "Mount the member by a relative url") + # The clone git would have made from the url that resolves to: the + # holder's own directory, `caf\xe9`, with the member's name in it. + git("remote", "remove", "origin", cwd=sibling) + with_origin(sibling, NOT_UTF8 + f"{FAMILY_MEMBER}.git".encode("ascii")) + 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 + entry = rows_of(result)["members"]["detail"]["members"][0] + assert entry["working_clone"] == sibling.as_posix(), entry + assert entry["other_origin"] is None, entry + if relative: + assert entry["mounted_from"] == f"../caf\ufffd/{FAMILY_MEMBER}.git", \ + entry + else: + # The bare repository the holder's `.gitmodules` names, as the + # control above compares it: separators folded, since git ESCAPES a + # Windows backslash on the way into the file. + assert entry["mounted_from"].replace("\\", "/") == str( + holder["base"] / "remotes" / f"{FAMILY_MEMBER}.git").replace( + "\\", "/"), entry + text = doctor(standard, root) + assert text.returncode == 0, text.stdout + text.stderr + assert "Traceback" not in text.stderr, text.stderr + + def test_a_credential_in_an_origin_is_not_printed_or_put_in_the_json( standard, holder, tmp_path): """A REPORT PRINTS WHAT IT FINDS, AND WHAT IT FINDS CAN BE A TOKEN. @@ -3349,6 +3454,51 @@ def test_working_clone_and_verify_sibling_ACCEPT_a_symlinked_mirror( assert stranger.state == "WRONG ORIGIN", what +def test_working_clone_and_verify_sibling_agree_on_a_non_utf8_origin( + tmp_path, siblings): + """#385: AN ORIGIN THAT IS NOT UTF-8, ASKED OF BOTH CALLERS. + + `siblings.py::git_text` reads `origin` with `errors="replace"`, so a + clone of a mirror under a directory named in Latin-1 is `make siblings`' + member clone by the row's `repository:` tail, and a clone of something + else under it is `WRONG ORIGIN`. On the base `working_clone` raised + `UnicodeDecodeError` for both. The issue's draft would have read the URL + as `""`, which calls the first one `(no origin)` and no working clone: + the two tools disagreeing about one directory, which #146 removed. So + the doctor reads it as `make siblings` does, and quotes the stranger's + URL with the replacement character in it. + """ + spec = importlib.util.spec_from_file_location( + "shape_doctor_not_utf8_origin", REPO / DOCTOR) + doctor = importlib.util.module_from_spec(spec) + spec.loader.exec_module(doctor) + row = {"project": FAMILY_MEMBER, "id": "7c9f1b2d", + "repository": f"{FAMILY_NAME}/{FAMILY_MEMBER}"} + mount = str(tmp_path / "elsewhere" / f"{FAMILY_MEMBER}.git") + cases = [ + ("a mirror whose tail is the row's", + f"{FAMILY_NAME}/{FAMILY_MEMBER}.git".encode("ascii"), True), + ("a stranger", b"Someone/Fork.git", False), + ] + for index, (what, tail, accepted) in enumerate(cases): + work = tmp_path / f"work-{index}" / FAMILY_MEMBER + work.mkdir(parents=True) + git("init", "-q", cwd=work) + (work / "project.yaml").write_text( + "kind: project-manifest\nid: 7c9f1b2d\n" + f"name: {FAMILY_MEMBER}\n", encoding="utf-8") + url = NOT_UTF8 + tail + with_origin(work, url) + read = url.decode("utf-8", "replace") + assert doctor.origin_url(work) == read, what + + sibling = siblings.Sibling(FAMILY_MEMBER, work) + assert siblings.verify_sibling( + row, work, mount, row["repository"], sibling) is accepted, what + assert doctor.working_clone(work, row, mount) == ( + (work.as_posix(), None) if accepted else (None, read)), what + + # --- the adversarial review on #96 ------------------------------------------ def test_a_leg_file_the_standard_GAINED_is_reported_and_never_invalid( @@ -6111,6 +6261,35 @@ def test_origin_name_reads_a_windows_style_local_path_remote(standard, assert "--project Atlas" in row.next_command, row.next_command +def test_an_origin_url_that_is_not_utf8_names_nothing_and_raises_nothing( + standard, tmp_path): + """#385: git keeps a URL as the bytes it was given, and all three of this + file's readers of it raised `UnicodeDecodeError` on one that is not + UTF-8 -- the `naming` row's, and so `--root`, first. + + THE NAME IS NONE, as for no origin, so `naming` and `the way in` read the + directory alone, exactly as they do for the same directory with no + `origin` (the twin, under another parent). THE URL IS READ AS `make + siblings` READS IT, the byte replaced, because the `members` row must + agree with `verify_sibling` about one directory (#146) -- the test of that + agreement is `test_working_clone_and_verify_sibling_agree_on_a_non_utf8 + _origin`. + """ + module = doctor_module(standard) + here, twin = tmp_path / "a" / "latin1", tmp_path / "b" / "latin1" + for path in (here, twin): + path.mkdir(parents=True) + git("init", "-q", "-b", "main", ".", cwd=path) + with_origin(here, NOT_UTF8 + b"Atlas.git") + assert module.origin_name(here) is None + assert module.repo_local_origin_name(here) is None + assert module.origin_url(here) == "../caf\ufffd/Atlas.git" + for check in (module.check_not_a_root_naming, module.check_the_way_in): + row, plain = check(module.Context(here)), check(module.Context(twin)) + assert row.detail == plain.detail, (row.detail, plain.detail) + assert "Atlas" not in row.reason + (row.next_command or ""), row + + def test_origin_name_reads_scp_like_syntax_with_no_slash_at_all(standard, tmp_path): """Copilot, second review of PR #110: `[user@]host:path` -- no group