update-shape.py and scripts/bump-leg.py: the three line rewriters find a first-line key after a byte-order mark, as every reader does, and write the mark back as it was (#933)

scripts/bump-leg.py

@@ -149,6 +149,9 @@
 # On its own line, so the list above, which other changes extend, is
 # not a line two of them both rewrite (#715).
 from shape_materialize import readable_branch  # noqa: E402
+# U+FEFF by the reader's own name, which `rewrite_pin` takes off line 1 as
+# `parse_yaml` does, and puts back (#933). On its own line, as the one above.
+from repo_shape import BYTE_ORDER_MARK  # noqa: E402
 from shape_materialize import (  # noqa: E402
     CommandFailed, captured_child_environment, check_program, commit_trailers,
     escape_what_stdout_cannot_encode, is_nested_key, pin_values_in,
@@ -370,10 +373,22 @@ def rewrite_pin(text: str, commit: str, digest: str) -> str:
     line rewrite cannot carry is refused HERE as this tool's failure, before
     any byte is written. The digest is the one this command RECOMPUTED from
     the leg's own objects; the read-back only asks that it landed.
+
+    A BYTE-ORDER MARK IN FRONT OF LINE 1 IS LOOKED PAST, AND KEPT (#933).
+    `parse_yaml` takes off every U+FEFF the text starts with (#855), so a
+    pin whose first line is `commit:` after one is read with that key by
+    the project's validators; the pin is read here as `utf-8`, and the
+    mark was the key's first character, so the pin was refused as having
+    0 `commit` lines. The marks are taken off before the lines are looked
+    at and put back in front of them, so `write_lf` writes them back byte
+    for byte, as every other line's bytes are. A mark anywhere else is
+    text, to the reader and here.
     """
+    unmarked = text.lstrip(BYTE_ORDER_MARK)
+    marks = text[:len(text) - len(unmarked)]
     out: list[str] = []
     seen = {"commit": 0, "tree_sha256": 0}
-    for line in text.splitlines():
+    for line in unmarked.splitlines():
         if line.startswith("commit:"):
             seen["commit"] += 1
             out.append(with_scalar(line, "commit", commit))
@@ -393,7 +408,7 @@ def rewrite_pin(text: str, commit: str, digest: str) -> str:
                 "does not recognise. Fix it by hand this once — `commit:` at "
                 "column 0 and `tree_sha256:` indented under `digests:` — or "
                 "re-scaffold.")
-    rewritten = "\n".join(out) + "\n"
+    rewritten = marks + "\n".join(out) + "\n"
     why = rewrite_fault(text, rewritten, commit, digest)
     if why is not None:
         raise Refusal(

tests/test_bom_rewrites.py

@@ -0,0 +1,165 @@
+# SPDX-License-Identifier: Apache-2.0
+"""A pin or a manifest whose first line is its key after a byte-order mark
+is rewritten, and the mark is kept (#933).
+
+THE DEFECT, measured on main dc7cd49b7 by calling the three line rewriters
+in process. Since #855 `parse_yaml` takes off every U+FEFF the text starts
+with, so `check`, `load_yaml` and both validators read such a file. The
+rewriters found their key at column 0 of the raw text, which their tools
+read as `utf-8`, so the mark was the key's first character and each said
+the key was not there:
+
+- `update-shape.py`'s `rewrite_shape_pin`, on a shape pin whose first line
+  is `commit:`: `update-pin-unrecognised`, "0 `commit` line(s)";
+- its `rewrite_manifest`, on a `project.yaml` whose first line is
+  `shape:`, through `_shape_block_bounds` and `_is_block_header`:
+  `update-manifest-no-shape`, "has no `shape:` block";
+- `scripts/bump-leg.py`'s `rewrite_pin`, on a leg pin whose first line is
+  `commit:`: `bump-leg-pin-unrecognised`, "0 `commit` line(s)".
+
+THE RULE NOW. Each takes the marks off the start of the text, as the reader
+does, finds its key, and puts the marks back in front of what it returns,
+so they are written back byte for byte. A file with no mark is rewritten
+as it always was. A mark anywhere else is text, to the reader and to the
+finders, and keeps today's answer.
+
+Every mark is written by this file as bytes (`codecs.BOM_UTF8`), never
+typed. Each file is the scaffold's rendering
+(`tests/test_pin_rewrite_layout.py`) with the key moved to line 1, where
+no template puts it.
+"""
+
+from __future__ import annotations
+
+import codecs
+from typing import NamedTuple
+
+import pytest
+
+from test_pin_rewrite_layout import MANIFEST as MANIFEST_TEXT
+from test_pin_rewrite_layout import (  # noqa: F401
+    NEW_COMMIT, NEW_ROWS, NEW_TREE, OLD_COMMIT, OLD_ROWS, OLD_TREE, make_pin,
+    refusal_code, rewrite, tools,
+)
+
+from repo_shape import Refusal
+from shape_materialize import pin_values_in
+
+MARK = codecs.BOM_UTF8
+#: The mark as the one character the reader reads it as, U+FEFF.
+MARK_TEXT = MARK.decode("utf-8")
+#: A line a person may put above the first key before saving the file.
+COMMENT = "# saved by hand\n"
+#: The leg's own `commit:` in the manifest, above its `shape:` block.
+MEMBER = "e" * 40
+
+
+class Finder(NamedTuple):
+    #: The key its file's line 1 holds here.
+    key: str
+    #: The block the validator reads the pin under.
+    within: tuple
+    #: The scaffold's pin kind, or None for the manifest.
+    kind: str | None
+    #: What it says of a file it finds no key in.
+    refused: str
+
+
+FINDERS = {
+    "shape-pin-commit": Finder("commit", (), "assembly shape pin",
+                               "has 0 `commit` line(s)"),
+    "manifest-shape-header": Finder("shape", ("shape",), None,
+                                    "has no `shape:` block"),
+    "leg-pin-commit": Finder("commit", (), "spec pin",
+                             "has 0 `commit` line(s)"),
+}
+
+
+def key_first(text: str, key: str) -> str:
+    """`text` with its column-0 `<key>:` line, and the block under it, moved
+    to line 1: an order the reader ignores and no template writes."""
+    lines = text.split("\n")
+    start = next(index for index, line in enumerate(lines)
+                 if line.startswith(f"{key}:"))
+    end = next((index for index in range(start + 1, len(lines))
+                if lines[index] and not lines[index].startswith(" ")),
+               len(lines))
+    return "\n".join(lines[start:end] + lines[:start] + lines[end:])
+
+
+def the_file(finder: str, commit: str, tree: str, rows) -> str:
+    """The finder's file pinned at `commit` and `tree`, its key on line 1."""
+    key, _within, kind, _refused = FINDERS[finder]
+    if kind is None:
+        text = MANIFEST_TEXT.format(member=MEMBER, commit=commit, tree=tree)
+    else:
+        text = make_pin(kind, commit, tree, rows)
+    return key_first(text, key)
+
+
+def rewritten(loaded, finder: str, text: str) -> str:
+    """The finder's rewriter onto the target, called as its tool calls it;
+    `loaded` is the `tools` fixture."""
+    kind = FINDERS[finder].kind
+    if kind is None:
+        return loaded.update_shape.rewrite_manifest(
+            text, NEW_COMMIT, NEW_TREE, loaded.update_shape.PROJECT_KIND)
+    return rewrite(loaded, kind, text, NEW_COMMIT, NEW_TREE)
+
+
+def read(finder: str, text: str) -> tuple[str, str]:
+    """`commit` and `digests.tree_sha256`, where the validator reads them."""
+    return pin_values_in(text, FINDERS[finder].within)
+
+
+@pytest.mark.parametrize("marks", [1, 2], ids=["mark", "two-marks"])
+@pytest.mark.parametrize("finder", list(FINDERS))
+def test_a_first_key_after_the_mark_is_rewritten_and_the_mark_kept(
+        tools, finder, marks):  # noqa: F811
+    """THE ISSUE'S THREE LINES. The file every reader reads is rewritten:
+    its first bytes are the marks it had, every byte after them is the
+    rewrite of the same file without them, and the validator reads the
+    target. Two marks are taken off and put back as one is, as the reader
+    takes off every mark the text starts with."""
+    before = MARK_TEXT * marks + the_file(finder, OLD_COMMIT, OLD_TREE,
+                                          OLD_ROWS)
+    assert read(finder, before) == (OLD_COMMIT, OLD_TREE), (
+        "fixture: the reader reads the marked file")
+
+    new = rewritten(tools, finder, before)
+
+    assert new.encode("utf-8") == MARK * marks + the_file(
+        finder, NEW_COMMIT, NEW_TREE, NEW_ROWS).encode("utf-8")
+    assert read(finder, new) == (NEW_COMMIT, NEW_TREE)
+
+
+@pytest.mark.parametrize("finder", list(FINDERS))
+def test_the_same_file_without_the_mark_is_rewritten_as_before(
+        tools, finder):  # noqa: F811
+    """THE CONTROL, which passes on main as well: with no mark, what is
+    returned is the file at the target, with not a byte in front of it."""
+    before = the_file(finder, OLD_COMMIT, OLD_TREE, OLD_ROWS)
+
+    new = rewritten(tools, finder, before)
+
+    assert new == the_file(finder, NEW_COMMIT, NEW_TREE, NEW_ROWS)
+    assert read(finder, new) == (NEW_COMMIT, NEW_TREE)
+
+
+@pytest.mark.parametrize("finder", list(FINDERS))
+def test_a_mark_after_line_1_keeps_todays_answer(tools, finder):  # noqa: F811
+    """LINE 1 ONLY. A mark on line 2, in front of the key, is the key's
+    first character to the reader, which reads no pin there; each finder
+    refuses the file as it did before, in its own words."""
+    before = COMMENT + MARK_TEXT + the_file(finder, OLD_COMMIT, OLD_TREE,
+                                            OLD_ROWS)
+    with pytest.raises(ValueError):
+        read(finder, before)
+
+    with pytest.raises(Refusal) as refused:
+        rewritten(tools, finder, before)
+
+    kind = FINDERS[finder].kind
+    assert refused.value.code == (
+        "update-manifest-no-shape" if kind is None else refusal_code(kind))
+    assert FINDERS[finder].refused in refused.value.detail

update-shape.py

@@ -178,6 +178,10 @@
 # is one to the reader (#437), and with the reader's rule for a key, so a
 # quoted key or a space before the colon is one here too (#674).
 from repo_shape import _split_key, _strip_comment  # noqa: E402
+# U+FEFF by the reader's own name: the two line rewrites take it off line 1
+# as `parse_yaml` does, and put it back (#933). On a line of its own, for the
+# reason the branch rule's import below gives.
+from repo_shape import BYTE_ORDER_MARK  # noqa: E402
 from shape_materialize import (  # noqa: E402
     ATTRIBUTES, COPIED_FROM_SHAPE, COPIED_VERBATIM, FAMILY_COPIED_FROM_SHAPE,
     FAMILY_COPIED_VERBATIM, SHAPE_REPOSITORY, CommandFailed, check_program,
@@ -1707,6 +1711,24 @@ def _header_as_it_was(line: str) -> str:
     return body + (tail if tail.strip() else "")
 
 
+def _marks_off(text: str) -> tuple[str, str]:
+    """The byte-order marks `text` starts with, and the text after them.
+
+    LINE 1 ONLY, AS THE READER TAKES THEM (#933). `parse_yaml` takes off
+    every U+FEFF the text starts with (#855), so a pin whose first line is
+    `commit:` after one, or a manifest whose first line is `shape:`, is
+    read with that key by `check` and both validators. `_repin` reads the
+    two files as `utf-8`, so the finders below met the mark as the first
+    character of the key and refused the file, in words that said the key
+    was not there. They look at the text after the marks now, and each
+    rewrite puts the marks back in front of what it returns, so they are
+    written back byte for byte, as every other line's bytes are. A mark
+    anywhere else is text, to the reader and here.
+    """
+    after = text.lstrip(BYTE_ORDER_MARK)
+    return text[:len(text) - len(after)], after
+
+
 def rewrite_shape_pin(text: str, commit: str, tree: str,
                       rows: list[Row]) -> str:
     """`commit:`, `digests.tree_sha256:` and the whole `files:` block.
@@ -1743,8 +1765,13 @@ def rewrite_shape_pin(text: str, commit: str, tree: str,
     WHAT IS RETURNED HAS BEEN READ BACK, as `rewrite_manifest`'s is
     (`shape_materialize.rewrite_fault`): a layout this line rewrite cannot
     carry is refused HERE as this tool's failure, before any byte is written.
+
+    A BYTE-ORDER MARK IN FRONT OF LINE 1 is taken off before the lines are
+    looked at and put back in front of them (`_marks_off`, #933), so a pin
+    whose first line is `commit:` after one is rewritten, mark and all.
     """
-    lines = text.splitlines()
+    marks, unmarked = _marks_off(text)
+    lines = unmarked.splitlines()
     out: list[str] = []
     seen = {"commit": 0, "tree": 0, "files": 0}
     for index, line in enumerate(lines):
@@ -1772,7 +1799,7 @@ def rewrite_shape_pin(text: str, commit: str, tree: str,
                 "exactly one was expected, so this tool will not rewrite it",
                 "Remediation: the pin was hand-edited into a shape this tool "
                 "does not recognise. Fix it by hand this once, or re-scaffold.")
-    rewritten = "\n".join(out) + "\n"
+    rewritten = marks + "\n".join(out) + "\n"
     why = rewrite_fault(text, rewritten, commit, tree)
     if why is not None:
         raise Refusal(
@@ -1810,8 +1837,15 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str:
     carry — a value on the line below its key — is refused HERE, as this
     tool's failure, rather than by a validator after the fact
     (`_refuse_unless_it_reads_back`).
+
+    A BYTE-ORDER MARK IN FRONT OF LINE 1 is taken off before the block is
+    looked for and put back in front of the lines (`_marks_off`, #933), so
+    `_shape_block_bounds` finds a `shape:` header that is line 1 after one,
+    which it refused `update-manifest-no-shape` while the validators read
+    the block.
     """
-    lines = text.splitlines()
+    marks, unmarked = _marks_off(text)
+    lines = unmarked.splitlines()
     start, end = _shape_block_bounds(lines, kind)
     seen = {"commit": 0, "tree": 0}
     for index in range(start + 1, end):
@@ -1831,7 +1865,8 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str:
                 "where exactly one was expected",
                 "Remediation: fix the block by hand this once; this tool will "
                 "not guess at a manifest it does not recognise.")
-    rewritten = "\n".join(lines) + ("\n" if text.endswith("\n") else "")
+    rewritten = (marks + "\n".join(lines)
+                 + ("\n" if text.endswith("\n") else ""))
     _refuse_unless_it_reads_back(text, rewritten, commit, tree, kind)
     return rewritten