Skip to main content

max / makenotwork

server: let a notes writer choose what a lost race does The retry loop merged both sides unconditionally, which is right for a note somebody typed and wrong for one stating a fact. Two build results for one commit are not two opinions to reconcile; merging them publishes a note claiming the build both passed and failed. write_note keeps merging and is unchanged for its two callers. write_note_with takes the choice, which is a property of what the namespace is for rather than of MNW, so it belongs at this layer.
Author: Max Johnson <me@maxj.phd> · 2026-08-09 03:06 UTC
Signed with PGP, not checked
Commit: 79ddd290e204aa07e7f8a50e5490d04ab02305e8
Parent: 1c0534e
2 files changed, +105 insertions, -5 deletions
M server/src/git/notes/mod.rs +40 -5
@@ -801,8 +801,39 @@
801 801 },
802 802 }
803 803
804 + /// What a write does when it loses the ref race and finds the note it was
805 + /// editing already changed by somebody else.
806 + ///
807 + /// Only reachable on a retry: winning the race means there was nothing to
808 + /// reconcile.
809 + #[derive(Debug, Clone, Copy, PartialEq, Eq)]
810 + pub enum LostRace {
811 + /// Keep both sides (see [`merge_edit`]). Two people who annotated the same
812 + /// commit each meant something, and dropping either is losing a note.
813 + Merge,
814 + /// Take ours and discard theirs. The right answer for a note that states a
815 + /// current fact rather than an opinion: a second build result for one
816 + /// commit supersedes the first, and merging them would produce a note that
817 + /// claims the build both passed and failed.
818 + Overwrite,
819 + }
820 +
804 821 /// Write, edit or remove one note, publishing it on `refs/notes/<namespace>`.
805 822 ///
823 + /// Reconciles a lost race by merging, which is what a note somebody typed
824 + /// wants. [`write_note_with`] is the same thing with the choice exposed.
825 + pub fn write_note<E: NoteObjects + NoteWrites>(
826 + engine: &E,
827 + namespace: &str,
828 + target: Oid,
829 + content: Option<&[u8]>,
830 + who: &Signature,
831 + ) -> Result<Written, NotesError> {
832 + write_note_with(engine, namespace, target, content, who, LostRace::Merge)
833 + }
834 +
835 + /// Write, edit or remove one note, choosing what a lost race does.
836 + ///
806 837 /// `content` of `None` removes the note. `who` is both author and committer:
807 838 /// the person doing it is the person who did it, and a notes commit has no
808 839 /// distinct patch author to credit.
@@ -810,14 +841,15 @@
810 841 /// The ref moves by compare-and-swap against the tip this write was built on,
811 842 /// so a concurrent writer is never overwritten — the loser reloads and tries
812 843 /// again against what the winner left. Two people annotating different commits
813 - /// both land. Two people annotating the *same* commit get their notes merged
814 - /// (see [`merge_edit`]) rather than one silently replacing the other.
815 - pub fn write_note<E: NoteObjects + NoteWrites>(
844 + /// both land whichever `on_lost_race` says; it decides only what happens when
845 + /// two writes touch the *same* note.
846 + pub fn write_note_with<E: NoteObjects + NoteWrites>(
816 847 engine: &E,
817 848 namespace: &str,
818 849 target: Oid,
819 850 content: Option<&[u8]>,
820 851 who: &Signature,
852 + on_lost_race: LostRace,
821 853 ) -> Result<Written, NotesError> {
822 854 let full_ref = format!("{NOTES_REF_PREFIX}{namespace}");
823 855 // What the note said when this write was composed. A retry compares
@@ -842,8 +874,11 @@
842 874 }
843 875
844 876 // The note moved under us and we are setting content, so what the
845 - // writer typed is no longer the whole story.
846 - let merged = attempt > 0 && current != base && content.is_some();
877 + // writer typed is no longer the whole story. Unless the caller said
878 + // theirs does not survive contact with ours, in which case there is
879 + // nothing to reconcile and the retry simply rebuilds on the new tip.
880 + let merged =
881 + on_lost_race == LostRace::Merge && attempt > 0 && current != base && content.is_some();
847 882 let mut buffer;
848 883 let content = match (merged, content, current) {
849 884 (true, Some(ours), Some(theirs)) => {
M server/src/git/notes/tests.rs +65
@@ -1429,6 +1429,71 @@
1429 1429 assert!(content.contains("a line only they wrote"), "{content}");
1430 1430 }
1431 1431
1432 + #[test]
1433 + fn an_overwriting_writer_discards_the_other_side() {
1434 + let (_tmp, repo) = init_bare();
1435 + let engine = Contended::new(&repo, 1, oid(T1), "build failed");
1436 +
1437 + // What a server-owned namespace wants. Two results for one commit are not
1438 + // two opinions to reconcile: the later one supersedes, and a merge would
1439 + // publish a note saying the build both passed and failed.
1440 + let written = write_note_with(
1441 + &engine,
1442 + DEFAULT_NAMESPACE,
1443 + oid(T1),
1444 + Some(b"build passed"),
1445 + &signature("makenot.work"),
1446 + LostRace::Overwrite,
1447 + )
1448 + .unwrap();
1449 +
1450 + let Written::Committed { tip, merged } = written else {
1451 + panic!("expected a commit, got {written:?}");
1452 + };
1453 + assert!(!merged, "nothing was merged, so nothing may be reported");
1454 +
1455 + let note = note_for(&engine.inner, tip, oid(T1)).unwrap().unwrap();
1456 + let content = note.content_lossy();
1457 + assert!(content.contains("build passed"), "{content}");
1458 + assert!(!content.contains("build failed"), "{content}");
1459 + }
1460 +
1461 + #[test]
1462 + fn overwriting_still_leaves_a_note_on_another_target_alone() {
1463 + let (_tmp, repo) = init_bare();
1464 + // The other writer annotates a different commit, which is not a conflict at
1465 + // all. Overwrite decides what happens to the same note, not to the ref.
1466 + let engine = Contended::new(&repo, 1, oid(T3), "theirs");
1467 +
1468 + let written = write_note_with(
1469 + &engine,
1470 + DEFAULT_NAMESPACE,
1471 + oid(T1),
1472 + Some(b"ours"),
1473 + &signature("makenot.work"),
1474 + LostRace::Overwrite,
1475 + )
1476 + .unwrap();
1477 +
1478 + let Written::Committed { tip, .. } = written else {
1479 + panic!("expected a commit, got {written:?}");
1480 + };
1481 + assert_eq!(
1482 + note_for(&engine.inner, tip, oid(T1))
1483 + .unwrap()
1484 + .unwrap()
1485 + .content_lossy(),
1486 + "ours"
1487 + );
1488 + assert_eq!(
1489 + note_for(&engine.inner, tip, oid(T3))
1490 + .unwrap()
1491 + .unwrap()
1492 + .content_lossy(),
1493 + "theirs"
1494 + );
1495 + }
1496 +
1432 1497 #[test]
1433 1498 fn a_merge_does_not_double_the_text_both_writers_started_from() {
1434 1499 let (_tmp, repo) = init_bare();