a73x

ab09aff4

Name revision refs by commit OID

a73x   2026-08-09 19:28

Commit message
Name revision refs by commit OID

refs/collab/patches/<id>/r/<n> becomes refs/collab/patches/<id>/rev/<oid>,
per the updated spec.

The numbered name had to be agreed between clones and could not be. Any
r/<n> the remote published for a commit our DAG did not list was ignored,
the number was handed to a local commit, and that push was rejected
non-fast-forward permanently - after which sync sat in resume mode and
stopped pushing anything at all. Reconciling the numbering instead made
r/<n> stop meaning revision n, consistently and permanently in every
clone.

A name derived from content has neither problem. There is no number to
negotiate, so pushes are additive by construction; the ref never moves,
so write-once is trivially true rather than enforced; and two clones
cannot disagree about a name neither of them chose. Numbering stays a
property of the DAG, which is where it was already authoritative.

reconcile_revision_refs becomes pin_dag_revisions - write a ref for every
commit the DAG lists - and write_revision_ref takes a commit rather than
a number. force=true is gone from every revision, migration and archive
write; the only one left in the file is the pre-existing seen marker,
which is a local cursor and has to move.

Migration now handles three shapes: the original bare <id>, the interim
<id>/r/<n>, and the target <id>/rev/<oid>. Numbered refs are re-pinned
from the DAG before being retired, so nothing is unreferenced in
between, and a numbered ref pointing at a commit no event vouches for is
dropped with a warning - it is the stranded name the OID scheme exists
to remove.

Dropping force also exposed a gap in the migration resume: an
interruption between the events ref and the pins left a patch that
listed fine but had nothing pinned, and the sweep would have cleared the
park without noticing. It now pins before clearing.

Keeps the r2 property that revision refs are derived from the signed DAG
and never adopted from a remote.

patch log prints one line per revision: a revision note is often a full
review response, and printing it whole left the log ~90% body text.
src/patch.rs
Old New
@@ -79,7 +79,7 @@ pub fn create(
79 let id = oid.to_string(); 79 let id = oid.to_string();
80 let events_ref = state::patch_events_ref(&id); 80 let events_ref = state::patch_events_ref(&id);
81 repo.reference(&events_ref, oid, false, "patch create")?; 81 repo.reference(&events_ref, oid, false, "patch create")?;
82 state::write_revision_ref(repo, &events_ref, 1, tip_oid)?; 82 state::write_revision_ref(repo, &events_ref, tip_oid)?;
83 Ok(id) 83 Ok(id)
84 } 84 }
85 85
@@ -370,15 +370,14 @@ pub fn revise(
370 let commit = repo.find_commit(tip_oid)?; 370 let commit = repo.find_commit(tip_oid)?;
371 let tree_oid = commit.tree()?.id(); 371 let tree_oid = commit.tree()?.id();
372 let base_oid = revision_base(repo, &patch.base_ref, tip_oid)?; 372 let base_oid = revision_base(repo, &patch.base_ref, tip_oid)?;
373 let number = patch.revisions.last().map(|r| r.number).unwrap_or(0) + 1;
374 let author = get_author(repo)?; 373 let author = get_author(repo)?;
375 374
376 // Claim the ref before recording the event. An event cannot be taken back, 375 // Pin the commit before recording the event. An event cannot be taken back,
377 // so appending first would let a write-once rejection leave a revision in 376 // so appending first would let a failure here leave a revision in the DAG
378 // the DAG with no ref pointing at it — precisely the state this design 377 // with nothing pinning its objects — precisely the state this design exists
379 // exists to prevent. This way a rejection records nothing and the user can 378 // to prevent. The other order fails clean: a stray pin costs nothing and
380 // simply retry. 379 // the next attempt reuses it.
381 state::write_revision_ref(repo, &ref_name, number, tip_oid)?; 380 state::write_revision_ref(repo, &ref_name, tip_oid)?;
382 381
383 let event = Event { 382 let event = Event {
384 timestamp: chrono::Utc::now().to_rfc3339(), 383 timestamp: chrono::Utc::now().to_rfc3339(),
@@ -526,6 +525,26 @@ pub fn patch_log(repo: &Repository, id_prefix: &str) -> Result<PatchState, Error
526 PatchState::from_ref(repo, &ref_name, &id) 525 PatchState::from_ref(repo, &ref_name, &id)
527 } 526 }
528 527
528 /// One revision per line means one line of body. A revision note is often a
529 /// full review response, and printing it whole turned `patch log` into mostly
530 /// body text with the revision list buried in it. `patch show` still prints
531 /// them in full.
532 ///
533 /// Truncating by chars rather than bytes keeps this total for any input.
534 fn summarize_body(body: &str) -> String {
535 const WIDTH: usize = 60;
536 let first_line = body.lines().next().unwrap_or_default().trim();
537 let more = body.lines().nth(1).is_some();
538 if first_line.chars().count() > WIDTH {
539 let head: String = first_line.chars().take(WIDTH).collect();
540 return format!("{}…", head.trim_end());
541 }
542 if more {
543 return format!("{}…", first_line);
544 }
545 first_line.to_string()
546 }
547
529 pub fn patch_log_to_writer( 548 pub fn patch_log_to_writer(
530 repo: &Repository, 549 repo: &Repository,
531 patch: &PatchState, 550 patch: &PatchState,
@@ -544,7 +563,7 @@ pub fn patch_log_to_writer(
544 let body_display = rev 563 let body_display = rev
545 .body 564 .body
546 .as_deref() 565 .as_deref()
547 .map(|b| format!(" \"{}\"", b)) 566 .map(|b| format!(" \"{}\"", summarize_body(b)))
548 .unwrap_or_default(); 567 .unwrap_or_default();
549 568
550 // Compute file-change summary between consecutive revisions 569 // Compute file-change summary between consecutive revisions
src/state.rs
Old New
@@ -828,13 +828,26 @@ impl PatchState {
828 /// A patch owns a subtree of refs, not a single ref: 828 /// A patch owns a subtree of refs, not a single ref:
829 /// 829 ///
830 /// ```text 830 /// ```text
831 /// refs/collab/patches/<id>/events the event DAG 831 /// refs/collab/patches/<id>/events the event DAG
832 /// refs/collab/patches/<id>/r/<n> revision n's commit, write-once 832 /// refs/collab/patches/<id>/rev/<oid> a revision's commit, pinned
833 /// ``` 833 /// ```
834 /// 834 ///
835 /// The `events` suffix is forced — git will not let `<id>` be both a ref and a 835 /// The `events` suffix is forced — git will not let `<id>` be both a ref and a
836 /// directory — and the split is what lets a revision's objects stay reachable 836 /// directory — and the split is what lets a revision's objects stay reachable
837 /// once the branch that produced them has been rebased away. 837 /// once the branch that produced them has been rebased away.
838 ///
839 /// Revision refs are named by commit OID rather than by revision number. A
840 /// number has to be agreed between clones, and two clones revising offline
841 /// cannot agree: both claim the same one, and the loser's push is rejected as a
842 /// non-fast-forward permanently, because a number the remote has published
843 /// cannot be renegotiated. Reconciling the numbering is possible but then the
844 /// number stops matching the DAG's, so the name means two different things.
845 ///
846 /// A name derived from content has neither problem: a ref never moves, two
847 /// clones cannot disagree about a name neither of them chose, and every push is
848 /// additive by construction. Revision *numbering* stays a property of the DAG,
849 /// which is where it is already authoritative. These refs exist only to keep
850 /// revision commits reachable, and that job needs a set, not a sequence.
838 pub const PATCH_PREFIX: &str = "refs/collab/patches/"; 851 pub const PATCH_PREFIX: &str = "refs/collab/patches/";
839 pub const ARCHIVE_PATCH_PREFIX: &str = "refs/collab/archive/patches/"; 852 pub const ARCHIVE_PATCH_PREFIX: &str = "refs/collab/archive/patches/";
840 853
@@ -843,11 +856,11 @@ pub fn patch_events_ref(id: &str) -> String {
843 format!("{}{}/events", PATCH_PREFIX, id) 856 format!("{}{}/events", PATCH_PREFIX, id)
844 } 857 }
845 858
846 /// The ref holding revision `n`'s commit, under whichever namespace the 859 /// The ref pinning `commit`, under whichever namespace the patch's events ref
847 /// patch's events ref lives in. 860 /// lives in.
848 pub fn patch_revision_ref(events_ref: &str, n: u32) -> String { 861 pub fn patch_revision_ref(events_ref: &str, commit: Oid) -> String {
849 let base = events_ref.strip_suffix("/events").unwrap_or(events_ref); 862 let base = events_ref.strip_suffix("/events").unwrap_or(events_ref);
850 format!("{}/r/{}", base, n) 863 format!("{}/rev/{}", base, commit)
851 } 864 }
852 865
853 /// Split a ref name under a patch prefix into (id, suffix), where suffix is 866 /// Split a ref name under a patch prefix into (id, suffix), where suffix is
@@ -1104,7 +1117,7 @@ pub fn archive_patch_ref(repo: &Repository, id: &str) -> Result<(), crate::error
1104 for (old_ref, suffix) in patch_subtree(repo, PATCH_PREFIX, id)? { 1117 for (old_ref, suffix) in patch_subtree(repo, PATCH_PREFIX, id)? {
1105 let oid = repo.refname_to_id(&old_ref)?; 1118 let oid = repo.refname_to_id(&old_ref)?;
1106 let new_ref = format!("{}{}/{}", ARCHIVE_PATCH_PREFIX, id, suffix); 1119 let new_ref = format!("{}{}/{}", ARCHIVE_PATCH_PREFIX, id, suffix);
1107 repo.reference(&new_ref, oid, true, "archive patch")?; 1120 repo.reference(&new_ref, oid, false, "archive patch")?;
1108 repo.find_reference(&old_ref)?.delete()?; 1121 repo.find_reference(&old_ref)?.delete()?;
1109 } 1122 }
1110 Ok(()) 1123 Ok(())
@@ -1135,9 +1148,9 @@ const MIGRATION_PARK_PREFIX: &str = "refs/collab/local/migrating/patches/";
1135 /// but unfindable. This is the only thing that makes parking worth anything. 1148 /// but unfindable. This is the only thing that makes parking worth anything.
1136 /// 1149 ///
1137 /// The later window — killed after the events ref exists but before the park 1150 /// The later window — killed after the events ref exists but before the park
1138 /// was dropped — leaves the park behind as litter. Both come out of one loop, 1151 /// was dropped — leaves the park behind as litter, and possibly a patch whose
1139 /// and because every write below forces, resuming over a partially migrated 1152 /// revisions were never pinned. Both come out of one loop, and the park is only
1140 /// patch is idempotent. 1153 /// dropped once the patch is whole.
1141 fn resume_interrupted_migrations(repo: &Repository) { 1154 fn resume_interrupted_migrations(repo: &Repository) {
1142 let Ok(refs) = repo.references_glob(&format!("{}*", MIGRATION_PARK_PREFIX)) else { 1155 let Ok(refs) = repo.references_glob(&format!("{}*", MIGRATION_PARK_PREFIX)) else {
1143 return; 1156 return;
@@ -1151,25 +1164,29 @@ fn resume_interrupted_migrations(repo: &Repository) {
1151 .collect(); 1164 .collect();
1152 1165
1153 for (park_ref, id) in parked { 1166 for (park_ref, id) in parked {
1154 // The park carries no namespace, so look for the patch in both. If it 1167 // The park carries no namespace, so look for the patch in both.
1155 // already has an events ref anywhere, the migration got far enough and 1168 let existing = [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX]
1156 // only the park needs clearing. 1169 .into_iter()
1157 let already_migrated = [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] 1170 .find(|p| repo.refname_to_id(&format!("{}{}/events", p, id)).is_ok());
1158 .iter() 1171
1159 .any(|p| repo.refname_to_id(&format!("{}{}/events", p, id)).is_ok()); 1172 let outcome = match existing {
1160 1173 // The events ref landed, so only the revisions might be missing.
1161 if !already_migrated { 1174 // Pinning is idempotent, so re-running it costs nothing and covers
1175 // an interruption between the two steps.
1176 Some(prefix) => pin_dag_revisions(repo, &format!("{}{}/events", prefix, id), &id),
1162 // Resume from the parked tip. The active namespace is the right 1177 // Resume from the parked tip. The active namespace is the right
1163 // home: a patch is only archived by an explicit close, and a close 1178 // home: a patch is only archived by an explicit close, and a close
1164 // moves the whole subtree, so nothing half-migrated belongs there. 1179 // moves the whole subtree, so nothing half-migrated belongs there.
1165 if let Err(e) = finish_migration(repo, PATCH_PREFIX, &id, &park_ref) { 1180 None => finish_migration(repo, PATCH_PREFIX, &id, &park_ref),
1166 eprintln!( 1181 };
1167 "warning: could not resume interrupted migration of patch {:.8}: {}", 1182 if let Err(e) = outcome {
1168 id, e 1183 eprintln!(
1169 ); 1184 "warning: could not resume interrupted migration of patch {:.8}: {}",
1170 continue; 1185 id, e
1171 } 1186 );
1187 continue;
1172 } 1188 }
1189 // Only now is the patch whole; the park has nothing left to protect.
1173 if let Ok(mut r) = repo.find_reference(&park_ref) { 1190 if let Ok(mut r) = repo.find_reference(&park_ref) {
1174 let _ = r.delete(); 1191 let _ = r.delete();
1175 } 1192 }
@@ -1210,7 +1227,67 @@ pub fn migrate_patch_layout(repo: &Repository) {
1210 eprintln!("warning: could not migrate patch {:.8}: {}", id, e); 1227 eprintln!("warning: could not migrate patch {:.8}: {}", id, e);
1211 } 1228 }
1212 } 1229 }
1230
1231 // The interim shape: revision refs named by number, from the draft this
1232 // design replaced. Re-pin from the DAG under the OID names and retire
1233 // the numbered ones.
1234 let Ok(numbered) = repo.references_glob(&format!("{}*/r/*", prefix)) else {
1235 continue;
1236 };
1237 let mut ids: Vec<String> = numbered
1238 .filter_map(|r| {
1239 let name = r.ok()?.name()?.to_string();
1240 let (id, suffix) = split_patch_ref(&name, prefix)?;
1241 suffix.starts_with("r/").then(|| id.to_string())
1242 })
1243 .collect();
1244 ids.sort();
1245 ids.dedup();
1246 for id in ids {
1247 if let Err(e) = retire_numbered_revision_refs(repo, prefix, &id) {
1248 eprintln!(
1249 "warning: could not convert numbered revision refs for patch {:.8}: {}",
1250 id, e
1251 );
1252 }
1253 }
1254 }
1255 }
1256
1257 /// Replace a patch's `r/<n>` refs with OID-named ones.
1258 ///
1259 /// The new refs are written before the old ones are dropped, so no commit is
1260 /// left unreferenced in between. A numbered ref pointing at a commit the DAG
1261 /// does not list is dropped too, and said so: it is exactly the wedge the OID
1262 /// naming removes — a published number nobody can renegotiate — and the DAG is
1263 /// the only thing that decides what a revision is.
1264 fn retire_numbered_revision_refs(
1265 repo: &Repository,
1266 prefix: &str,
1267 id: &str,
1268 ) -> Result<(), crate::error::Error> {
1269 let events_ref = format!("{}{}/events", prefix, id);
1270 if repo.refname_to_id(&events_ref).is_err() {
1271 return Err(git2::Error::from_str("no events ref beside numbered revision refs").into());
1272 }
1273 pin_dag_revisions(repo, &events_ref, id)?;
1274
1275 for (ref_name, suffix) in patch_subtree(repo, prefix, id)? {
1276 if !suffix.starts_with("r/") {
1277 continue;
1278 }
1279 if let Ok(oid) = repo.refname_to_id(&ref_name) {
1280 let pinned = patch_revision_ref(&events_ref, oid);
1281 if repo.refname_to_id(&pinned).is_err() {
1282 eprintln!(
1283 "warning: dropping {} — commit {:.8} is not a revision of patch {:.8}",
1284 ref_name, oid, id
1285 );
1286 }
1287 }
1288 repo.find_reference(&ref_name)?.delete()?;
1213 } 1289 }
1290 Ok(())
1214 } 1291 }
1215 1292
1216 fn migrate_one_patch( 1293 fn migrate_one_patch(
@@ -1230,7 +1307,7 @@ fn migrate_one_patch(
1230 // namespace first — `resume_interrupted_migrations` reads it back, which is 1307 // namespace first — `resume_interrupted_migrations` reads it back, which is
1231 // what makes the delete-then-create window survivable. 1308 // what makes the delete-then-create window survivable.
1232 let parked = format!("{}{}", MIGRATION_PARK_PREFIX, id); 1309 let parked = format!("{}{}", MIGRATION_PARK_PREFIX, id);
1233 repo.reference(&parked, oid, true, "migrate patch: park tip")?; 1310 repo.reference(&parked, oid, false, "migrate patch: park tip")?;
1234 repo.find_reference(old_ref)?.delete()?; 1311 repo.find_reference(old_ref)?.delete()?;
1235 1312
1236 finish_migration(repo, prefix, id, &parked)?; 1313 finish_migration(repo, prefix, id, &parked)?;
@@ -1239,8 +1316,10 @@ fn migrate_one_patch(
1239 Ok(()) 1316 Ok(())
1240 } 1317 }
1241 1318
1242 /// Create the events ref from a parked tip and give every recoverable revision 1319 /// Create the events ref from a parked tip and pin every recoverable revision.
1243 /// a ref. Idempotent, so it can resume over a partially migrated patch. 1320 /// Callers must have established that no events ref exists yet; the caller that
1321 /// resumes an interrupted migration checks exactly that, and pins directly when
1322 /// one does.
1244 fn finish_migration( 1323 fn finish_migration(
1245 repo: &Repository, 1324 repo: &Repository,
1246 prefix: &str, 1325 prefix: &str,
@@ -1249,124 +1328,53 @@ fn finish_migration(
1249 ) -> Result<(), crate::error::Error> { 1328 ) -> Result<(), crate::error::Error> {
1250 let oid = repo.refname_to_id(parked)?; 1329 let oid = repo.refname_to_id(parked)?;
1251 let events_ref = format!("{}{}/events", prefix, id); 1330 let events_ref = format!("{}{}/events", prefix, id);
1252 repo.reference(&events_ref, oid, true, "migrate patch: events")?; 1331 repo.reference(&events_ref, oid, false, "migrate patch: events")?;
1253 1332
1254 // Give every revision whose commit is still present a ref of its own. 1333 // Give every revision whose commit is still present a ref of its own.
1255 // Revisions whose objects were already lost to a force-push cannot be 1334 // Revisions whose objects were already lost to a force-push cannot be
1256 // recovered and keep the `""`-means-unknown convention. 1335 // recovered and keep the `""`-means-unknown convention.
1257 // 1336 pin_dag_revisions(repo, &events_ref, id)
1258 // Write-once applies here as everywhere else: migration runs off a DAG no
1259 // reconcile has touched, so its numbering is settled and a revision ref
1260 // that already exists can only be one this migration wrote on an earlier,
1261 // interrupted attempt.
1262 let state = PatchState::from_ref_uncached(repo, &events_ref, id)?;
1263 for rev in &state.revisions {
1264 let Ok(commit) = Oid::from_str(&rev.commit) else {
1265 continue;
1266 };
1267 if repo.find_commit(commit).is_err() {
1268 continue;
1269 }
1270 write_revision_ref(repo, &events_ref, rev.number, commit)?;
1271 }
1272 Ok(())
1273 } 1337 }
1274 1338
1275 /// Point revision `number` at `commit`, refusing to move a revision that is 1339 /// Pin `commit` so the objects behind it stay reachable. The ref's name is its
1276 /// already recorded elsewhere. This is the write-once rule, in one place: a 1340 /// content, so this is write-once for free: the only ref it can collide with is
1277 /// revision is never rewritten, a changed patch adds the next one. 1341 /// one already pointing at the same commit.
1278 pub fn write_revision_ref( 1342 pub fn write_revision_ref(
1279 repo: &Repository, 1343 repo: &Repository,
1280 events_ref: &str, 1344 events_ref: &str,
1281 number: u32,
1282 commit: Oid, 1345 commit: Oid,
1283 ) -> Result<(), crate::error::Error> { 1346 ) -> Result<(), crate::error::Error> {
1284 let name = patch_revision_ref(events_ref, number); 1347 let name = patch_revision_ref(events_ref, commit);
1285 if let Ok(existing) = repo.refname_to_id(&name) { 1348 if repo.refname_to_id(&name).is_ok() {
1286 if existing != commit {
1287 return Err(crate::error::Error::Cmd(format!(
1288 "revision {} already points at {} — revisions are write-once",
1289 number, existing
1290 )));
1291 }
1292 return Ok(()); 1349 return Ok(());
1293 } 1350 }
1294 repo.reference(&name, commit, false, "record revision")?; 1351 repo.reference(&name, commit, false, "record revision")?;
1295 Ok(()) 1352 Ok(())
1296 } 1353 }
1297 1354
1298 /// Numbering a remote already published for this patch, read from the fetched 1355 /// Pin every revision the patch's signed event DAG lists.
1299 /// sync refs. Only a hint: it is applied to commits the signed event DAG lists
1300 /// and to nothing else.
1301 pub type RevisionNumbering = std::collections::HashMap<u32, Oid>;
1302
1303 /// Make the patch's revision refs a function of its signed event DAG.
1304 /// 1356 ///
1305 /// Two problems meet here. Revision numbers come from the DAG walk while ref 1357 /// Revision refs are derived, never adopted: a remote cannot make this repo
1306 /// names are chosen at write time, so a concurrent `revise` renumbers one side 1358 /// reference an object by pushing a ref name, because the only names written
1307 /// and can leave its commit referenced by nothing once the sync refs are swept 1359 /// here come from commits the verified DAG vouches for. And because a name is
1308 /// — the force-push orphan this design exists to prevent, arriving through 1360 /// its content, this only ever adds — there is nothing to negotiate with a
1309 /// concurrency instead of rebase. And a number the remote has already published 1361 /// remote and nothing that can be rejected as a non-fast-forward.
1310 /// cannot be renegotiated: pushing a different commit at it is a non-fast-
1311 /// forward that no retry will ever clear.
1312 /// 1362 ///
1313 /// So: honour the remote's numbering for any commit the DAG also lists, give 1363 /// Revisions whose objects are absent are skipped; there is nothing to pin.
1314 /// every remaining revision its own number where free and the lowest free one 1364 pub fn pin_dag_revisions(
1315 /// otherwise, and write that. Whoever pushes first settles the numbering and
1316 /// everyone else adapts to it, which converges. A ref may move as a result, but
1317 /// no commit loses its ref: reconciliation only ever adds revisions, so every
1318 /// previously referenced commit is still in the list and still gets a number.
1319 ///
1320 /// Nothing outside the DAG is ever referenced, which is also what keeps a peer
1321 /// from planting reachable objects by pushing a ref name we would otherwise
1322 /// adopt on trust.
1323 pub fn reconcile_revision_refs(
1324 repo: &Repository, 1365 repo: &Repository,
1325 events_ref: &str, 1366 events_ref: &str,
1326 id: &str, 1367 id: &str,
1327 remote: &RevisionNumbering,
1328 ) -> Result<(), crate::error::Error> { 1368 ) -> Result<(), crate::error::Error> {
1329 let state = PatchState::from_ref_uncached(repo, events_ref, id)?; 1369 let state = PatchState::from_ref_uncached(repo, events_ref, id)?;
1330 1370 for rev in &state.revisions {
1331 // Revisions we can actually point a ref at, in DAG order. 1371 let Ok(oid) = Oid::from_str(&rev.commit) else {
1332 let recoverable: Vec<(u32, Oid)> = state
1333 .revisions
1334 .iter()
1335 .filter_map(|r| {
1336 let oid = Oid::from_str(&r.commit).ok()?;
1337 repo.find_commit(oid).ok()?;
1338 Some((r.number, oid))
1339 })
1340 .collect();
1341
1342 let mut assigned: std::collections::BTreeMap<u32, Oid> = std::collections::BTreeMap::new();
1343 let mut placed: std::collections::HashSet<Oid> = std::collections::HashSet::new();
1344
1345 for (number, oid) in remote {
1346 if recoverable.iter().any(|(_, o)| o == oid) && !assigned.contains_key(number) {
1347 assigned.insert(*number, *oid);
1348 placed.insert(*oid);
1349 }
1350 }
1351 for (number, oid) in &recoverable {
1352 if placed.contains(oid) {
1353 continue; 1372 continue;
1354 }
1355 let slot = if assigned.contains_key(number) {
1356 (1u32..).find(|n| !assigned.contains_key(n)).unwrap_or(*number)
1357 } else {
1358 *number
1359 }; 1373 };
1360 assigned.insert(slot, *oid); 1374 if repo.find_commit(oid).is_err() {
1361 placed.insert(*oid);
1362 }
1363
1364 for (number, oid) in assigned {
1365 let name = patch_revision_ref(events_ref, number);
1366 if repo.refname_to_id(&name).ok() == Some(oid) {
1367 continue; 1375 continue;
1368 } 1376 }
1369 repo.reference(&name, oid, true, "reconcile revision refs")?; 1377 write_revision_ref(repo, events_ref, oid)?;
1370 } 1378 }
1371 Ok(()) 1379 Ok(())
1372 } 1380 }
src/sync.rs
Old New
@@ -1,4 +1,3 @@
1 use std::collections::HashMap;
2 use std::fs; 1 use std::fs;
3 use std::path::{Path, PathBuf}; 2 use std::path::{Path, PathBuf};
4 use std::process::Command; 3 use std::process::Command;
@@ -628,14 +627,13 @@ fn push_refs(workdir: &Path, remote_name: &str, refs: &[String]) -> SyncResult {
628 enum SyncRef { 627 enum SyncRef {
629 /// An event DAG. Verified, then reconciled against the local ref. 628 /// An event DAG. Verified, then reconciled against the local ref.
630 Events { local_ref: String }, 629 Events { local_ref: String },
631 /// A patch revision. Never adopted on the strength of its name: it carries 630 /// A patch revision, ignored on the way in. It carries no event signature,
632 /// no event signature, and a ref we adopt is reachable in every clone that 631 /// and a ref we adopt is reachable in every clone that syncs afterwards and
633 /// syncs afterwards and removable through no CLI command, since nothing 632 /// removable through no CLI command, since nothing enumerates a patch with
634 /// enumerates a patch with no events ref. It contributes only the number 633 /// no events ref. Local revision refs are derived from the verified DAG
635 /// the remote published, which `state::reconcile_revision_refs` applies to 634 /// instead, by `state::pin_dag_revisions`. The fetch has already brought
636 /// commits the signed DAG lists and to nothing else. The objects arrive 635 /// the objects, which is all the remote ref was ever needed for.
637 /// with the fetch either way. 636 Revision,
638 Revision { number: u32 },
639 } 637 }
640 638
641 /// Classify a fetched sync ref by the part of its name after the kind prefix, 639 /// Classify a fetched sync ref by the part of its name after the kind prefix,
@@ -662,10 +660,10 @@ fn classify_sync_ref(kind: &str, rest: &str) -> Option<(String, SyncRef)> {
662 SyncRef::Events { 660 SyncRef::Events {
663 local_ref: format!("refs/collab/patches/{}/events", id), 661 local_ref: format!("refs/collab/patches/{}/events", id),
664 } 662 }
665 } else if let Some(n) = suffix.strip_prefix("r/") { 663 } else if suffix.starts_with("rev/") || suffix.starts_with("r/") {
666 SyncRef::Revision { 664 // `r/` is the interim numbered shape, still published by peers that
667 number: n.parse().ok()?, 665 // have not converted. Ignored the same way as the current one.
668 } 666 SyncRef::Revision
669 } else { 667 } else {
670 return None; 668 return None;
671 }; 669 };
@@ -692,22 +690,6 @@ fn reconcile_refs(
692 .collect() 690 .collect()
693 }; 691 };
694 692
695 // The numbering each patch's revisions carry on the remote, keyed by patch
696 // id. Gathered before any events are reconciled so it describes what the
697 // remote actually published, and applied only to commits the signed DAG
698 // lists.
699 let mut remote_numbering: HashMap<String, state::RevisionNumbering> = HashMap::new();
700 for (remote_ref, id, classified) in &sync_refs {
701 if let SyncRef::Revision { number } = classified {
702 if let Ok(oid) = repo.refname_to_id(remote_ref) {
703 remote_numbering
704 .entry(id.clone())
705 .or_default()
706 .insert(*number, oid);
707 }
708 }
709 }
710
711 // Load trust policy once for all refs of this kind 693 // Load trust policy once for all refs of this kind
712 let trust_policy = trust::load_trust_policy(repo)?; 694 let trust_policy = trust::load_trust_policy(repo)?;
713 let mut warned_unconfigured = false; 695 let mut warned_unconfigured = false;
@@ -721,7 +703,7 @@ fn reconcile_refs(
721 703
722 // Revision refs are derived from the reconciled DAG below, never 704 // Revision refs are derived from the reconciled DAG below, never
723 // adopted from the remote. 705 // adopted from the remote.
724 if matches!(classified, SyncRef::Revision { .. }) { 706 if matches!(classified, SyncRef::Revision) {
725 continue; 707 continue;
726 } 708 }
727 709
@@ -782,19 +764,13 @@ fn reconcile_refs(
782 println!(" New {} {:.8} from remote", kind, id); 764 println!(" New {} {:.8} from remote", kind, id);
783 } 765 }
784 766
785 // Bring the revision refs back into agreement with the DAG we just 767 // Pin every revision the DAG we just reconciled lists, while the
786 // reconciled, while the fetched sync refs still hold the remote's 768 // fetched sync refs still hold the remote's objects — `cleanup_sync_refs`
787 // objects — `cleanup_sync_refs` drops them shortly after this returns, 769 // drops them shortly after this returns, and any revision commit
788 // and any revision commit without a ref of its own becomes gc-eligible 770 // without a ref of its own becomes gc-eligible at that point.
789 // at that point.
790 if kind == "patches" { 771 if kind == "patches" {
791 let empty = state::RevisionNumbering::new(); 772 if let Err(e) = state::pin_dag_revisions(repo, local_ref, id) {
792 let numbering = remote_numbering.get(id).unwrap_or(&empty); 773 eprintln!(" Failed to pin revisions for patch {:.8}: {}", id, e);
793 if let Err(e) = state::reconcile_revision_refs(repo, local_ref, id, numbering) {
794 eprintln!(
795 " Failed to reconcile revision refs for patch {:.8}: {}",
796 id, e
797 );
798 } 774 }
799 } 775 }
800 } 776 }
tests/revision_refs_test.rs
Old New
@@ -1,12 +1,15 @@
1 //! Patches carried as revision refs rather than branches. 1 //! Patches carried as revision refs rather than branches.
2 //! 2 //!
3 //! Each revision is an immutable ref in the patch's own namespace: 3 //! Each revision is pinned by a ref in the patch's own namespace:
4 //! 4 //!
5 //! refs/collab/patches/<id>/events the event DAG 5 //! refs/collab/patches/<id>/events the event DAG
6 //! refs/collab/patches/<id>/r/<n> revision n's commit, write-once 6 //! refs/collab/patches/<id>/rev/<oid> a revision's commit, pinned
7 //! 7 //!
8 //! so a patch and every revision it ever had travel under the refspecs `sync` 8 //! so a patch and every revision it ever had travel under the refspecs `sync`
9 //! already uses, and nothing depends on a `refs/heads/*` ref surviving. 9 //! already uses, and nothing depends on a `refs/heads/*` ref surviving.
10 //!
11 //! The refs are named by commit OID, so a name is never negotiated between
12 //! clones and never moves. Revision *numbering* lives in the DAG alone.
10 13
11 mod common; 14 mod common;
12 15
@@ -35,6 +38,22 @@ fn ref_target(repo: &TestRepo, name: &str) -> Option<String> {
35 } 38 }
36 } 39 }
37 40
41 /// The ref that pins `commit` for patch `id`.
42 fn rev_ref(id: &str, commit: &str) -> String {
43 format!("refs/collab/patches/{}/rev/{}", id, commit)
44 }
45
46 /// Every commit pinned for `id`, read back out of the ref names, sorted.
47 fn pinned_commits(repo: &TestRepo, id: &str) -> Vec<String> {
48 let prefix = format!("refs/collab/patches/{}/rev/", id);
49 let mut pinned: Vec<String> = patch_refs(repo)
50 .iter()
51 .filter_map(|r| r.strip_prefix(&prefix).map(str::to_string))
52 .collect();
53 pinned.sort();
54 pinned
55 }
56
38 fn show_json(repo: &TestRepo, id: &str) -> serde_json::Value { 57 fn show_json(repo: &TestRepo, id: &str) -> serde_json::Value {
39 let out = repo.run_ok(&["patch", "show", id, "--json"]); 58 let out = repo.run_ok(&["patch", "show", id, "--json"]);
40 serde_json::from_str(&out).unwrap() 59 serde_json::from_str(&out).unwrap()
@@ -73,7 +92,7 @@ fn full_id(repo: &TestRepo, short: &str) -> String {
73 // =========================================================================== 92 // ===========================================================================
74 93
75 #[test] 94 #[test]
76 fn patch_create_writes_an_events_ref_and_revision_one() { 95 fn patch_create_writes_an_events_ref_and_pins_the_first_revision() {
77 let repo = TestRepo::new("Alice", "alice@example.com"); 96 let repo = TestRepo::new("Alice", "alice@example.com");
78 let (_short, id, tip) = patch_on_branch(&repo, "feat", "a.txt"); 97 let (_short, id, tip) = patch_on_branch(&repo, "feat", "a.txt");
79 98
@@ -84,8 +103,8 @@ fn patch_create_writes_an_events_ref_and_revision_one() {
84 refs 103 refs
85 ); 104 );
86 assert!( 105 assert!(
87 refs.contains(&format!("refs/collab/patches/{}/r/1", id)), 106 refs.contains(&rev_ref(&id, &tip)),
88 "expected r/1, got {:?}", 107 "expected the created commit to be pinned, got {:?}",
89 refs 108 refs
90 ); 109 );
91 assert!( 110 assert!(
@@ -94,29 +113,38 @@ fn patch_create_writes_an_events_ref_and_revision_one() {
94 a directory: {:?}", 113 a directory: {:?}",
95 refs 114 refs
96 ); 115 );
116 }
117
118 #[test]
119 fn a_revision_ref_is_named_by_the_commit_it_pins() {
120 // This is what makes write-once structural rather than enforced: a ref
121 // whose name is its content has nothing to move to, so two clones can never
122 // disagree about it and a push can never be a non-fast-forward.
123 let repo = TestRepo::new("Alice", "alice@example.com");
124 let (_short, id, tip) = patch_on_branch(&repo, "feat", "a.txt");
125
97 assert_eq!( 126 assert_eq!(
98 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(), 127 ref_target(&repo, &rev_ref(&id, &tip)).as_deref(),
99 Some(tip.as_str()), 128 Some(tip.as_str()),
100 "r/1 must point at the commit the patch was created from" 129 "the ref named for a commit must point at that commit"
101 ); 130 );
131 assert_eq!(pinned_commits(&repo, &id), vec![tip]);
102 } 132 }
103 133
104 #[test] 134 #[test]
105 fn revise_writes_the_next_revision_ref_and_leaves_earlier_ones_alone() { 135 fn revise_pins_the_new_commit_and_leaves_earlier_ones_alone() {
106 let repo = TestRepo::new("Alice", "alice@example.com"); 136 let repo = TestRepo::new("Alice", "alice@example.com");
107 let (short, id, r1_commit) = patch_on_branch(&repo, "feat", "a.txt"); 137 let (short, id, r1_commit) = patch_on_branch(&repo, "feat", "a.txt");
108 138
109 let r2_commit = repo.commit_file("b.txt", "v2", "second commit"); 139 let r2_commit = repo.commit_file("b.txt", "v2", "second commit");
110 repo.run_ok(&["patch", "revise", &short, "-b", "addressed review"]); 140 repo.run_ok(&["patch", "revise", &short, "-b", "addressed review"]);
111 141
142 let mut expected = vec![r1_commit, r2_commit];
143 expected.sort();
112 assert_eq!( 144 assert_eq!(
113 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(), 145 pinned_commits(&repo, &id),
114 Some(r1_commit.as_str()), 146 expected,
115 "r/1 is write-once and must not move" 147 "both revisions must stay pinned"
116 );
117 assert_eq!(
118 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(),
119 Some(r2_commit.as_str())
120 ); 148 );
121 149
122 let json = show_json(&repo, &short); 150 let json = show_json(&repo, &short);
@@ -151,7 +179,7 @@ fn a_revision_ref_keeps_its_commit_alive_after_the_branch_is_rewritten() {
151 ); 179 );
152 180
153 assert_eq!( 181 assert_eq!(
154 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(), 182 ref_target(&repo, &rev_ref(&id, &r1_commit)).as_deref(),
155 Some(r1_commit.as_str()), 183 Some(r1_commit.as_str()),
156 "the revision ref is what keeps the commit reachable" 184 "the revision ref is what keeps the commit reachable"
157 ); 185 );
@@ -175,8 +203,8 @@ fn nested_revision_refs_are_visible_to_the_patch_glob() {
175 .unwrap(); 203 .unwrap();
176 repo.reference("refs/collab/patches/abc/events", oid, false, "t") 204 repo.reference("refs/collab/patches/abc/events", oid, false, "t")
177 .unwrap(); 205 .unwrap();
178 repo.reference("refs/collab/patches/abc/r/1", oid, false, "t") 206 let nested = format!("refs/collab/patches/abc/rev/{}", oid);
179 .unwrap(); 207 repo.reference(&nested, oid, false, "t").unwrap();
180 208
181 let names: Vec<String> = repo 209 let names: Vec<String> = repo
182 .references_glob("refs/collab/patches/*") 210 .references_glob("refs/collab/patches/*")
@@ -184,7 +212,7 @@ fn nested_revision_refs_are_visible_to_the_patch_glob() {
184 .filter_map(|r| r.ok()?.name().map(str::to_string)) 212 .filter_map(|r| r.ok()?.name().map(str::to_string))
185 .collect(); 213 .collect();
186 assert!( 214 assert!(
187 names.contains(&"refs/collab/patches/abc/r/1".to_string()), 215 names.contains(&nested),
188 "refs/collab/patches/* must match nested refs, got {:?}", 216 "refs/collab/patches/* must match nested refs, got {:?}",
189 names 217 names
190 ); 218 );
@@ -242,37 +270,37 @@ fn revise_reads_head_by_default_and_a_named_branch_on_request() {
242 } 270 }
243 271
244 #[test] 272 #[test]
245 fn revise_records_nothing_when_the_revision_ref_is_taken() { 273 fn every_revision_in_the_dag_stays_pinned_across_many_revises() {
246 // The event must not outlive the ref write. A PatchRevision in the DAG with 274 // The invariant the refs exist for: whatever the DAG says a revision is,
247 // no r/<n> beside it is exactly the state this design exists to prevent, so 275 // its objects are reachable. With OID names there is no collision to
248 // a write-once rejection has to leave the patch untouched, not half-written. 276 // resolve, so this holds by construction — assert it anyway, because it is
277 // the property, and the numbered scheme it replaced could not keep it.
249 let repo = TestRepo::new("Alice", "alice@example.com"); 278 let repo = TestRepo::new("Alice", "alice@example.com");
250 let (short, id, _r1) = patch_on_branch(&repo, "feat", "a.txt"); 279 let (short, id, first) = patch_on_branch(&repo, "feat", "a.txt");
251 280
252 // Squat on r/2 with an unrelated commit. 281 let mut commits = vec![first];
253 let squatter = repo.git(&["rev-parse", "main"]).trim().to_string(); 282 for n in 2..=4 {
254 repo.git(&[ 283 commits.push(repo.commit_file(&format!("v{}.txt", n), "x", "more work"));
255 "update-ref", 284 repo.run_ok(&["patch", "revise", &short]);
256 &format!("refs/collab/patches/{}/r/2", id), 285 }
257 &squatter,
258 ]);
259
260 repo.commit_file("b.txt", "v2", "second commit");
261 let err = repo.run_err(&["patch", "revise", &short]);
262 assert!(err.contains("write-once"), "unexpected error: {}", err);
263 286
264 let json = show_json(&repo, &short); 287 let json = show_json(&repo, &short);
265 assert_eq!( 288 let revisions = json["revisions"].as_array().unwrap();
266 json["revisions"].as_array().unwrap().len(), 289 assert_eq!(revisions.len(), 4);
267 1, 290
268 "a rejected revise must not leave a revision in the DAG: {}", 291 let pinned = pinned_commits(&repo, &id);
269 json 292 for rev in revisions {
270 ); 293 let commit = rev["commit"].as_str().unwrap();
271 assert_eq!( 294 assert!(
272 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(), 295 pinned.contains(&commit.to_string()),
273 Some(squatter.as_str()), 296 "revision {} ({}) is not pinned; pinned: {:?}",
274 "and must not move the ref it collided with" 297 rev["number"],
275 ); 298 commit,
299 pinned
300 );
301 }
302 commits.sort();
303 assert_eq!(pinned, commits, "nothing beyond the DAG should be pinned");
276 } 304 }
277 305
278 // =========================================================================== 306 // ===========================================================================
@@ -658,7 +686,7 @@ fn an_inline_comment_on_revision_one_still_resolves_after_a_rebase() {
658 fn closing_a_patch_archives_every_revision_ref() { 686 fn closing_a_patch_archives_every_revision_ref() {
659 let repo = TestRepo::new("Alice", "alice@example.com"); 687 let repo = TestRepo::new("Alice", "alice@example.com");
660 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt"); 688 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
661 repo.commit_file("b.txt", "v2", "second commit"); 689 let r2 = repo.commit_file("b.txt", "v2", "second commit");
662 repo.run_ok(&["patch", "revise", &short]); 690 repo.run_ok(&["patch", "revise", &short]);
663 691
664 repo.run_ok(&["patch", "close", &short, "-r", "not now"]); 692 repo.run_ok(&["patch", "close", &short, "-r", "not now"]);
@@ -670,12 +698,20 @@ fn closing_a_patch_archives_every_revision_ref() {
670 "nothing may be left in the active namespace: {:?}", 698 "nothing may be left in the active namespace: {:?}",
671 refs 699 refs
672 ); 700 );
673 for suffix in ["events", "r/1", "r/2"] { 701 for suffix in [
702 "events".to_string(),
703 format!("rev/{}", r1),
704 format!("rev/{}", r2),
705 ] {
674 let name = format!("refs/collab/archive/patches/{}/{}", id, suffix); 706 let name = format!("refs/collab/archive/patches/{}/{}", id, suffix);
675 assert!(refs.contains(&name), "missing {} in {:?}", name, refs); 707 assert!(refs.contains(&name), "missing {} in {:?}", name, refs);
676 } 708 }
677 assert_eq!( 709 assert_eq!(
678 ref_target(&repo, &format!("refs/collab/archive/patches/{}/r/1", id)).as_deref(), 710 ref_target(
711 &repo,
712 &format!("refs/collab/archive/patches/{}/rev/{}", id, r1)
713 )
714 .as_deref(),
679 Some(r1.as_str()) 715 Some(r1.as_str())
680 ); 716 );
681 717
@@ -767,17 +803,51 @@ fn migration_resumes_after_an_interrupted_run() {
767 "{:?}", 803 "{:?}",
768 refs 804 refs
769 ); 805 );
770 assert_eq!( 806 let mut expected = vec![r1, r2];
771 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(), 807 expected.sort();
772 Some(r1.as_str()) 808 assert_eq!(pinned_commits(&repo, &id), expected);
809 assert!(
810 ref_target(&repo, &format!("refs/collab/local/migrating/patches/{}", id)).is_none(),
811 "the park must be cleared once the patch is whole"
773 ); 812 );
813 }
814
815 #[test]
816 fn migration_resumes_between_the_events_ref_and_the_pins() {
817 // The narrower window: the events ref landed but the revisions were never
818 // pinned. The patch lists fine, so nothing else would notice — only the
819 // park says the migration never finished, and dropping it on sight would
820 // leave the revisions unpinned for good.
821 let repo = TestRepo::new("Alice", "alice@example.com");
822 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
823 let r2 = repo.commit_file("b.txt", "v2", "second commit");
824 repo.run_ok(&["patch", "revise", &short]);
825
826 let tip = ref_target(&repo, &format!("refs/collab/patches/{}/events", id)).unwrap();
827 for name in patch_refs(&repo) {
828 if name.starts_with(&format!("refs/collab/patches/{}/rev/", id)) {
829 repo.git(&["update-ref", "-d", &name]);
830 }
831 }
832 repo.git(&[
833 "update-ref",
834 &format!("refs/collab/local/migrating/patches/{}", id),
835 &tip,
836 ]);
837 assert!(pinned_commits(&repo, &id).is_empty(), "precondition");
838
839 repo.run_ok(&["patch", "list"]);
840
841 let mut expected = vec![r1, r2];
842 expected.sort();
774 assert_eq!( 843 assert_eq!(
775 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(), 844 pinned_commits(&repo, &id),
776 Some(r2.as_str()) 845 expected,
846 "the resume must pin what the interrupted run did not"
777 ); 847 );
778 assert!( 848 assert!(
779 ref_target(&repo, &format!("refs/collab/local/migrating/patches/{}", id)).is_none(), 849 ref_target(&repo, &format!("refs/collab/local/migrating/patches/{}", id)).is_none(),
780 "the park must be cleared once the patch is whole" 850 "and only then drop the park"
781 ); 851 );
782 } 852 }
783 853
@@ -829,14 +899,9 @@ fn an_old_layout_patch_is_migrated_on_first_use() {
829 "the old ref must be gone: {:?}", 899 "the old ref must be gone: {:?}",
830 refs 900 refs
831 ); 901 );
832 assert_eq!( 902 let mut expected = vec![r1, r2];
833 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(), 903 expected.sort();
834 Some(r1.as_str()) 904 assert_eq!(pinned_commits(&repo, &id), expected);
835 );
836 assert_eq!(
837 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(),
838 Some(r2.as_str())
839 );
840 905
841 // The migrated patch still diffs. 906 // The migrated patch still diffs.
842 let diff = repo.run_ok(&["patch", "diff", &short, "--revision", "1"]); 907 let diff = repo.run_ok(&["patch", "diff", &short, "--revision", "1"]);
@@ -906,17 +971,91 @@ fn migration_skips_a_revision_whose_commit_is_already_lost() {
906 refs 971 refs
907 ); 972 );
908 assert!( 973 assert!(
909 !refs.contains(&format!("refs/collab/patches/{}/r/1", id)), 974 !refs.contains(&rev_ref(&id, missing)),
910 "a lost commit must not get a ref: {:?}", 975 "a lost commit must not get a ref: {:?}",
911 refs 976 refs
912 ); 977 );
913 assert_eq!( 978 assert_eq!(
914 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(), 979 pinned_commits(&repo, &id),
915 Some(head.as_str()), 980 vec![head],
916 "the surviving revision still gets one" 981 "only the surviving revision is pinned"
917 ); 982 );
918 } 983 }
919 984
985 /// Rewrite a patch into the interim numbered layout: the events ref stays, but
986 /// revisions are pinned by `r/<n>` instead of `rev/<oid>`. This shape is not
987 /// hypothetical — it is what a repo migrated by the previous draft is in.
988 fn demote_to_numbered_revision_refs(repo: &TestRepo, id: &str) {
989 let prefix = format!("refs/collab/patches/{}/rev/", id);
990 let mut n = 0;
991 for name in patch_refs(repo) {
992 let Some(commit) = name.strip_prefix(&prefix) else {
993 continue;
994 };
995 n += 1;
996 repo.git(&[
997 "update-ref",
998 &format!("refs/collab/patches/{}/r/{}", id, n),
999 commit,
1000 ]);
1001 repo.git(&["update-ref", "-d", &name]);
1002 }
1003 assert!(n > 0, "nothing to demote");
1004 }
1005
1006 #[test]
1007 fn numbered_revision_refs_are_converted_to_oid_names() {
1008 let repo = TestRepo::new("Alice", "alice@example.com");
1009 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
1010 let r2 = repo.commit_file("b.txt", "v2", "second commit");
1011 repo.run_ok(&["patch", "revise", &short]);
1012 demote_to_numbered_revision_refs(&repo, &id);
1013
1014 repo.run_ok(&["patch", "list"]);
1015
1016 let mut expected = vec![r1, r2];
1017 expected.sort();
1018 assert_eq!(
1019 pinned_commits(&repo, &id),
1020 expected,
1021 "every revision must be pinned under its OID name"
1022 );
1023 assert!(
1024 patch_refs(&repo)
1025 .iter()
1026 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/r/", id))),
1027 "the numbered refs must be retired, or they keep being pushed"
1028 );
1029 }
1030
1031 #[test]
1032 fn converting_numbered_refs_drops_one_the_dag_does_not_vouch_for() {
1033 // A numbered ref pointing at a commit no event lists is exactly the wedge
1034 // OID naming removes: a published number nobody can renegotiate. It goes,
1035 // and the revisions that are real stay.
1036 let repo = TestRepo::new("Alice", "alice@example.com");
1037 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
1038 demote_to_numbered_revision_refs(&repo, &id);
1039 let stray = repo.git(&["rev-parse", "main"]).trim().to_string();
1040 repo.git(&[
1041 "update-ref",
1042 &format!("refs/collab/patches/{}/r/7", id),
1043 &stray,
1044 ]);
1045
1046 repo.run_ok(&["patch", "list"]);
1047
1048 assert_eq!(pinned_commits(&repo, &id), vec![r1]);
1049 assert!(
1050 patch_refs(&repo)
1051 .iter()
1052 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/r/", id))),
1053 "the stray numbered ref must go too"
1054 );
1055 // The patch itself is untouched.
1056 assert_eq!(show_json(&repo, &short)["revisions"].as_array().unwrap().len(), 1);
1057 }
1058
920 #[test] 1059 #[test]
921 fn a_fully_legacy_patch_with_no_recorded_commits_still_materializes() { 1060 fn a_fully_legacy_patch_with_no_recorded_commits_still_materializes() {
922 let repo = TestRepo::new("Alice", "alice@example.com"); 1061 let repo = TestRepo::new("Alice", "alice@example.com");
@@ -954,9 +1093,8 @@ fn a_fully_legacy_patch_with_no_recorded_commits_still_materializes() {
954 refs 1093 refs
955 ); 1094 );
956 assert!( 1095 assert!(
957 refs.iter() 1096 pinned_commits(&repo, &id).is_empty(),
958 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/r/", id))), 1097 "no commits were ever recorded, so there is nothing to pin: {:?}",
959 "no commits were ever recorded, so there is nothing to point a ref at: {:?}",
960 refs 1098 refs
961 ); 1099 );
962 } 1100 }
tests/sync_test.rs
Old New
@@ -1871,11 +1871,11 @@ fn patch_revisions_reach_a_second_clone_without_pushing_any_branch() {
1871 let patch = &patches[0]; 1871 let patch = &patches[0];
1872 assert_eq!(patch.title, "No branch push"); 1872 assert_eq!(patch.title, "No branch push");
1873 1873
1874 let revision_ref = format!("refs/collab/patches/{}/r/1", patch.id); 1874 let revision_ref = format!("refs/collab/patches/{}/rev/{}", patch.id, feat);
1875 assert_eq!( 1875 assert_eq!(
1876 bob_repo.refname_to_id(&revision_ref).ok(), 1876 bob_repo.refname_to_id(&revision_ref).ok(),
1877 Some(feat), 1877 Some(feat),
1878 "revision 1's ref must have travelled" 1878 "revision 1 must be pinned in bob's clone"
1879 ); 1879 );
1880 assert!( 1880 assert!(
1881 bob_repo.find_commit(feat).is_ok(), 1881 bob_repo.find_commit(feat).is_ok(),
@@ -1943,12 +1943,13 @@ fn sync_migrates_an_old_layout_repo_before_reconciling() {
1943 let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap(); 1943 let alice_repo = Repository::open(cluster.alice_dir.path()).unwrap();
1944 let events = format!("refs/collab/patches/{}/events", id); 1944 let events = format!("refs/collab/patches/{}/events", id);
1945 let tip = alice_repo.refname_to_id(&events).unwrap(); 1945 let tip = alice_repo.refname_to_id(&events).unwrap();
1946 for suffix in ["events", "r/1"] { 1946 let subtree: Vec<String> = alice_repo
1947 alice_repo 1947 .references_glob(&format!("refs/collab/patches/{}/*", id))
1948 .find_reference(&format!("refs/collab/patches/{}/{}", id, suffix)) 1948 .unwrap()
1949 .unwrap() 1949 .filter_map(|r| r.ok()?.name().map(str::to_string))
1950 .delete() 1950 .collect();
1951 .unwrap(); 1951 for name in subtree {
1952 alice_repo.find_reference(&name).unwrap().delete().unwrap();
1952 } 1953 }
1953 alice_repo 1954 alice_repo
1954 .reference(&format!("refs/collab/patches/{}", id), tip, false, "demote") 1955 .reference(&format!("refs/collab/patches/{}", id), tip, false, "demote")
@@ -1964,7 +1965,7 @@ fn sync_migrates_an_old_layout_repo_before_reconciling() {
1964 ); 1965 );
1965 assert_eq!( 1966 assert_eq!(
1966 alice_repo 1967 alice_repo
1967 .refname_to_id(&format!("refs/collab/patches/{}/r/1", id)) 1968 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, feat))
1968 .ok(), 1969 .ok(),
1969 Some(feat) 1970 Some(feat)
1970 ); 1971 );
@@ -1975,7 +1976,7 @@ fn sync_migrates_an_old_layout_repo_before_reconciling() {
1975 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap(); 1976 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
1976 assert_eq!( 1977 assert_eq!(
1977 bob_repo 1978 bob_repo
1978 .refname_to_id(&format!("refs/collab/patches/{}/r/1", id)) 1979 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, feat))
1979 .ok(), 1980 .ok(),
1980 Some(feat) 1981 Some(feat)
1981 ); 1982 );
@@ -2003,17 +2004,19 @@ fn sync_does_not_adopt_a_revision_ref_the_signed_dag_does_not_vouch_for() {
2003 state::list_patches(&repo).unwrap()[0].id.clone() 2004 state::list_patches(&repo).unwrap()[0].id.clone()
2004 }; 2005 };
2005 2006
2006 // Plant three shapes directly on the remote: a well-formed id with no 2007 // Plant four shapes directly on the remote: an OID-named ref under a
2007 // events ref, a non-decimal revision number, and a revision number on a 2008 // well-formed id with no events ref, an OID-named ref on a real patch whose
2008 // real patch whose commit the signed DAG never mentions. 2009 // commit the signed DAG never mentions, a ref whose name disagrees with the
2010 // commit it points at, and a leftover numbered ref.
2009 let planted = commit_on_branch(&alice_repo, "planted", base, "planted.txt", b"payload"); 2011 let planted = commit_on_branch(&alice_repo, "planted", base, "planted.txt", b"payload");
2010 let orphan_id = "0".repeat(40); 2012 let orphan_id = "0".repeat(40);
2011 Command::new("git") 2013 Command::new("git")
2012 .args([ 2014 .args([
2013 "push", 2015 "push",
2014 "origin", 2016 "origin",
2015 &format!("{}:refs/collab/patches/{}/r/1", planted, orphan_id), 2017 &format!("{}:refs/collab/patches/{}/rev/{}", planted, orphan_id, planted),
2016 &format!("{}:refs/collab/patches/{}/r/notanumber", planted, id), 2018 &format!("{}:refs/collab/patches/{}/rev/{}", planted, id, planted),
2019 &format!("{}:refs/collab/patches/{}/rev/{}", planted, id, feat),
2017 &format!("{}:refs/collab/patches/{}/r/9", planted, id), 2020 &format!("{}:refs/collab/patches/{}/r/9", planted, id),
2018 ]) 2021 ])
2019 .current_dir(cluster.alice_dir.path()) 2022 .current_dir(cluster.alice_dir.path())
@@ -2024,17 +2027,18 @@ fn sync_does_not_adopt_a_revision_ref_the_signed_dag_does_not_vouch_for() {
2024 sync::sync(&bob_repo, "origin").unwrap(); 2027 sync::sync(&bob_repo, "origin").unwrap();
2025 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap(); 2028 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
2026 2029
2027 // The real patch still arrives intact. 2030 // The real patch still arrives intact, pinned at its own commit — not at
2031 // the payload a lying ref name claimed for it.
2028 assert_eq!( 2032 assert_eq!(
2029 bob_repo 2033 bob_repo
2030 .refname_to_id(&format!("refs/collab/patches/{}/r/1", id)) 2034 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, feat))
2031 .ok(), 2035 .ok(),
2032 Some(feat) 2036 Some(feat)
2033 ); 2037 );
2034 2038
2035 for bad in [ 2039 for bad in [
2036 format!("refs/collab/patches/{}/r/1", orphan_id), 2040 format!("refs/collab/patches/{}/rev/{}", orphan_id, planted),
2037 format!("refs/collab/patches/{}/r/notanumber", id), 2041 format!("refs/collab/patches/{}/rev/{}", id, planted),
2038 format!("refs/collab/patches/{}/r/9", id), 2042 format!("refs/collab/patches/{}/r/9", id),
2039 ] { 2043 ] {
2040 assert!( 2044 assert!(
@@ -2047,12 +2051,12 @@ fn sync_does_not_adopt_a_revision_ref_the_signed_dag_does_not_vouch_for() {
2047 2051
2048 #[test] 2052 #[test]
2049 fn concurrent_revise_leaves_every_revision_reachable() { 2053 fn concurrent_revise_leaves_every_revision_reachable() {
2050 // Revision numbering is derived from the DAG walk while ref names are 2054 // Two clones revising offline both record "revision 2". Under a numbered
2051 // derived at write time, so a concurrent revise can renumber the loser. 2055 // ref layout they would both claim the same name, and the loser's push
2052 // If nothing reconciles the refs against the DAG afterwards, that 2056 // would be rejected as a non-fast-forward permanently — wedging sync — or,
2053 // revision's commit ends up referenced by nothing once the sync refs are 2057 // if the numbering were reconciled instead, would leave one commit pinned
2054 // swept — the force-push orphan this design exists to prevent, arriving 2058 // by nothing once the sync refs are swept. Naming by OID removes the
2055 // through concurrency instead of rebase. 2059 // collision, so both survive and both pushes are additive.
2056 let cluster = TestCluster::new(); 2060 let cluster = TestCluster::new();
2057 let alice_repo = cluster.alice_repo(); 2061 let alice_repo = cluster.alice_repo();
2058 2062
@@ -2115,7 +2119,7 @@ fn concurrent_revise_leaves_every_revision_reachable() {
2115 // Every revision the DAG lists must be reachable from a ref in this 2119 // Every revision the DAG lists must be reachable from a ref in this
2116 // repo, or the sync-ref sweep has just made it gc-eligible. 2120 // repo, or the sync-ref sweep has just made it gc-eligible.
2117 let referenced: Vec<git2::Oid> = repo 2121 let referenced: Vec<git2::Oid> = repo
2118 .references_glob(&format!("refs/collab/patches/{}/r/*", id)) 2122 .references_glob(&format!("refs/collab/patches/{}/rev/*", id))
2119 .unwrap() 2123 .unwrap()
2120 .filter_map(|r| r.ok()?.target()) 2124 .filter_map(|r| r.ok()?.target())
2121 .collect(); 2125 .collect();
@@ -2140,3 +2144,98 @@ fn concurrent_revise_leaves_every_revision_reachable() {
2140 } 2144 }
2141 } 2145 }
2142 } 2146 }
2147
2148 #[test]
2149 fn a_stale_revision_ref_on_the_remote_does_not_wedge_a_later_sync() {
2150 // A revision ref can reach the remote without the events that vouch for it
2151 // — a batched push falls back to per-ref pushes, and one can land while the
2152 // other is rejected. Under a numbered layout that stranded name was
2153 // unusable by anyone: the next clone to number a revision the same way had
2154 // its push rejected non-fast-forward, permanently, and sync then wedged in
2155 // resume mode and stopped pushing anything at all. An OID name cannot
2156 // collide, so a stale ref is inert.
2157 let cluster = TestCluster::new();
2158 let alice_repo = cluster.alice_repo();
2159
2160 let base = make_commit_with_message(&alice_repo, "base");
2161 let r1 = commit_on_branch(&alice_repo, "feat", base, "feature.txt", b"v1");
2162 cluster.run_collab_ok(
2163 cluster.alice_dir.path(),
2164 &["patch", "create", "-t", "Stale ref", "-B", "feat"],
2165 );
2166 sync::sync(&alice_repo, "origin").unwrap();
2167 let id = {
2168 let repo = Repository::open(cluster.alice_dir.path()).unwrap();
2169 state::list_patches(&repo).unwrap()[0].id.clone()
2170 };
2171
2172 // A revision commit published with no event vouching for it, exactly as the
2173 // partial-push fallback would leave it.
2174 let stranded = commit_on_branch(&alice_repo, "stranded", r1, "stranded.txt", b"orphan");
2175 Command::new("git")
2176 .args([
2177 "push",
2178 "origin",
2179 &format!(
2180 "{}:refs/collab/patches/{}/rev/{}",
2181 stranded, id, stranded
2182 ),
2183 ])
2184 .current_dir(cluster.alice_dir.path())
2185 .status()
2186 .unwrap();
2187
2188 // A clone that did nothing wrong revises and syncs, repeatedly.
2189 let bob_repo = cluster.bob_repo();
2190 sync::sync(&bob_repo, "origin").unwrap();
2191 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
2192 let origin_main = bob_repo
2193 .refname_to_id("refs/remotes/origin/main")
2194 .unwrap_or(base);
2195 bob_repo
2196 .reference("refs/heads/main", origin_main, true, "seed local main")
2197 .unwrap();
2198 let bob_rev = commit_on_branch(&bob_repo, "feat", r1, "bob.txt", b"bob");
2199 cluster.run_collab_ok(
2200 cluster.bob_dir.path(),
2201 &["patch", "revise", &id[..8], "-B", "feat"],
2202 );
2203
2204 for round in 0..3 {
2205 sync::sync(&Repository::open(cluster.bob_dir.path()).unwrap(), "origin")
2206 .unwrap_or_else(|e| panic!("bob's sync {} failed: {}", round, e));
2207 }
2208
2209 // Bob's revision reached the remote, and a third clone picks it up cleanly.
2210 let carol_dir = TempDir::new().unwrap();
2211 let carol = Repository::clone(
2212 cluster.bare_dir().to_str().unwrap(),
2213 carol_dir.path(),
2214 )
2215 .unwrap();
2216 {
2217 let mut config = carol.config().unwrap();
2218 config.set_str("user.name", "Carol").unwrap();
2219 config.set_str("user.email", "carol@example.com").unwrap();
2220 }
2221 sync::init(&carol).unwrap();
2222 sync::sync(&carol, "origin").expect("a fresh clone must not be wedged");
2223
2224 let carol = Repository::open(carol_dir.path()).unwrap();
2225 assert!(
2226 carol.find_commit(bob_rev).is_ok(),
2227 "bob's revision should have travelled"
2228 );
2229 assert!(
2230 carol
2231 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, bob_rev))
2232 .is_ok(),
2233 "and be pinned"
2234 );
2235 assert!(
2236 carol
2237 .refname_to_id(&format!("refs/collab/patches/{}/rev/{}", id, stranded))
2238 .is_err(),
2239 "the stranded ref is vouched for by nothing and must not be adopted"
2240 );
2241 }