From 283b97118683f40d9717cf59f389a45698174d53 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 6 Sep 2026 22:57:55 -0400 Subject: [PATCH] Offer the rollback, and call it Downgrade Picking an old commit from the phone detaches HEAD, so every rollback is parked by construction -- and a parked checkout withheld the commit comparison entirely. The card answered the move with "up to date" and nothing to press, which left a rollback unfinishable from the phone. The withholding was right for one direction only. A checkout parked past what was built is somebody looking, and nagging about it is nagging about a decision. A checkout parked *behind* it is somebody rolling back, and there the point of the move is to get that commit built and installed. So `git::is_ancestor` asks which of the two commits is the later one -- one extra process, and only where they already differ -- and `Freshness::BuiltAhead` says the build is. It is stale like `Behind`, so the card leaves "Up to date" and both Update buttons come alive; `updateWord` then says Downgrade, at both scales and by one rule, the way `buildWord` does. Nothing else about the press differs, which is what was asked for. The row says "newer than the checkout" rather than "out of date", which is the wrong direction. Two things fell out of it. `UpdateButton` asks about the direction before comparing file times, since what is on disk stays older than the phone's copy right up until the press rebuilds it -- otherwise a card with a rollback waiting says "Reinstall". And a project-wide Update now does the build-and-install half alone when there is nothing to pull *from* -- a parked checkout, a branch tracking nothing, or `gitPull` off -- rather than failing on a pull it was never going to make, which is what a downgrade press used to hit. Verified end to end against a throwaway checkout driven through a scratch server: `current` on the built commit, `builtAhead` once parked back on the one before it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KuRzBWJuEGaWmMhDuX3X3p --- AGENTS.md | 51 +++++-- .../com/example/devupdater/UpdateManifest.kt | 44 +++++- .../com/example/devupdater/UpdaterScreen.kt | 71 ++++++--- server/src/build_state.rs | 141 ++++++++++++++++-- server/src/git.rs | 32 ++++ 5 files changed, 287 insertions(+), 52 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 32160cc..e276bad 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -473,18 +473,41 @@ mutable at runtime from the phone. parked further back than the window gets its own commit appended, so the picker never has nothing to show for where it is. And `freshness` takes a `parked` flag, reporting a differing commit as - **unknown** rather than `Behind` while the checkout is detached: every - way it reads as behind while parked is a build that failed, a - declaration waiting to be accepted, or a component with no build step, - and each of those already says so on the same card beside the button - for it -- so "out of date" there is either redundant or a nag about a - decision somebody made. Unknown rather than current, because nothing - measured the output to be what was wanted. `component_is_stale` is - deliberately *not* changed: Update and Rebuild still rebuild a parked - checkout, which is the whole point of parking one. The flag is passed - down from `describe` rather than read per component, since git has - already been asked for that project's status once and `/manifest` is - fetched on every open, resume and Refresh. + **unknown** rather than `Behind` while the checkout is detached *past* + what was built: every way it reads as behind while parked is a build + that failed, a declaration waiting to be accepted, or a component with + no build step, and each of those already says so on the same card + beside the button for it -- so "out of date" there is either redundant + or a nag about a decision somebody made. Unknown rather than current, + because nothing measured the output to be what was wanted. + `component_is_stale` is deliberately *not* changed: Update and Rebuild + still rebuild a parked checkout, which is the whole point of parking + one. The flag is passed down from `describe` rather than read per + component, since git has already been asked for that project's status + once and `/manifest` is fetched on every open, resume and Refresh. + +- **The other direction is reported, and its word is Downgrade.** A + checkout parked *behind* its build is somebody rolling back, and + withholding that was the whole of why a rollback could not be finished + from the phone: picking an old commit detaches HEAD, so every rollback + is parked by construction, and the card answered the move with "up to + date" and nothing to press (Bryan, 2026-09-06). `git::is_ancestor` + asks which of the two commits is the later one -- one extra process, + and only where they already differ -- and `Freshness::BuiltAhead` says + the build is. It is stale like `Behind`, so the card leaves "Up to + date" and both Update buttons come alive; `updateWord` then says + **Downgrade**, at both scales and by one rule, the way `buildWord` + does. Nothing else about the press differs, which is what Bryan asked + for. The row says "newer than the checkout" rather than "out of date", + which is the wrong direction. + Two things fell out of it. `UpdateButton` asks about the direction + *before* comparing file times, since what is on disk is older than + this phone's copy right up until the press rebuilds it -- otherwise a + card with a rollback waiting says "Reinstall". And a project-wide + Update now does the build-and-install half alone when there is nothing + to pull *from* -- a parked checkout, a branch tracking nothing, or + `gitPull` off -- rather than failing on a pull it was never going to + make, which is what a downgrade press used to hit. - **Unrelated histories are the one pull failure the phone may override.** A checkout sharing no commit with its upstream has no fast-forward and @@ -622,8 +645,8 @@ mutable at runtime from the phone. only difference lived in a config file on the other machine. That silence cost two rounds of "why isn't it flagging anything" that no amount of looking at the phone could answer. It now names the state: - `behind`, `neverBuilt` and `otherMode` are measured and have something - to do about them; `uncommitted`, `notBuiltHere`, `noCheckout` and + `behind`, `builtAhead`, `neverBuilt` and `otherMode` are measured and + have something to do about them; `uncommitted`, `notBuiltHere`, `noCheckout` and `parked` are the ways of not knowing, and each says so in the row in the ordinary text colour -- words, not a colour, because "we could not check" is a difference in kind from "there is something here". diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt index affc535..4643f62 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt @@ -108,6 +108,23 @@ const val FRESHNESS_WITHHELD = "withheld" fun buildWord(components: List): String = if (components.all { it.freshness == "current" }) "Rebuild" else "Build" +/** + * What an Update button says: **Downgrade** where pressing it takes the phone *back*, because the + * build machine's checkout has been moved behind what it built. + * + * The word is the whole of the difference -- the press does exactly what Update does, and rolling + * back is a thing somebody chose rather than a mistake to warn about. Named for what you end up + * with, like Install and Update beside it. + * + * "Update" wherever anything is plainly behind, so a project with one component of each does not + * describe itself by the half that reads as a warning. One rule at both scales, like [buildWord], + * so a project's button and its components' cannot come to disagree about what pressing them means. + * Bryan asked for the word on 2026-09-06, when rolling back offered nothing to press at all. + */ +fun updateWord(freshness: List): String = + if (freshness.any { it == "builtAhead" } && freshness.none { it == "behind" }) "Downgrade" + else "Update" + // One thing a project produces. [kind] is "apk" (installed on this phone) // or "server" (installed and run on the build machine). // @@ -122,7 +139,9 @@ data class ProjectComponent( val checking: Boolean, val error: String?, // Whether what is built is current with the checkout: "current", - // "behind", or "unknown". Unknown is a real answer, not a fallback -- + // "behind", "builtAhead" -- the checkout moved back behind the build + // -- or one of the ways of not knowing. Not knowing is a real answer, + // not a fallback -- // never built here, no checkout to compare against, or uncommitted // work in this component's directory, which makes the comparison // unreliable rather than negative. @@ -190,8 +209,9 @@ data class ProjectComponent( val apk: ComponentApk?, ) { /** - * Whether there is something measured to act on: the build is behind the checkout, there is no - * build at all, or what is there was built in another mode. + * Whether there is something measured to act on: the build is on a different commit from the + * checkout in either direction, there is no build at all, or what is there was built in another + * mode. * * The one place that division is written down, because it decides three things that must not * disagree -- whether the row says "out of date", whether the card sits above the "Up to date" @@ -201,7 +221,18 @@ data class ProjectComponent( * updated". */ val isStale: Boolean - get() = freshness in setOf("behind", "neverBuilt", "otherMode") + get() = commitDiffers || freshness in setOf("neverBuilt", "otherMode") + + /** + * Whether this build and this checkout are at different commits -- behind it, or ahead of it + * because somebody moved the checkout back. + * + * Its own name because two things ask it and they are the same question: whether there is work + * waiting, and whether a sibling component installed alongside this one would be built from + * something else. + */ + val commitDiffers: Boolean + get() = freshness in setOf("behind", "builtAhead") /** * What the row says about this component's build, or null where it says nothing. @@ -217,6 +248,11 @@ data class ProjectComponent( get() = when (freshness) { "behind" -> "out of date" + // Not "out of date", which would be the wrong direction: + // what is built here is *newer* than the commit the build + // machine is sitting on, and the button beside this one + // says Downgrade. + "builtAhead" -> "newer than the checkout" "neverBuilt" -> "never built" "otherMode" -> "built in another mode" "uncommitted" -> "uncommitted changes" diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt index fdae4b5..bb7510d 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -1140,12 +1140,24 @@ private fun AppListScreen( */ fun startProjectUpdate(entry: ManifestEntry, force: Boolean = false) { scope.launch { + // A checkout with nothing to pull *from* is the second half + // alone: build what is there and install it. That covers a + // branch tracking nothing and a project told never to be moved + // from a phone -- and it is the whole of what a rollback + // wants, since picking an old commit detaches HEAD. Pulling + // regardless is how this answered a parked checkout with + // "branch HEAD tracks no upstream", from the button the card + // had just enabled to offer the downgrade. val built = - followBuild( - entry, - start = { pullProject(entry.key, force) }, - progress = { ProjectState.Working("Updating", it) }, - ) ?: return@launch + if (entry.canPull) { + followBuild( + entry, + start = { pullProject(entry.key, force) }, + progress = { ProjectState.Working("Updating", it) }, + ) ?: return@launch + } else { + null + } val fresh = (manifestState as? ManifestState.Loaded)?.manifest?.entries?.firstOrNull { it.key == entry.key @@ -1162,7 +1174,7 @@ private fun AppListScreen( // Not the one whose build just failed: its row already // says so, and asking for it again would run the same // failing command a second time to say it twice. - if (built.component(component.name)?.error != null) continue + if (built?.component(component.name)?.error != null) continue val handedOver = updateComponent(fresh, component.name, offerInstall = !installerTaken) installerTaken = installerTaken || handedOver @@ -2189,6 +2201,7 @@ private fun AppCard( needsBuild = entry.needsBuild, installed = installed != null, upToDate = upToDate, + freshness = component.freshness, state = componentState, projectState = projectState, onUpdate = { onUpdate(entry, component.name) }, @@ -2302,7 +2315,7 @@ private fun AppCard( !projectBusy && hasWorkWaiting(entry, installedTimes, chosenVariants), colors = ActionTone.Primary.colors(), ) { - Text("Update") + Text(updateWord(entry.components.map { it.freshness })) } } if (awaitingApproval) { @@ -2861,6 +2874,12 @@ private fun UpdateButton( /** Whether this phone has the app at all, which decides "Install". */ installed: Boolean, upToDate: Boolean, + /** + * This component's build against the build machine's checkout, which is what tells an update + * from a downgrade. The freshness word itself rather than a flag, so there is one place that + * knows which values mean which. + */ + freshness: String, /** What this component is doing, which is what decides whether the button can be pressed. */ state: ComponentState?, /** @@ -2902,11 +2921,19 @@ private fun UpdateButton( // newer one -- the same thing Start and Install are elsewhere, and // coloured to match them. !installed -> "Install" to ActionTone.Go + // Before the "already have this" case below, and deliberately: + // what is on disk here is older than this phone's copy right up + // until the press rebuilds it at the checkout's commit, so + // comparing the two files answers "Reinstall" to a card that has + // a rollback waiting. Coloured like Update, because it is Update + // -- the direction is the word, and going back to last week's + // build is a thing somebody chose rather than one to warn about. + freshness == "builtAhead" -> updateWord(listOf(freshness)) to ActionTone.Primary // Reinstalling replaces a build with the same build -- the same "are // you sure that's what you meant" as a Restart, and coloured to // match it. upToDate -> "Reinstall" to ActionTone.Caution - else -> "Update" to ActionTone.Primary + else -> updateWord(listOf(freshness)) to ActionTone.Primary } // Disabled while something is running, rather than replaced by the bar // that reports it. A control that disappears takes the reader's @@ -3131,9 +3158,11 @@ private fun componentBuild( * it talks to. Building in order used to make that impossible by never reaching the later component * at all; building in parallel buys the time back and gives that up, so it is said instead. * - * Only when *this* component is current and another is behind. Both behind is the ordinary state of - * a project nobody has built yet, and those two still match each other — warning about it would - * fire on every card with work waiting, which is how a warning stops being read. + * Only when *this* component is current and another is on a different commit -- behind the + * checkout, or ahead of it because somebody moved the checkout back. Both on the same wrong commit + * is the ordinary state of a project nobody has built yet, and those two still match each other — + * warning about it would fire on every card with work waiting, which is how a warning stops being + * read. * * Said rather than prevented, and the button is left alone: sometimes the mismatch is exactly what * somebody wants to install, and hiding the control would not stop them so much as leave them @@ -3142,17 +3171,17 @@ private fun componentBuild( @Composable private fun MismatchedPairNote(self: ProjectComponent, others: List) { if (self.freshness != "current") return - val behind = others.filter { it.name != self.name && it.freshness == "behind" } - if (behind.isEmpty()) return - val names = behind.joinToString(", ") { it.name } + val differing = others.filter { it.name != self.name && it.commitDiffers } + if (differing.isEmpty()) return + // Each one said in `freshnessNote`'s own words rather than in words of + // this note's -- a second word for the same measurement reads as a + // second, weaker signal rather than the same one said twice, and the + // two are drawn from the same field. Non-null for every freshness in + // `commitDiffers`, which is the whole of what got here. + val said = differing.joinToString(", ") { "${it.name} is ${it.freshnessNote}" } Text( - // "Out of date" rather than "older": that is already the word - // `freshnessNote` uses for this exact freshness ("behind"), and a - // second word for the same measurement reads as a second, weaker - // signal rather than the same one said twice -- worth avoiding - // since this note and that one are drawn from the same field. - if (behind.size == 1) "$names is out of date, so the two would not match." - else "$names are out of date, so they would not match.", + if (differing.size == 1) "$said, so the two would not match." + else "$said, so they would not match.", style = MaterialTheme.typography.bodySmall, color = ActionTone.Caution.color, ) diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 84f90c1..a92c6ff 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -45,7 +45,8 @@ const PROGRESS_MARKER: &str = "@@progress "; /// /// The states divide into three groups, and the phone is where that /// division is written down (`ProjectComponent.isStale`, one place): -/// something to do about it (`Behind`, `NeverBuilt`, `OtherMode`), +/// something to do about it (`Behind`, `BuiltAhead`, `NeverBuilt`, +/// `OtherMode`), /// nothing to do because it is current, and *cannot tell* (the rest). /// Only the first group makes a card read as out of date -- saying that /// about something nobody measured, or about a commit somebody @@ -57,6 +58,21 @@ pub enum Freshness { Current, /// Its directory has moved past the commit it was built from. Behind, + /// Built from a commit that comes *after* the one its directory is on, + /// which is a checkout somebody moved back: the build is the newer of + /// the two, so matching it to the checkout is a rollback rather than + /// an update, and the phone says "Downgrade" where it would say + /// "Update". + /// + /// Reported whether or not the checkout is parked, unlike `Parked` + /// below. Withholding it there was the whole of what made rolling + /// back unreachable from the phone -- picking an old commit detaches + /// HEAD, so every rollback is parked by construction, and the card + /// answered the move with "up to date" and nothing to press. Bryan + /// asked for this on 2026-09-06. There is also nothing here to + /// withhold: which way round the two commits are is a measurement, + /// not a guess about what somebody meant by parking. + BuiltAhead, /// Nothing where this component's builds land, so there is a first /// build to make rather than a comparison to draw. NeverBuilt, @@ -78,8 +94,10 @@ pub enum Freshness { NotBuiltHere, /// Cannot tell: no checkout under this project, or git would not say. NoCheckout, - /// Withheld: the checkout is parked on a commit somebody chose, so a - /// differing commit is a decision rather than something behind. + /// Withheld: the checkout is parked on a commit somebody chose and + /// the build is the *older* of the two, so a differing commit is a + /// decision rather than something behind. The other direction is + /// `BuiltAhead` above, which is reported. Parked, } @@ -625,14 +643,17 @@ impl BuildState { /// rather than following a branch, which is the one way a phone can /// leave it: picking a commit in the settings sheet detaches HEAD. /// Saying "out of date" about a checkout somebody deliberately moved - /// backwards is nagging about a decision already made -- and it is - /// nothing to act on either, because every way this reads as behind - /// while parked is a build that failed, a declaration waiting to be - /// accepted, or a component with no build step, each of which is - /// already said on the same card beside the button for it. So while - /// parked a differing commit is reported as unknown: withheld, rather - /// than claimed current, since nothing here measured the output to be - /// what somebody wanted. + /// forward is nagging about a decision already made, so while parked + /// that direction is reported as unknown: withheld, rather than + /// claimed current, since nothing here measured the output to be what + /// somebody wanted. + /// + /// The other direction is not withheld. A checkout parked *behind* + /// what was built is somebody rolling back, and there the decision + /// argues the opposite way: the whole point of the move is to get + /// that older commit built and installed, and withholding it left the + /// card saying "up to date" with nothing to press. See + /// `Freshness::BuiltAhead`. /// /// Passed in rather than read here. The caller has already asked git /// for this project's status, and asking again would be one more @@ -680,12 +701,34 @@ impl BuildState { } match crate::git::subtree_head(&self.project_path, &component.watched_paths()) { Some(current) if ¤t == built => Freshness::Current, - Some(_) if parked => Freshness::Parked, - Some(_) => Freshness::Behind, + Some(current) => self.which_way_round(¤t, built, parked), None => Freshness::NoCheckout, } } + /// Which of two differing commits is the later one, and so whether + /// there is an update or a rollback waiting. + /// + /// One more process on the manifest path, which is fetched on every + /// open, resume and Refresh -- paid only where the two commits already + /// differ, so a project whose builds match its checkout, which is the + /// ordinary state, spawns nothing extra. That is also the only place + /// the answer is worth anything. + /// + /// Anything git could not answer, and every other way the two commits + /// can relate -- a checkout moved forward, or onto a branch that + /// diverged -- keeps what this said before: behind, or withheld while + /// parked. + fn which_way_round(&self, current: &str, built: &str, parked: bool) -> Freshness { + if crate::git::is_ancestor(&self.project_path, current, built) == Some(true) { + return Freshness::BuiltAhead; + } + if parked { + return Freshness::Parked; + } + Freshness::Behind + } + /// Whether this component has no build output at all. /// /// The state a commit cannot express, which is why both readers of @@ -2284,6 +2327,78 @@ mod tests { ); } + /// The other direction, which is what rolling back from the phone + /// produces: picking an older commit detaches HEAD, so the checkout is + /// parked *behind* the build rather than in front of it. + /// + /// Withheld like the case above, this was the whole of why a rollback + /// could not be finished from the phone -- the card read as up to date + /// and offered nothing to press, with the older commit sitting + /// unbuilt on the build machine. Reported here whichever way the + /// checkout got there, since which commit is the later one is + /// something git was asked rather than something inferred from the + /// move. + #[test] + fn a_checkout_moved_back_behind_its_build_says_the_build_is_the_newer_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let components = two_component_checkout(root); + let first = crate::git::subtree_head(root, &[PathBuf::from("app")]).expect("a commit"); + + std::fs::write(root.join("app/main.kt"), "two").expect("write"); + run(root, &["git", "commit", "-qam", "two"]); + let second = crate::git::subtree_head(root, &[PathBuf::from("app")]).expect("a commit"); + + // Built at the second commit, and the checkout then moved back to + // the first: exactly what the commit picker leaves behind. + run(root, &["git", "checkout", "-q", &first]); + + let state = state_for(root, components); + state + .inner + .lock() + .unwrap() + .built_from + .insert("app".to_string(), second); + let app = state + .components + .iter() + .find(|component| component.name() == "app") + .expect("the app component"); + + assert_eq!( + state.freshness(app, true), + Freshness::BuiltAhead, + "a checkout parked behind its build has a rollback waiting, not nothing to do", + ); + assert_eq!( + state.freshness(app, false), + Freshness::BuiltAhead, + "and the same is true of one reset back on its own branch", + ); + } + + /// The words themselves, because they are half of an agreement with a + /// file in the other language: the phone matches these strings to + /// decide whether a card has work waiting, what its row says, and + /// whether its button offers an update or a downgrade. A rename here + /// is a card that silently says nothing there, which is why the two + /// states that mean "there is a commit difference" are pinned rather + /// than left to `rename_all` and a reader's memory. + #[test] + fn the_freshness_words_are_what_the_phone_matches_on() { + for (state, word) in [ + (Freshness::Behind, "\"behind\""), + (Freshness::BuiltAhead, "\"builtAhead\""), + ] { + assert_eq!( + serde_json::to_string(&state).expect("serialize"), + word, + "UpdateManifest.kt matches this string", + ); + } + } + /// The three ways of not knowing are three different answers, and the /// card has to be able to say which. /// diff --git a/server/src/git.rs b/server/src/git.rs index 685472a..06f8c7b 100644 --- a/server/src/git.rs +++ b/server/src/git.rs @@ -138,6 +138,38 @@ pub fn subtree_head(project: &Path, within: &[PathBuf]) -> Option { (!sha.is_empty()).then_some(sha) } +/// Whether `ancestor` is reachable from `descendant` -- the question +/// "which way round is this difference?" once two commits are known to +/// differ. +/// +/// Its one caller compares the commit a component was built from against +/// the one its directory is on now, and a checkout sitting at an ancestor +/// of the build is one somebody moved *back*: the build is the newer of +/// the two, and bringing them together means building something older +/// than what is installed. +/// +/// The exit status is the whole answer, which is why this does not go +/// through [`git`]: `--is-ancestor` reports no with a status of 1 and +/// nothing on stderr, so a helper that turns every non-zero exit into an +/// error cannot tell "no" from "could not tell". Anything but 0 or 1 is +/// the latter -- most often a commit this checkout no longer has -- and +/// `None` keeps that out of the answer rather than passing it off as a +/// direction. +pub fn is_ancestor(project: &Path, ancestor: &str, descendant: &str) -> Option { + let output = Command::new("git") + .arg("-C") + .arg(project) + .args(["merge-base", "--is-ancestor", ancestor, descendant]) + .env("GIT_TERMINAL_PROMPT", "0") + .output() + .ok()?; + match output.status.code() { + Some(0) => Some(true), + Some(1) => Some(false), + _ => None, + } +} + /// Commits the upstream branch has that this one doesn't, counted from /// refs already on disk. Only meaningful right after a fetch, which is why /// the only caller is the pull itself.