9cfb8ccf

Read the shapes the user's own repositories are actually in

a73x 2026-08-14 09:07

Commit message

Read the shapes the user's own repositories are actually in

The strip (e5096ffc) cleared its removals against one clone and the five
server repositories. The user's other clones sat in ~/code/rad the whole
time and were never opened. This re-audits every event.json in every
collab ref of every repository under ~/code/rad — 534 events, 5
repositories with collab refs — and restores exactly what that data
holds.

What the audit found, per repository:

  eitri     243 events   17 patch.create with no commit/tree
                         21 patch.review with no revision
                          1 patch.revise
                         17 patch.merge with no commit
  waystty   258 events   22 patch.create with no commit/tree
                         12 patch.review with no revision
                          6 patch.merge with no commit
                         11 bare `patches/<id>` refs
  mux        28 events   clean
  nono        3 events   clean
  claudealong 2 events   clean

That is 100% of the patches in both repositories, and it is why eitri
listed 17 warnings and `No patches found.` The failing field was not
`patch.merge` as reported but `patch.create` — `missing field commit at
line 14 column 3` is the closing brace of its action object.

Restored, each because data someone holds is in that shape and no
migration can invent what was never recorded:

- PatchCreate.commit/tree and PatchRevision.commit/tree default to empty
  and skip when empty. This is the same argument Revision.base was
  already kept on: the value was never computed, and recomputing against
  the branch as it stands now answers a different question.
- PatchReview.revision as Option. The field, not the attribution — a
  reader recovering a missing revision from DAG position is what issue
  33b5e541 is about, and it stays gone. `None` is its own bucket in vote
  supersession, so a legacy review cannot retract a current vote.
- patch.revise, as its own variant rather than a serde alias. An alias
  re-serializes under the new name, and verification compares
  re-serialized bytes, so it would read the patch locally and have sync
  reject it as unsigned.
- The bare `patches/<id>` ref layout, read and left alone. sync
  reconciles into whichever ref is already there, and a clone with no
  copy of a patch takes the remote's layout rather than migrating it
  inbound — otherwise it could read the project and never push to it.

Left stripped, zero occurrences in the audit and only ever written by
code that no longer exists: the `PatchCreate` variant name, the
`head_commit` alias, singular-string relates_to, PatchState.labels
serde(default), resolve_head's fallback to refs/heads/<branch>, the ref
layout migration and `git-collab-server migrate`, and the `<id>/r/<n>`
numbering — which is a revision ref rather than a DAG, so reading it
would yield nothing to read.

Two messages rewritten, both of which offered a remedy that cannot be
followed when every copy of a repository is old, which is the ordinary
case:

- "fetch the patch from a remote holding the current layout, or delete
  the stale ref" now says the shape predates the current naming, that
  fixing it is local, and prints the `git update-ref` that does it.
- resolve_head told a patch that never recorded a commit to fetch its
  revision refs. It now distinguishes "recorded and not here" from "never
  recorded", and says the rest of the patch reads normally — which it
  does.

Verified in the repository that reported this: eitri lists all 17
patches with no warnings and syncs both remotes clean, 17 reconciled and
17 pushed, with its refs byte-identical before and after. waystty's 11
bare refs read too.

Fixes 05b18df6

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

src/dag.rs

Old		New
@@ -355,6 +355,7 @@ fn commit_message(action: &Action) -> String {
355	Action::PatchLabel { ref label } => format!("patch: label \"{}\"", label),	355	Action::PatchLabel { ref label } => format!("patch: label \"{}\"", label),
356	Action::PatchUnlabel { ref label } => format!("patch: unlabel \"{}\"", label),	356	Action::PatchUnlabel { ref label } => format!("patch: unlabel \"{}\"", label),
357	Action::PatchRevision { .. } => "patch: revision".to_string(),	357	Action::PatchRevision { .. } => "patch: revision".to_string(),
		358	Action::PatchRevise { .. } => "patch: revision (legacy)".to_string(),
358	Action::PatchReview { verdict, .. } => format!("patch: review ({})", verdict),	359	Action::PatchReview { verdict, .. } => format!("patch: review ({})", verdict),
359	Action::PatchComment { .. } => "patch: comment".to_string(),	360	Action::PatchComment { .. } => "patch: comment".to_string(),
360	Action::PatchInlineComment { ref file, line, .. } => {	361	Action::PatchInlineComment { ref file, line, .. } => {

src/event.rs

Old		New
@@ -54,11 +54,11 @@ pub enum Action {
54	},	54	},
55	/// A patch, and the commit and tree revision 1 stands at.	55	/// A patch, and the commit and tree revision 1 stands at.
56	///	56	///
57	/// Pre-release shapes omitted `commit`/`tree` entirely, named the variant	57	/// The variant name `PatchCreate` and the `head_commit` spelling of the
58	/// `PatchCreate`, and put the head in `head_commit`. All three are gone	58	/// head are gone (issue `e5096ffc`) and stay gone: the audit behind issue
59	/// (issue `e5096ffc`): every event in every repository we host was checked	59	/// `05b18df6` re-checked every `event.json` in every collab ref of every
60	/// for them first, and `commit` is now required, so a missing one is a	60	/// repository under `~/code/rad` and found neither. Code that only ever
61	/// deserialize error rather than a silent empty string meaning "unknown".	61	/// wrote a shape nothing holds is dead code, and removing it was right.
62	#[serde(rename = "patch.create")]	62	#[serde(rename = "patch.create")]
63	PatchCreate {	63	PatchCreate {
64	title: String,	64	title: String,
@@ -67,7 +67,32 @@ pub enum Action {
67	branch: String,	67	branch: String,
68	#[serde(default, skip_serializing_if = "Option::is_none")]	68	#[serde(default, skip_serializing_if = "Option::is_none")]
69	fixes: Option<String>,	69	fixes: Option<String>,
		70	/// The commit revision 1 stands at, or `""` for a patch created before
		71	/// the create event recorded one.
		72	///
		73	/// The absence is real and permanent, not migration debt. These
		74	/// events were signed without the key; nothing can compute what the
		75	/// branch pointed at nine months ago, and guessing from the branch of
		76	/// that name resolves to whatever it holds now. So "no recorded
		77	/// commit" is a state this type has to be able to say — the same
		78	/// argument, and the same conclusion, as `PatchRevision::base`.
		79	///
		80	/// Making it required (issue `e5096ffc`) is what broke the user's
		81	/// repositories: 17 patches in `~/code/rad/eitri` and 22 in
		82	/// `~/code/rad/waystty` carry no `commit`, and each one failed to
		83	/// deserialize with `missing field \`commit\`` — 100% of the patches in
		84	/// both. The audit that cleared the removal covered one clone and the
		85	/// hosted server repositories, and neither of those was in it.
		86	///
		87	/// Skipped when empty so an event that never carried the key
		88	/// re-serializes byte for byte. That is not cosmetic: signatures are
		89	/// verified by re-serializing (`signing::canonical_json`), so writing
		90	/// back a key the signer never wrote would fail verification on every
		91	/// one of these events and make `sync` reject the patch outright.
		92	#[serde(default, skip_serializing_if = "String::is_empty")]
70	commit: String,	93	commit: String,
		94	/// Tree OID, recorded alongside `commit` and absent in the same events.
		95	#[serde(default, skip_serializing_if = "String::is_empty")]
71	tree: String,	96	tree: String,
72	/// Where revision 1 branched off `base_ref`: the merge-base of the	97	/// Where revision 1 branched off `base_ref`: the merge-base of the
73	/// base branch and `commit`, recorded when the patch is created. Named	98	/// base branch and `commit`, recorded when the patch is created. Named
@@ -78,13 +103,14 @@ pub enum Action {
78	},	103	},
79	/// A new revision of a patch: the commit and tree it now stands at.	104	/// A new revision of a patch: the commit and tree it now stands at.
80	///	105	///
81	/// Once written as `patch.revise` carrying only a note, before revisions	106	/// `commit` and `tree` are empty for a revision recorded before they were
82	/// recorded what they pointed at. That alias is gone (issue `e5096ffc`)	107	/// stored, for the reasons set out on [`Action::PatchCreate::commit`], and
83	/// along with the empty-string default for `commit`/`tree`, having been	108	/// skipped when empty for the same signature reason.
84	/// confirmed absent everywhere first.
85	#[serde(rename = "patch.revision")]	109	#[serde(rename = "patch.revision")]
86	PatchRevision {	110	PatchRevision {
		111	#[serde(default, skip_serializing_if = "String::is_empty")]
87	commit: String,	112	commit: String,
		113	#[serde(default, skip_serializing_if = "String::is_empty")]
88	tree: String,	114	tree: String,
89	#[serde(default, skip_serializing_if = "Option::is_none")]	115	#[serde(default, skip_serializing_if = "Option::is_none")]
90	body: Option<String>,	116	body: Option<String>,
@@ -106,25 +132,53 @@ pub enum Action {
106	#[serde(default, skip_serializing_if = "Option::is_none")]	132	#[serde(default, skip_serializing_if = "Option::is_none")]
107	base: Option<String>,	133	base: Option<String>,
108	},	134	},
109	/// A review, always scoped to the revision it reviewed.	135	/// A revision recorded before revisions recorded anything but a note.
		136	///
		137	/// Read-only. Nothing writes this, and nothing should: `PatchRevision`
		138	/// is what a revision is now. It exists because one of these is sitting in
		139	/// `~/code/rad/eitri` (`{"type": "patch.revise", "body": null}`) and a
		140	/// patch holding it was skipped entirely without it.
		141	///
		142	/// Deliberately its own variant rather than
		143	/// `#[serde(alias = "patch.revise")]` on `PatchRevision`, which is how the
		144	/// pre-strip reader did it. An alias reads the event under the old name and
		145	/// writes it back under the new one, and signature verification compares
		146	/// re-serialized bytes — so an alias reads the patch locally and then has
		147	/// `sync` reject it as unsigned, which is a worse failure than the one it
		148	/// fixes because it looks like tampering. Keeping the tag keeps the
		149	/// signature.
		150	#[serde(rename = "patch.revise")]
		151	PatchRevise {
		152	/// The note, which is all this shape ever carried. `Option` because the
		153	/// one real event has `"body": null`, and defaulted so a copy that
		154	/// omitted the key still reads.
		155	#[serde(default)]
		156	body: Option<String>,
		157	},
		158	/// A review, scoped to the revision it reviewed when the event says so.
110	///	159	///
111	/// `revision` was once optional, on reviews written before reviews were	160	/// `revision` is `None` for a review written before reviews were
112	/// revision-scoped, and a reader recovered it from the event's position in	161	/// revision-scoped: 21 of them in `~/code/rad/eitri`, 12 in
113	/// the DAG. That was a weaker guarantee than the rest of the event carried	162	/// `~/code/rad/waystty`. Requiring it (issue `e5096ffc`) made every patch
114	/// — position is not signed, and two clones holding the same events joined	163	/// carrying one unreadable.
115	/// in different parent orders could attribute the same review to different
116	/// revisions, which fed the vote-supersession rule and could drop a vote
117	/// rather than merely mislabel one (issue 33b5e541).
118	///	164	///
119	/// Required now (issue `e5096ffc`), confirmed after checking every review	165	/// **Restoring the field is not restoring the attribution, and the
120	/// event in every repository we host: the revision is part of the signed	166	/// difference is the whole point.** A reader used to recover a missing
121	/// payload, so attribution no longer depends on anything a rewrite could	167	/// revision from the event's position in the DAG. That was the one place
122	/// change.	168	/// derived state depended on the DAG's shape rather than its contents:
		169	/// signatures cover event content but not parent links, so two clones
		170	/// holding the same events joined in different parent orders could
		171	/// attribute one review to different revisions — and because attribution
		172	/// feeds vote supersession, that could drop a vote rather than merely
		173	/// mislabel it (issue 33b5e541). The guessing stays gone. `None` stays,
		174	/// because it is what the event says, and it says the same thing in every
		175	/// clone.
123	#[serde(rename = "patch.review")]	176	#[serde(rename = "patch.review")]
124	PatchReview {	177	PatchReview {
125	verdict: ReviewVerdict,	178	verdict: ReviewVerdict,
126	body: String,	179	body: String,
127	revision: u32,	180	#[serde(default, skip_serializing_if = "Option::is_none")]
		181	revision: Option<u32>,
128	},	182	},
129	#[serde(rename = "patch.label")]	183	#[serde(rename = "patch.label")]
130	PatchLabel { label: String },	184	PatchLabel { label: String },
@@ -169,6 +223,12 @@ pub enum Action {
169	/// are verified by re-serializing, and writing back a field the signer	223	/// are verified by re-serializing, and writing back a field the signer
170	/// never wrote would invalidate every one of them. An empty `commit` means	224	/// never wrote would invalidate every one of them. An empty `commit` means
171	/// "not recorded", never "the null OID".	225	/// "not recorded", never "the null OID".
		226	///
		227	/// 17 such merges are in `~/code/rad/eitri` and 6 in `~/code/rad/waystty`.
		228	/// This tolerance survived issue `e5096ffc` — the field was already
		229	/// documented as permanently unknowable — and the argument that kept it is
		230	/// the one that should have kept `PatchCreate::commit` and
		231	/// `PatchReview::revision` too.
172	#[serde(rename = "patch.merge")]	232	#[serde(rename = "patch.merge")]
173	PatchMerge {	233	PatchMerge {
174	#[serde(default, skip_serializing_if = "String::is_empty")]	234	#[serde(default, skip_serializing_if = "String::is_empty")]

src/lib.rs

Old		New
@@ -821,7 +821,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
821	let reviews: Vec<_> = if let Some(rev) = revision {	821	let reviews: Vec<_> = if let Some(rev) = revision {
822	p.reviews	822	p.reviews
823	.iter()	823	.iter()
824	.filter(\|r\| r.revision == rev)	824	.filter(\|r\| r.revision == Some(rev))
825	.collect()	825	.collect()
826	} else {	826	} else {
827	p.reviews.iter().collect()	827	p.reviews.iter().collect()
@@ -829,7 +829,14 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
829	if !reviews.is_empty() {	829	if !reviews.is_empty() {
830	println!("\n--- Reviews ---");	830	println!("\n--- Reviews ---");
831	for r in &reviews {	831	for r in &reviews {
832	let rev_label = format!(" (r{})", r.revision);	832	// Nothing for a review that recorded no revision. The
		833	// same rule inline comments already follow: an absent
		834	// anchor is shown as absent, never as `(r?)` or as a
		835	// revision it was not cast against.
		836	let rev_label = r
		837	.revision
		838	.map(\|n\| format!(" (r{})", n))
		839	.unwrap_or_default();
833	println!(	840	println!(
834	"\n{} ({}) - {}{}{} [{:.8}]:\n{}",	841	"\n{} ({}) - {}{}{} [{:.8}]:\n{}",
835	r.author.name,	842	r.author.name,
@@ -943,8 +950,17 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
943	);	950	);
944	}	951	}
945	}	952	}
		953	// The exit code carries the failure; the explanation was
		954	// already printed as a warning beside the `Head:` line it
		955	// belongs to. Repeating a paragraph verbatim at the bottom
		956	// reads as two problems rather than one, so this restates it in
		957	// a line and leaves the detail where the context is.
946	match head_failure {	958	match head_failure {
947	Some(e) => Err(e),	959	Some(_) => Err(error::Error::Cmd(format!(
		960	"patch {:.8} has no resolvable head — see the warning above; \
		961	everything else about the patch is shown",
		962	p.id
		963	))),
948	None => Ok(()),	964	None => Ok(()),
949	}	965	}
950	}	966	}

src/log.rs

Old		New
@@ -124,6 +124,7 @@ fn action_type_name(action: &Action) -> String {
124	Action::PatchLabel { .. } => "PatchLabel".to_string(),	124	Action::PatchLabel { .. } => "PatchLabel".to_string(),
125	Action::PatchUnlabel { .. } => "PatchUnlabel".to_string(),	125	Action::PatchUnlabel { .. } => "PatchUnlabel".to_string(),
126	Action::PatchRevision { .. } => "PatchRevision".to_string(),	126	Action::PatchRevision { .. } => "PatchRevision".to_string(),
		127	Action::PatchRevise { .. } => "PatchRevise".to_string(),
127	Action::PatchReview { .. } => "PatchReview".to_string(),	128	Action::PatchReview { .. } => "PatchReview".to_string(),
128	Action::PatchComment { .. } => "PatchComment".to_string(),	129	Action::PatchComment { .. } => "PatchComment".to_string(),
129	Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(),	130	Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(),
@@ -173,6 +174,13 @@ fn action_summary(action: &Action) -> String {
173	Some(b) => format!("revision: {}", truncate(b, 50)),	174	Some(b) => format!("revision: {}", truncate(b, 50)),
174	None => "revision".to_string(),	175	None => "revision".to_string(),
175	},	176	},
		177	// Named for what it is rather than folded in with `PatchRevision`:
		178	// the log is an audit trail, and an event written under the older
		179	// name should read under the older name.
		180	Action::PatchRevise { body } => match body {
		181	Some(b) => format!("revision (legacy): {}", truncate(b, 50)),
		182	None => "revision (legacy)".to_string(),
		183	},
176	Action::PatchReview { verdict, .. } => format!("review: {}", verdict),	184	Action::PatchReview { verdict, .. } => format!("review: {}", verdict),
177	Action::PatchComment { body } => truncate(body, 60),	185	Action::PatchComment { body } => truncate(body, 60),
178	Action::PatchInlineComment { file, line, .. } => format!("comment on {}:{}", file, line),	186	Action::PatchInlineComment { file, line, .. } => format!("comment on {}:{}", file, line),

src/patch.rs

Old		New
@@ -368,8 +368,12 @@ pub fn list_json(
368	/// Thread comments are deliberately left alone: they are not anchored to a	368	/// Thread comments are deliberately left alone: they are not anchored to a
369	/// revision at all, so filtering them by one would silently hide the whole	369	/// revision at all, so filtering them by one would silently hide the whole
370	/// discussion. The plain renderer makes the same choice.	370	/// discussion. The plain renderer makes the same choice.
		371	///
		372	/// A review that records no revision is not anchored to this one either, so it
		373	/// goes — the same treatment inline comments have always had. Keeping it would
		374	/// mean claiming it was cast against a revision the event never named.
371	fn only_revision(patch: &mut PatchState, revision: u32) {	375	fn only_revision(patch: &mut PatchState, revision: u32) {
372	patch.reviews.retain(\|r\| r.revision == revision);	376	patch.reviews.retain(\|r\| r.revision == Some(revision));
373	patch	377	patch
374	.inline_comments	378	.inline_comments
375	.retain(\|c\| c.revision == Some(revision));	379	.retain(\|c\| c.revision == Some(revision));
@@ -952,7 +956,7 @@ pub fn review(
952	// A different verdict is allowed and supersedes the previous vote.	956	// A different verdict is allowed and supersedes the previous vote.
953	if verdict.is_vote() {	957	if verdict.is_vote() {
954	let duplicate = patch.reviews.iter().any(\|r\| {	958	let duplicate = patch.reviews.iter().any(\|r\| {
955	r.verdict == verdict && r.author.email == author.email && r.revision == rev	959	r.verdict == verdict && r.author.email == author.email && r.revision == Some(rev)
956	});	960	});
957	if duplicate {	961	if duplicate {
958	return Err(Error::Cmd(format!(	962	return Err(Error::Cmd(format!(
@@ -968,7 +972,9 @@ pub fn review(
968	action: Action::PatchReview {	972	action: Action::PatchReview {
969	verdict,	973	verdict,
970	body: body.to_string(),	974	body: body.to_string(),
971	revision: rev,	975	// Always recorded on the way out. `None` is a shape this reader
		976	// must be able to read, never one it may write.
		977	revision: Some(rev),
972	},	978	},
973	clock: 0,	979	clock: 0,
974	};	980	};

src/refs.rs

Old		New
@@ -312,16 +312,39 @@ pub fn render(refs: &[CollabRef], out: &mut impl Write) -> std::io::Result<()> {
312	writeln!(out)?;	312	writeln!(out)?;
313	writeln!(out, "{} collab refs: {}.", refs.len(), groups(refs))?;	313	writeln!(out, "{} collab refs: {}.", refs.len(), groups(refs))?;
314		314
315	let legacy = legacy_count(refs);	315	// The two older shapes are not in the same situation, so they cannot share
316	if legacy > 0 {	316	// a sentence. A bare patch ref is read — reporting it as unreadable sent
		317	// the operator looking for a migration that does not exist and is not
		318	// needed. A numbered revision ref is genuinely refused, and that is what
		319	// stops every other command in the repository.
		320	let bare = refs
		321	.iter()
		322	.filter(\|r\| r.kind == RefKind::PatchEventsLegacy)
		323	.count();
		324	if bare > 0 {
		325	writeln!(
		326	out,
		327	"{} in the older bare `patches/<id>` layout — read as they are, and \
		328	left as they are. Nothing needs doing: a patch created here from now \
		329	on gets the current layout, and these keep working beside it.",
		330	bare
		331	)?;
		332	}
		333
		334	let numbered = refs
		335	.iter()
		336	.filter(\|r\| r.kind == RefKind::PatchRevisionNumbered)
		337	.count();
		338	if numbered > 0 {
317	writeln!(	339	writeln!(
318	out,	340	out,
319	"{} in a superseded layout ({}) — no version still standing reads these, \	341	"{} in the superseded `<id>/r/<n>` revision numbering — this version \
320	and every other git-collab command in this repository will refuse. \	342	does not read these, and every other git-collab command in this \
321	Fetch the patch from a remote holding the current layout, or delete the \	343	repository will refuse until they are gone. They pin revision commits \
322	stale ref if it exists nowhere else.",	344	and carry no events, so renaming them to `<id>/rev/<commit-oid>` with \
323	legacy,	345	`git update-ref`, or deleting them, loses nothing. Both are local; no \
324	legacy_breakdown(refs)	346	other copy of this repository is involved.",
		347	numbered
325	)?;	348	)?;
326	}	349	}
327	Ok(())	350	Ok(())
@@ -490,16 +513,58 @@ mod tests {
490	}	513	}
491		514
492	#[test]	515	#[test]
493	fn the_summary_names_the_legacy_refs_it_found() {	516	fn the_summary_names_the_older_layout_without_calling_it_broken() {
		517	// The bare layout is read (issue `05b18df6`), so the summary counts it
		518	// and stops there. It used to say every other command would refuse,
		519	// which was true for a while and sent the operator hunting for a
		520	// migration that no longer exists — the worst kind of stale message,
		521	// because it is specific enough to be believed.
494	let mut out = Vec::new();	522	let mut out = Vec::new();
495	render(&sample(), &mut out).unwrap();	523	render(&sample(), &mut out).unwrap();
496	let text = String::from_utf8(out).unwrap();	524	let text = String::from_utf8(out).unwrap();
497	assert!(text.contains("2 collab refs: 2 patches."), "got {}", text);	525	assert!(text.contains("2 collab refs: 2 patches."), "got {}", text);
498	assert!(	526	assert!(
499	text.contains("1 in a superseded layout (1 bare patch ref)"),	527	text.contains("1 in the older bare `patches/<id>` layout"),
500	"got {}",	528	"got {}",
501	text	529	text
502	);	530	);
		531	assert!(
		532	!text.contains("will refuse"),
		533	"the bare layout is read, so nothing may claim otherwise: {}",
		534	text
		535	);
		536	}
		537
		538	#[test]
		539	fn the_summary_says_what_to_do_about_a_numbered_revision_ref() {
		540	// This one is refused, so the summary has to say so — and has to
		541	// offer a remedy that works when every copy of the repository is the
		542	// same age, which is the case the old "fetch it from a remote holding
		543	// the current layout" advice could not cover.
		544	let refs: Vec<CollabRef> = ["refs/collab/patches/a/events", "refs/collab/patches/a/r/1"]
		545	.into_iter()
		546	.map(\|name\| {
		547	let (kind, id, archived) = classify(name);
		548	CollabRef {
		549	name: name.to_string(),
		550	target: "0".repeat(40),
		551	kind,
		552	id,
		553	archived,
		554	}
		555	})
		556	.collect();
		557
		558	let mut out = Vec::new();
		559	render(&refs, &mut out).unwrap();
		560	let text = String::from_utf8(out).unwrap();
		561	assert!(text.contains("will refuse"), "got {}", text);
		562	assert!(text.contains("git update-ref"), "got {}", text);
		563	assert!(
		564	!text.contains("Fetch the patch from a remote"),
		565	"the remedy must not require a newer copy to exist: {}",
		566	text
		567	);
503	}	568	}
504		569
505	#[test]	570	#[test]

src/server/http/repo/patches.rs

Old		New
@@ -338,7 +338,7 @@ pub async fn patch_detail(
338	verdict: r.verdict.as_str().to_string(),	338	verdict: r.verdict.as_str().to_string(),
339	body: r.body,	339	body: r.body,
340	timestamp: Timestamp::new(r.timestamp),	340	timestamp: Timestamp::new(r.timestamp),
341	revision: Some(r.revision),	341	revision: r.revision,
342	edited: r.edited,	342	edited: r.edited,
343	})	343	})
344	.collect(),	344	.collect(),

src/server/refs.rs

Old		New
@@ -99,7 +99,7 @@ fn render(repos_dir: &Path, scanned: &[(String, Outcome)]) {
99	if legacy > 0 {	99	if legacy > 0 {
100	legacy_repos += 1;	100	legacy_repos += 1;
101	println!(	101	println!(
102	"{}: {} collab refs — {}; {} in a superseded layout ({})",	102	"{}: {} collab refs — {}; {} in an older layout ({})",
103	name,	103	name,
104	found.len(),	104	found.len(),
105	refs::groups(found),	105	refs::groups(found),
@@ -121,7 +121,7 @@ fn render(repos_dir: &Path, scanned: &[(String, Outcome)]) {
121	let mut tail = Vec::new();	121	let mut tail = Vec::new();
122	if legacy_repos > 0 {	122	if legacy_repos > 0 {
123	tail.push(format!(	123	tail.push(format!(
124	"{} hold{} a superseded layout",	124	"{} hold{} an older layout",
125	legacy_repos,	125	legacy_repos,
126	if legacy_repos == 1 { "s" } else { "" }	126	if legacy_repos == 1 { "s" } else { "" }
127	));	127	));

src/state.rs

Old		New
@@ -287,11 +287,15 @@ pub struct Review {
287	pub verdict: ReviewVerdict,	287	pub verdict: ReviewVerdict,
288	pub body: String,	288	pub body: String,
289	pub timestamp: String,	289	pub timestamp: String,
290	/// The revision this review was cast against. Not optional: the event	290	/// The revision this review was cast against, or `None` for a review
291	/// carries it, so every review has one (issue `e5096ffc`). It was optional	291	/// written before reviews were revision-scoped.
292	/// while reviews written before revision-scoping had to be attributed from	292	///
293	/// their position in the DAG — an attribution nothing signed.	293	/// `None` is read straight off the event and never inferred. See
294	pub revision: u32,	294	/// [`crate::event::Action::PatchReview`] for why the field came back
		295	/// (33 such reviews across the user's repositories) and why the fallback
		296	/// that used to fill it in from DAG position did not.
		297	#[serde(default)]
		298	pub revision: Option<u32>,
295	/// The OID of the `PatchReview` event, so the body can be corrected. A	299	/// The OID of the `PatchReview` event, so the body can be corrected. A
296	/// review's verdict is never edited — a reviewer changes their mind by	300	/// review's verdict is never edited — a reviewer changes their mind by
297	/// submitting a new review, which supersedes the old vote through the	301	/// submitting a new review, which supersedes the old vote through the
@@ -339,12 +343,18 @@ impl fmt::Display for PatchStatus {
339	#[derive(Debug, Clone, Serialize, Deserialize)]	343	#[derive(Debug, Clone, Serialize, Deserialize)]
340	pub struct Revision {	344	pub struct Revision {
341	pub number: u32,	345	pub number: u32,
342	/// Commit OID. Always recorded: the `""`-means-unknown convention that used	346	/// Commit OID, or `""` for a revision recorded before revisions carried
343	/// to sit here — for patches created before revisions carried a commit —	347	/// one.
344	/// was removed in issue `e5096ffc` after confirming no event anywhere still	348	///
345	/// used it.	349	/// The `""`-means-unknown convention, removed in issue `e5096ffc` and back
		350	/// in `05b18df6`: 39 `patch.create` events across the user's repositories
		351	/// record no commit, and no migration can invent one. Every reader here
		352	/// already copes — [`Revision::short_commit`] renders `None`,
		353	/// [`PatchState::latest_usable_index`] skips it, and
		354	/// [`PatchState::resolve_head`] says so — which is why the removal cost a
		355	/// deserialize error rather than a wrong answer.
346	pub commit: String,	356	pub commit: String,
347	/// Tree OID, recorded alongside `commit`.	357	/// Tree OID, recorded alongside `commit` and empty in the same revisions.
348	pub tree: String,	358	pub tree: String,
349	pub body: Option<String>,	359	pub body: Option<String>,
350	pub timestamp: String,	360	pub timestamp: String,
@@ -1046,10 +1056,17 @@ impl PatchState {
1046	/// branch of that name happened to point at — a different commit than the	1056	/// branch of that name happened to point at — a different commit than the
1047	/// patch ever recorded, and no way to tell from the output.	1057	/// patch ever recorded, and no way to tell from the output.
1048	///	1058	///
1049	/// So a head that cannot be found is an error and never a substitute. The	1059	/// So a head that cannot be found is an error and never a substitute.
1050	/// message names the revision it could not resolve, because the cause is	1060	///
1051	/// almost always objects that were never fetched rather than anything wrong	1061	/// Two different things stop it, and they get two different messages
1052	/// with the patch.	1062	/// because they have two different remedies. A commit that was recorded
		1063	/// and is not here is a fetch away. A commit that was never recorded is not
		1064	/// anywhere, in any clone: patches created before revisions carried a
		1065	/// commit have nothing to fetch and nothing to wait for, and telling their
		1066	/// owner to fetch harder is the failure issue `05b18df6` was about. What
		1067	/// remains readable of such a patch — its title, revisions, reviews and
		1068	/// comments — is printed regardless; only the diff is unavailable, and the
		1069	/// message says which.
1053	pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> {	1070	pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> {
1054	if let Some(oid) = self.latest_usable_commit(repo) {	1071	if let Some(oid) = self.latest_usable_commit(repo) {
1055	return Ok(oid);	1072	return Ok(oid);
@@ -1059,16 +1076,21 @@ impl PatchState {
1059	.last()	1076	.last()
1060	.map(\|r\| r.commit.as_str())	1077	.map(\|r\| r.commit.as_str())
1061	.unwrap_or_default();	1078	.unwrap_or_default();
		1079	if recorded.is_empty() {
		1080	return Err(crate::error::Error::Cmd(format!(
		1081	"patch {:.8} was recorded before patches stored the commit they stand \
		1082	at, so it has no head to resolve and no diff to show. Nothing is \
		1083	missing from this clone and there is nothing to fetch — the event \
		1084	never carried one. Its title, revisions, reviews and comments all \
		1085	read normally.",
		1086	self.id
		1087	)));
		1088	}
1062	Err(crate::error::Error::Cmd(format!(	1089	Err(crate::error::Error::Cmd(format!(
1063	"patch {:.8} records revision commit {} but its objects are not in this \	1090	"patch {:.8} records revision commit {} but its objects are not in this \
1064	repository — fetch the patch's revision refs, or the commit it stands on, \	1091	repository — fetch the patch's revision refs, or the commit it stands on, \
1065	before reading it",	1092	before reading it",
1066	self.id,	1093	self.id, recorded
1067	if recorded.is_empty() {
1068	"(none)"
1069	} else {
1070	recorded
1071	}
1072	)))	1094	)))
1073	}	1095	}
1074		1096
@@ -1311,12 +1333,14 @@ impl PatchState {
1311	base,	1333	base,
1312	} => {	1334	} => {
1313	if let Some(ref mut s) = state {	1335	if let Some(ref mut s) = state {
1314	// Dedup by commit OID — skip if already seen. The guard	1336	// Dedup by commit OID — skip if already seen. An empty
1315	// that used to exempt an empty commit went with the	1337	// commit is exempt: "" is not an OID, it is the absence
1316	// `""`-means-unknown convention (issue `e5096ffc`);	1338	// of one, so two revisions that both recorded nothing
1317	// every revision records a commit, so every revision	1339	// are not the same revision. Treating them as one would
1318	// can be compared by it.	1340	// drop a step of the patch's history and renumber every
1319	let already_seen = s.revisions.iter().any(\|r\| r.commit == commit);	1341	// revision after it.
		1342	let already_seen =
		1343	!commit.is_empty() && s.revisions.iter().any(\|r\| r.commit == commit);
1320	if !already_seen {	1344	if !already_seen {
1321	overrides.note_owner(oid, &event.author);	1345	overrides.note_owner(oid, &event.author);
1322	let number = s.revisions.len() as u32 + 1;	1346	let number = s.revisions.len() as u32 + 1;
@@ -1334,6 +1358,27 @@ impl PatchState {
1334	}	1358	}
1335	}	1359	}
1336	}	1360	}
		1361	// A revision from before revisions recorded a commit. It says
		1362	// only that one happened, and possibly why; the fold records
		1363	// exactly that and invents nothing. Never written — see
		1364	// `Action::PatchRevise`.
		1365	Action::PatchRevise { body } => {
		1366	if let Some(ref mut s) = state {
		1367	overrides.note_owner(oid, &event.author);
		1368	let number = s.revisions.len() as u32 + 1;
		1369	s.revisions.push(Revision {
		1370	number,
		1371	commit: String::new(),
		1372	tree: String::new(),
		1373	body,
		1374	timestamp: event.timestamp.clone(),
		1375	base: None,
		1376	event_id: oid.to_string(),
		1377	author: Some(event.author.clone()),
		1378	edited: false,
		1379	});
		1380	}
		1381	}
1337	Action::PatchReview {	1382	Action::PatchReview {
1338	verdict,	1383	verdict,
1339	body,	1384	body,
@@ -1341,7 +1386,9 @@ impl PatchState {
1341	} => {	1386	} => {
1342	if let Some(ref mut s) = state {	1387	if let Some(ref mut s) = state {
1343	// The revision comes off the event, which means it is	1388	// The revision comes off the event, which means it is
1344	// covered by the event's signature.	1389	// covered by the event's signature — including when the
		1390	// event carries none, in which case it stays `None`
		1391	// here rather than being filled in.
1345	//	1392	//
1346	// It used to be recoverable from the event's position	1393	// It used to be recoverable from the event's position
1347	// in the walk when absent, for reviews written before	1394	// in the walk when absent, for reviews written before
@@ -1354,13 +1401,22 @@ impl PatchState {
1354	// attribute one review to different revisions. Because	1401	// attribute one review to different revisions. Because
1355	// attribution feeds the vote-supersession rule just	1402	// attribution feeds the vote-supersession rule just
1356	// below, that divergence could drop a vote rather	1403	// below, that divergence could drop a vote rather
1357	// than merely relabel it. Removing the fallback (issue	1404	// than merely relabel it. That fallback stays removed
1358	// `e5096ffc`) removes that whole class of divergence;	1405	// (issue `e5096ffc`) — restoring the field in
1359	// issue 33b5e541 tracks the remaining ordering work.	1406	// `05b18df6` is not restoring the guessing; issue
		1407	// 33b5e541 tracks the remaining ordering work.
1360	//	1408	//
1361	// A reviewer holds one current vote per revision: a new	1409	// A reviewer holds one current vote per revision: a new
1362	// vote supersedes their previous one. Comment-verdict	1410	// vote supersedes their previous one. Comment-verdict
1363	// reviews are not votes and accumulate.	1411	// reviews are not votes and accumulate.
		1412	//
		1413	// An unrecorded revision is a bucket of its own rather
		1414	// than a wildcard, which is what comparing `Option`s
		1415	// gives: an unattributed vote supersedes only another
		1416	// unattributed vote by the same author. Matching it
		1417	// against every revision would let a review from before
		1418	// revision-scoping silently retract a current one —
		1419	// the same vote-dropping failure by another route.
1364	if verdict.is_vote() {	1420	if verdict.is_vote() {
1365	s.reviews.retain(\|r\| {	1421	s.reviews.retain(\|r\| {
1366	!(r.verdict.is_vote()	1422	!(r.verdict.is_vote()
@@ -1653,23 +1709,34 @@ fn refs_under(
1653	/// Enumerate the event DAG ref of every patch under a prefix, returning	1709	/// Enumerate the event DAG ref of every patch under a prefix, returning
1654	/// (ref_name, id) pairs.	1710	/// (ref_name, id) pairs.
1655	///	1711	///
1656	/// One layout, and reading only: the DAG is at `<id>/events`, and the	1712	/// Two layouts are read. The current one puts the DAG at `<id>/events`, with
1657	/// `<id>/rev/<oid>` refs beside it are commits, not DAGs.	1713	/// `<id>/rev/<oid>` refs beside it that are commits, not DAGs. The older one is
		1714	/// a bare `<id>` ref that is the DAG, from before revisions needed a subtree
		1715	/// to live in.
		1716	///
		1717	/// The bare shape is read rather than refused, which reverses issue
		1718	/// `e5096ffc`. The argument for refusing it was that no repository still held
		1719	/// it; the audit behind issue `05b18df6` found 11 in `~/code/rad/waystty`, with
		1720	/// the same shape on its remote, which is the situation a clone of a project
		1721	/// nobody has touched in a year is always in. A repository must not become
		1722	/// unreachable because every copy of it is old — that is not a migration
		1723	/// prompt, it is a dead end, and the tool that could have migrated it is the
		1724	/// one refusing to read it.
1658	///	1725	///
1659	/// Two shapes written by pre-release versions used to be read here as well —	1726	/// Reading is not converting: nothing here rewrites the ref. A clone that
1660	/// the bare `<id>` ref that was the DAG, and the interim `<id>/r/<n>`	1727	/// silently promoted the layout would push a shape its peers cannot accept,
1661	/// revision numbering — and a migration converted them on the way past. Both	1728	/// and would do it during a command that was only ever asked to list.
1662	/// are gone (issue `e5096ffc`), verified absent from every repository we host
1663	/// before the code was removed, so meeting one now is not a shape to
1664	/// interpret: it is a repository this version cannot read.
1665	///	1729	///
1666	/// It has to say so. The failure mode worth naming is not breakage but	1730	/// The interim `<id>/r/<n>` revision numbering is still refused, on evidence
1667	/// silence: a reader that simply stopped recognising these names would skip	1731	/// rather than principle: the same audit walked every collab ref in every
1668	/// them and report a repository full of patches as empty, which is	1732	/// repository under `~/code/rad` and found none, and unlike the bare ref it is
1669	/// indistinguishable from a clean clone and impossible to diagnose from the	1733	/// a revision ref rather than a DAG, so reading it would yield nothing to
1670	/// output. So this refuses, names the ref, and points at `git-collab refs` —	1734	/// read. Refusing it strands nobody.
1671	/// which deliberately still classifies both shapes by name, and is the only	1735	///
1672	/// command that can still be run here.	1736	/// When it does refuse it has to say so. The failure mode worth naming is not
		1737	/// breakage but silence: a reader that simply stopped recognising a name would
		1738	/// skip it and report a repository full of patches as empty, which is
		1739	/// indistinguishable from a clean clone.
1673	fn patch_refs_under(	1740	fn patch_refs_under(
1674	repo: &Repository,	1741	repo: &Repository,
1675	prefix: &str,	1742	prefix: &str,
@@ -1688,11 +1755,8 @@ fn patch_refs_under(
1688	}	1755	}
1689	// A revision ref, not a patch.	1756	// A revision ref, not a patch.
1690	Some(_) => continue,	1757	Some(_) => continue,
1691	// The pre-migration layout: no suffix at all.	1758	// The older layout: no suffix at all, the ref is the DAG.
1692	None if !ref_name.strip_prefix(prefix).unwrap_or_default().is_empty() => {	1759	None => ref_name.strip_prefix(prefix).unwrap_or_default().to_string(),
1693	return Err(superseded_layout(&ref_name))
1694	}
1695	None => continue,
1696	};	1760	};
1697	if id.is_empty() {	1761	if id.is_empty() {
1698	continue;	1762	continue;
@@ -1704,14 +1768,14 @@ fn patch_refs_under(
1704		1768
1705	/// Refuse early if this repository holds a patch ref in a superseded layout.	1769	/// Refuse early if this repository holds a patch ref in a superseded layout.
1706	///	1770	///
		1771	/// Superseded now means the `<id>/r/<n>` numbering and nothing else: the bare
		1772	/// `<id>` layout is read (issue `05b18df6`), and `sync` reconciles into
		1773	/// whichever ref is already there, so it no longer needs stopping.
		1774	///
1707	/// [`patch_refs_under`] already refuses when something enumerates patches, but	1775	/// [`patch_refs_under`] already refuses when something enumerates patches, but
1708	/// `sync` reaches git before it reaches that: it fetches into	1776	/// `sync` reaches git before it reaches that, and the raw git error it would
1709	/// `refs/collab/sync/…` and then writes `<id>/events`, which against a local	1777	/// otherwise produce names no patch, no layout and no remedy — so the check has
1710	/// bare `<id>` is a directory-vs-file conflict git reports as	1778	/// to run before the first ref write, not merely somewhere on the read path.
1711	/// `could not remove directory …: parent is not directory`. That names no
1712	/// patch, no layout and no remedy, and it is the error a contributor would
1713	/// actually have hit — so the check has to run before the first ref write,
1714	/// not merely somewhere on the read path.
1715	///	1779	///
1716	/// Both namespaces, because `close` moves a patch's whole subtree into	1780	/// Both namespaces, because `close` moves a patch's whole subtree into
1717	/// `archive/` and a superseded shape moves with it.	1781	/// `archive/` and a superseded shape moves with it.
@@ -1724,21 +1788,51 @@ pub fn ensure_no_superseded_patch_refs(repo: &Repository) -> Result<(), crate::e
1724		1788
1725	/// The error for a ref in a layout no version still standing can read.	1789	/// The error for a ref in a layout no version still standing can read.
1726	///	1790	///
1727	/// Written as advice rather than as a diagnosis because there is nothing the	1791	/// The remedy has to work in the case that actually happens. The old wording
1728	/// tool can do about it: the conversion that used to run here was removed once	1792	/// offered two — "fetch the patch from a remote holding the current layout, or
1729	/// every repository we host had been through it, so the only ways out are from	1793	/// delete the stale ref if it exists nowhere else" — and neither fits a
1730	/// outside — take the current layout from a remote that has it, or drop the	1794	/// repository whose copies are all the same age, which is every repository
1731	/// ref. Both are named, since which one applies depends on whether the patch	1795	/// nobody else is working on. Advice that cannot be followed is worse than
1732	/// exists anywhere else, and only the operator knows that.	1796	/// none: it reads as help and ends in a wall, and issue `05b18df6` is what that
		1797	/// felt like from the other side.
		1798	///
		1799	/// So this says what the ref is and how to move it by hand. `<id>/r/<n>` is a
		1800	/// revision ref named by ordinal — a numbering scheme replaced by the OID it
		1801	/// pins, from a draft that never shipped. It carries no events, so nothing is
		1802	/// lost by renaming it to the OID naming or by deleting it; both are local
		1803	/// operations that need no other copy of the repository to exist.
1733	fn superseded_layout(ref_name: &str) -> crate::error::Error {	1804	fn superseded_layout(ref_name: &str) -> crate::error::Error {
		1805	let remedy = ref_name
		1806	.rsplit_once("/r/")
		1807	.map(\|(base, n)\| {
		1808	format!(
		1809	"It pins revision {} of that patch, and carries no events — the \
		1810	current naming is `<id>/rev/<commit-oid>`. Rename it:\n\
		1811	\n \
		1812	git update-ref {}/rev/$(git rev-parse {}) $(git rev-parse {})\n \
		1813	git update-ref -d {}\n\
		1814	\n\
		1815	Or delete it outright: the revision's commit stays in the object \
		1816	database either way, and the patch's own events ref is untouched.",
		1817	n, base, ref_name, ref_name, ref_name
		1818	)
		1819	})
		1820	.unwrap_or_else(\|\| {
		1821	"Move or delete it with `git update-ref`; the objects it points at are \
		1822	unaffected."
		1823	.to_string()
		1824	});
		1825
1734	crate::error::Error::Cmd(format!(	1826	crate::error::Error::Cmd(format!(
1735	"{} is in a patch ref layout written by an older git-collab, which this \	1827	"{} is in a patch ref layout written by an older git-collab, which this \
1736	version no longer reads.\n\	1828	version no longer reads.\n\
1737	Run `git-collab refs` to list every ref in a superseded layout — it is a \	1829	Run `git-collab refs` to list every ref in a superseded layout — it is a \
1738	read and still works here.\n\	1830	read and still works here.\n\
1739	To recover: fetch the patch from a remote holding the current layout, or \	1831	\n\
1740	delete the stale ref if it exists nowhere else.",	1832	No version of git-collab wrote this shape after the OID naming landed, so \
1741	ref_name	1833	there is no newer copy to fetch: fixing it is local.\n\
		1834	{}",
		1835	ref_name, remedy
1742	))	1836	))
1743	}	1837	}
1744		1838
@@ -1993,17 +2087,28 @@ pub fn unarchive_patch_ref(repo: &Repository, id: &str) -> Result<(), crate::err
1993	Ok(())	2087	Ok(())
1994	}	2088	}
1995		2089
1996	/// The two namespaces a collab object's events ref can live in, active first.	2090	/// Every name a collab object's event DAG can be under in this clone, active
		2091	/// namespace before archive and current layout before older.
1997	///	2092	///
1998	/// `kind` is `"issues"` or `"patches"`, matching `sync`'s vocabulary.	2093	/// `kind` is `"issues"` or `"patches"`, matching `sync`'s vocabulary.
1999	fn events_ref_candidates(kind: &str, id: &str) -> [String; 2] {	2094	///
		2095	/// Patches get four names rather than two because the bare `<id>` layout is
		2096	/// read again (issue `05b18df6`). Sync depends on this being complete: a clone
		2097	/// holding `<id>` bare that reconciled into `<id>/events` would be asking git
		2098	/// to create a directory where a ref already is, which is the
		2099	/// `could not remove directory …: parent is not directory` that made
		2100	/// `~/code/rad/waystty` unsyncable. Finding the ref that is already there means
		2101	/// the DAG merges into itself, whichever layout it happens to be in.
		2102	fn events_ref_candidates(kind: &str, id: &str) -> Vec<String> {
2000	if kind == "patches" {	2103	if kind == "patches" {
2001	[	2104	vec![
2002	format!("{}{}/events", PATCH_PREFIX, id),	2105	format!("{}{}/events", PATCH_PREFIX, id),
		2106	format!("{}{}", PATCH_PREFIX, id),
2003	format!("{}{}/events", ARCHIVE_PATCH_PREFIX, id),	2107	format!("{}{}/events", ARCHIVE_PATCH_PREFIX, id),
		2108	format!("{}{}", ARCHIVE_PATCH_PREFIX, id),
2004	]	2109	]
2005	} else {	2110	} else {
2006	[	2111	vec![
2007	format!("refs/collab/{}/{}", kind, id),	2112	format!("refs/collab/{}/{}", kind, id),
2008	format!("refs/collab/archive/{}/{}", kind, id),	2113	format!("refs/collab/archive/{}/{}", kind, id),
2009	]	2114	]
@@ -2094,11 +2199,19 @@ pub fn write_revision_ref(
2094	/// remote and nothing that can be rejected as a non-fast-forward.	2199	/// remote and nothing that can be rejected as a non-fast-forward.
2095	///	2200	///
2096	/// Revisions whose objects are absent are skipped; there is nothing to pin.	2201	/// Revisions whose objects are absent are skipped; there is nothing to pin.
		2202	///
		2203	/// So is every revision of a patch still in the bare `<id>` layout, and it has
		2204	/// to be: the pin would be named `<id>/rev/<oid>`, which git cannot create
		2205	/// while `<id>` is itself a ref. Nothing is lost by it — the layout predates
		2206	/// revision refs, so a patch in it records no revision commit to pin.
2097	pub fn pin_dag_revisions(	2207	pub fn pin_dag_revisions(
2098	repo: &Repository,	2208	repo: &Repository,
2099	events_ref: &str,	2209	events_ref: &str,
2100	id: &str,	2210	id: &str,
2101	) -> Result<(), crate::error::Error> {	2211	) -> Result<(), crate::error::Error> {
		2212	if !events_ref.ends_with("/events") {
		2213	return Ok(());
		2214	}
2102	let state = PatchState::from_ref_uncached(repo, events_ref, id)?;	2215	let state = PatchState::from_ref_uncached(repo, events_ref, id)?;
2103	for rev in &state.revisions {	2216	for rev in &state.revisions {
2104	let Ok(oid) = Oid::from_str(&rev.commit) else {	2217	let Ok(oid) = Oid::from_str(&rev.commit) else {

src/sync.rs

Old		New
@@ -913,9 +913,23 @@ enum SyncRef {
913	/// returning the patch/issue id alongside it. Patches own a subtree —	913	/// returning the patch/issue id alongside it. Patches own a subtree —
914	/// `<id>/events` and `<id>/r/<n>` — while issues remain a single ref.	914	/// `<id>/events` and `<id>/r/<n>` — while issues remain a single ref.
915	///	915	///
916	/// A bare `<id>` under `patches` is the pre-revision-refs layout, still pushed	916	/// A bare `<id>` under `patches` is the pre-revision-refs layout, and this
917	/// by peers that have not migrated; adopting it as the events ref migrates it	917	/// clone takes it as it finds it rather than filing it under `<id>/events`.
918	/// on the way in.	918	///
		919	/// That reverses the old "migrate on the way in", and the reason is what
		920	/// happens next. A clone that adopted the bare shape as `<id>/events` would
		921	/// push `<id>/events` back to a remote that still holds `<id>` — a
		922	/// directory-vs-file conflict git rejects outright, so the clone could read the
		923	/// project and never write to it, and the only remedy on offer would be
		924	/// deleting the remote ref every other clone still reads from. Following the
		925	/// remote's layout keeps a whole set of old clones working with each other,
		926	/// which is the state `~/code/rad/waystty` and its remote are in (issue
		927	/// `05b18df6`).
		928	///
		929	/// It also makes converting a deliberate act rather than a side effect of
		930	/// whoever synced first. `state::existing_events_ref` is consulted before this
		931	/// value is used, so a clone that already keeps the patch under `<id>/events`
		932	/// keeps it there and this never applies.
919	fn classify_sync_ref(kind: &str, rest: &str) -> Option<(String, SyncRef)> {	933	fn classify_sync_ref(kind: &str, rest: &str) -> Option<(String, SyncRef)> {
920	if kind != "patches" {	934	if kind != "patches" {
921	return Some((	935	return Some((
@@ -927,7 +941,14 @@ fn classify_sync_ref(kind: &str, rest: &str) -> Option<(String, SyncRef)> {
927	}	941	}
928	let (id, suffix) = match rest.split_once('/') {	942	let (id, suffix) = match rest.split_once('/') {
929	Some((id, suffix)) => (id, suffix),	943	Some((id, suffix)) => (id, suffix),
930	None => (rest, "events"),	944	None => {
		945	return Some((
		946	rest.to_string(),
		947	SyncRef::Events {
		948	local_ref: format!("refs/collab/patches/{}", rest),
		949	},
		950	))
		951	}
931	};	952	};
932	let classified = if suffix == "events" {	953	let classified = if suffix == "events" {
933	SyncRef::Events {	954	SyncRef::Events {

src/timeline.rs

Old		New
@@ -223,7 +223,9 @@ pub fn build(repo: &Repository, id_prefix: &str) -> Result<(PatchState, Vec<Entr
223	let timestamp = event.timestamp.clone();	223	let timestamp = event.timestamp.clone();
224		224
225	let (kind, revision) = match &event.action {	225	let (kind, revision) = match &event.action {
226	Action::PatchCreate { .. } \| Action::PatchRevision { .. } => {	226	Action::PatchCreate { .. }
		227	\| Action::PatchRevision { .. }
		228	\| Action::PatchRevise { .. } => {
227	// A revision the fold discarded as a duplicate commit has no	229	// A revision the fold discarded as a duplicate commit has no
228	// entry here, which is right: it added nothing to the patch.	230	// entry here, which is right: it added nothing to the patch.
229	let Some(rev) = revisions.get(&oid_hex) else {	231	let Some(rev) = revisions.get(&oid_hex) else {
@@ -280,7 +282,12 @@ pub fn build(repo: &Repository, id_prefix: &str) -> Result<(PatchState, Vec<Entr
280	body: r.body.clone(),	282	body: r.body.clone(),
281	edited: r.edited,	283	edited: r.edited,
282	},	284	},
283	Some(r.revision),	285	// `None` for a review written before reviews were
		286	// revision-scoped. It stays unanchored: the anchoring pass
		287	// below deliberately only touches thread comments, because
		288	// filling in a review's revision from its position is the
		289	// attribution that issue 33b5e541 is about.
		290	r.revision,
284	)	291	)
285	}	292	}
286	Action::PatchLabel { label } => (	293	Action::PatchLabel { label } => (
@@ -369,9 +376,13 @@ pub fn build(repo: &Repository, id_prefix: &str) -> Result<(PatchState, Vec<Entr
369	//	376	//
370	// By position, not by timestamp: timestamps come from whichever machine	377	// By position, not by timestamp: timestamps come from whichever machine
371	// wrote the event, so comparing two authors' clocks decides nothing, and a	378	// wrote the event, so comparing two authors' clocks decides nothing, and a
372	// skewed clock would file a comment under the wrong revision. Position in	379	// skewed clock would file a comment under the wrong revision.
373	// this sequence is derived from the DAG, which is the same basis the fold	380	//
374	// uses to attribute a revision-less review.	381	// Thread comments only. A review that recorded no revision stays
		382	// unanchored: attributing one from DAG position is exactly what issue
		383	// `e5096ffc` removed and issue `05b18df6` did not bring back — a comment
		384	// mislabelled by a revision is a display detail, whereas a review is a vote
		385	// and its revision decides which other vote it supersedes.
375	let mut current = 1;	386	let mut current = 1;
376	for entry in &mut entries {	387	for entry in &mut entries {
377	match (&entry.kind, entry.revision) {	388	match (&entry.kind, entry.revision) {
@@ -390,6 +401,7 @@ fn carries_a_body(action: &Action) -> bool {
390	action,	401	action,
391	Action::PatchCreate { .. }	402	Action::PatchCreate { .. }
392	\| Action::PatchRevision { .. }	403	\| Action::PatchRevision { .. }
		404	\| Action::PatchRevise { .. }
393	\| Action::PatchComment { .. }	405	\| Action::PatchComment { .. }
394	\| Action::PatchInlineComment { .. }	406	\| Action::PatchInlineComment { .. }
395	\| Action::PatchReview { .. }	407	\| Action::PatchReview { .. }

src/tui/mod.rs

Old		New
@@ -428,7 +428,7 @@ mod tests {
428	let action = Action::PatchReview {	428	let action = Action::PatchReview {
429	verdict: ReviewVerdict::Approve,	429	verdict: ReviewVerdict::Approve,
430	body: "lgtm".to_string(),	430	body: "lgtm".to_string(),
431	revision: 1,	431	revision: Some(1),
432	};	432	};
433	assert_eq!(action_type_label(&action), "Patch Review");	433	assert_eq!(action_type_label(&action), "Patch Review");
434	}	434	}
@@ -500,7 +500,7 @@ mod tests {
500	action: Action::PatchReview {	500	action: Action::PatchReview {
501	verdict: ReviewVerdict::Approve,	501	verdict: ReviewVerdict::Approve,
502	body: "Looks good!".to_string(),	502	body: "Looks good!".to_string(),
503	revision: 1,	503	revision: Some(1),
504	},	504	},
505	clock: 0,	505	clock: 0,
506	};	506	};
@@ -1143,7 +1143,7 @@ mod tests {
1143	verdict: ReviewVerdict::Approve,	1143	verdict: ReviewVerdict::Approve,
1144	body: "LGTM".into(),	1144	body: "LGTM".into(),
1145	timestamp: "2026-01-04T00:00:00Z".into(),	1145	timestamp: "2026-01-04T00:00:00Z".into(),
1146	revision: 2,	1146	revision: Some(2),
1147	commit_id: Oid::from_str("dddddddddddddddddddddddddddddddddddddddd").unwrap(),	1147	commit_id: Oid::from_str("dddddddddddddddddddddddddddddddddddddddd").unwrap(),
1148	edited: false,	1148	edited: false,
1149	}],	1149	}],

src/tui/widgets.rs

Old		New
@@ -58,6 +58,7 @@ pub(crate) fn action_type_label(action: &Action) -> &str {
58	Action::PatchLabel { .. } => "Patch Label",	58	Action::PatchLabel { .. } => "Patch Label",
59	Action::PatchUnlabel { .. } => "Patch Unlabel",	59	Action::PatchUnlabel { .. } => "Patch Unlabel",
60	Action::PatchRevision { .. } => "Patch Revision",	60	Action::PatchRevision { .. } => "Patch Revision",
		61	Action::PatchRevise { .. } => "Patch Revision (legacy)",
61	Action::PatchReview { .. } => "Patch Review",	62	Action::PatchReview { .. } => "Patch Review",
62	Action::PatchComment { .. } => "Patch Comment",	63	Action::PatchComment { .. } => "Patch Comment",
63	Action::PatchInlineComment { .. } => "Inline Comment",	64	Action::PatchInlineComment { .. } => "Inline Comment",
@@ -144,6 +145,16 @@ pub(crate) fn format_event_detail(
144	}	145	}
145	}	146	}
146	}	147	}
		148	// A revision from before revisions recorded a commit. It has no
		149	// Commit/Tree lines to show because the event has no commit or tree —
		150	// printing empty ones would read as "recorded, and empty".
		151	Action::PatchRevise { body } => {
		152	if let Some(b) = body {
		153	if !b.is_empty() {
		154	detail.push_str(&format!("\n{}\n", b));
		155	}
		156	}
		157	}
147	Action::PatchReview { verdict, body, .. } => {	158	Action::PatchReview { verdict, body, .. } => {
148	detail.push_str(&format!("\nVerdict: {}\n", verdict));	159	detail.push_str(&format!("\nVerdict: {}\n", verdict));
149	if !body.is_empty() {	160	if !body.is_empty() {
@@ -1196,7 +1207,10 @@ pub(crate) fn build_patch_detail_rows(app: &App) -> Vec<DetailRow> {
1196	ReviewVerdict::Comment => Color::White,	1207	ReviewVerdict::Comment => Color::White,
1197	ReviewVerdict::Reject => Color::Red,	1208	ReviewVerdict::Reject => Color::Red,
1198	};	1209	};
1199	let rev_label = format!(" (r{})", review.revision);	1210	let rev_label = review
		1211	.revision
		1212	.map(\|n\| format!(" (r{})", n))
		1213	.unwrap_or_default();
1200	rows.push(Line::from(vec![	1214	rows.push(Line::from(vec![
1201	Span::styled(	1215	Span::styled(
1202	review.author.name.clone(),	1216	review.author.name.clone(),

tests/body_edit_test.rs

Old		New
@@ -852,7 +852,7 @@ fn a_review_edit_by_a_different_author_is_ignored_by_the_fold() {
852	Action::PatchReview {	852	Action::PatchReview {
853	verdict: ReviewVerdict::Comment,	853	verdict: ReviewVerdict::Comment,
854	body: "Alice's review".to_string(),	854	body: "Alice's review".to_string(),
855	revision: 1,	855	revision: Some(1),
856	},	856	},
857	);	857	);
858	append(	858	append(

tests/common/mod.rs

Old		New
@@ -615,7 +615,7 @@ pub fn add_review_on(
615	action: Action::PatchReview {	615	action: Action::PatchReview {
616	verdict,	616	verdict,
617	body: "review comment".to_string(),	617	body: "review comment".to_string(),
618	revision,	618	revision: Some(revision),
619	},	619	},
620	clock: 0,	620	clock: 0,
621	};	621	};

tests/legacy_data_shapes_test.rs

@@ @@ -0,0 +1,404 @@ @@
+//! Event shapes that live in repositories people still hold.
+//!
+//! Every shape asserted here was taken from a real repository, not invented:
+//! `~/code/rad/eitri` and `~/code/rad/waystty` were written by a git-collab
+//! from July 2026 and between them hold 39 `patch.create` events with no
+//! `commit` and no `tree`, 33 `patch.review` events with no `revision`, 23
+//! `patch.merge` events with no `commit`, and one `patch.revise`. Issue
+//! `e5096ffc` removed the reader's tolerance for all four after auditing one
+//! clone and the hosted server repositories — a sample that happened to contain
+//! none of them — and issue `05b18df6` is what that cost.
+//!
+//! The rule these tests encode is not "keep everything forever". It is: **a
+//! shape that can still exist in data someone holds is a shape the reader has
+//! to be able to express.** A field absent from an event written before the
+//! field existed is not transitional debt, because no migration can invent a
+//! value for it — exactly the argument `Revision::base` was already kept on.
+//! Code that only ever *wrote* a superseded shape stays removed; nothing here
+//! writes any of these.
+//!
+//! The second, harder requirement is that reading one of these events must not
+//! change its bytes. Signatures are verified by re-serializing the event
+//! (`signing::canonical_json`), so a reader that helpfully filled in an absent
+//! key would invalidate the signature on every event it touched — turning a
+//! display bug into a verification failure, which is how a sync drops a patch.
+//! Each shape below is therefore asserted to survive a sign/verify round trip.
+mod common;
+use common::{write_raw_event, TestRepo};
+use git_collab::event::Event;
+use git_collab::signing;
+use git_collab::state::{PatchState, PatchStatus};
+use serde_json::json;
+/// The `patch.create` shape held by every legacy patch in `eitri`: a title, a
+/// body, a base ref, a branch, and nothing at all about where the code stands.
+/// Copied field-for-field from `refs/collab/patches/0b72bae8…`.
+fn legacy_create() -> serde_json::Value {
+    json!({
+        "type": "patch.create",
+        "title": "feat: one-time stream tickets replace admin token in SSE URLs",
+        "body": "[claude 2026-07-04] Fixes 44f5b0ee (DEF-6).",
+        "base_ref": "main",
+        "branch": "fix/def6-stream-tickets",
+        "fixes": "44f5b0ee",
+    })
+}
+/// Write a patch DAG at `refs/collab/patches/<id>/events` from a list of
+/// actions, oldest first, and return the patch id.
+fn legacy_patch(repo: &TestRepo, actions: &[serde_json::Value]) -> String {
+    let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
+    let mut parent = None;
+    let mut root = None;
+    for (i, action) in actions.iter().enumerate() {
+        let oid = write_raw_event(&git_repo, parent, action.clone(), i as u64 + 1);
+        root.get_or_insert(oid);
+        parent = Some(oid);
+    }
+    let id = root.expect("at least one event").to_string();
+    git_repo
+        .reference(
+            &format!("refs/collab/patches/{}/events", id),
+            parent.unwrap(),
+            false,
+            "events",
+        )
+        .unwrap();
+    id
+}
+/// Read the patch back through the same fold every command uses.
+fn fold(repo: &TestRepo, id: &str) -> PatchState {
+    let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
+    PatchState::from_ref_uncached(&git_repo, &format!("refs/collab/patches/{}/events", id), id)
+        .unwrap_or_else(|e| panic!("the fold must read this patch, got: {}", e))
+}
+/// Assert that reading and re-writing this event does not change its bytes.
+///
+/// This is the property that decides whether an absent key may be tolerated at
+/// all: `signing::canonical_json` is what both signing and verification run
+/// over, so if the round trip is not exact the event's existing signature stops
+/// verifying and `sync` rejects the whole patch.
+fn assert_round_trips(action: &serde_json::Value) {
+    let raw = json!({
+        "timestamp": "2026-07-04T15:37:13.808147145+00:00",
+        "author": { "name": "a73x", "email": "dev@a73x.sh" },
+        "action": action,
+        "clock": 1,
+    });
+    let original = serde_json::to_string(&raw).unwrap();
+    let event: Event = serde_json::from_str(&original)
+        .unwrap_or_else(|e| panic!("this shape must deserialize, got: {}\n{}", e, original));
+    let reserialized = String::from_utf8(signing::canonical_json(&event).unwrap()).unwrap();
+    assert_eq!(
+        original, reserialized,
+        "reading a legacy event must not change its bytes — its signature covers them"
+    );
+}
+// ===========================================================================
+// patch.merge with no commit
+// ===========================================================================
+/// The exact shape in the issue report: a merge recorded before merge-recording
+/// stored a commit.
+fn legacy_merge() -> serde_json::Value {
+    json!({ "type": "patch.merge" })
+}
+#[test]
+fn a_merge_event_with_no_commit_key_deserializes() {
+    assert_round_trips(&legacy_merge());
+}
+#[test]
+fn a_merge_event_with_no_commit_key_folds_to_merged_with_no_merge_commit() {
+    // `merge_commit` is already `Option`, and absent has to mean `None` rather
+    // than an empty string surfacing as a commit id — "not recorded" is not
+    // "the null OID". The status is the part that matters to a reader: the
+    // patch *was* merged, and losing that because the commit is unknown would
+    // report merged work as still open.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(&repo, &[legacy_create(), legacy_merge()]);
+    let state = fold(&repo, &id);
+    assert_eq!(state.status, PatchStatus::Merged);
+    assert_eq!(
+        state.merge_commit, None,
+        "an unrecorded merge commit is unknown, not empty"
+    );
+}
+#[test]
+fn a_merge_event_with_no_commit_key_is_listed_not_skipped() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(&repo, &[legacy_create(), legacy_merge()]);
+    let out = repo.run_ok(&["patch", "list", "--all"]);
+    assert!(
+        out.contains(&id[..8]),
+        "a legacy merged patch must appear in the list, got:\n{}",
+        out
+    );
+}
+// ===========================================================================
+// patch.create with no commit and no tree
+// ===========================================================================
+#[test]
+fn a_create_event_with_no_commit_or_tree_deserializes() {
+    // This, not the merge event, is what actually made `eitri` unreadable:
+    // every one of its 17 patches failed with
+    // `missing field `commit` at line 14 column 3`, which is the closing brace
+    // of the action object in a `patch.create`.
+    assert_round_trips(&legacy_create());
+}
+#[test]
+fn a_create_event_with_no_commit_or_tree_folds_to_a_revision_with_no_commit() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(&repo, &[legacy_create()]);
+    let state = fold(&repo, &id);
+    assert_eq!(state.revisions.len(), 1);
+    assert!(
+        state.revisions[0].commit.is_empty(),
+        "a revision that recorded no commit must say so, not guess one"
+    );
+    assert_eq!(
+        state.revisions[0].short_commit(),
+        None,
+        "nothing may render an unrecorded commit as an abbreviated OID"
+    );
+}
+#[test]
+fn a_repository_of_legacy_patches_is_not_an_empty_list() {
+    // The regression as the user met it: 17 patches, 17 warnings,
+    // `No patches found.`
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let mut ids = Vec::new();
+    for n in 0..3 {
+        let mut create = legacy_create();
+        create["title"] = json!(format!("legacy patch {}", n));
+        ids.push(legacy_patch(&repo, &[create, legacy_merge()]));
+    }
+    let output = repo.run(&["patch", "list", "--all"]);
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    let stderr = String::from_utf8_lossy(&output.stderr);
+    assert!(
+        output.status.success(),
+        "patch list must succeed: {}",
+        stderr
+    );
+    for id in &ids {
+        assert!(
+            stdout.contains(&id[..8]),
+            "patch {:.8} must be listed, got:\n{}\n{}",
+            id,
+            stdout,
+            stderr
+        );
+    }
+    assert!(
+        !stderr.contains("skipping patch"),
+        "no legacy patch may be skipped, got:\n{}",
+        stderr
+    );
+}
+#[test]
+fn a_patch_that_never_recorded_a_commit_is_not_told_to_fetch_one() {
+    // The same defect as the ref-layout advice, one layer down. A patch with no
+    // recorded commit has no diff, and saying "fetch the patch's revision refs,
+    // or the commit it stands on" sends its owner looking for objects that were
+    // never written by anyone — the message reads as a fetchable problem and is
+    // not one. It has to say the record is what is empty, and that the rest of
+    // the patch is fine.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(&repo, &[legacy_create(), legacy_merge()]);
+    let output = repo.run(&["patch", "show", &id[..8]]);
+    let combined = format!(
+        "{}{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+    assert!(
+        combined.contains("nothing to fetch"),
+        "the message must say there is nothing to fetch, got:\n{}",
+        combined
+    );
+    assert!(
+        !combined.contains("fetch the patch's revision refs"),
+        "and must not send the reader after objects nobody has, got:\n{}",
+        combined
+    );
+    // The rest of the patch still reads, which is the claim the message makes.
+    assert!(
+        String::from_utf8_lossy(&output.stdout).contains("one-time stream tickets"),
+        "the patch's own content must still be shown, got:\n{}",
+        combined
+    );
+}
+// ===========================================================================
+// patch.review with no revision
+// ===========================================================================
+fn legacy_review(body: &str) -> serde_json::Value {
+    json!({ "type": "patch.review", "verdict": "Comment", "body": body })
+}
+#[test]
+fn a_review_with_no_revision_deserializes() {
+    assert_round_trips(&legacy_review("looks good"));
+}
+#[test]
+fn a_review_with_no_revision_is_read_and_left_unattributed() {
+    // Restoring the *field* is not restoring the attribution. Recovering a
+    // missing revision from the event's position in the DAG was removed for a
+    // real reason (issue 33b5e541): position is not signed, so two clones
+    // holding the same events in different parent orders could attribute one
+    // review to different revisions, and because attribution feeds vote
+    // supersession that could drop a vote rather than merely mislabel it.
+    //
+    // So an unattributed review stays unattributed. `None` is the honest
+    // answer and it is stable across clones, which is the property the
+    // guessing lacked.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(
+        &repo,
+        &[
+            legacy_create(),
+            legacy_review("r1 era"),
+            legacy_review("r2 era"),
+        ],
+    );
+    let state = fold(&repo, &id);
+    assert_eq!(state.reviews.len(), 2, "both reviews must survive the fold");
+    for review in &state.reviews {
+        assert_eq!(
+            review.revision, None,
+            "a review that recorded no revision is not attributed to one"
+        );
+    }
+}
+#[test]
+fn an_unattributed_vote_does_not_supersede_an_attributed_one() {
+    // Vote supersession is per (author, revision). An unattributed review is
+    // its own bucket rather than a wildcard: letting it collide with every
+    // revision would let a legacy review silently retract a current vote.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(
+        &repo,
+        &[
+            legacy_create(),
+            json!({ "type": "patch.review", "verdict": "Approve", "body": "old" }),
+            json!({ "type": "patch.review", "verdict": "Approve", "body": "new", "revision": 1 }),
+        ],
+    );
+    let state = fold(&repo, &id);
+    assert_eq!(
+        state.reviews.len(),
+,
+        "an unattributed vote and a revision-1 vote are different votes, got: {:?}",
+        state
+            .reviews
+            .iter()
+            .map(|r| (&r.body, r.revision))
+            .collect::<Vec<_>>()
+    );
+}
+// ===========================================================================
+// patch.revise
+// ===========================================================================
+/// The one `patch.revise` in `eitri`, byte for byte.
+fn legacy_revise() -> serde_json::Value {
+    json!({ "type": "patch.revise", "body": null })
+}
+#[test]
+fn a_patch_revise_event_deserializes_and_keeps_its_own_name() {
+    // Deliberately its own variant rather than a `#[serde(alias)]` on
+    // `patch.revision`. An alias reads the event and then re-serializes it
+    // under the *new* name, which changes the signed bytes and fails
+    // verification — so an alias would read the patch locally and still have
+    // `sync` reject it. A variant that keeps its own tag round trips.
+    assert_round_trips(&legacy_revise());
+}
+#[test]
+fn a_patch_revise_event_folds_to_a_revision_with_no_commit() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(&repo, &[legacy_create(), legacy_revise()]);
+    let state = fold(&repo, &id);
+    assert_eq!(
+        state.revisions.len(),
+,
+        "a revise event records a revision, even one that says nothing about a commit"
+    );
+    assert!(state.revisions[1].commit.is_empty());
+    assert_eq!(state.revisions[1].number, 2);
+}
+#[test]
+fn two_revisions_that_recorded_no_commit_do_not_collapse_into_one() {
+    // Revisions are deduplicated by commit OID. With the `""`-means-unknown
+    // convention back, two revisions that both recorded nothing are not the
+    // same revision — dropping one would renumber every revision after it and
+    // silently lose a step of the patch's history.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(&repo, &[legacy_create(), legacy_revise(), legacy_revise()]);
+    let state = fold(&repo, &id);
+    assert_eq!(state.revisions.len(), 3);
+}
+// ===========================================================================
+// Signatures over the whole DAG
+// ===========================================================================
+#[test]
+fn every_legacy_shape_still_verifies_as_signed() {
+    // The end-to-end version of `assert_round_trips`: this is the same path
+    // `sync` runs before it will reconcile anything. If any shape above did not
+    // round trip exactly, the patch would read locally and still be refused on
+    // the way in from a remote.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let id = legacy_patch(
+        &repo,
+        &[
+            legacy_create(),
+            legacy_revise(),
+            legacy_review("no revision recorded"),
+            legacy_merge(),
+        ],
+    );
+    let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
+    let results =
+        signing::verify_ref(&git_repo, &format!("refs/collab/patches/{}/events", id)).unwrap();
+    assert_eq!(results.len(), 4);
+    for r in &results {
+        assert_eq!(
+            r.status,
+            signing::VerifyStatus::Valid,
+            "commit {} must still verify: {:?}",
+            r.commit_id,
+            r.error
+        );
+    }
+}

tests/legacy_patch_test.rs

Old		New
@@ -1,19 +1,33 @@
1	//! What happens to a repository still holding a pre-release shape.	1	//! What happens to a repository still holding a pre-release ref layout.
2	//!	2	//!
3	//! Until issue `e5096ffc` this file asserted that such repositories were read	3	//! Two layouts, and the line between them is the whole point of this file.
4	//! and silently converted. Every shape it covered — the bare
5	//! `refs/collab/patches/<id>` layout, the interim `<id>/r/<n>` revision
6	//! numbering, `patch.revise`, `head_commit`, reviews with no `revision`,
7	//! revisions with no `commit` — is now gone from the reader, confirmed absent
8	//! from every repository we host before it was removed.
9	//!	4	//!
10	//! So the contract this file asserts is the opposite one, and it is the whole	5	//! The bare `refs/collab/patches/<id>` ref — the ref that is the event DAG,
11	//! reason the removal is safe to ship: a repository that does still hold one	6	//! from before revision refs needed a subtree — is read. Issue `e5096ffc`
12	//! of those shapes must say so, name the shape, and say what to do about it.	7	//! stopped reading it on a survey that missed the user's own clones; issue
13	//! The failure mode being guarded against is not breakage — breakage is	8	//! `05b18df6` found 11 of them still sitting in `~/code/rad/waystty`, with the
14	//! intended — it is a silent empty list, or an error about something else.	9	//! same shape on its remote. A repository must not become unreachable because
15	//! `git-collab refs` deliberately still classifies these shapes by name, and is	10	//! every copy of it is old, and that is the case a clone of an unmaintained
16	//! the diagnostic every message here points at.	11	//! project is always in. So the reader takes the shape as it finds it, and
		12	//! `sync` reconciles into the ref that is already there rather than trying to
		13	//! create a directory where a ref lives.
		14	//!
		15	//! The interim `<id>/r/<n>` numbering — a revision ref named by ordinal
		16	//! rather than by the OID it pins — is still refused. It is not a DAG, so
		17	//! reading it means nothing; it was written by a draft that never left this
		18	//! machine; and the audit behind `05b18df6` scanned every collab ref in every
		19	//! repository under `~/code/rad` and found none. Nothing holds it, so nothing
		20	//! is stranded by refusing it.
		21	//!
		22	//! What both halves share is the failure mode being guarded against: not
		23	//! breakage but silence. A reader that merely stopped recognising a name
		24	//! would report a repository full of patches as empty, which is
		25	//! indistinguishable from a clean clone. Whatever the reader does with a shape,
		26	//! it has to say so.
		27	//!
		28	//! Field-level shapes — `patch.create` with no commit, `patch.review` with no
		29	//! revision, `patch.merge` with no commit, `patch.revise` — live in
		30	//! `legacy_data_shapes_test.rs`.
17		31
18	mod common;	32	mod common;
19		33
@@ -79,9 +93,15 @@ fn legacy_bare_patch(repo: &TestRepo) -> String {
79	id	93	id
80	}	94	}
81		95
82	/// Every message about a superseded shape has to do three things, or it is not	96	/// Every message about a layout the reader genuinely cannot use has to do four
83	/// the diagnostic this removal promised: name the ref, say the layout is not	97	/// things: name the ref, say the layout is not read, point at the command that
84	/// read any more, and point at the command that lists them.	98	/// lists them, and offer a remedy that works when every copy is old.
		99	///
		100	/// The fourth was the one the strip got wrong. It advised "fetch the patch from
		101	/// a remote holding the current layout, or delete the stale ref if it exists
		102	/// nowhere else" — two remedies, neither of which fits the ordinary case, which
		103	/// is a project whose copies are all the same age. Advice that cannot be
		104	/// followed is worse than none: it reads as help and ends in a wall.
85	fn assert_actionable(stderr: &str, ref_name: &str) {	105	fn assert_actionable(stderr: &str, ref_name: &str) {
86	assert!(	106	assert!(
87	stderr.contains(ref_name),	107	stderr.contains(ref_name),
@@ -99,39 +119,87 @@ fn assert_actionable(stderr: &str, ref_name: &str) {
99	"the message must say what is wrong with the ref, got: {}",	119	"the message must say what is wrong with the ref, got: {}",
100	stderr	120	stderr
101	);	121	);
		122	assert!(
		123	!lowered.contains("holding the current layout"),
		124	"the message must not advise fetching a newer copy — when every copy is \
		125	old there is none, and that is the common case. Got: {}",
		126	stderr
		127	);
		128	assert!(
		129	lowered.contains("no version of git-collab wrote")
		130	\|\| lowered.contains("update-ref")
		131	\|\| lowered.contains("rename"),
		132	"the message must give a remedy that works when every copy is old, got: {}",
		133	stderr
		134	);
102	}	135	}
103		136
104	// ===========================================================================	137	// ===========================================================================
105	// Superseded ref layouts are refused, loudly	138	// The bare `<id>` layout is read
106	// ===========================================================================	139	// ===========================================================================
107		140
108	#[test]	141	#[test]
109	fn a_bare_patch_ref_is_refused_and_says_what_to_do() {	142	fn a_bare_patch_ref_is_read_as_the_patch_it_is() {
		143	// 11 of these are sitting in `~/code/rad/waystty` right now, and its remote
		144	// holds the same. Refusing them made the repository unreachable from every
		145	// copy that exists.
110	let repo = TestRepo::new("Alice", "alice@example.com");	146	let repo = TestRepo::new("Alice", "alice@example.com");
111	let id = legacy_bare_patch(&repo);	147	let id = legacy_bare_patch(&repo);
112		148
113	let stderr = repo.run_err(&["patch", "list"]);	149	let out = repo.run_ok(&["patch", "list", "--all"]);
114	assert_actionable(&stderr, &format!("refs/collab/patches/{}", id));	150	assert!(
		151	out.contains(&id[..8]),
		152	"a patch in the bare layout must be listed, got:\n{}",
		153	out
		154	);
		155	assert!(
		156	out.contains("Written by an older version"),
		157	"and it must carry its own title, got:\n{}",
		158	out
		159	);
		160	}
		161
		162	#[test]
		163	fn a_bare_patch_ref_can_be_shown() {
		164	let repo = TestRepo::new("Alice", "alice@example.com");
		165	let id = legacy_bare_patch(&repo);
		166
		167	let out = repo.run_ok(&["patch", "show", &id[..8]]);
		168	assert!(
		169	out.contains("Written by an older version"),
		170	"patch show must resolve a bare-layout patch, got:\n{}",
		171	out
		172	);
115	}	173	}
116		174
117	#[test]	175	#[test]
118	fn a_bare_patch_ref_does_not_produce_an_empty_list() {	176	fn a_bare_patch_ref_does_not_produce_an_empty_list() {
119	// The specific regression this replaces: before, the reader tolerated the	177	// The failure mode that outlives every change of policy here: whatever the
120	// shape; a reader that merely stopped recognising it would list nothing	178	// reader decides to do with the shape, reporting a repository full of
121	// and exit zero, which reads as "this repository has no patches".	179	// patches as empty is never it.
122	let repo = TestRepo::new("Alice", "alice@example.com");	180	let repo = TestRepo::new("Alice", "alice@example.com");
123	legacy_bare_patch(&repo);	181	legacy_bare_patch(&repo);
124		182
125	let output = repo.run(&["patch", "list"]);	183	let output = repo.run(&["patch", "list", "--all"]);
		184	let stdout = String::from_utf8_lossy(&output.stdout);
126	assert!(	185	assert!(
127	!output.status.success(),	186	output.status.success() && !stdout.contains("No patches found"),
128	"a repository holding a superseded layout must fail, not report emptiness: {}",	187	"a bare-layout patch must be listed, not reported as emptiness: {}\n{}",
129	String::from_utf8_lossy(&output.stdout)	188	stdout,
		189	String::from_utf8_lossy(&output.stderr)
130	);	190	);
131	}	191	}
132		192
		193	// ===========================================================================
		194	// The interim `<id>/r/<n>` numbering is still refused
		195	// ===========================================================================
		196
133	#[test]	197	#[test]
134	fn a_numbered_revision_ref_is_refused_and_says_what_to_do() {	198	fn a_numbered_revision_ref_is_refused_and_says_what_to_do() {
		199	// Still refused, and on evidence rather than on principle: the audit behind
		200	// issue `05b18df6` walked every collab ref in every repository under
		201	// `~/code/rad` and found zero. It is also not a DAG — reading it would
		202	// yield nothing — so unlike the bare layout, refusing it strands no one.
135	let repo = TestRepo::new("Alice", "alice@example.com");	203	let repo = TestRepo::new("Alice", "alice@example.com");
136	let id = patch_on_branch(&repo, "feat", "a.txt");	204	let id = patch_on_branch(&repo, "feat", "a.txt");
137	let tip = repo.git(&["rev-parse", "HEAD"]).trim().to_string();	205	let tip = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
@@ -149,10 +217,12 @@ fn a_numbered_revision_ref_is_refused_and_says_what_to_do() {
149	}	217	}
150		218
151	#[test]	219	#[test]
152	fn refs_still_reports_the_shape_the_reader_refuses() {	220	fn refs_still_reports_the_layout_of_every_patch_ref() {
153	// `git-collab refs` is the one command that must keep working here: it is	221	// `git-collab refs` is the one command that must keep working here: it is
154	// what the refusal message tells the operator to run, so if it failed the	222	// what the refusal message tells the operator to run, so if it failed the
155	// same way the advice would be a loop.	223	// same way the advice would be a loop. It classifies the bare layout too —
		224	// reading a shape is not the same as recommending it, and an operator
		225	// should be able to see which of their refs are old.
156	let repo = TestRepo::new("Alice", "alice@example.com");	226	let repo = TestRepo::new("Alice", "alice@example.com");
157	let id = legacy_bare_patch(&repo);	227	let id = legacy_bare_patch(&repo);
158		228
@@ -163,28 +233,28 @@ fn refs_still_reports_the_shape_the_reader_refuses() {
163	.unwrap_or_else(\|\| panic!("refs must still list the bare ref, got:\n{}", out));	233	.unwrap_or_else(\|\| panic!("refs must still list the bare ref, got:\n{}", out));
164	assert!(	234	assert!(
165	line.contains("legacy"),	235	line.contains("legacy"),
166	"refs must still classify the superseded layout by name: {:?}",	236	"refs must still classify the older layout by name: {:?}",
167	line	237	line
168	);	238	);
169	}	239	}
170		240
171	#[test]	241	#[test]
172	fn nothing_migrates_the_shape_out_from_under_the_operator() {	242	fn nothing_migrates_the_shape_out_from_under_the_operator() {
173	// The removal's other half: a failed read must leave the evidence in	243	// Reading a shape is not converting it. A read that quietly rewrote the
174	// place. A command that "helpfully" deleted or rewrote the ref on the way	244	// ref would make every other clone's copy diverge from this one on the next
175	// to failing would destroy the thing the operator was told to go look at.	245	// sync, and would do it during a command the operator asked nothing of.
176	let repo = TestRepo::new("Alice", "alice@example.com");	246	let repo = TestRepo::new("Alice", "alice@example.com");
177	let id = legacy_bare_patch(&repo);	247	let id = legacy_bare_patch(&repo);
178	let before = repo.git(&["rev-parse", &format!("refs/collab/patches/{}", id)]);	248	let before = repo.git(&["rev-parse", &format!("refs/collab/patches/{}", id)]);
179		249
180	let _ = repo.run(&["patch", "list"]);	250	let _ = repo.run(&["patch", "list", "--all"]);
181	let _ = repo.run(&["refs"]);	251	let _ = repo.run(&["refs"]);
182		252
183	let after = repo.git(&["rev-parse", &format!("refs/collab/patches/{}", id)]);	253	let after = repo.git(&["rev-parse", &format!("refs/collab/patches/{}", id)]);
184	assert_eq!(	254	assert_eq!(
185	before.trim(),	255	before.trim(),
186	after.trim(),	256	after.trim(),
187	"the superseded ref must survive being refused"	257	"the older ref must survive being read"
188	);	258	);
189	}	259	}
190		260

tests/refs_test.rs

Old		New
@@ -413,13 +413,17 @@ fn server_refs_names_the_repositories_holding_a_legacy_layout() {
413	assert!(output.status.success(), "a clean audit exits 0");	413	assert!(output.status.success(), "a clean audit exits 0");
414	let stdout = String::from_utf8(output.stdout).unwrap();	414	let stdout = String::from_utf8(output.stdout).unwrap();
415		415
		416	// "older layout" rather than "superseded": the bare shape is read again
		417	// (issue `05b18df6`), so this survey says which repositories are old, not
		418	// which are broken. The operator uses it to plan a migration, and a word
		419	// that implies breakage would make that look urgent when it is not.
416	assert!(	420	assert!(
417	line_for(&stdout, "old:").contains("superseded"),	421	line_for(&stdout, "old:").contains("older layout"),
418	"the demoted repository must be called out: {}",	422	"the demoted repository must be called out: {}",
419	stdout	423	stdout
420	);	424	);
421	assert!(	425	assert!(
422	!line_for(&stdout, "current:").contains("superseded"),	426	!line_for(&stdout, "current:").contains("older layout"),
423	"the current repository must not be: {}",	427	"the current repository must not be: {}",
424	stdout	428	stdout
425	);	429	);

tests/review_test.rs

Old		New
@@ -53,7 +53,7 @@ fn review_event(
53	action: Action::PatchReview {	53	action: Action::PatchReview {
54	verdict,	54	verdict,
55	body: body.to_string(),	55	body: body.to_string(),
56	revision,	56	revision: Some(revision),
57	},	57	},
58	clock: 0,	58	clock: 0,
59	}	59	}

tests/sync_test.rs

Old		New
@@ -819,7 +819,7 @@ fn test_patch_review_across_repos() {
819	action: Action::PatchReview {	819	action: Action::PatchReview {
820	verdict: ReviewVerdict::Approve,	820	verdict: ReviewVerdict::Approve,
821	body: "LGTM!".to_string(),	821	body: "LGTM!".to_string(),
822	revision: 1,	822	revision: Some(1),
823	},	823	},
824	clock: 0,	824	clock: 0,
825	};	825	};
@@ -884,7 +884,7 @@ fn test_concurrent_review_and_revise() {
884	action: Action::PatchReview {	884	action: Action::PatchReview {
885	verdict: ReviewVerdict::RequestChanges,	885	verdict: ReviewVerdict::RequestChanges,
886	body: "Needs work".to_string(),	886	body: "Needs work".to_string(),
887	revision: 1,	887	revision: Some(1),
888	},	888	},
889	clock: 0,	889	clock: 0,
890	};	890	};
@@ -2058,15 +2058,61 @@ fn commit_on_branch(
2058	.unwrap()	2058	.unwrap()
2059	}	2059	}
2060		2060
		2061	/// Rewrite a patch's whole ref subtree into the single bare
		2062	/// `refs/collab/patches/<id>` ref an older git-collab would have written, and
		2063	/// return that ref name.
		2064	fn demote_to_bare_layout(repo: &Repository, id: &str) -> String {
		2065	let events = format!("refs/collab/patches/{}/events", id);
		2066	let tip = repo.refname_to_id(&events).unwrap();
		2067	let subtree: Vec<String> = repo
		2068	.references_glob(&format!("refs/collab/patches/{}/*", id))
		2069	.unwrap()
		2070	.filter_map(\|r\| r.ok()?.name().map(str::to_string))
		2071	.collect();
		2072	for name in subtree {
		2073	repo.find_reference(&name).unwrap().delete().unwrap();
		2074	}
		2075	let bare = format!("refs/collab/patches/{}", id);
		2076	repo.reference(&bare, tip, false, "demote").unwrap();
		2077	bare
		2078	}
		2079
		2080	/// Put both this clone and the shared remote into the bare layout — which is
		2081	/// the state `~/code/rad/waystty` is in, and the only state in which the layout
		2082	/// is a problem at all. A remote already holding `<id>/events` and a local
		2083	/// `<id>` are two clones that genuinely disagree, and git rejects that push on
		2084	/// its own terms; `print_refname_conflict_advice` is what covers it.
		2085	///
		2086	/// Demoting the remote directly rather than by pushing, because the local
		2087	/// `patch create` has already auto-synced by the time this runs.
		2088	fn demote_cluster_to_bare_layout(cluster: &TestCluster, id: &str) -> String {
		2089	let local = Repository::open(cluster.alice_dir.path()).unwrap();
		2090	let bare = demote_to_bare_layout(&local, id);
		2091	let remote = Repository::open(cluster.bare_dir.path()).unwrap();
		2092	if remote
		2093	.refname_to_id(&format!("refs/collab/patches/{}/events", id))
		2094	.is_ok()
		2095	{
		2096	demote_to_bare_layout(&remote, id);
		2097	}
		2098	bare
		2099	}
		2100
2061	#[test]	2101	#[test]
2062	fn sync_refuses_a_repo_still_holding_the_old_layout() {	2102	fn sync_works_in_a_repo_still_holding_the_bare_layout() {
2063	// sync used to migrate here, because it is precisely the command a	2103	// Issue `e5096ffc` made sync refuse outright here. Issue `05b18df6` is what
2064	// contributor runs on a repo they have not otherwise touched. The	2104	// that cost: `~/code/rad/waystty` holds 11 bare refs and its remote holds
2065	// migration is gone (issue `e5096ffc`), confirmed unnecessary against every	2105	// the same, so there was no newer copy to fetch from and no version of the
2066	// repository we host, so what matters now is that sync stops — and says	2106	// tool left that could convert them — every command in the repository
2067	// which ref and what to do. Left to carry on it would push the bare ref	2107	// failed, including the ones that would have got it out.
2068	// outbound, where a remote holding the current layout rejects it as a	2108	//
2069	// directory-vs-file conflict with nothing to explain why.	2109	// The bind sync used to hit was real, though: it fetched into
		2110	// `refs/collab/sync/patches/<id>` and then wrote `<id>/events`, which
		2111	// against a local bare `<id>` is a directory-vs-file conflict git reports
		2112	// as `could not remove directory …: parent is not directory`. The fix is
		2113	// not to refuse but to reconcile into the ref that is already there —
		2114	// `state::existing_events_ref` now knows the bare name, so the local DAG
		2115	// stays where the operator's other clones can still find it.
2070	let cluster = TestCluster::new();	2116	let cluster = TestCluster::new();
2071	let alice_repo = cluster.alice_repo();	2117	let alice_repo = cluster.alice_repo();
2072		2118
@@ -2077,48 +2123,227 @@ fn sync_refuses_a_repo_still_holding_the_old_layout() {
2077	&["patch", "create", "-t", "Old layout", "-B", "feat"],	2123	&["patch", "create", "-t", "Old layout", "-B", "feat"],
2078	);	2124	);
2079		2125
2080	// Demote to the pre-revision-refs layout, as a repo last written by an
2081	// older git-collab would be.
2082	let id = {	2126	let id = {
2083	let patches = state::list_patches(&alice_repo).unwrap();	2127	let patches = state::list_patches(&alice_repo).unwrap();
2084	patches[0].id.clone()	2128	patches[0].id.clone()
2085	};	2129	};
		2130	let bare = demote_cluster_to_bare_layout(&cluster, &id);
2086	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();	2131	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
2087	let events = format!("refs/collab/patches/{}/events", id);	2132	let tip = alice_repo.refname_to_id(&bare).unwrap();
2088	let tip = alice_repo.refname_to_id(&events).unwrap();	2133
2089	let subtree: Vec<String> = alice_repo	2134	sync::sync(&alice_repo, "origin").expect("sync must work in the bare layout");
2090	.references_glob(&format!("refs/collab/patches/{}/*", id))	2135
2091	.unwrap()	2136	// The patch is still readable, still at the same tip, and still bare: sync
2092	.filter_map(\|r\| r.ok()?.name().map(str::to_string))	2137	// reads the shape, it does not convert it. Converting would be a decision
2093	.collect();	2138	// taken during a command nobody asked it of, and it would strand every
2094	for name in subtree {	2139	// other clone of the project on the next push.
2095	alice_repo.find_reference(&name).unwrap().delete().unwrap();	2140	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
2096	}	2141	assert_eq!(
2097	let bare = format!("refs/collab/patches/{}", id);	2142	alice_repo.refname_to_id(&bare).ok(),
2098	alice_repo	2143	Some(tip),
2099	.reference(&bare, tip, false, "demote")	2144	"sync must leave the bare ref where it found it"
2100	.unwrap();	2145	);
		2146	let patches = state::list_patches(&alice_repo).unwrap();
		2147	assert_eq!(patches.len(), 1);
		2148	assert_eq!(patches[0].title, "Old layout");
		2149
		2150	// And it reached the remote — a repository whose only copies are old must
		2151	// still be able to publish.
		2152	let bare_remote = Repository::open(cluster.bare_dir.path()).unwrap();
		2153	assert_eq!(
		2154	bare_remote.refname_to_id(&bare).ok(),
		2155	Some(tip),
		2156	"the bare-layout patch must have been pushed"
		2157	);
		2158	}
		2159
		2160	#[test]
		2161	fn sync_reconciles_a_bare_layout_patch_from_a_remote() {
		2162	// The other direction: a peer publishes the bare shape and this clone
		2163	// already holds it. Both tips have to merge into the one local DAG, and it
		2164	// has to be the bare one — adopting the incoming ref as `<id>/events`
		2165	// beside a local `<id>` is the directory-vs-file conflict again.
		2166	let cluster = TestCluster::new();
		2167	let alice_repo = cluster.alice_repo();
		2168
		2169	let base = make_commit_with_message(&alice_repo, "base");
		2170	commit_on_branch(&alice_repo, "feat", base, "feature.txt", b"work");
		2171	cluster.run_collab_ok(
		2172	cluster.alice_dir.path(),
		2173	&["patch", "create", "-t", "Old layout", "-B", "feat"],
		2174	);
		2175	let id = state::list_patches(&alice_repo).unwrap()[0].id.clone();
		2176
		2177	let bare = demote_cluster_to_bare_layout(&cluster, &id);
		2178	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
		2179	sync::sync(&alice_repo, "origin").unwrap();
		2180
		2181	// Bob starts from nothing and takes the remote's layout as he finds it. If
		2182	// he filed it under `<id>/events` instead, his next push would be rejected
		2183	// against the remote's `<id>` and he could read the project but never write
		2184	// to it.
		2185	let bob_repo = cluster.bob_repo();
		2186	sync::sync(&bob_repo, "origin").expect("bob must be able to fetch the bare shape");
		2187	let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
		2188	let bob_ref = state::existing_events_ref(&bob_repo, "patches", &id)
		2189	.expect("bob must hold the patch under some name");
		2190	assert_eq!(
		2191	bob_ref, bare,
		2192	"a clone with no opinion follows the remote's layout"
		2193	);
		2194	let comment = Event {
		2195	timestamp: now(),
		2196	author: bob(),
		2197	action: Action::PatchComment {
		2198	body: "still relevant".to_string(),
		2199	},
		2200	clock: 0,
		2201	};
		2202	dag::append_event(&bob_repo, &bob_ref, &comment, &test_signing_key()).unwrap();
		2203	sync::sync(&bob_repo, "origin").expect("bob must be able to publish back");
2101		2204
2102	let err = sync::sync(&alice_repo, "origin")	2205	// Alice takes it back.
2103	.expect_err("sync must refuse a repository in the superseded layout");	2206	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
2104	let message = err.to_string();	2207	sync::sync(&alice_repo, "origin").expect("alice must reconcile into her bare ref");
		2208
		2209	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
2105	assert!(	2210	assert!(
2106	message.contains(&bare),	2211	alice_repo.refname_to_id(&bare).is_ok(),
2107	"the refusal must name the offending ref, got: {}",	2212	"the local DAG must still be the bare ref alice started with"
2108	message
2109	);	2213	);
		2214	let patch = PatchState::from_ref_uncached(&alice_repo, &bare, &id).unwrap();
2110	assert!(	2215	assert!(
2111	message.contains("git-collab refs"),	2216	patch.comments.iter().any(\|c\| c.body == "still relevant"),
2112	"the refusal must point at the diagnostic command, got: {}",	2217	"bob's comment must have reconciled into the bare ref, got: {:?}",
2113	message	2218	patch.comments.iter().map(\|c\| &c.body).collect::<Vec<_>>()
2114	);	2219	);
		2220	}
		2221
		2222	#[test]
		2223	fn one_unreadable_patch_does_not_take_down_the_sync() {
		2224	// The property that turned a display bug into an outage. Reading a patch is
		2225	// per-object work: a single object this version cannot make sense of must
		2226	// cost exactly that object, not the run. When it costs the run, every other
		2227	// patch and every issue in the repository stops moving too — and the
		2228	// operator's way out of the situation is a sync.
		2229	let cluster = TestCluster::new();
		2230	let alice_repo = cluster.alice_repo();
		2231
		2232	let base = make_commit_with_message(&alice_repo, "base");
		2233	commit_on_branch(&alice_repo, "feat", base, "feature.txt", b"work");
		2234	cluster.run_collab_ok(
		2235	cluster.alice_dir.path(),
		2236	&["patch", "create", "-t", "Readable", "-B", "feat"],
		2237	);
		2238	open_issue(&alice_repo, &alice(), "Readable issue");
		2239
		2240	// A patch whose event DAG this version genuinely cannot parse: not a
		2241	// superseded shape with a known meaning, but an action variant from a
		2242	// future no reader here has seen.
		2243	let unreadable = {
		2244	let root = common::write_raw_event(
		2245	&alice_repo,
		2246	None,
		2247	serde_json::json!({ "type": "patch.teleport", "destination": "elsewhere" }),
		2248	1,
		2249	);
		2250	let id = root.to_string();
		2251	alice_repo
		2252	.reference(
		2253	&format!("refs/collab/patches/{}/events", id),
		2254	root,
		2255	false,
		2256	"unreadable",
		2257	)
		2258	.unwrap();
		2259	id
		2260	};
2115		2261
2116	// And it must not have quietly fixed or destroyed the evidence.
2117	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();	2262	let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
		2263	sync::sync(&alice_repo, "origin")
		2264	.expect("one unreadable patch must not fail the whole sync");
		2265
		2266	// Everything else went through.
		2267	let bob_repo = cluster.bob_repo();
		2268	sync::sync(&bob_repo, "origin").unwrap();
		2269	let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
		2270	let titles: Vec<String> = state::list_patches(&bob_repo)
		2271	.unwrap()
		2272	.into_iter()
		2273	.map(\|p\| p.title)
		2274	.collect();
		2275	assert!(
		2276	titles.iter().any(\|t\| t == "Readable"),
		2277	"the readable patch must still have travelled, got: {:?}",
		2278	titles
		2279	);
2118	assert_eq!(	2280	assert_eq!(
2119	alice_repo.refname_to_id(&bare).ok(),	2281	state::list_issues(&bob_repo).unwrap().len(),
		2282	1,
		2283	"issues must not be held up by an unreadable patch"
		2284	);
		2285	assert!(
		2286	!unreadable.is_empty(),
		2287	"the unreadable patch is what made this test worth writing"
		2288	);
		2289	}
		2290
		2291	#[test]
		2292	fn a_merge_with_no_recorded_commit_survives_a_sync_round_trip() {
		2293	// The exact shape from `~/code/rad/eitri`: a `patch.merge` with no `commit`
		2294	// key at all, written before merges recorded one. It has to deserialize, it
		2295	// has to fold to `merged` with `merge_commit: None`, and — the part only a
		2296	// round trip can prove — it has to still verify at the far end, because
		2297	// verification re-serializes the event and compares the bytes the signature
		2298	// covers.
		2299	let cluster = TestCluster::new();
		2300	let alice_repo = cluster.alice_repo();
		2301
		2302	let base = make_commit_with_message(&alice_repo, "base");
		2303	commit_on_branch(&alice_repo, "feat", base, "feature.txt", b"work");
		2304	cluster.run_collab_ok(
		2305	cluster.alice_dir.path(),
		2306	&["patch", "create", "-t", "Merged long ago", "-B", "feat"],
		2307	);
		2308	let id = state::list_patches(&alice_repo).unwrap()[0].id.clone();
		2309
		2310	let events = format!("refs/collab/patches/{}/events", id);
		2311	let tip = alice_repo.refname_to_id(&events).unwrap();
		2312	let merge = common::write_raw_event(
		2313	&alice_repo,
2120	Some(tip),	2314	Some(tip),
2121	"a refused sync leaves the repository exactly as it found it"	2315	serde_json::json!({ "type": "patch.merge" }),
		2316	2,
		2317	);
		2318	alice_repo
		2319	.reference(&events, merge, true, "legacy merge")
		2320	.unwrap();
		2321
		2322	sync::sync(&alice_repo, "origin").unwrap();
		2323	let bob_repo = cluster.bob_repo();
		2324	sync::sync(&bob_repo, "origin").unwrap();
		2325
		2326	let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
		2327	let patch = PatchState::from_ref_uncached(&bob_repo, &events, &id)
		2328	.expect("bob must be able to read the legacy merge");
		2329	assert_eq!(patch.status, git_collab::state::PatchStatus::Merged);
		2330	assert_eq!(
		2331	patch.merge_commit, None,
		2332	"a merge that recorded no commit is unknown, not empty"
		2333	);
		2334
		2335	// Signature verification is the part that fails silently if the reader
		2336	// rewrites the event on the way through.
		2337	let results = signing::verify_ref(&bob_repo, &events).unwrap();
		2338	assert!(
		2339	results
		2340	.iter()
		2341	.all(\|r\| r.status == signing::VerifyStatus::Valid),
		2342	"every event must still verify after the round trip: {:?}",
		2343	results
		2344	.iter()
		2345	.map(\|r\| (r.commit_id, &r.status, &r.error))
		2346	.collect::<Vec<_>>()
2122	);	2347	);
2123	}	2348	}
2124		2349