diff --git a/plan/stacks.md b/plan/stacks.md index ccd8cb9..7e72338 100644 --- a/plan/stacks.md +++ b/plan/stacks.md @@ -43,6 +43,15 @@ under Done, and they are the reason the refusals are as loud as they are. resubmit agrees is unchecked ## Done +- [x] **A refusal no longer prints as a success.** `stack resubmit` said its + whole per-member plan and *then* checked the push lease, so a branch + that had moved refused underneath lines that read exactly like a + completed reconcile — to a person skimming, and to a script grepping + stdout for `rounds:`. The lease is taken before the first line now, so + a refusal is the entire output. The message also named two causes and + not the likely one: a plain `git rebase` since the last round, whose + answer is `atgc stack sync`, which rebases and reconciles together + (`3mukgupwiij2b`) - [x] **`stack create --patch-only` writes the chain without a push.** The default create publishes the branch and records `source`, which is how the stack commands find the chain; a target the acting account cannot diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 943b544..92ace03 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -246,6 +246,17 @@ pub(crate) async fn print_thread(thread: &Thread) { /// Returns whether it pushed. `advice` is the caller's line about what to do /// when the branch cannot be published at all — the two callers have /// different ones, and only they know which. +pub(in crate::cmd) fn publish_branch( + remote: &str, + branch: &str, + head: &str, + expected: Option<&str>, + advice: &str, +) -> Result { + let lease = check_lease(remote, branch, head, expected, None)?; + publish_leased(lease, remote, branch, head, expected, advice) +} + /// The scope string of the session this process would actually act with. /// /// The one answer the scope pre-flight in @@ -263,21 +274,44 @@ pub(in crate::cmd) fn acting_scope(did: &str) -> Option { .and_then(|s| s.scope) } -pub(in crate::cmd) fn publish_branch( +/// What the remote held when the lease was taken. +/// +/// Carried from [`check_lease`] to [`publish_leased`] rather than asked for +/// twice: the two are one decision about one moment, and a second +/// `ls-remote` would answer about a different one — besides costing a round +/// trip to say what is already known. +pub(in crate::cmd) struct Lease { + /// The outer `None` is "could not ask" — no network, a URL git cannot + /// reach — which is not an answer about the branch. The inner one is the + /// branch itself. + observed: Option>, +} + +/// The refusing half of [`publish_branch`]: ask where `remote` has `branch`, +/// and stop if it is not where the last round left it. +/// +/// Split out so a caller can take the lease **before** it prints what it +/// would send. `stack resubmit` printed its whole per-member plan first and +/// refused underneath it, which reads — to a person skimming, and to a +/// script grepping for `rounds:` — exactly like a reconcile that happened. +/// Nothing between here and the push depends on the answer, so the check +/// costs the same either way and only the output ordering changes. +/// +/// `reconcile` is the caller's line naming the command that rebases and +/// republishes together, when it has one; the causes this can name are the +/// same for everybody, but the way out of them is not. +pub(in crate::cmd) fn check_lease( remote: &str, branch: &str, head: &str, expected: Option<&str>, - advice: &str, -) -> Result { + reconcile: Option<&str>, +) -> Result { // Asked of the URL the push will use, which a `pushInsteadOf` rewrite // can make a different machine entirely from the one `remote` fetches // from: looking at the fetch side would be a question about another // server's copy of the ref this is about to move. let target = crate::clients::git::run::push_url(remote).unwrap_or_else(|| remote.to_string()); - // The outer `None` is "could not ask" — no network, a URL git cannot - // reach — which is not an answer about the branch. The inner one is the - // branch itself. let observed = crate::clients::git::run::remote_head(&target, branch); crate::logging::debug::log(format!( "publish {branch}: local {head}, {target} {}, record expects {}", @@ -288,27 +322,43 @@ pub(in crate::cmd) fn publish_branch( }, expected.unwrap_or("(nothing)"), )); - if observed.as_ref().and_then(Option::as_deref) == Some(head) { - crate::term::say::step!( - Git, - "{remote} already has {branch} at {}; nothing to push", - short(head) - ); - return Ok(false); - } if let (Some(Some(observed)), Some(expected)) = (&observed, expected) && observed != expected { bail!( "{remote} has {branch} at {}, and the last round of this pull was cut from \ {}\n\ - something else moved the branch — another checkout, or a resubmit from \ - tangled.org — so nothing has been sent\n\ - `git fetch {remote} {branch}` and look at what is there; publishing over it \ - is `git push --force {remote} {branch}` once you are sure", + the branch moved since that round — a local rebase or amend, another \ + checkout, or a resubmit from tangled.org — so nothing has been sent\n\ + {}`git fetch {remote} {branch}` and look at what is there; publishing over \ + it is `git push --force {remote} {branch}` once you are sure", short(observed), short(expected), + reconcile.map(|r| format!("{r}\n")).unwrap_or_default(), + ); + } + Ok(Lease { observed }) +} + +/// The sending half of [`publish_branch`], once [`check_lease`] has allowed +/// it. Takes the [`Lease`] by value, so nothing can push without having +/// asked. +pub(in crate::cmd) fn publish_leased( + lease: Lease, + remote: &str, + branch: &str, + head: &str, + expected: Option<&str>, + advice: &str, +) -> Result { + let Lease { observed } = lease; + if observed.as_ref().and_then(Option::as_deref) == Some(head) { + crate::term::say::step!( + Git, + "{remote} already has {branch} at {}; nothing to push", + short(head) ); + return Ok(false); } crate::term::say::step!(Git, "pushing {branch} to {remote}..."); // A lease is only meaningful against a ref that is there: leasing a sha @@ -321,7 +371,7 @@ pub(in crate::cmd) fn publish_branch( anyhow::anyhow!( "{e}\n\ {advice}\n\ - A `stale info` rejection above is the lease: {remote} has {branch} somewhere \n\ + A `stale info` rejection above is the lease: {remote} has {branch} somewhere \ other than the {} this pull's last round recorded. Look before forcing.\n\ If the push should have worked, `atgc key add` is what registers an SSH key \ with a knot.", diff --git a/src/cmd/stack/mod.rs b/src/cmd/stack/mod.rs index 36672d3..f461206 100644 --- a/src/cmd/stack/mod.rs +++ b/src/cmd/stack/mod.rs @@ -533,8 +533,11 @@ pub(crate) enum Command { /// The branch is republished to `--remote` first, leased against the /// head the last round recorded: every member records `source: /// {branch}`, so a reconcile that left the branch behind would write - /// rounds describing commits the knot does not have. A branch something - /// else moved refuses instead of being overwritten. + /// rounds describing commits the knot does not have. A branch that moved + /// refuses instead of being overwritten, and the lease is checked before + /// the plan is printed, so a refusal prints no plan at all. A plain `git + /// rebase` is the ordinary way into that refusal: `atgc stack sync` is + /// the verb for it, rebasing and reconciling together. /// /// After a rebase, amend, reorder or drop, this matches each pull to /// its commits by change-id: matched pulls whose patch changed get a new diff --git a/src/cmd/stack/write.rs b/src/cmd/stack/write.rs index 9e5e1a4..ed4aad2 100644 --- a/src/cmd/stack/write.rs +++ b/src/cmd/stack/write.rs @@ -2728,6 +2728,37 @@ async fn resubmit_inner( let plan = reconcile(&old, &planned, args.prune)?; + // The lease, taken before a single line of the plan is printed. + // + // Nothing below here reads the answer — the push happens after the + // no-op and dry-run returns, where it always has — so the only thing + // this ordering changes is what a refusal looks like. Printed + // underneath the per-member lines it looked exactly like a reconcile + // that had happened: a person skimming, or a script grepping for + // `rounds:`, saw a completed run with an error tacked on. Above them, + // stdout carries nothing at all and the refusal is the whole output. + // + // What the last round published, read off its own patch: the sha to + // lease against, and never the local remote-tracking ref, which a + // fetch can advance onto the commits worth protecting. + let expected = old.last().and_then(|m| gitpatch::head_sha(&m.latest_patch)); + let lease = match args.dry_run { + // A dry run sends nothing, so there is nothing to lease — and a + // refusal here would stop the one command that exists to be run + // before the state is understood. + true => None, + false => Some(crate::cmd::check_lease( + &args.remote, + &branch, + commits.last().map(String::as_str).unwrap_or_default(), + expected.as_deref(), + Some( + "if that rebase was yours, `atgc stack sync` rebases and reconciles \ + together", + ), + )?), + }; + // Say the plan, then do it (or stop). let total = plan.chain.len(); if !args.json { @@ -2862,12 +2893,9 @@ async fn resubmit_inner( // records already match an unpushed branch is precisely the state this // has to be able to repair — and when the remote is already there, this // costs one `ls-remote` and sends nothing. - if !args.dry_run { - // What the last round published, read off its own patch: the sha to - // lease against, and never the local remote-tracking ref, which a - // fetch can advance onto the commits worth protecting. - let expected = old.last().and_then(|m| gitpatch::head_sha(&m.latest_patch)); - report.pushed = crate::cmd::publish_branch( + if let Some(lease) = lease { + report.pushed = crate::cmd::publish_leased( + lease, &args.remote, &branch, commits.last().map(String::as_str).unwrap_or_default(), diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs index c6fb70e..1e12827 100644 --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -381,7 +381,7 @@ fn a_round_refuses_to_publish_over_a_branch_something_else_moved() { world.checkout.amend_file("two.txt", "two, revised\n"); world .run(&["pr", "resubmit", &rkey]) - .refused("something else moved the branch"); + .refused("the branch moved since that round"); assert_eq!(world.rounds(ALICE, &rkey), 1, "a round landed anyway"); assert_eq!( diff --git a/tests/stack_flows.rs b/tests/stack_flows.rs index 8c964a7..619552f 100644 --- a/tests/stack_flows.rs +++ b/tests/stack_flows.rs @@ -440,9 +440,16 @@ fn a_reconcile_republishes_the_rewritten_branch() { assert_ne!(world.checkout.pushed_head("feature"), published); } -/// A branch somebody else moved stops the reconcile before any of it: no -/// push, and no records either. The stack's whole chain is rewritten in one -/// batch, so this is the one refusal that has to happen first. +/// A branch that moved under the stack stops the reconcile before any of it: +/// no push, and no records either. The stack's whole chain is rewritten in +/// one batch, so this is the one refusal that has to happen first. +/// +/// **Before anything is printed, too.** The plan used to print its whole +/// per-member listing and refuse underneath it, which reads like a reconcile +/// that happened — to a person skimming, and to a script grepping stdout for +/// `rounds:`. So stdout is asserted empty here, not merely "not a success": +/// the refusal is the entire output, and the message names the local rebase +/// that is the usual way in along with the command that undoes it. #[test] fn a_reconcile_refuses_over_a_branch_something_else_moved() { let world = published_stack("resubmit-moved-branch"); @@ -456,9 +463,26 @@ fn a_reconcile_refuses_over_a_branch_something_else_moved() { world.checkout.publish_elsewhere("feature", &theirs); world.checkout.amend_below(1, "two.txt", "two, revised\n"); - world + let run = world .run(&["stack", "resubmit"]) - .refused("something else moved the branch"); + .refused("the branch moved since that round"); + assert_eq!( + run.stdout, "", + "the plan printed above a refusal reads as a completed reconcile" + ); + assert!( + run.stderr.contains("atgc stack sync"), + "the refusal never names the command that fixes it:\n{}", + run.stderr + ); + + // The same refusal under `--json`, whose contract is that stdout is one + // document or nothing: a plan half-printed as JSON is worse than a plan + // half-printed as text, because something is parsing it. + let json = world + .run(&["stack", "resubmit", "--json"]) + .refused("the branch moved since that round"); + assert_eq!(json.stdout, "", "--json emitted a document under a refusal"); assert_eq!(keys(&world), before, "records changed under a refusal"); assert_eq!(