diff --git a/src/cmd/stack/write.rs b/src/cmd/stack/write.rs index 949381f..8178e0f 100644 --- a/src/cmd/stack/write.rs +++ b/src/cmd/stack/write.rs @@ -1269,6 +1269,12 @@ pub(in crate::cmd) struct ReconciledMemberJson { /// Rounds the record will hold once this run is done: one more than it /// had for an `update`, unchanged otherwise, and 1 for an `add`. pub rounds: usize, + /// True on an `update` whose stored description has been written over + /// since its last round — by `pr edit` or Tangled's edit box — and so + /// is left exactly as it is instead of being regenerated from the + /// commit message. Always false for every other fate: nothing else + /// touches a body at all. + pub body_kept: bool, pub images: Vec, } @@ -1338,6 +1344,7 @@ fn reconciled_member_json( sha: Some(planned[*index].sha().to_string()), shas: planned[*index].shas.clone(), rounds: old[*member].rounds + 1, + body_kept: !old[*member].body_is_its_commits(), images: images(*index), }, Slot::Keep { member, relink } => ReconciledMemberJson { @@ -1348,6 +1355,7 @@ fn reconciled_member_json( sha: None, shas: Vec::new(), rounds: old[*member].rounds, + body_kept: false, images: Vec::new(), }, Slot::Frozen { member } => ReconciledMemberJson { @@ -1358,6 +1366,7 @@ fn reconciled_member_json( sha: None, shas: Vec::new(), rounds: old[*member].rounds, + body_kept: false, images: Vec::new(), }, Slot::Add { index } => ReconciledMemberJson { @@ -1369,6 +1378,7 @@ fn reconciled_member_json( sha: Some(planned[*index].sha().to_string()), shas: planned[*index].shas.clone(), rounds: 1, + body_kept: false, images: images(*index), }, } @@ -1407,6 +1417,10 @@ struct OldMember { title: String, /// State label as the listings print it: open, closed, merged, or `?`. state: String, + /// The description the record holds now. Paired with `latest_patch`, + /// this is what [`OldMember::body_is_its_commits`] compares: what is + /// stored against what that patch's own message would generate. + body: Option, /// Every change-id in the latest round's patch, one per commit the /// member carries, bottom first — the record itself has no change-id /// field anywhere; the patch headers are the identity. Empty for a @@ -1426,6 +1440,54 @@ impl OldMember { fn owns(&self, id: &str) -> bool { self.change_ids.iter().any(|mine| mine == id) } + + /// Whether the description this record holds is still the one its + /// latest round's commit message produced — that is, whether anything + /// has been written over it since. + /// + /// This is what stands between a round and somebody's typing. A stacked + /// pull's body comes from its commit message, so a resubmit that + /// regenerated it unconditionally was right for the bodies nobody had + /// touched and silently destructive for every other: `atgc pr edit` and + /// Tangled's own edit box both write descriptions — screenshots, + /// context, the whole reason a reviewer reads a pull — that no commit + /// message has, and each round reverted the lot to `%b`. Thirteen pulls + /// across two stacks were rewritten by hand after that happened. + /// + /// Comparing against the *stored* patch rather than the new commit is + /// what makes both halves work: an untouched body still follows an + /// amended message, because the round it was generated from is the one + /// being replaced. Anything this cannot parse compares unequal and so + /// keeps what is stored, which is the safe direction — the commit + /// message is recoverable from git and an edited body is not. + fn body_is_its_commits(&self) -> bool { + comparable_body(self.body.as_deref()) == body_of_patch(&self.latest_patch) + } +} + +/// A body as it is compared: line endings normalized, blank counted as +/// absent. `stack create` writes no body at all for a commit with none, but +/// `pr edit --body ''` clears one to a field that may be present and empty. +fn comparable_body(body: Option<&str>) -> Option { + let text = body?.replace("\r\n", "\n"); + let text = text.trim().to_string(); + (!text.is_empty()).then_some(text) +} + +/// The pull body a patch's own commit message produces — [`subject_and_body`]'s +/// second half, read out of the patch text instead of out of git. +/// +/// A format-patch is a mail message: headers, a blank line, the commit +/// message body, then the `---` scissors and the diff. So `%b` is what sits +/// between the two, and the `Change-Id:` trailer comes off it here for the +/// same reason it comes off there. `None` when there is no body, and also +/// when the text is not a patch at all — the caller reads both as "not +/// generated from this". +fn body_of_patch(patch: &str) -> Option { + let text = patch.replace("\r\n", "\n"); + let message = text.split_once("\n---\n").map_or(text.as_str(), |(m, _)| m); + let body = strip_change_id_trailers(message.split_once("\n\n")?.1); + (!body.is_empty()).then_some(body) } /// What the reconcile decided for one position of the new chain, bottom @@ -1976,10 +2038,12 @@ async fn old_member( let value = &item["value"]; let title = value["title"].as_str().unwrap_or("(untitled)").to_string(); let rounds = value["rounds"].as_array().map(|r| r.len()).unwrap_or(0); + let body = value["body"].as_str().map(str::to_string); let latest_patch = latest_round_patch(pds, did, value, &format!("{title} ({rkey})")).await?; Ok(OldMember { change_ids: change_id_headers(&latest_patch), dependent_on: value["dependentOn"].as_str().map(str::to_string), + body, latest_patch, uri, rkey, @@ -2235,6 +2299,13 @@ async fn resubmit_inner( ), }; println!(" {}/{total} {line}", i + 1); + // An update is the only fate that would rewrite a description, + // so it is the only one that can decline to. + if let Slot::Update { member, .. } = slot + && !old[*member].body_is_its_commits() + { + println!(" body kept (edited since its last round)"); + } // The bodies that will be (re)written are the ones whose images // matter to the plan; a kept member's body is not touched. if let Slot::Update { index, .. } | Slot::Add { index } = slot { @@ -2388,10 +2459,16 @@ async fn resubmit_inner( extra_data: None, }); pull.dependent_on = parent.clone().map(|s| AtUri::new(s.into())).transpose()?; - // Titles follow the commit, as Tangled's resubmit has them; - // the body was already rewritten at planning time. + // Titles follow the commit, as Tangled's resubmit has them. + // The body does too, but only while it is still the one the + // last round generated: see `body_is_its_commits`, which is + // what keeps a hand-written description from being reverted + // to `%b` by every round. It was rewritten at planning time, + // so what goes on the wire here is final either way. pull.title = p.subject.clone().into(); - pull.body = p.body.clone().map(Into::into); + if m.body_is_its_commits() { + pull.body = p.body.clone().map(Into::into); + } if let Some(new_blobs) = upload_member_images(&agent, p).await? { let existing: Option> = pull .blobs @@ -3388,6 +3465,7 @@ mod tests { .collect(), latest_patch: patch.to_string(), dependent_on: dep.map(|d| format!("at://did:plc:me/sh.tangled.repo.pull/{d}")), + body: None, rounds: 1, } } @@ -4124,4 +4202,71 @@ mod tests { let states = ["open", "merged", "closed"]; assert_eq!(select_merge_range(&states, 2), Ok(vec![0])); } + + /// A patch fixture in the shape `format_patch_one` emits, with the + /// `Change-Id` header `with_change_id_header` injects. + fn patch_with_message(message: &str) -> String { + format!( + "From cff78b1 Mon Sep 17 00:00:00 2001\n\ + From: A U Thor \n\ + Date: Wed, 19 Aug 2026 14:25:41 -0400\n\ + Subject: [PATCH] feat: a thing\n\ + Change-Id: Ithing\n\ + \n\ + {message}---\n \ + f | 1 +\n" + ) + } + + /// The body a patch generates is `%b` with the trailer off — the same + /// text `subject_and_body` hands `stack create`, which is the whole + /// point: the two have to agree or every stored body reads as edited. + #[test] + fn a_patch_yields_the_body_its_commit_message_would() { + assert_eq!( + super::body_of_patch(&patch_with_message( + "Why this change.\n\nAnd a second paragraph.\n\nChange-Id: Ithing\n" + )), + Some("Why this change.\n\nAnd a second paragraph.".to_string()) + ); + // A commit whose message is a subject and a trailer has no body, + // exactly as `strip_change_id_trailers` decides for the commit. + assert_eq!( + super::body_of_patch(&patch_with_message("Change-Id: Ithing\n")), + None + ); + // No message at all: the header block runs straight into the + // scissors. + assert_eq!(super::body_of_patch(&patch_with_message("")), None); + // Not a patch: no verdict, which the caller reads as "not + // generated from this" and so keeps what is stored. + assert_eq!(super::body_of_patch("nothing mail-shaped here"), None); + } + + /// The comparison a round turns on: a description nobody has written + /// over is regenerated from the commit, and one somebody has is not. + #[test] + fn only_an_untouched_body_still_belongs_to_its_commit() { + let patch = patch_with_message("Why this change.\n\nChange-Id: Ithing\n"); + let mut m = member("aaa", "Ithing", &patch, "open", None); + + m.body = Some("Why this change.".to_string()); + assert!(m.body_is_its_commits(), "the body the last round generated"); + + m.body = Some("Why this change.\n\n![proof](blob+at://did:plc:me/bafk)".to_string()); + assert!(!m.body_is_its_commits(), "a body with a screenshot added"); + + m.body = None; + assert!(!m.body_is_its_commits(), "a body someone cleared"); + + // A commit with no message body and a record with no body agree, + // so an amended message still reaches a pull that never had one. + let bare = patch_with_message("Change-Id: Ithing\n"); + let mut m = member("bbb", "Ithing", &bare, "open", None); + assert!(m.body_is_its_commits(), "neither side has a body"); + // `pr edit --body ''` leaves the field present and empty; that is + // the same nothing, not an edit to preserve. + m.body = Some(" ".to_string()); + assert!(m.body_is_its_commits(), "an empty body is no body"); + } } diff --git a/tests/stack_flows.rs b/tests/stack_flows.rs index 76ffe8e..e5f7662 100644 --- a/tests/stack_flows.rs +++ b/tests/stack_flows.rs @@ -880,6 +880,95 @@ fn amending_one_commit_appends_a_round_to_the_record_carrying_its_change_id() { world.with(|w| assert_eq!(w.calls_to("com.atproto.repo.applyWrites").len(), 1)); } +/// The description a stacked pull holds, if it holds one. +fn body(world: &Scenario, rkey: &str) -> Option { + world + .pulls(ALICE) + .into_iter() + .find(|(k, _)| k == rkey) + .and_then(|(_, value)| value["body"].as_str().map(str::to_string)) +} + +/// A description written by hand is not reverted by the next round. +/// +/// The bug this holds shut: a stacked pull's body is generated from its +/// commit message, and every resubmit regenerated it — so a description +/// edited afterwards (screenshots, context, everything a reviewer actually +/// reads) was silently replaced by `%b` the next time any commit changed. +/// Two stacks, thirteen pulls, rewritten by hand. +#[test] +fn an_edited_body_survives_the_rounds_that_follow_it() { + let world = published_stack("resubmit-edited-body"); + let before = keys(&world); + let written = "Why this exists, with a screenshot."; + world + .run(&["pr", "edit", &before[1], "--body", written]) + .success(); + + world.checkout.amend_below(1, "two.txt", "two, revised\n"); + world.run(&["stack", "resubmit"]).success(); + + assert_eq!( + world.rounds(ALICE, &before[1]), + 2, + "the round itself must still land" + ); + assert_eq!( + body(&world, &before[1]).as_deref(), + Some(written), + "the edited body was regenerated from the commit message" + ); +} + +/// And an amended commit message does not overrule it either. +/// +/// The comparison is against the body the *stored* round generated, not +/// against the incoming commit, so once somebody has written over a +/// description the commit message stops driving it. The alternative — let +/// a message amend win — is a silent overwrite of the same text again, in +/// the one case where it is least expected. +#[test] +fn an_amended_message_does_not_overrule_an_edited_body() { + let world = published_stack("resubmit-edited-body-amend"); + let before = keys(&world); + let written = "Hand-written, and it stays."; + world + .run(&["pr", "edit", &before[2], "--body", written]) + .success(); + + world.checkout.amend_message(&format!( + "feat: top\n\nA generated description.\n\nChange-Id: {TOP}\n" + )); + world.run(&["stack", "resubmit"]).success(); + + assert_eq!(world.rounds(ALICE, &before[2]), 2, "the round still landed"); + assert_eq!(body(&world, &before[2]).as_deref(), Some(written)); +} + +/// A body nobody has touched still follows the commit message it came from. +/// +/// The other half of the same rule, and the reason it is a comparison +/// rather than a blanket "never rewrite a body": the commit message is +/// still the source of a stacked pull's description until somebody says +/// otherwise. +#[test] +fn an_untouched_body_follows_an_amended_commit_message() { + let world = published_stack("resubmit-body-follows"); + let before = keys(&world); + assert_eq!(body(&world, &before[2]), None, "these commits have no body"); + + world.checkout.amend_message(&format!( + "feat: top\n\nA fuller explanation.\n\nChange-Id: {TOP}\n" + )); + world.run(&["stack", "resubmit"]).success(); + + assert_eq!( + body(&world, &before[2]).as_deref(), + Some("A fuller explanation."), + "an untouched body must still track its commit" + ); +} + /// Reordering the branch re-points the chain onto the new order, reusing /// every record. /// diff --git a/tests/support/git.rs b/tests/support/git.rs index 188f961..281ba3c 100644 --- a/tests/support/git.rs +++ b/tests/support/git.rs @@ -161,6 +161,13 @@ impl Checkout { self.git(&["commit", "-q", "--amend", "--no-edit"]); } + /// Rewrite `HEAD`'s whole commit message, as `git commit --amend` with + /// an editor would. The message a stacked pull's description comes + /// from, so this is how a test changes one without touching a file. + pub fn amend_message(&self, message: &str) { + self.git(&["commit", "-q", "--amend", "-m", message]); + } + /// Swap the top two commits, keeping both change-ids. pub fn swap_top_two(&self) { // A rebase would need an editor; cherry-picking onto HEAD~2 in the