From 6bfbd2bf0077665be1f5f3fef2d7f6087a696769 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Tue, 25 Aug 2026 21:56:41 -0400 Subject: [PATCH] test(pr,stack): drive the lease, and ask the URL the push uses A `pushInsteadOf` rewrite makes the fetch URL a different machine, so the head a push is about to overwrite has to be read from `remote get-url --push`; the null OID is no expectation at all and now yields none. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: Iefdbb6fa0da9d0af25bdb11bf658255c55f361be --- TODO.md | 14 ++++++++ src/clients/git/patch.rs | 15 +++++++- src/clients/git/run.rs | 18 ++++++++++ src/cmd/mod.rs | 45 +++++++++++++++++------- tests/pr_flows.rs | 76 ++++++++++++++++++++++++++++++++++++++++ tests/stack_flows.rs | 51 +++++++++++++++++++++++++++ tests/support/git.rs | 19 ++++++++++ 7 files changed, 224 insertions(+), 14 deletions(-) diff --git a/TODO.md b/TODO.md index 0aadbe9..f942373 100644 --- a/TODO.md +++ b/TODO.md @@ -747,6 +747,20 @@ here sends commits the round never read — which is also the answer for a source branch that no longer exists locally: fetch it from the knot and check it out +- [x] a round republishes its branch with a lease, which is what settled the + force question `stack resubmit` was waiting on. Both resubmits push + `--force-with-lease=:`, and the sha is read off the last + round's own patch — the `From ` line git and the knot both write + (`knotserver/git/diff.go` runs plain `format-patch`, no + `--zero-commit`) — never off the remote-tracking ref, whose argument- + less lease a `git fetch` quietly disarms by advancing it onto the + commits worth protecting. So a rewritten branch lands without a manual + force-push, and a branch somebody else moved refuses with both shas + named, before anything is sent. `stack resubmit` pushes at all now, + including on the no-op path, since records that already match an + unpushed branch are the state it has to repair. With no lease to be + had — a create, or a patch that names no head — the push is unleased + and fast-forward-only, which is what it always was - [ ] the branch a `--patch-only` pull came off is recorded nowhere, so `pr view`, `browse --pr`, `pr diff` and `pr checkout` cannot find it from the checkout it was opened in. `pr create --patch-only` says so diff --git a/src/clients/git/patch.rs b/src/clients/git/patch.rs index 3d904c7..e3cdd17 100644 --- a/src/clients/git/patch.rs +++ b/src/clients/git/patch.rs @@ -175,7 +175,12 @@ pub fn head_sha(patch: &str) -> Option { let line = patch[last..].lines().next()?; let sha = line.strip_prefix("From ")?.split(' ').next()?; let hex = sha.len() == 40 && sha.chars().all(|c| c.is_ascii_hexdigit()); - hex.then(|| sha.to_string()) + // The null OID is git's word for "no such object", and it is what tools + // that anonymize a mailbox (`format-patch --zero-commit`) leave behind. + // As a lease it would mean "only if this branch does not exist", which + // is the opposite of what a round is asking for. + let null = sha.chars().all(|c| c == '0'); + (hex && !null).then(|| sha.to_string()) } /// The commit's change-id, if it carries one. `None` is "not set up for @@ -512,6 +517,14 @@ mod tests { None, "a short sha is not a sha: leasing against it would be a force" ); + assert_eq!( + head_sha(&format!( + "From {} Mon Sep 17 00:00:00 2001\nSubject: x\n", + "0".repeat(40) + )), + None, + "the null OID leases against the branch not existing" + ); } /// A stack is cut out of this list bottom-up, so it has to come back diff --git a/src/clients/git/run.rs b/src/clients/git/run.rs index 3ef3072..9f09319 100644 --- a/src/clients/git/run.rs +++ b/src/clients/git/run.rs @@ -521,6 +521,24 @@ pub fn remote_url(name: &str) -> Result { } } +/// Where a push to `remote` actually lands. +/// +/// Not the same string as [`remote_url`] whenever a checkout carries a +/// `pushInsteadOf` rewrite — a mirror, a read-only fetch URL, or the +/// scenario tests, which fetch from a mock knot over HTTP and push into a +/// bare repo beside it. Anything that wants to look at what a push is about +/// to overwrite has to look *there*, or it is asking a different server +/// about a different ref. +/// +/// `None` when git cannot answer, which callers treat as "ask the remote by +/// name and let the push find out". +pub fn push_url(remote: &str) -> Option { + git(&["remote", "get-url", "--push", "--", remote]) + .ok() + .map(|url| url.trim().to_string()) + .filter(|url| !url.is_empty()) +} + /// The remote's default branch, if `refs/remotes//HEAD` is set /// (it usually is after a clone, but not after `git remote add`). pub fn remote_default_branch(remote: &str) -> Option { diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 6cee8e3..5b259cb 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -231,6 +231,11 @@ pub(crate) async fn print_thread(thread: &Thread) { /// fast-forward-only: exactly what this did before leases existed, failing /// on a diverged branch rather than deciding for the person. /// +/// The comparison is a courtesy and the lease is the guarantee, which is why +/// a remote that cannot be reached for the *question* still gets the lease +/// on the push: the server checks it there, atomically, and a rejection is +/// the same refusal arriving one round trip later. +/// /// 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. @@ -241,16 +246,25 @@ pub(in crate::cmd) fn publish_branch( expected: Option<&str>, advice: &str, ) -> Result { - // `None` is "could not ask" — no network, no such remote — which is not - // an answer about the branch and must not be read as one. The push - // itself is then the thing that finds out, unleased. - let observed = crate::clients::git::run::remote_head(remote, branch).flatten(); + // 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}, {remote} {}, record expects {}", - observed.as_deref().unwrap_or("(absent or unasked)"), + "publish {branch}: local {head}, {target} {}, record expects {}", + match &observed { + Some(Some(sha)) => sha.as_str(), + Some(None) => "(no such branch)", + None => "(could not ask)", + }, expected.unwrap_or("(nothing)"), )); - if observed.as_deref() == Some(head) { + if observed.as_ref().and_then(Option::as_deref) == Some(head) { crate::term::say::step!( Git, "{remote} already has {branch} at {}; nothing to push", @@ -258,7 +272,7 @@ pub(in crate::cmd) fn publish_branch( ); return Ok(false); } - if let (Some(observed), Some(expected)) = (observed.as_deref(), expected) + if let (Some(Some(observed)), Some(expected)) = (&observed, expected) && observed != expected { bail!( @@ -273,16 +287,21 @@ pub(in crate::cmd) fn publish_branch( ); } crate::term::say::step!(Git, "pushing {branch} to {remote}..."); - // The lease is only meaningful against a branch that is there: leasing a - // sha at a ref the remote does not have refuses a push that is creating - // it, which is the one case where there is nothing to protect. - let lease = expected.filter(|_| observed.is_some()); + // A lease is only meaningful against a ref that is there: leasing a sha + // at a branch the remote is *known* not to have refuses the push that + // would create it. "Could not ask" is not that knowledge, and keeps the + // lease — an expectation that cannot be checked locally is exactly the + // one worth sending to the server. + let lease = expected.filter(|_| observed != Some(None)); crate::clients::git::run::push(remote, branch, lease).map_err(|e| { anyhow::anyhow!( "{e}\n\ {advice}\n\ + A `stale info` rejection above is the lease: {remote} has {branch} somewhere \n\ + 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." + with a knot.", + expected.map(short).unwrap_or("(nothing recorded)"), ) })?; Ok(true) diff --git a/tests/pr_flows.rs b/tests/pr_flows.rs index a528863..3827e71 100644 --- a/tests/pr_flows.rs +++ b/tests/pr_flows.rs @@ -315,6 +315,82 @@ fn a_branch_based_resubmit_republishes_the_branch_and_re_compares() { ); } +/// The mock knot's compare answer for a real commit. +/// +/// [`KNOT_PATCH`] carries the null OID, which is nobody's commit; a knot +/// formats with `git format-patch` and its mailboxes name the commits they +/// are made of (`knotserver/git/diff.go`). The lease a round pushes with is +/// read out of exactly that line, so the tests that drive it need a knot +/// that answers like one. +fn knot_patch_for(sha: &str) -> String { + format!( + "From {sha} Mon Sep 17 00:00:00 2001\nFrom: the knot \n\ + Subject: [PATCH] as the knot formatted it\n\n---\n" + ) +} + +/// The case a round is actually made of: the branch was rewritten, so the +/// push is not a fast-forward. It lands anyway, because the push carries a +/// lease on the head the last round recorded — and before that lease +/// existed, this exact flow made every rebased round a manual force-push. +#[test] +fn a_rewritten_branch_resubmits_without_a_manual_force_push() { + let world = Scenario::new("pr-resubmit-rewritten"); + feature_branch(&world); + world.with(|w| w.compare = Ok((2, knot_patch_for(&world.checkout.head())))); + let rkey = open_pull(&world); + let published = world.checkout.pushed_head("feature"); + assert_eq!(published, Some(world.checkout.head())); + + // An amend: same commit, new sha, and the branch now diverged from what + // the knot holds. + world.checkout.amend_file("two.txt", "two, revised\n"); + world.with(|w| w.compare = Ok((2, SECOND_KNOT_PATCH.to_string()))); + let report = world + .run(&["pr", "resubmit", &rkey, "--json"]) + .success() + .json(); + + assert_eq!(report["pushed"], true); + assert_eq!( + world.checkout.pushed_head("feature"), + Some(world.checkout.head()), + "the rewritten branch never reached the knot" + ); + assert_ne!(world.checkout.pushed_head("feature"), published); +} + +/// The other half of the lease. A branch somebody else moved is not this +/// round's to overwrite: the refusal names both shas, and nothing is sent — +/// no push, and no round on the record either. +#[test] +fn a_round_refuses_to_publish_over_a_branch_something_else_moved() { + let world = Scenario::new("pr-resubmit-moved-branch"); + feature_branch(&world); + world.with(|w| w.compare = Ok((2, knot_patch_for(&world.checkout.head())))); + let rkey = open_pull(&world); + + // Their commit, on the branch, on the far side only. + world.checkout.branch("theirs"); + let theirs = world + .checkout + .commit("theirs.txt", "theirs\n", "feat: not mine", None); + world.checkout.git(&["checkout", "-q", "feature"]); + world.checkout.publish_elsewhere("feature", &theirs); + + world.checkout.amend_file("two.txt", "two, revised\n"); + world + .run(&["pr", "resubmit", &rkey]) + .refused("something else moved the branch"); + + assert_eq!(world.rounds(ALICE, &rkey), 1, "a round landed anyway"); + assert_eq!( + world.checkout.pushed_head("feature"), + Some(theirs), + "their commit was overwritten" + ); +} + /// A pull opened `--patch-only` stays patch-only. The round is formatted /// here, nothing is pushed, and — the half that matters — the record does /// not grow a `source`: a source is the claim that the branch is on the diff --git a/tests/stack_flows.rs b/tests/stack_flows.rs index e5f7662..9914992 100644 --- a/tests/stack_flows.rs +++ b/tests/stack_flows.rs @@ -412,6 +412,57 @@ fn a_dry_run_creates_nothing_and_touches_no_blob() { assert_eq!(world.checkout.pushed_head("feature"), None); } +/// A reconcile republishes the branch, which is the half `stack resubmit` +/// did not do at all: every member records `source: {branch}`, and rounds +/// written against a branch left behind describe commits the knot does not +/// have. The rewrite a reconcile follows means the push has to be leased, +/// not fast-forward. +#[test] +fn a_reconcile_republishes_the_rewritten_branch() { + let world = published_stack("resubmit-publishes"); + let published = world.checkout.pushed_head("feature"); + assert_eq!(published, Some(world.checkout.head())); + + world.checkout.amend_below(1, "two.txt", "two, revised\n"); + let report = world.run(&["stack", "resubmit", "--json"]).success().json(); + + assert_eq!(report["pushed"], true); + assert_eq!( + world.checkout.pushed_head("feature"), + Some(world.checkout.head()), + "the records describe commits the knot never received" + ); + 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. +#[test] +fn a_reconcile_refuses_over_a_branch_something_else_moved() { + let world = published_stack("resubmit-moved-branch"); + let before = keys(&world); + + world.checkout.branch("theirs"); + let theirs = world + .checkout + .commit("theirs.txt", "theirs\n", "feat: not mine", None); + world.checkout.git(&["checkout", "-q", "feature"]); + world.checkout.publish_elsewhere("feature", &theirs); + + world.checkout.amend_below(1, "two.txt", "two, revised\n"); + world + .run(&["stack", "resubmit"]) + .refused("something else moved the branch"); + + assert_eq!(keys(&world), before, "records changed under a refusal"); + assert_eq!( + world.checkout.pushed_head("feature"), + Some(theirs), + "their commit was overwritten" + ); +} + /// The branch is on the knot before any record says it is. /// /// `source: {branch}` is the field the appview tells a branch-based pull diff --git a/tests/support/git.rs b/tests/support/git.rs index 281ba3c..1918cb1 100644 --- a/tests/support/git.rs +++ b/tests/support/git.rs @@ -104,6 +104,25 @@ impl Checkout { (!sha.is_empty()).then_some(sha) } + /// Move `branch` in the bare repo behind somebody else's back. + /// + /// What a collaborator's push, or a resubmit from tangled.org, looks + /// like from this checkout: the branch on the other side is somewhere + /// this checkout never put it, and the lease a round pushes with is the + /// only thing standing between that commit and being overwritten. + pub fn publish_elsewhere(&self, branch: &str, sha: &str) { + // Straight at the bare repo's path, and forced: this is standing in + // for a push atgc did not make, so it neither goes through the + // remote's rewrite rules nor cares what the branch held before. + self.git(&[ + "push", + "-q", + "--force", + &self.bare.to_string_lossy(), + &format!("{sha}:refs/heads/{branch}"), + ]); + } + /// Put an uncommitted file in the working tree. /// /// For the image tests: `cmd/images.rs` resolves a body's relative paths -- 2.51.2