diff --git a/docs/testing.md b/docs/testing.md index 9649cfe..eeff733 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -26,6 +26,23 @@ code that did not use one. A value derived from a partial read should carry its reach; where a function returns a bare `Vec` or a bare enum from a listing, that is the thing to check. +## The rule about the rules + +Every entry above was one rule with several copies, or one rule in the wrong +place. So the fix for an instance is not finished when the instance stops +happening — it is finished when there is one copy of the rule, somewhere that +owns it. + +Two of these were written twice on the same day. "Absence and ignorance" was +fixed in `pr close` and then found again in `stack view`'s "not stacked". +"Decided and unknown" was fixed in `stack merge` and then implemented a +second time, by hand, in `repo delete`. Both second copies were written by +somebody who had just written the first, which is the strongest evidence +available that noticing a pattern is not the same as removing it. +`knot::decided` is the one definition now, and it takes a tag and a status +rather than an error, because its two callers hold different things and the +rule is about neither. + ## Practices these came out of - **Mutate before believing a test.** Three tests in this suite passed for diff --git a/src/clients/tangled/knot.rs b/src/clients/tangled/knot.rs index ab21de9..9b6b8d4 100644 --- a/src/clients/tangled/knot.rs +++ b/src/clients/tangled/knot.rs @@ -128,6 +128,23 @@ pub struct Refused { pub detail: String, } +/// **Whether a knot failure is the knot deciding, or the knot failing.** +/// +/// The distinction two commands need and neither should own. A 4xx carrying +/// an error name is a refusal the knot composed on purpose, which means it +/// got far enough to compose one and nothing it guards has changed. A 5xx, a +/// status with no name, or a transport error that never became a [`Refused`] +/// leaves the outcome open: `merge` may have moved the branch, `deleteRepo` +/// may have removed the git data, and the answer in both cases is to look +/// before acting again. +/// +/// Takes the parts rather than the error, because the two callers hold +/// different things — one an `anyhow::Error` it can downcast, the other a +/// status and body it already unpacked — and the rule is about neither. +pub fn decided(tag: &str, status: reqwest::StatusCode) -> bool { + !tag.is_empty() && status.is_client_error() +} + impl Refused { /// The status atgc should exit with, from the knot's answer. /// diff --git a/src/cmd/repo/mod.rs b/src/cmd/repo/mod.rs index 6686118..b6e3fec 100644 --- a/src/cmd/repo/mod.rs +++ b/src/cmd/repo/mod.rs @@ -1681,15 +1681,30 @@ mod tests { } /// The one message no retry can reach past: `repo delete` has already - /// removed the record when the knot refuses, so a re-run finds nothing to + /// removed the record when the knot fails, so a re-run finds nothing to /// read and this text is the only description of what is left. + /// + /// Both halves, because they differ in the one claim that matters — + /// whether the git data's fate is known — and a note that made the + /// settled claim for an unanswered call would send somebody to an + /// operator to remove what may already be gone. #[test] fn the_orphan_note_names_the_knot_and_says_a_re_run_cannot_help() { - let note = orphan_note("knot1.tangled.sh"); - assert!(note.contains("knot1.tangled.sh"), "{note}"); + for settled in [true, false] { + let note = orphan_note("knot1.tangled.sh", settled); + assert!(note.contains("knot1.tangled.sh"), "{note}"); + assert!( + note.contains("re-running"), + "the note has to say a retry is not the answer: {note}" + ); + } + assert!( + orphan_note("knot1.tangled.sh", true).contains("still on"), + "a refusal settles what became of the git data and should say so" + ); assert!( - note.contains("re-running"), - "the note has to say a retry is not the answer: {note}" + orphan_note("knot1.tangled.sh", false).contains("is not known"), + "an unanswered call settles nothing and must not claim otherwise" ); } diff --git a/src/cmd/repo/write.rs b/src/cmd/repo/write.rs index f317196..aafbbb6 100644 --- a/src/cmd/repo/write.rs +++ b/src/cmd/repo/write.rs @@ -948,15 +948,16 @@ pub(super) async fn delete(args: DeleteArgs) -> Result<()> { // The call never completed, so whether the git data went with it is // not known here — unlike the refusal below, where a status came // back and the answer is no. - .with_context(|| unknown_orphan_note(&owned.record.knot))?; + .with_context(|| orphan_note(&owned.record.knot, false))?; if !status.is_success() { // **A 4xx is the knot deciding; a 5xx is the knot failing.** Only the // first settles what became of the git data, and only the first may // use the note that says so — the same line this command's merge // sibling draws, for the same reason. - let (verb, note) = match status.is_client_error() { - true => ("refused to delete", orphan_note(&owned.record.knot)), - false => ("could not delete", unknown_orphan_note(&owned.record.knot)), + let tag = body["error"].as_str().unwrap_or_default(); + let (verb, note) = match crate::clients::tangled::knot::decided(tag, status) { + true => ("refused to delete", orphan_note(&owned.record.knot, true)), + false => ("could not delete", orphan_note(&owned.record.knot, false)), }; return Err(crate::exit::fail( crate::exit::from_status(status), @@ -976,35 +977,26 @@ pub(super) async fn delete(args: DeleteArgs) -> Result<()> { Ok(()) } -/// What a knot *refusal* after the record deletion leaves behind, said -/// plainly because no retry can reach this state: the record a re-run would -/// read is already gone. +/// What a knot failure after the record deletion leaves behind. /// -/// A refusal is the knot answering, so the git data's fate is settled and -/// this can say what it is. [`unknown_orphan_note`] is the other half. -pub(super) fn orphan_note(knot: &str) -> String { - format!( - "the record is deleted, so the repo is already gone from tangled.org and re-running \ - cannot pick up where this stopped: the git data is still on {knot}, and its \ - operator can remove it" - ) -} - -/// The same, for a call that never came back. -/// -/// **A knot that refuses and a knot that does not answer leave different -/// worlds behind**, and the note above asserts one of them. A delete that -/// reached the knot removes the git data whether or not the answer arrived, -/// so telling somebody it is "still on {knot}" sends them to an operator to -/// remove something that may already be gone — and, worse, implies the -/// deletion did not happen when it may have. -pub(super) fn unknown_orphan_note(knot: &str) -> String { - format!( - "the record is deleted, so the repo is already gone from tangled.org and re-running \ - cannot pick up where this stopped. Whether the git data went with it is not known: \ - {knot} did not answer, and a delete that reached it removes the data either way. \ - Its operator can say which, and remove what is left" - ) +/// The record is gone by the time this is reached, so no retry can pick up +/// where the command stopped and this note is the whole of what a reader +/// gets. `settled` is [`crate::clients::tangled::knot::decided`]: a knot +/// that refused has answered, so the git data's fate is known and can be +/// stated. A knot that failed has not, and a delete that reached it removes +/// the data whether or not the answer came back — saying it survived would +/// send somebody to an operator to remove what may already be gone. +pub(super) fn orphan_note(knot: &str, settled: bool) -> String { + let head = "the record is deleted, so the repo is already gone from tangled.org and \ + re-running cannot pick up where this stopped"; + match settled { + true => format!("{head}: the git data is still on {knot}, and its operator can remove it"), + false => format!( + "{head}. Whether the git data went with it is not known: {knot} did not answer, \ + and a delete that reached it removes the data either way. Its operator can say \ + which, and remove what is left" + ), + } } /// Apply one optional-text flag, and say so if it changed anything. diff --git a/src/cmd/stack/write.rs b/src/cmd/stack/write.rs index f30de27..4e6877f 100644 --- a/src/cmd/stack/write.rs +++ b/src/cmd/stack/write.rs @@ -4070,15 +4070,13 @@ fn explain_refusal(error: anyhow::Error, plan: &MergePlan) -> anyhow::Error { /// Whether the knot composed this refusal itself, rather than stopping. /// -/// A tagged 4xx is one the knot got far enough to decide on. Anything else — -/// no tag, a 5xx, or a transport error that never became a -/// [`crate::clients::tangled::knot::Refused`] at all — leaves the outcome -/// open. Read before [`explain_refusal`] runs, since that replaces the error -/// with one carrying no `Refused` to find. +/// The rule is [`crate::clients::tangled::knot::decided`]; this is only the +/// unwrapping, and it has to run *before* [`explain_refusal`], which replaces +/// the error with one carrying no `Refused` to find. fn knot_decided(error: &anyhow::Error) -> bool { error .downcast_ref::() - .is_some_and(|r| !r.tag.is_empty() && r.status.is_client_error()) + .is_some_and(|r| crate::clients::tangled::knot::decided(&r.tag, r.status)) } /// Say so when a failed merge call may have moved the branch anyway.