diff --git a/plan/stacks.md b/plan/stacks.md index 389a7ff..0af9148 100644 --- a/plan/stacks.md +++ b/plan/stacks.md @@ -57,6 +57,31 @@ under Done, and they are the reason the refusals are as loud as they are. ## Done +- [x] `stack link` refuses members whose patches share a commit, which is + the shape it used to *require*. A merge takes a member plus everything + unmerged below it and applies them as one series, so two members + carrying the same commit apply it twice — and a pull opened by + `pr create` records a patch of `..`, so two of + those overlap whenever one branch contains the other. The only checks + here were about *order*, and the ancestry they demand is exactly what + produces the overlap; the refusal for the reverse order even advised + "rebase {b} onto {a}", which steered people into it. + Found by walking into it: a three-pull chain linked this way was opened + against this repo, and the knot refused the merge naming six files that + had nothing to do with the cause. The check is on the patches rather + than the branches, because the patch is what the merge applies — one + `From` boundary in common is the whole test. `gitpatch::commit_shas` + reads them, skipping anything that is not a well-formed sha, which errs + toward allowing a link rather than refusing over bytes nobody can read. + The order check stays; it answers a different question, and its advice + no longer points at the broken arrangement. + What this leaves undone is a way *out*: pulls already linked into an + overlapping chain cannot be serviced by either resubmit verb — + `pr resubmit` refuses a stacked member, and `stack resubmit` cannot + adopt pulls whose patches carry no change-id header, which is every + pull `pr create` ever wrote. Clearing a `dependentOn` currently needs + `atgc api` + - [x] Foundations: `dependentOn` named on the pull record (was preserved opaquely in `Pull::extra`), per-commit git helpers (`commits_since`, `format_patch_one`, `change_id` — jj commit header first, else diff --git a/src/clients/git/patch.rs b/src/clients/git/patch.rs index e3cdd17..3c7fd2e 100644 --- a/src/clients/git/patch.rs +++ b/src/clients/git/patch.rs @@ -170,6 +170,38 @@ pub fn message_offsets(patch: &str) -> Vec { /// name. A branch whose tip is a *merge* also lands here holding the wrong /// answer — format-patch omits merges — which is why the stack commands /// refuse a range containing one before they ever ask. +/// Every commit a mailbox carries, oldest first, by the sha on its `From` +/// boundary. +/// +/// The set a patch would *apply*, which is the question two patches have to +/// be compared on before they can be members of one stack: a merge takes a +/// member plus everything unmerged below it as one series, so two patches +/// holding the same commit apply it twice and the second application fails +/// on a tree that already has it. +/// +/// Only well-formed 40-character hex, for [`head_sha`]'s reason — an +/// anonymized or malformed boundary is not evidence of which commit this is, +/// and a comparison drawn from one would be a guess. A message whose sha +/// cannot be read is skipped rather than matched, which errs toward +/// *allowing* a link: refusing on something unreadable would block a stack +/// over bytes nobody can interpret. +pub fn commit_shas(patch: &str) -> Vec { + message_offsets(patch) + .into_iter() + .filter_map(|at| { + let sha = patch[at..] + .lines() + .next()? + .strip_prefix("From ")? + .split(' ') + .next()?; + let hex = sha.len() == 40 && sha.chars().all(|c| c.is_ascii_hexdigit()); + let null = sha.chars().all(|c| c == '0'); + (hex && !null).then(|| sha.to_string()) + }) + .collect() +} + pub fn head_sha(patch: &str) -> Option { let last = *message_offsets(patch).last()?; let line = patch[last..].lines().next()?; diff --git a/src/cmd/stack/write.rs b/src/cmd/stack/write.rs index b972797..5f0fbe7 100644 --- a/src/cmd/stack/write.rs +++ b/src/cmd/stack/write.rs @@ -3029,6 +3029,50 @@ pub(crate) async fn link(args: LinkArgs) -> Result<()> { } } + // **Every member's patch has to hold its own commits and nobody else's.** + // + // A merge takes a member plus everything unmerged below it and applies + // them as one series, so two members carrying the same commit apply it + // twice: the knot answers "patch doesn't apply" and "file already + // exists", which names a file rather than the cause and sends people + // looking for a conflict that is not there. `stack create` gets this for + // free by cutting one branch into ranges. `link` takes pulls that + // already exist, and a pull opened by `pr create` records a patch of + // `..` — so two of those overlap whenever one + // branch contains the other, *or* whenever they are the same branch. + // + // This is checked on the patches rather than on the branches because the + // patch is what the merge applies. The branch check below is about + // *order* and answers a different question; it used to be the only one, + // and it accepts — indeed requires — the ancestry that guarantees the + // overlap. Its own refusal advises "rebase {b} onto {a}", which is to + // say it steered people into the shape that cannot merge. + let pds = crate::clients::atproto::did::pds_or_fail(&me).await?; + let mut patches: Vec> = Vec::new(); + for m in &members { + let patch = latest_round_patch(&pds, &me, &m.value, &m.rkey).await?; + patches.push(gitpatch::commit_shas(&patch)); + } + for (i, (a, b)) in members.iter().zip(&patches).enumerate() { + for (other, others) in members.iter().zip(&patches).skip(i + 1) { + if let Some(shared) = b.iter().find(|sha| others.contains(sha)) { + bail!( + "{} and {} both carry commit {}, so merging them as a series \ + would apply it twice\n\ + a stack's members hold separate commits: `atgc stack create` cuts \ + one branch into members that do, and `link` can only chain pulls \ + that already have them\n\ + to land these as they are, merge them in order — the bottom \ + first, then rebase and `atgc pr resubmit` the one above, whose \ + patch then holds only its own work", + a.rkey, + other.rkey, + &shared[..12.min(shared.len())], + ); + } + } + } + // Order, checked against git where git can answer it. A stack merges // bottom-up, so each member's commits have to sit on top of the one // below: a chain written in the wrong order lands a patch onto a tree @@ -3047,7 +3091,7 @@ pub(crate) async fn link(args: LinkArgs) -> Result<()> { bail!( "{} ({a}) is not an ancestor of {} ({b}), so this order does \ not stack\n\ - rebase {b} onto {a}, or name them the other way round", + name them the other way round", lower.rkey, upper.rkey, ); diff --git a/tests/stack_flows.rs b/tests/stack_flows.rs index bd24c15..23ee3fa 100644 --- a/tests/stack_flows.rs +++ b/tests/stack_flows.rs @@ -2395,6 +2395,98 @@ fn link_chains_open_pulls_in_one_write() { ); } +/// One mailbox message per sha, in the shape `git format-patch` writes. +fn mailbox(shas: &[&str]) -> String { + shas.iter() + .map(|sha| { + format!( + "From {sha} Mon Sep 17 00:00:00 2001\n\ + From: alice \n\ + Subject: [PATCH] a commit\n\n---\n" + ) + }) + .collect() +} + +/// Two pulls whose patches share a commit cannot be a stack, and `link` +/// refuses before it writes. +/// +/// **The shape this command used to accept, and the one it required.** A +/// merge takes a member plus everything unmerged below it and applies them +/// as one series, so two members carrying the same commit apply it twice. +/// The knot answers that with "patch doesn't apply" and "file already +/// exists" — which names a file rather than the cause, and sends the reader +/// looking for a conflict that is not there. +/// +/// It is not a hypothetical: a chain linked this way was opened against this +/// repo and the knot refused to merge it, naming six files that were nothing +/// to do with the problem. The only checks here were about *order*, and the +/// ancestry they require is precisely what makes one branch's patch contain +/// the other's commits. +#[test] +fn link_refuses_members_whose_patches_share_a_commit() { + let world = Scenario::new("link-overlap"); + let lower_sha = "1111111111111111111111111111111111111111"; + let upper_sha = "2222222222222222222222222222222222222222"; + + world.checkout.branch("lower"); + world + .checkout + .commit("one.txt", "one\n", "feat: bottom", None); + world.with(|w| w.compare = Ok((1, mailbox(&[lower_sha])))); + let bottom = open_pull(&world, "the lower half"); + + world.checkout.branch("upper"); + world.checkout.commit("two.txt", "two\n", "feat: top", None); + // What `pr create` records for a branch opened on top of another: its + // own commit *and* the one below it, because the patch spans the target + // branch to this branch's head. + world.with(|w| w.compare = Ok((2, mailbox(&[lower_sha, upper_sha])))); + let top = open_pull(&world, "the upper half"); + world.clear_journal(); + + let run = world + .run(&["stack", "link", &bottom, &top]) + .refused("apply it twice"); + assert!( + run.stderr.contains(&lower_sha[..12]), + "the refusal has to name the commit they share:\n{}", + run.stderr + ); + world.with(|w| { + assert!( + w.calls_to("com.atproto.repo.applyWrites").is_empty(), + "a refused link must write nothing" + ) + }); +} + +/// Patches that share nothing still link. +/// +/// The rule above is about overlap and not about branches, so the case +/// `link` exists for — pulls whose patches are already separate ranges — +/// goes through untouched. +#[test] +fn link_accepts_members_whose_patches_are_separate() { + let world = Scenario::new("link-disjoint"); + + world.checkout.branch("lower"); + world + .checkout + .commit("one.txt", "one\n", "feat: bottom", None); + world.with(|w| w.compare = Ok((1, mailbox(&["3333333333333333333333333333333333333333"])))); + let bottom = open_pull(&world, "the lower half"); + + world.checkout.branch("upper"); + world.checkout.commit("two.txt", "two\n", "feat: top", None); + world.with(|w| w.compare = Ok((1, mailbox(&["4444444444444444444444444444444444444444"])))); + let top = open_pull(&world, "the upper half"); + world.clear_journal(); + + world.run(&["stack", "link", &bottom, &top]).success(); + assert_eq!(parent_of(&world, &top), Some(bottom)); +} + /// The order is checked against git where git can check it, and a chain that /// does not stack is refused before anything is written. #[test]