docs: add the simplification and meta-ref architecture plan
commit
9b2832adocs: add the simplification and meta-ref architecture plan
Records the adversarial review (the fixed push bug, correctness findings C1-C5, simplifications P1-P8) and the agreed meta-ref design: Path A compile-time typing, the members/config/checks/runs/issues layout, config as { description, homepage, topics }, decomposed refs with aggregated views, and no worktree metadata.
Assisted-by: Claude:claude-opus-4-8
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
docs/simplification-plan.adoc
@@ -1,0 +1,235 @@
+= Simplification & Adversarial Review Plan
+:doctype: article
+:toc:
+:toclevels: 2
+
+NOTE: Working plan from an adversarial review of the whole workspace (~5k LOC
+across `git-ents`, `git-ents-server`, `git-store`). Goal: drastically simplify
+while keeping all functionality. Not yet executed — pending decisions on P4/P5
+and the `tempfile` dependency promotion (P3).
+
+== Status
+
+* *Fixed and committed* (`c5984991`): the push bug. A recent refactor wrapped
+ each check command in a `CheckDef` struct, changing the `refs/meta/checks`
+ tree from `checks/<name>` blobs to `checks/<name>/command` subtrees, so
+ loading a check set written before the change failed with "object … is not a
+ tree" and broke every push. Reverted to a plain command map, which both
+ restores compatibility with the live data and removes the indirection.
+
+== Correctness / robustness findings (open)
+
+[cols="1,4",options="header"]
+|===
+| # | Finding
+
+| C1
+| *Meta-ref format fragility, unguarded.* `refs/meta/auth` (`Auth`) and
+ `refs/meta/runs` (`RunDoc`) carry the same risk as the bug just fixed: any
+ incompatible change to the Facet struct silently breaks reading existing
+ on-disk data, surfacing only at push/render time. No format versioning, and no
+ test loads old-layout data. Add a round-trip-against-fixed-bytes test per
+ document type, and decide a policy (version field, or "never change a meta-ref
+ type incompatibly").
+
+| C2
+| *Unbounded blob/diff rendering (web OOM).* `blob_page`, `blob_pane`, and
+ `commit_page` read arbitrarily large objects fully into memory via
+ `git cat-file -p` / `git show`; `is_binary` only samples 8 KB but the whole
+ object is still highlighted and emitted. Cap the rendered size.
+
+| C3
+| *Checks worker head-of-line blocks.* `worker` drains the queue through one
+ `spawn_blocking` call in sequence; a check may run up to `CHECK_TIMEOUT`
+ (30 min). One slow repo stalls every repo's checks. Acknowledged in comments
+ but a real multi-repo availability bug.
+
+| C4
+| *`gather_meta` shells out to git 4× per web request* (branch, description,
+ topics, releases) even on tabs that use none of it — e.g. opening a blob still
+ runs `tag --list` and `cat-file …/.gitents/topics`. Gather lazily per tab.
+
+| C5 (minor)
+| `RepoMeta.issues` is hardcoded `0`, so the Issues tab count can never render —
+ dead wiring.
+|===
+
+== Simplifications (prioritized)
+
+[cols="1,3,1,1",options="header"]
+|===
+| P | Change | Files | Risk
+
+| P1
+| *Collapse the auth/checks CLI duplication.* `sync_auth`/`sync_checks`,
+ `push_auth`/`push_checks`, `list`/`list_checks`, `add`/`add_check`,
+ `remove`/`remove_check`, `run_auth`/`run_checks` differ only by ref name and
+ noun. Unify into one generic meta-ref-set flow. (~200 lines.)
+| `git-ents/src/main.rs`
+| Low
+
+| P2
+| *Unify the meta-ref "named-string-pair set" document.* The "stored
+ `BTreeMap<String,String>` ↔ public `Vec` of two-field structs" pattern appears
+ three times: `Auth`→`Vec<Signer>`, `Checks`→`Vec<Check>`, and
+ `RunDoc`→`Vec<RunOutcome>`. Extract one generic helper (in `git-store`) and
+ delete the duplicated document structs and their `load`/`store`.
+| `git-store`, `signers.rs`, `checks.rs`
+| Low–Med
+
+| P3
+| *Delete two hand-rolled temp dirs.* `Scratch` (main.rs) and `TempDir`
+ (verify.rs) are identical Drop-cleanup temp dirs; `tempfile` is already a
+ workspace dependency (test-only today). Replace both with `tempfile::TempDir`.
+ *Requires promoting `tempfile` from dev- to normal dependency — needs OK.*
+| `git-ents/src/main.rs`, `git-ents-server/src/verify.rs`, two `Cargo.toml`
+| Low
+
+| P4
+| *Keep the Facet `Render` trait.* WITHDRAWN (was: cut it). Under the
+ schema-driven meta-ref direction (see "New direction" below) a generic
+ structural renderer is the mechanism that lets new meta-ref types render
+ without bespoke Rust, so it is load-bearing, not over-engineering. The two
+ overrides (`Signer`, `Run`) are the trait working as intended.
+| `git-ents-server/src/web/render.rs`
+| — (keep)
+
+| P5
+| *Make Settings & Issues functional via typed meta refs.* WITHDRAWN cut (was:
+ trim the stubs). Settings is a projection over `refs/meta/members` +
+ `refs/meta/config` + derived checks/releases; Issues become typed meta-ref
+ documents. `Render`'s structural walk presents them. See "New direction".
+| `pages.rs`, new config module, `git-store`
+| Med–High
+
+| P6
+| *Simplify request routing.* `wants_web_ui` / `is_git_path` / `is_browse_route`
+ / `is_service_request` / `is_receive_pack` / `query_service` are overlapping
+ string heuristics on a single catch-all fallback. Explicit Axum routes for the
+ git wire endpoints + a separate web router would remove most of them.
+ Integration tests (`server.rs`, `pre_receive.rs`) are the safety net. Do last.
+| `git-ents-server/src/http.rs`, `main.rs`
+| High
+
+| P7
+| *Micro.* Hoist the duplicated zero-oid const (3 copies); delete `tagline()`
+ (confirmed unused dead code).
+| `main.rs`, `git-ents/src/lib.rs`, `server/checks.rs`
+| Low
+
+| P8
+| *Delete `--max-requests` and the shutdown plumbing.* The flag is used only by
+ the `responds_and_shuts_down` test, which can `kill()` the child like the other
+ three tests do. Removes the flag, the counter middleware, the `Notify`, and
+ `with_graceful_shutdown`. (~25 lines.)
+| `git-ents-server/src/main.rs`, `tests/server.rs`
+| Low
+|===
+
+== Also noted (lower value)
+
+* Test helper duplication: a `unique_repo()` / `git init` temp-repo helper is
+ copied across `checks.rs`, `signers.rs`, `pre_receive.rs`.
+* Server-side sync git invocations are spawned ad hoc in `verify.rs` and
+ `checks.rs`; the async `git_output`/`git_output_bytes` in `web/git.rs` is the
+ only shared wrapper and is web-private.
+* `reconcile_head` is ~45 lines to fix an unborn `HEAD` after first push; could
+ shrink once the routing/handler knows the pushed branch.
+
+== New direction: meta-ref structure (decided)
+
+Rather than cutting the non-functional UI, make it real, backed by typed
+meta-ref documents rendered through `Render`. *Path A (compile-time typed docs)
+chosen* — each meta ref is a Rust `Facet` struct; the compiler holds the schema.
+Path B (runtime schema / facet-styx) stays reachable on the same `Peek` +
+facet-git-tree substrate, so nothing here is wasted, but is not built now.
+
+=== Ref layout
+
+----
+refs/meta/members write-access keys read ONLY by pre-receive
+refs/meta/config repo metadata read by UI chrome / write path
+refs/meta/checks check definitions self-contained
+refs/meta/runs/<commit> run logs self-contained
+refs/meta/issues/<id> issue docs (future) self-contained; labels are strings
+----
+
+* *`members`* (rename of today's `auth`) — the OpenSSH keys whose signed pushes
+ are accepted. Named for the project concept (*not* "contributors", which are
+ commit authors); the document stays keyed by fingerprint → key (a member is
+ one or more keys). Kept on its own ref because it is the *trust root*:
+ `pre-receive` parses exactly this one well-known document on every push, so
+ unrelated config can never widen its parse surface or share its history.
+
+* *`config`* — the repo's loose metadata promoted to first-class, members-gated,
+ versioned data. Today exactly:
++
+[source,rust]
+----
+struct Config {
+ description: String, // was git's `.git/description` file
+ homepage: String, // "" when unset; the About card link
+ topics: Vec<String>, // migrated off the tracked HEAD:.gitents/topics file
+}
+----
++
+`topics` is the important migration: today it lives in the worktree
+(`HEAD:.gitents/topics`), versioned with code and editable by anyone who can
+push content. Moving it into `config` makes it members-gated, out of the
+worktree, and independently historied.
+
+*Principle: no project metadata in the worktree — there is never a `.gitents`
+directory.* All project metadata lives on `refs/meta/*`. The lone offender is the
+topics read at `web/mod.rs:122` (`HEAD:.gitents/topics`); the `topics` migration
+removes it and the convention for good.
+
+=== Principles that fix the apparent tensions
+
+* *Decomposed refs, aggregated views.* Storage is normalized — one ref per
+ concern, each independently loadable. Presentation denormalizes — the Settings
+ page is a *projection* that reads members + config + (derived) checks/releases
+ and stacks them as sections. There is no `refs/meta/settings` god-doc; the
+ unification is in the renderer, not the data. (The repo header band already
+ aggregates this way.)
+* *A content loader never reads another ref.* Config is read only by the write
+ path and by aggregating views (UI chrome), never by load-one-issue /
+ load-one-check. This is what keeps the components decoupled.
+* *Editing a unified page over decomposed refs* is either per-section save (one
+ ref per form, as forges actually do) or one `git push --atomic …` across the
+ refs — the latter is exactly the "atomic updates across multiple refs" the
+ README claims, so the decomposition is what makes atomic settings meaningful.
+
+=== Deliberately excluded from config (the membership test)
+
+Belongs in config only if repo-global policy that *can't be derived from
+content* AND is read by the write path or UI chrome (never a content loader):
+
+* *Default branch* — derivable; it is `HEAD`.
+* *Primary language / license* — derivable from content (extensions; `LICENSE`).
+* *Visibility (public/private)* — not derivable, but unenforced today (access is
+ open), so it would be a fake control. Add it when read-auth lands.
+* *Feature toggles* — a toggle means only a deliberate opt-*out* of an otherwise
+ available feature; the UI already shows "enabled but empty" via blankslates,
+ and nothing needs opt-out yet. The Settings "Features" card shows *derived,
+ read-only* status, not fake switches.
+* *Issue labels / templates* — component-owned; issues carry labels as strings,
+ and the index derives its filter set from the labels that exist.
+
+== Proposed execution order
+
+Each as its own commit, tests run between:
+
+. P8 → P3 → P7 (smallest, lowest risk)
+. P2 → P1 (the structural dedup wins)
+. P4 / P5 (after decisions)
+. P6 (last, behind the integration tests)
+. Correctness C1–C4 as separate hardening commits.
+
+== Open decisions
+
+None — all resolved.
+
+Resolved: P3 (`tempfile` → normal dependency, approved); P4 (keep `Render`); P5
+(make Settings/Issues functional); config altitude = Path A (compile-time);
+write-access set named `members`; config = `{ description, homepage, topics }`;
+decomposed refs with aggregated views; no worktree metadata (no `.gitents`).