diff --git a/AGENTS.md b/AGENTS.md index cc6584f..cd2efe0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -256,6 +256,23 @@ mutable at runtime from the phone. remote deliberately, and a card that mentions it every time is nagging about a choice somebody made. +- **Unrelated histories are the one pull failure the phone may override.** + A checkout sharing no commit with its upstream has no fast-forward and + never will, so with nothing offered the card is one that can never be + pulled again -- and the way out is on the build machine, which is + exactly where the person holding the phone isn't. So `?force=true` on + the pull route resets onto the upstream instead of merging, behind a + dialog that names the branch it is about to overwrite (the only place + that is seen before it goes). Whether this *is* that failure is decided + structurally -- `git merge-base` finding no common ancestor -- and never + by matching what git printed: those messages are translated, and a + button that appeared only on an English build machine is worse than no + button. It travels as `PullError::unrelated_histories` rather than + inside the message for the same reason. The dirty-tree refusal stays in + front of it, so a forced pull can only ever discard something that was + committed, and a merely *diverged* history is not offered it -- there is + something better than throwing that away. + - **The branch line is how a failed remote check is visible at all.** With it gone, `newCommits` stays false and the card reads as an app with no updates -- the same silent failure the self-entry note below describes. diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt index 595f962..36c00b6 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt @@ -17,6 +17,13 @@ data class BuildStatus( val stale: Boolean, val building: Boolean, val error: String?, + // The failure above was a pull with no fast-forward to make, because + // the checkout on the build machine shares no commit with its + // upstream. The one failure this app offers a way past, and it is + // said as a fact of its own rather than read out of [error]: git's + // wording is translated, so a button that matched on it would appear + // only on an English build machine. + val unrelatedHistories: Boolean, // What the whole *project* is doing ("fetching", "pulling"), and how // long this run has taken. Work belonging to one component is in // [components] instead, because that is where it is drawn. @@ -66,6 +73,7 @@ private fun requestBuildStatus(path: String, method: String): BuildStatus = stale = json.getBoolean("stale"), building = json.getBoolean("building"), error = if (json.isNull("error")) null else json.getString("error"), + unrelatedHistories = json.optBoolean("unrelatedHistories", false), phase = if (json.isNull("phase")) null else json.optString("phase").ifEmpty { null }, elapsedMs = json.optLong("elapsedMs", 0), components = @@ -93,7 +101,16 @@ private fun requestBuildStatus(path: String, method: String): BuildStatus = ) } -fun pullAndBuild(key: String): BuildStatus = requestBuildStatus("/apps/$key/pull", "POST") +/** + * Fetches and fast-forwards the checkout on the build machine, then builds it. + * + * [force] resets the checkout onto its upstream instead, throwing away every commit it has that the + * remote doesn't. Only ever sent after a plain pull has come back saying the two histories are + * unrelated, and only after the dialog that says so has been confirmed -- there is no fast-forward + * for that case ever, so the alternative is a project that can never be pulled again. + */ +fun pullAndBuild(key: String, force: Boolean = false): BuildStatus = + requestBuildStatus("/apps/$key/pull" + if (force) "?force=true" else "", "POST") fun prepareBuild(key: String): BuildStatus = requestBuildStatus("/apps/$key/prepare", "POST") 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 dbea663..e9da428 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -428,6 +428,11 @@ private fun AppListScreen( // local storage rather than the manifest. Held as state so picking one // redraws the card without a round trip. var chosenVariants by remember { mutableStateOf>(emptyMap()) } + // The project whose pull came back saying its checkout and its remote + // share no history, waiting on an answer about throwing that history + // away. One slot rather than one per card: it is a modal, so only one + // can be open, and the entry in it says which card asked. + var forcePull by remember { mutableStateOf(null) } // Which component of which project has a service action in flight. // Per project, because that is the granularity of a card, and the row // that is busy is the one that shows it. @@ -625,6 +630,10 @@ private fun AppListScreen( val failure = status.error if (failure != null) { cardStates = cardStates + (entry.key to CardState.Error(failure, retryPull)) + // The card keeps the message either way -- the dialog is + // dismissible, and a failure that vanished with it would + // leave the card looking as though nothing had happened. + if (status.unrelatedHistories) forcePull = entry return } // The APK's mtime is what decides "update available", so the @@ -669,13 +678,18 @@ private fun AppListScreen( } } - /** Pull acts on the build machine: fetch, fast-forward, rebuild. */ - fun startPull(entry: ManifestEntry) { + /** + * Pull acts on the build machine: fetch, fast-forward, rebuild. + * + * [force] is the answer to the dialog below, and nothing else ever passes it: it abandons + * whatever history that checkout has of its own. + */ + fun startPull(entry: ManifestEntry, force: Boolean = false) { scope.launch { followBuild( entry, retryPull = true, - start = { pullAndBuild(entry.key) }, + start = { pullAndBuild(entry.key, force) }, progress = { CardState.Pulling(it) }, ) } @@ -1073,9 +1087,67 @@ private fun AppListScreen( ) { Text(PLUS_GLYPH, fontFamily = NerdIcons, fontSize = 22.sp) } + + forcePull?.let { entry -> + ForcePullDialog( + entry = entry, + onDismiss = { forcePull = null }, + onForce = { + forcePull = null + startPull(entry, force = true) + }, + ) + } } } +/** + * Offered when a pull comes back saying the checkout and its remote share no history at all. + * + * There is no fast-forward between two unrelated histories and there never will be, so without this + * the card is one that can never be pulled again — and the way out is on the build machine, which + * is exactly where the person holding the phone isn't. So the capability is shown rather than + * withheld, with what it costs said in front of it: the alternative to a destructive button here is + * not safety, it is a dead card. + * + * The branch and its upstream are named because they are what is about to be overwritten, and this + * is the only place they are seen before it happens. + */ +@Composable +private fun ForcePullDialog(entry: ManifestEntry, onDismiss: () -> Unit, onForce: () -> Unit) { + // Named where they are known. Pull is only offered for a checkout + // with an upstream, so the fallbacks are for a list that has moved on + // since the failure rather than for the ordinary case. + val branch = entry.git?.branch ?: "the branch" + val upstream = entry.git?.upstream ?: "the remote" + AlertDialog( + onDismissRequest = onDismiss, + title = { Text("${entry.label} shares no history with $upstream") }, + text = { + Text( + "Its checkout on the build machine has no commit in common with $upstream, so " + + "there is no fast-forward to make and Pull can go no further.\n\n" + + "Forcing resets $branch onto $upstream. Every commit the build machine has " + + "that the remote doesn't is abandoned -- they stay in that checkout's " + + "reflog, but nothing on this phone will bring them back.\n\n" + + "Uncommitted changes are not touched: a pull refuses over those before it " + + "ever gets this far." + ) + }, + confirmButton = { + TextButton( + // The red every control that takes something away wears, + // so this doesn't read as the ordinary way past a message. + colors = ActionTone.Destructive.colors(), + onClick = onForce, + ) { + Text("Force pull") + } + }, + dismissButton = { TextButton(onClick = onDismiss) { Text("Cancel") } }, + ) +} + @Composable private fun AppCard( entry: ManifestEntry, @@ -2006,13 +2078,6 @@ private fun ComponentCard( // together -- controls belong to an APK, service buttons to a // server -- so each gets the card's full width instead of // sharing a row. A progress bar in particular wants all of it. - // Before the controls, and inside this component's own card: - // what is happening to *this* component belongs with it, not - // under the project where it could be any of them. - build?.let { - Spacer(Modifier.height(6.dp)) - ComponentBuildProgress(it) - } controls() // Nothing to offer until its script has been asked: the buttons // depend on the answer, and guessing which to show would mean @@ -2073,6 +2138,16 @@ private fun ComponentCard( } } } + + // Under the buttons, and inside this component's own card. It + // reports on the press that started it, so it reads in the + // order it happened -- and putting it above meant the buttons + // moved down the moment a build began, so the row somebody had + // just pressed slid out from under their finger. + build?.let { + Spacer(Modifier.height(6.dp)) + ComponentBuildProgress(it) + } } } diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 0b4535b..9243693 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -102,6 +102,12 @@ struct Inner { /// where this is being driven from and usually has no access to the /// server's log. error: Option, + /// Whether that failure was a pull with no fast-forward to make, + /// which is the one the phone can offer a way past. Beside the + /// message rather than inside it because the phone acts on it: a + /// button that appears only when git happened to phrase itself a + /// certain way is a button nobody can rely on. + unrelated_histories: bool, /// The component whose step produced that failure. /// /// Kept beside the message so the card can open the right log without @@ -180,6 +186,10 @@ pub struct BuildStatus { pub stale: bool, pub building: bool, pub error: Option, + /// That failure was a pull the checkout has no fast-forward for, + /// because it shares no history with its upstream. The phone offers + /// the forced pull for this and nothing else. + pub unrelated_histories: bool, /// What the whole project is doing -- fetching, pulling -- absent /// between runs and while the work belongs to a component instead. pub phase: Option, @@ -387,6 +397,7 @@ impl BuildState { inner.building = true; inner.error = None; inner.failed = None; + inner.unrelated_histories = false; inner.started = Some(Instant::now()); inner.runs.clear(); } @@ -509,8 +520,14 @@ impl BuildState { /// whose new commits changed the declaration built anyway -- the card /// went off and updated the app instead of stopping to ask, which is /// the single case this gate exists for. + /// `force` abandons this checkout's own history in favour of the + /// upstream's, and is only ever pressed after a pull has already + /// reported that the two are unrelated (`git::PullError`). It is a + /// parameter rather than something decided here because it is a + /// person's answer to that report, not a state of the repository. pub fn pull_and_build( self: &Arc, + force: bool, may_build: impl Fn() -> bool + Send + 'static, record: RecordBuilt, ) { @@ -522,18 +539,18 @@ impl BuildState { inner.building = true; inner.error = None; inner.failed = None; + inner.unrelated_histories = false; inner.started = Some(Instant::now()); inner.runs.clear(); } let this = Arc::clone(self); tokio::task::spawn_blocking(move || { - let outcome = this.pull(); + let outcome = this.pull(force); match outcome { - Err(message) => { - tracing::error!("pull failed: {message}"); - // No component: a pull fails before the walk starts. - this.finish(Some(message), None); + Err(error) => { + tracing::error!("pull failed: {}", error.message); + this.fail_pull(error); } Ok(pulled) => { // Nothing configured to build, or nothing allowed to: @@ -553,7 +570,7 @@ impl BuildState { } /// Fetches and fast-forwards, reporting whether anything arrived. - fn pull(&self) -> Result { + fn pull(&self, force: bool) -> Result { self.begin_project("fetching"); crate::git::fetch(&self.project_path, self.git_ipv4)?; // Counted after the fetch, from refs now on disk: this is the one @@ -563,7 +580,7 @@ impl BuildState { return Ok(false); } self.begin_project("pulling"); - crate::git::pull(&self.project_path, self.git_ipv4)?; + crate::git::pull(&self.project_path, self.git_ipv4, force)?; tracing::info!( "pulled {behind} commit(s) into {}", self.project_path.display() @@ -990,6 +1007,19 @@ impl BuildState { inner.failed = failed; } + /// Finishes a run that never got past its pull. + /// + /// Separate from [`Self::finish`] only because a pull failure carries + /// the one thing a build failure cannot: whether abandoning this + /// checkout's own history would clear it. Cleared where its siblings + /// are, at the start of every run, so this is the only thing that can + /// ever make it true. + fn fail_pull(&self, error: crate::git::PullError) { + self.inner.lock().unwrap().unrelated_histories = error.unrelated_histories; + // No component: a pull fails before the walk starts. + self.finish(Some(error.message), None); + } + /// Records how far the running command says it has got. Replaces the /// previous count rather than accumulating: the command reports its /// own total, and `begin` clears this so one component's count is @@ -1041,6 +1071,7 @@ impl BuildState { stale, building: inner.building, error: inner.error.clone(), + unrelated_histories: inner.unrelated_histories, phase: inner.phase.clone(), elapsed_ms: inner .started @@ -1230,6 +1261,7 @@ mod tests { let project = clone.clone(); state.pull_and_build( + false, move || crate::config::project_config(&project).matches_accepted(&accepted, None), Arc::new(|_: &str, _: String| {}), ); diff --git a/server/src/git.rs b/server/src/git.rs index bef464b..58fe7c5 100644 --- a/server/src/git.rs +++ b/server/src/git.rs @@ -246,28 +246,70 @@ fn run_bounded(project: &Path, ipv4: bool, args: &[&str]) -> Result for PullError { + fn from(message: String) -> Self { + Self { + message, + unrelated_histories: false, + } + } +} + /// Fast-forwards the current branch onto its upstream. /// /// `--ff-only` deliberately: a merge or a rebase can conflict, and /// resolving a conflict is not something to start from a phone with no way /// to finish it. Refusing to touch a dirty tree is the same reasoning -- /// the failure is reported and the working tree is left exactly as it was. -pub fn pull(project: &Path, ipv4: bool) -> Result<(), String> { +/// +/// `force` replaces the fast-forward with a hard reset onto the upstream, +/// which is what somebody presses after being told the histories are +/// unrelated: it throws away every commit this checkout has that the +/// remote does not. The dirty check stays in front of it, so what it +/// discards is always something that was committed -- still in the +/// reflog on the build machine -- and never somebody's uncommitted work. +pub fn pull(project: &Path, ipv4: bool, force: bool) -> Result<(), PullError> { let status = status(project).ok_or_else(|| "not a git repository".to_string())?; if status.upstream.is_none() { return Err(format!( "branch {} tracks no upstream, so there is nothing to pull", status.branch - )); + ) + .into()); } if status.dirty { return Err(format!( "{} has uncommitted changes -- refusing to pull over them. Commit or stash on the \ build machine first.", project.display() - )); + ) + .into()); + } + if force { + git(project, &["reset", "--hard", "@{u}"])?; + } else if let Err(message) = git(project, &["merge", "--ff-only", "@{u}"]) { + return Err(PullError { + unrelated_histories: unrelated_to_upstream(project), + message, + }); } - git(project, &["merge", "--ff-only", "@{u}"])?; // A merge moves the *pointer* a submodule is recorded at without // touching its working tree, so a pull that updated one leaves the @@ -298,6 +340,22 @@ pub fn pull(project: &Path, ipv4: bool) -> Result<(), String> { Ok(()) } +/// Whether HEAD and its upstream share any commit at all. +/// +/// Asked of the object store rather than read off what git printed: +/// "refusing to merge unrelated histories" is a translated string, so +/// matching it would work on this machine and quietly stop working on one +/// running in another language. `merge-base` fails, printing nothing, +/// exactly when there is no common ancestor. +/// +/// Only asked once a fast-forward has already failed, which is what keeps +/// its other ways of failing -- an upstream that doesn't resolve, say -- +/// from being reported as unrelated history: by then the merge has just +/// resolved both revisions itself. +fn unrelated_to_upstream(project: &Path) -> bool { + git(project, &["merge-base", "HEAD", "@{u}"]).is_err() +} + /// The phone-sized version of a failed remote command: the first line git /// printed, plus what this process can see of the ssh agent when what /// failed was authentication. @@ -571,7 +629,7 @@ mod tests { run(&origin, &["git", "commit", "-qam", "two"]); fetch(&clone, true).expect("fetch with ipv4 forced"); - pull(&clone, true).expect("pull with ipv4 forced"); + pull(&clone, true, false).expect("pull with ipv4 forced"); assert!(!has_new_commits(&clone, true).expect("check")); } @@ -622,7 +680,7 @@ mod tests { // Pull is what actually takes them. fetch(&clone, false).expect("fetch"); assert_eq!(behind(&clone), 1); - pull(&clone, false).expect("pull"); + pull(&clone, false, false).expect("pull"); assert!(!has_new_commits(&clone, false).expect("check")); } @@ -654,7 +712,7 @@ mod tests { "fetched but not merged is still something to pull", ); - pull(&clone, false).expect("pull"); + pull(&clone, false, false).expect("pull"); run( &clone, &["git", "commit", "-q", "--allow-empty", "-m", "local"], @@ -759,7 +817,7 @@ mod tests { run(&origin, &["git", "commit", "-qam", "two"]); fetch(&clone, false).expect("fetch"); - pull(&clone, false).expect("pull"); + pull(&clone, false, false).expect("pull"); assert_eq!(behind(&clone), 0); assert_eq!( std::fs::read_to_string(clone.join("file")).expect("read"), @@ -780,8 +838,12 @@ mod tests { std::fs::write(clone.join("file"), "local edit").expect("write"); assert!(status(&clone).expect("status").dirty); - let err = pull(&clone, false).expect_err("should refuse"); - assert!(err.contains("uncommitted changes"), "{err}"); + let err = pull(&clone, false, false).expect_err("should refuse"); + assert!( + err.message.contains("uncommitted changes"), + "{}", + err.message + ); // Left exactly as it was. assert_eq!( std::fs::read_to_string(clone.join("file")).expect("read"), @@ -789,6 +851,63 @@ mod tests { ); } + /// The one failure the phone may override, so it has to be + /// distinguishable from every other reason a pull stops -- and the + /// override has to actually take the remote's history rather than + /// somehow merging the two. + /// + /// The second half is the case the button must *not* appear on: a + /// history that has merely diverged still shares a commit, so there is + /// something better than throwing it away and this must not offer to. + #[test] + fn only_an_unrelated_history_offers_the_override() { + let dir = tempfile::tempdir().expect("tempdir"); + let (origin, clone) = origin_and_clone(dir.path()); + // A local history sharing no commit with the remote's, which is + // what a checkout re-created from somewhere else looks like. + run(&clone, &["git", "checkout", "-q", "--orphan", "rebuilt"]); + std::fs::write(clone.join("file"), "local").expect("write"); + run(&clone, &["git", "add", "."]); + run(&clone, &["git", "commit", "-qm", "unrelated"]); + run(&clone, &["git", "branch", "-qM", "main"]); + run( + &clone, + &[ + "git", + "branch", + "-q", + "--set-upstream-to=origin/main", + "main", + ], + ); + + std::fs::write(origin.join("file"), "two").expect("write"); + run(&origin, &["git", "commit", "-qam", "two"]); + fetch(&clone, false).expect("fetch"); + + let err = pull(&clone, false, false).expect_err("no fast-forward exists"); + assert!(err.unrelated_histories, "{}", err.message); + + pull(&clone, false, true).expect("forced pull"); + assert_eq!(behind(&clone), 0); + assert_eq!( + std::fs::read_to_string(clone.join("file")).expect("read"), + "two" + ); + + // Now the two share everything up to here and each add a commit, + // which is a pull that cannot fast-forward for an entirely + // ordinary reason. + std::fs::write(clone.join("file"), "mine").expect("write"); + run(&clone, &["git", "commit", "-qam", "mine"]); + std::fs::write(origin.join("file"), "theirs").expect("write"); + run(&origin, &["git", "commit", "-qam", "theirs"]); + fetch(&clone, false).expect("fetch"); + + let err = pull(&clone, false, false).expect_err("diverged"); + assert!(!err.unrelated_histories, "{}", err.message); + } + #[test] fn a_branch_tracking_nothing_has_nothing_to_pull() { let dir = tempfile::tempdir().expect("tempdir"); @@ -799,8 +918,9 @@ mod tests { assert_eq!(status.branch, "detached-work"); assert_eq!(status.upstream, None); assert!( - pull(&clone, false) + pull(&clone, false, false) .expect_err("no upstream") + .message .contains("no upstream") ); // Nothing to compare against, so the list is told plainly rather diff --git a/server/src/routes.rs b/server/src/routes.rs index 8b0b9a4..dc598a8 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -12,6 +12,7 @@ //! PUT /roots {roots} set the directories /suggestions scans //! GET /apps/{key}/apk[?variant=] the payload itself (ranged) //! POST /apps/{key}/pull fetch, fast-forward, and build +//! ?force=true resets onto the upstream //! POST /apps/{key}/prepare run the on-demand build step, if any //! POST /apps/{key}/build run it whether or not it looks stale //! POST /apps/{key}/approve accept the build step it asks for @@ -1024,6 +1025,19 @@ struct PurgeQuery { config: bool, } +/// Whether a pull may throw this checkout's own history away. +/// +/// Off unless the phone says otherwise, and it only says so after being +/// told the checkout and its upstream are unrelated -- there is no +/// fast-forward for that ever, so the alternative to this is a card that +/// can never be pulled again. Destructive, and confirmed on the phone +/// against the branch it names before it is ever sent. +#[derive(Deserialize)] +struct PullQuery { + #[serde(default)] + force: bool, +} + /// Which build a download wants. Absent means the newest, which is what /// every phone gets until it says otherwise. #[derive(Deserialize)] @@ -1180,6 +1194,7 @@ async fn build_now( async fn build_pull( State(state): State>, key: UrlPath, + Query(query): Query, ) -> Result, ApiError> { let entry = lookup(&state, Some(key))?; let build = entry @@ -1191,6 +1206,7 @@ async fn build_pull( // here is the answer for the commit being replaced. let gate = Arc::clone(&entry); build.pull_and_build( + query.force, move || gate.pending_declaration().is_none(), records_builds(&state, &entry.key), );