update-shape.py: apply keeps each of the manifest's shape: pin lines at the indentation it already has and reads the rewrite back, so a project.yaml indented four spaces takes the update instead of being rolled back under the validators' lockstep remedy (#208)

tests/test_update_shape.py

@@ -421,6 +421,204 @@ def test_apply_with_no_shape_block_names_this_kinds_own_manifest_validator(
         == upstream_and_project["a"], "a refused apply writes nothing at all"
 
 
+# --- the manifest's own indentation (#208) ----------------------------------
+
+#: Indentations of the `shape:` block that `load_yaml` and both validators
+#: accept, as (direct children, `digests:` children). `(2, 4)` is what the
+#: scaffold writes; the rest are what a hand re-indent or an editor's
+#: formatter leaves. The reader asks only that a level nest deeper than its
+#: parent, so every one of these is a manifest this project may hold.
+SHAPE_INDENTS = [(2, 4), (4, 6), (3, 6), (1, 2), (8, 12), (4, 5), (2, 6),
+                 (3, 4)]
+
+
+def indent_shape_block(text: str, child: int, grandchild: int) -> str:
+    """`text` with ONLY its `shape:` block re-indented: each direct child at
+    `child` spaces, each `digests:` child at `grandchild`. Every other line,
+    the comments above the block included, is left as it was."""
+    out, inside = [], False
+    for line in text.split("\n"):
+        if line.rstrip() == "shape:":
+            inside = True
+        elif inside and line.strip() and not line.startswith(" "):
+            inside = False
+        elif inside and line.strip():
+            depth = grandchild if line.startswith("    ") else child
+            line = " " * depth + line.strip()
+        out.append(line)
+    return "\n".join(out)
+
+
+def commit_manifest(root, text: str, message: str) -> None:
+    """Write `project.yaml` as exactly these bytes and commit it. Bytes, not
+    `write_text`, so the line endings under test are the ones written on
+    every platform."""
+    (root / "project.yaml").write_bytes(text.encode("utf-8"))
+    git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m",
+        message, "--", "project.yaml", cwd=root)
+
+
+@pytest.mark.parametrize("child, grandchild", SHAPE_INDENTS)
+def test_the_manifest_rewrite_moves_two_values_and_keeps_every_indent(
+        update_shape, upstream_and_project, tmp_path, child, grandchild):
+    """ONLY THE TWO VALUES MOVE (#208). The lines were found at any
+    indentation and written back at a fixed two and four spaces, so a block
+    indented four spaces came back with `commit:` at two among siblings at
+    four — YAML the same reader refuses. The expected text here is the input
+    with the two quoted values substituted and nothing else, which for
+    `(2, 4)` is byte for byte what the fixed-indent rewrite wrote before."""
+    a = upstream_and_project["a"]
+    scaffolded = (upstream_and_project["clone"] / "project.yaml").read_text(
+        encoding="utf-8")
+    text = indent_shape_block(scaffolded, child, grandchild)
+    (tmp_path / "before.yaml").write_bytes(text.encode("utf-8"))
+    old_tree = load_yaml(tmp_path / "before.yaml")["shape"]["digests"][
+        "tree_sha256"]
+    commit, tree = "c" * 40, "d" * 64
+
+    new = update_shape.rewrite_manifest(text, commit, tree,
+                                        update_shape.PROJECT_KIND)
+
+    assert new == (text.replace(f'commit: "{a}"', f'commit: "{commit}"')
+                   .replace(f'tree_sha256: "{old_tree}"',
+                            f'tree_sha256: "{tree}"'))
+    (tmp_path / "after.yaml").write_bytes(new.encode("utf-8"))
+    shape = load_yaml(tmp_path / "after.yaml")["shape"]
+    assert (shape["commit"], shape["digests"]["tree_sha256"]) == (commit, tree)
+
+
+#: A family holder's manifest, cut down to the lines that matter here: a
+#: member row carrying a `commit:` of its own BEFORE the block, and a key
+#: after it, so the block's two bounds are both exercised. `family.yaml`'s
+#: `shape:` block has the shape `project.yaml`'s has, and goes through the
+#: same `rewrite_manifest`.
+FAMILY_MANIFEST = """schema_version: 1
+kind: family-manifest
+
+members:
+  - name: Atlas
+    commit: "{member}"
+
+shape:
+  repository: opensoft/openRepoShape
+  revision_kind: commit
+  commit: "{commit}"
+  digest_algorithm: sha256
+  digests:
+    tree_sha256: "{tree}"
+
+visibility: private
+"""
+
+
+@pytest.mark.parametrize("child, grandchild", [(2, 4), (4, 6), (3, 6)])
+def test_the_family_manifest_rewrite_keeps_its_indent_and_its_members_pin(
+        update_shape, tmp_path, child, grandchild):
+    """THE FAMILY HOLDER IS THE SAME REWRITE (#208). `family.yaml` carries
+    the same `shape:` block, so the same two lines move and nothing else does:
+    not the member row's own `commit:`, which sits above the block and spells
+    the key the same way."""
+    member, old_commit, old_tree = "a" * 40, "b" * 40, "e" * 64
+    text = indent_shape_block(
+        FAMILY_MANIFEST.format(member=member, commit=old_commit,
+                               tree=old_tree), child, grandchild)
+    commit, tree = "c" * 40, "d" * 64
+
+    new = update_shape.rewrite_manifest(text, commit, tree,
+                                        update_shape.FAMILY_KIND)
+
+    assert new == (text.replace(f'commit: "{old_commit}"',
+                                f'commit: "{commit}"')
+                   .replace(f'tree_sha256: "{old_tree}"',
+                            f'tree_sha256: "{tree}"'))
+    assert f'commit: "{member}"' in new
+    (tmp_path / "family.yaml").write_bytes(new.encode("utf-8"))
+    loaded = load_yaml(tmp_path / "family.yaml")
+    assert (loaded["shape"]["commit"],
+            loaded["shape"]["digests"]["tree_sha256"]) == (commit, tree)
+
+
+@pytest.mark.parametrize("child, grandchild", [(4, 6), (3, 4), (2, 6)])
+def test_apply_leaves_a_manifest_indented_its_own_way_green_and_indented(
+        root, upstream_and_project, child, grandchild):
+    """The issue's reproduction, end to end: a project whose `shape:` block
+    is indented four spaces (`tree_sha256:` at six) takes the update, both
+    validators stay green, and the diff to `project.yaml` is the two values.
+    `(3, 4)` nests by one space; `(2, 6)` is the scaffold's own two with
+    `digests:` nested by four, which the fixed-indent rewrite did not break
+    but silently re-indented."""
+    a, target = upstream_and_project["a"], upstream_and_project["b"]
+    upstream = upstream_and_project["upstream"]
+    manifest = root / "project.yaml"
+    commit_manifest(root, indent_shape_block(
+        manifest.read_text(encoding="utf-8"), child, grandchild),
+        "Re-indent the shape block")
+    validators_are_green(root)
+    before = manifest.read_text(encoding="utf-8")
+    old_tree = load_yaml(manifest)["shape"]["digests"]["tree_sha256"]
+
+    result = apply(root, upstream_and_project)
+
+    assert result.returncode == 0, result.stdout + result.stderr
+    tree = tree_digest(upstream, target)
+    assert manifest.read_text(encoding="utf-8") == (
+        before.replace(f'commit: "{a}"', f'commit: "{target}"')
+        .replace(f'tree_sha256: "{old_tree}"', f'tree_sha256: "{tree}"'))
+    shape = load_yaml(manifest)["shape"]
+    assert shape["commit"].lower() == target
+    assert shape["digests"]["tree_sha256"].lower() == tree
+    validators_are_green(root)
+
+
+def test_a_layout_the_rewrite_cannot_carry_is_refused_as_its_own_fault(
+        root, upstream_and_project):
+    """A VALUE ON THE LINE BELOW ITS KEY reads, and validates, and is the
+    one layout a line rewrite cannot move: it replaces the key's line and
+    leaves the value's line behind, which the reader then refuses. Before
+    #208 that reached the validators, and `apply` relayed their lockstep
+    remedy — advance the gitlink — over a manifest the project had not
+    broken. The read-back refuses it first, by this tool's name, and the
+    tree is rolled back to the byte."""
+    a = upstream_and_project["a"]
+    manifest = root / "project.yaml"
+    text = manifest.read_text(encoding="utf-8")
+    folded = text.replace(f'  commit: "{a}"\n', f"  commit: >-\n    {a}\n", 1)
+    assert folded != text, "fixture: the scaffold's commit line was found"
+    commit_manifest(root, folded, "Fold the shape commit onto its own line")
+    validators_are_green(root)
+
+    result = apply(root, upstream_and_project)
+
+    assert_refused_and_unpinned(result, root, upstream_and_project,
+                                "update-manifest-unrecognised",
+                                "the fault is this tool's line rewrite",
+                                "unexpected indentation")
+    assert "update-validators-red" not in result.stderr
+    assert "advance the gitlink" not in result.stderr
+    assert manifest.read_bytes() == folded.encode("utf-8")
+    assert git("status", "--porcelain", cwd=root).stdout == ""
+
+
+def test_a_manifest_that_did_not_read_before_is_left_to_its_validators(
+        update_shape, upstream_and_project):
+    """THE READ-BACK BLAMES THIS TOOL ONLY WHEN THE FAULT IS ITS OWN. A
+    manifest the reader refused before a byte was rewritten was red before
+    `apply` ran; refusing it as `update-manifest-unrecognised` would blame
+    the rewrite for the project's own line. It is rewritten as before and
+    left to the validators, which name that line."""
+    text = (upstream_and_project["clone"] / "project.yaml").read_text(
+        encoding="utf-8")
+    broken = text.replace("\nkind: project-manifest\n",
+                          "\nkind: project-manifest\n\tstray: tab\n", 1)
+    assert broken != text, "fixture: the line to break was found"
+
+    new = update_shape.rewrite_manifest(broken, "c" * 40, "d" * 64,
+                                        update_shape.PROJECT_KIND)
+
+    assert '  commit: "' + "c" * 40 + '"' in new
+    assert "\tstray: tab" in new
+
+
 # --- drift the project introduced ------------------------------------------
 
 def edit_locally(root, rel: str) -> None:

update-shape.py

@@ -101,8 +101,9 @@
 SHAPE_ROOT = Path(__file__).resolve().parent
 sys.path.insert(0, str(SHAPE_ROOT / "scripts"))
 from repo_shape import (  # noqa: E402
-    COMMIT_RE, PYTHON, TREE_DIGEST_DEFINITION, Refusal, checked_value, die,
-    file_sha256, git_out, load_yaml, tree_digest,
+    COMMIT_RE, PYTHON, TREE_DIGEST_DEFINITION, Refusal, YamlError,
+    checked_value, die, file_sha256, git_out, load_yaml, parse_yaml,
+    tree_digest,
 )
 from shape_materialize import (  # noqa: E402
     COPIED_FROM_SHAPE, COPIED_VERBATIM, FAMILY_COPIED_FROM_SHAPE,
@@ -861,18 +862,33 @@ def rewrite_manifest(text: str, commit: str, tree: str, kind: Kind) -> str:
     Scoped to the block on purpose: `commit:` and `tree_sha256:` are spelled
     the same way in every pin, and a whole-file replacement would move a leg's
     pin while claiming to move the shape's.
+
+    EACH LINE KEEPS ITS OWN INDENTATION (#208). The two lines are FOUND by
+    their stripped text, so a block at any indentation `load_yaml` accepts is
+    recognised; they were then WRITTEN at a fixed two and four spaces, which
+    turned a block indented four spaces — accepted by the reader and by both
+    validators — into one the reader refuses, and the validators' refusal
+    that followed blamed the project for this tool's write. Only the value
+    after the key changes now; the whitespace in front of it is the line's.
+
+    WHAT IS RETURNED HAS BEEN READ BACK. A layout this line rewrite cannot
+    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`).
     """
     lines = text.splitlines()
     start, end = _shape_block_bounds(lines, kind)
     seen = {"commit": 0, "tree": 0}
     for index in range(start + 1, end):
-        stripped = lines[index].strip()
+        line = lines[index]
+        stripped = line.strip()
+        indent = line[:len(line) - len(line.lstrip())]
         if stripped.startswith("commit:"):
             seen["commit"] += 1
-            lines[index] = f'  commit: "{commit}"'
+            lines[index] = f'{indent}commit: "{commit}"'
         elif stripped.startswith("tree_sha256:"):
             seen["tree"] += 1
-            lines[index] = f'    tree_sha256: "{tree}"'
+            lines[index] = f'{indent}tree_sha256: "{tree}"'
     for key, count in seen.items():
         if count != 1:
             raise Refusal(
@@ -881,7 +897,63 @@ 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.")
-    return "\n".join(lines) + ("\n" if text.endswith("\n") else "")
+    rewritten = "\n".join(lines) + ("\n" if text.endswith("\n") else "")
+    _refuse_unless_it_reads_back(text, rewritten, commit, tree, kind)
+    return rewritten
+
+
+def _shape_pin_of(text: str) -> tuple[str, str]:
+    """`shape.commit` and `shape.digests.tree_sha256`, read out of a
+    manifest's text the way its validator reads them, or a `ValueError`
+    saying why they cannot be (#208)."""
+    try:
+        manifest = parse_yaml(text)
+    except YamlError as exc:
+        raise ValueError(str(exc)) from exc
+    shape = manifest.get("shape") if isinstance(manifest, dict) else None
+    digests = shape.get("digests") if isinstance(shape, dict) else None
+    commit = shape.get("commit") if isinstance(shape, dict) else None
+    tree = digests.get("tree_sha256") if isinstance(digests, dict) else None
+    if commit is None or tree is None:
+        raise ValueError("no `shape.commit` and `shape.digests.tree_sha256` "
+                         "where the validator reads them")
+    return str(commit), str(tree)
+
+
+def _refuse_unless_it_reads_back(before: str, after: str, commit: str,
+                                 tree: str, kind: Kind) -> None:
+    """Refuse a rewrite that does not read back to the target (#208).
+
+    THE REFUSAL IS THIS TOOL'S, SO IT IS RAISED ONLY WHEN THE FAULT IS. A
+    manifest that read correctly before the rewrite and does not after it
+    was broken by the rewrite, and the remedy says so. One that did not read
+    before — unparsable, or no pin where the validator looks — was red
+    before this command ran: that is left to the validators, which name the
+    project's own line, and blaming this tool for it would be the same wrong
+    answer turned around.
+    """
+    try:
+        found = _shape_pin_of(after)
+        if found == (commit, tree):
+            return
+        why = (f"it would read `commit: {found[0]}` and "
+               f"`tree_sha256: {found[1]}`")
+    except ValueError as exc:
+        why = f"the reader would refuse it: {exc}"
+    try:
+        _shape_pin_of(before)
+    except ValueError:
+        return
+    raise Refusal(
+        "update-manifest-unrecognised",
+        f"{kind.manifest}'s `shape:` block reads as it stands, but this "
+        f"tool's rewrite of it does not read back to the target ({why}), so "
+        "this tool will not write it",
+        "Remediation: the fault is this tool's line rewrite, not the "
+        "project's manifest. Put each of `commit:` and `tree_sha256:` on one "
+        "line with its quoted value, as the scaffold writes them, and run "
+        "the same command again; this tool will not guess at a manifest it "
+        "does not recognise.")
 
 
 def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: