diff --git a/AGENTS.md b/AGENTS.md index 500b471..e43c5ad 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -340,22 +340,37 @@ mutable at runtime from the phone. remote deliberately, and a card that mentions it every time is nagging about a choice somebody made. -- **A checkout can be moved from the phone, and it is the same act as a - pull.** The project settings sheet lists the checkout's branches and its - last fifty commits, and picking one moves the checkout and then builds - whatever that left behind. `BuildState::after_moving` is the half a pull - and a checkout share -- one copy, because the part that is easy to get - wrong is not the git command but handing the run over under a single - lock, so nothing can observe the moment between the move ending and the - builds it decided on starting. It reports through the project's own - state for the same reason: one progress path, not a second to keep in - step. +- **A checkout can be moved from the phone, and it is nearly the same act + as a pull.** The project settings sheet lists the checkout's branches and + its last fifty commits, and picking one moves the checkout. + `BuildState::after_moving` is the half a pull and a checkout share -- one + copy, because the part that is easy to get wrong is not the git command + but handing the run over under a single lock, so nothing can observe the + moment between the move ending and the builds it decided on starting. It + reports through the project's own state for the same reason: one progress + path, not a second to keep in step. + **Where they differ is that a move builds nothing** (`then_build` is + `None`, which is what `BuildAfterMoving` exists to say). A pull is + somebody taking new work, so building it is the point; moving the + checkout is somebody *looking* -- at another branch, at last week's + commit -- and charging a full build of every component for a look means + an accidental tap starts a minute of work with no way to cancel it, and + replaces outputs that were wanted. What it leaves behind is a component + whose build no longer matches the checkout, which the card already says + and already carries the button for. Iris asked for this on 2026-09-02. + Which is why **the build button's word follows the state**: `buildWord` + in `UpdateManifest.kt` says `Rebuild` only where every component it + covers is *known* current, and `Build` otherwise -- behind, never built, + or parked on a commit nobody measured the output against. One rule at + both scales, so the project row and a component row cannot come to + disagree about what pressing them means; the project's asks only about + the components it actually builds, since one with no command has no + output to be current with anything and counting it would pin that button + to `Build` for ever. The two lists are read by `GET /apps/{key}/refs` when the sheet is - opened, never on the manifest -- both spawn git. They are **local reads - only**: what the remote has and this checkout has not fetched is not - offered, because Pull is what brings those in and the card already says - when there are some, so opening the sheet cannot stall on a round trip - or fail the way a remote check can. + opened, never on the manifest -- both spawn git, and both are **local + reads**, so opening the sheet cannot stall on a round trip or fail the + way a remote check can. Two things about listing branches were wrong until a real checkout was looked at, and neither shows up in a fixture built to pass: `git branch --format` adds a **`(HEAD detached at abc123)` pseudo-entry** @@ -367,6 +382,54 @@ mutable at runtime from the phone. test that was meant to cover the second asserted the same wrong thing and passed. +- **Fetch is a button, because nothing else here writes a + remote-tracking ref.** The only `git fetch` this server ran was the one + inside a pull, and a pull is reachable only when the branch you are + already on is behind -- so a branch pushed from another machine reached + the picker only as a side effect of pulling something else, and a project + already up to date could never be moved onto a new branch at all. The way + out was a terminal on the build machine, which is where the person + holding the phone is not. `POST /apps/{key}/fetch` is the fix, sitting + under the two pickers in the project settings sheet. Deliberately still + not on a timer and not on opening the sheet: a fetch mutates the checkout + and pulls down objects, so it stays something somebody pressed, and + `/refs` stays the local read described above. + Both pickers sit in a `SettingRow`, which weights the label *and* the + control rather than letting the control take what it likes: unweighted, + a pill showing `second-branch-from-elsewhere` squeezes "Branch" into a + three-character column that wraps one letter per line. The same rule the + component rows already follow, in the other place a row mixes text with + a control. The fetch failure and the "reading branches" note go + *below* both pickers for the same family of reason -- above them, each + appears and disappears mid-sheet and shoves the pickers down the screen + as somebody is reaching for one, and the failure ends up nowhere near + the button that produced it. + It answers with the **whole ref list** rather than an acknowledgement, so + the pickers repopulate from after the fetch in the same round trip -- + `routes::read_refs` is the one place both routes build that list. + `--prune` is asked for here and nowhere else: this is the call whose job + is to make the picker say what the remote says, where a pull should + change as little as it can. The phone's read timeout for it is longer + than the server's own 30s hard stop for a remote command, so a slow + remote is reported in git's words rather than as the phone giving up on a + request that is still running. While it runs, both pickers are disabled + -- it is replacing the lists they are showing -- but uncommitted work is + *not* a reason to disable it, since a fetch touches no file in the + working tree. It is the one control in that sheet that still works on a + dirty checkout. + +- **`git checkout origin/topic` detaches HEAD**, which is why + `git::local_branch_for` exists. Git's DWIM that starts a tracking branch + fires on the bare name `topic` and on nothing else, and the picker's + remote-only entries are named `origin/topic` because that is what they + are -- so picking a branch left the checkout on no branch, with no + upstream and so no Pull: the state picking a *commit* is meant to + produce, reached from the control that says it is picking a branch. + Nothing about it looks like a failure either, since `git checkout` + prints its detached-HEAD advice and succeeds. Found by running it + against a real clone once the Fetch button made remote-only branches + something you could actually reach. + - **The checkout guard is deliberately weaker than the pull guard, and that is what stops it being a one-way door.** `git::checkout` refuses only on **tracked** modifications (`--untracked-files=no`), where diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt index 8af3eec..c59951c 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt @@ -1,5 +1,6 @@ package com.example.devupdater +import java.net.HttpURLConnection import org.json.JSONArray import org.json.JSONObject @@ -200,34 +201,62 @@ data class CheckoutRefs( * the checkout has not fetched is Pull's business, and the card already says when there is any. */ fun checkoutRefs(key: String): CheckoutRefs = - requestFromServer("/apps/$key/refs") { connection -> - val body = JSONObject(connection.inputStream.bufferedReader().readText()) - val branches = body.getJSONArray("branches") - val commits = body.getJSONArray("commits") - CheckoutRefs( - branches = - (0 until branches.length()).map { i -> - val branch = branches.getJSONObject(i) - GitBranch( - name = branch.getString("name"), - current = branch.optBoolean("current", false), - remoteOnly = branch.optBoolean("remoteOnly", false), - ) - }, - commits = - (0 until commits.length()).map { i -> - val commit = commits.getJSONObject(i) - GitCommit( - sha = commit.getString("sha"), - short = commit.getString("short"), - subject = commit.optString("subject"), - at = commit.optLong("at", 0), - current = commit.optBoolean("current", false), - ) - }, - head = body.optString("head").takeIf { it.isNotEmpty() }, - ) - } + requestFromServer("/apps/$key/refs", readBody = ::refs) + +/** + * Brings the build machine's checkout up to date with its remote, and answers with the refs that + * leaves. + * + * The one call in this sheet that reaches the network, and the only way a branch pushed from + * another machine ever appears in the picker: nothing on the build machine writes a new + * remote-tracking ref except the fetch inside a pull, and a pull is only offered when the branch + * you are already on is behind. Answering with the refs rather than an acknowledgement is what + * makes one round trip enough -- reading them again afterwards would leave a moment where the + * button had finished and the list was still the old one. + */ +fun fetchRefs(key: String): CheckoutRefs = + requestFromServer( + "/apps/$key/fetch", + method = "POST", + // Longer than the build machine's own hard stop for a remote + // command, so a slow remote is reported in git's words rather + // than as this side giving up on a request that is still running. + readTimeoutMs = FETCH_READ_TIMEOUT_MS, + readBody = ::refs, + ) + +/** Beyond the 30s the build machine allows a remote command, so its answer arrives first. */ +private const val FETCH_READ_TIMEOUT_MS = 35000 + +/** The one reader for [checkoutRefs] and [fetchRefs], so the two cannot read a checkout apart. */ +private fun refs(connection: HttpURLConnection): CheckoutRefs { + val body = JSONObject(connection.inputStream.bufferedReader().readText()) + val branches = body.getJSONArray("branches") + val commits = body.getJSONArray("commits") + return CheckoutRefs( + branches = + (0 until branches.length()).map { i -> + val branch = branches.getJSONObject(i) + GitBranch( + name = branch.getString("name"), + current = branch.optBoolean("current", false), + remoteOnly = branch.optBoolean("remoteOnly", false), + ) + }, + commits = + (0 until commits.length()).map { i -> + val commit = commits.getJSONObject(i) + GitCommit( + sha = commit.getString("sha"), + short = commit.getString("short"), + subject = commit.optString("subject"), + at = commit.optLong("at", 0), + current = commit.optBoolean("current", false), + ) + }, + head = body.optString("head").takeIf { it.isNotEmpty() }, + ) +} /** One component's log, as the modal shows it. */ /** 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 e166b08..11395e5 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt @@ -84,6 +84,21 @@ data class GitStatus( val root: String, ) +/** + * What a build button says for the components it covers. + * + * **Rebuild** only where every one of them is known to match the checkout, because that is the case + * where pressing it does the same work over again. Anything else -- behind the checkout, or never + * built, or parked on a commit nobody measured the output against -- has a build to make rather + * than one to repeat, and saying "Rebuild" there reads as though the work had already been done. + * + * One rule at both scales, so a project's button and its components' cannot come to disagree about + * what pressing them means. Iris asked for the word to follow the state on 2026-09-02, when moving + * a checkout stopped building what it left behind. + */ +fun buildWord(components: List): String = + if (components.all { it.freshness == "current" }) "Rebuild" else "Build" + // One thing a project produces. [kind] is "apk" (installed on this phone) // or "server" (installed and run on the build machine). // 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 7f06fff..03e94f3 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -38,6 +38,7 @@ import androidx.compose.material3.DropdownMenuItem import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.material3.FloatingActionButton import androidx.compose.material3.IconButton +import androidx.compose.material3.LocalContentColor import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedButton import androidx.compose.material3.OutlinedCard @@ -895,11 +896,13 @@ private fun AppListScreen( } /** - * Moves this project's checkout onto a branch or a commit on the build machine, and builds what - * that leaves behind. + * Moves this project's checkout onto a branch or a commit on the build machine, and builds + * nothing. * * Followed exactly as a pull is, because it is the same act on the same single checkout: one * progress path, reported in the project's own row, rather than a second one to keep in step. + * It is over as soon as git is, though, since there is no build behind it -- what the move + * leaves behind is a component the card reports as out of date, with its own Build beside it. */ fun startCheckout(entry: ManifestEntry, target: String) { scope.launch { @@ -2035,7 +2038,12 @@ private fun AppCard( // behind is not obvious from here. colors = ActionTone.Caution.colors(), ) { - Text("Rebuild") + // Asked of the components this actually builds: + // one with no command of its own has no output to + // be current with anything, and counting it would + // leave every such project's button saying Build + // for ever. + Text(buildWord(entry.components.filter { it.hasBuild })) } } if (awaitingApproval) { @@ -2226,6 +2234,12 @@ private fun ProjectSettingsDialog( * that does nothing: not a checkout at all, something already running on it, uncommitted work in * the way, still reading, or the read failed. Those are five different things to do next, and a * disabled dropdown with no sentence beside it is the same picture for all of them. + * + * Fetch is the third control and the only one that touches the network. It is what puts a branch + * pushed from another machine into the picker above it -- without it the list is whatever the last + * pull happened to bring in, so a project already up to date on its own branch could never be moved + * onto a new one. It stays a press rather than something the sheet does on opening, because a fetch + * writes into the checkout and pulls down objects: opening a sheet should not do either. */ @Composable private fun CheckoutSection( @@ -2235,6 +2249,8 @@ private fun CheckoutSection( ) { var refs by remember(entry.key) { mutableStateOf(null) } var failure by remember(entry.key) { mutableStateOf(null) } + var fetching by remember(entry.key) { mutableStateOf(false) } + val scope = rememberCoroutineScope() val git = entry.git // Only for a project actually in a checkout; asking git about a @@ -2250,7 +2266,6 @@ private fun CheckoutSection( } } - SettingsHeading("Checkout") when { git == null -> { SettingsNote("This project is not in a git repository, so there is nothing to move.") @@ -2264,25 +2279,14 @@ private fun CheckoutSection( "or stash on the build machine first." ) busy -> SettingsNote("Something is already running on this checkout.") - else -> - SettingsNote( - "Moves the checkout on the build machine and builds what that leaves behind, the " + - "same as Pull does. Picking a commit parks it there, on no branch; HEAD is " + - "the way back to following the branch." - ) } - val enabled = !busy && !git.dirty && refs != null + // A fetch replaces the very lists these two are showing, so they are + // disabled while it runs rather than left offering the old answer. + val enabled = !busy && !git.dirty && refs != null && !fetching val loaded = refs - if (loaded == null && failure == null) { - SettingsNote("Reading this checkout's branches...") - } - // The server's own words, so selectable like every other machine - // output here -- the fix is on the other machine. - failure?.let { OutputText(it, style = MaterialTheme.typography.bodySmall) } - Row(verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth()) { - Text("Branch", style = MaterialTheme.typography.bodyLarge, modifier = Modifier.weight(1f)) + SettingRow("Branch") { BranchPicker( branches = loaded?.branches.orEmpty(), // The branch line on the card is the same answer, and it is @@ -2293,8 +2297,7 @@ private fun CheckoutSection( onSelect = onCheckout, ) } - Row(verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth()) { - Text("Commit", style = MaterialTheme.typography.bodyLarge, modifier = Modifier.weight(1f)) + SettingRow("Commit") { CommitPicker( commits = loaded?.commits.orEmpty(), head = loaded?.head, @@ -2306,6 +2309,81 @@ private fun CheckoutSection( onSelect = onCheckout, ) } + Row( + horizontalArrangement = Arrangement.End, + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth(), + ) { + TextButton( + onClick = { + fetching = true + failure = null + scope.launch { + try { + refs = withContext(Dispatchers.IO) { fetchRefs(entry.key) } + } catch (e: DownloadServerException) { + failure = e.message ?: "the fetch failed" + } finally { + fetching = false + } + } + }, + // Not while the checkout is being worked on: a pull is + // writing the same refs, and git would refuse one of the two + // for a reason nobody pressed anything to hear. Uncommitted + // work is deliberately *not* a reason -- a fetch touches no + // file in the working tree, so this is the one control in + // here that still does its job on a dirty checkout. + enabled = !busy && !fetching, + colors = ActionTone.Primary.colors(), + ) { + if (fetching) { + // The button's own content colour, which while it is + // running is the disabled one -- a full-strength mark + // beside a dimmed label reads as two things happening + // rather than as one button that is busy. + Working(color = LocalContentColor.current) + Spacer(Modifier.width(8.dp)) + } + Text("Fetch") + } + } + // Both of these belong *below* the controls rather than above them. + // Above, each one appears and disappears in the middle of the sheet + // and shoves the two pickers down the screen as it does -- so the row + // somebody is reaching for moves while they reach, and the failure + // ends up nowhere near the button that produced it. + if (loaded == null && failure == null) { + SettingsNote("Reading this checkout's branches...") + } + // The server's own words, so selectable like every other machine + // output here -- the fix is on the other machine. + failure?.let { OutputText(it, style = MaterialTheme.typography.bodySmall) } +} + +/** + * One setting: what it is on the left, the control for it on the right. + * + * The two get [Modifier.weight] rather than the control being left to take whatever width it likes, + * because a picker's label is a branch name and those run long: unweighted, the pill grows to fit + * `second-branch-from-elsewhere` and squeezes "Branch" down to a column three characters wide, + * which then wraps one letter per line. Both sides truncate instead -- the control at its own end, + * since a pill that has to be cut is still recognisably a control, and the name it is showing is + * the thing the reader can open it to see in full. + */ +@Composable +private fun SettingRow(label: String, control: @Composable () -> Unit) { + Row(verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth()) { + Text( + label, + style = MaterialTheme.typography.bodyLarge, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + modifier = Modifier.weight(1f), + ) + Spacer(Modifier.width(8.dp)) + Box(Modifier.weight(2f), contentAlignment = Alignment.CenterEnd) { control() } + } } @Composable @@ -2610,7 +2688,7 @@ private fun ComponentBuildButton( enabled = !state.busy && !projectState.busy, colors = ActionTone.Caution.colors(), ) { - Text("Rebuild") + Text(buildWord(listOf(component))) } } diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/Working.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/Working.kt index 871774f..471b949 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/Working.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/Working.kt @@ -2,8 +2,10 @@ package com.example.devupdater import androidx.compose.foundation.layout.size import androidx.compose.material3.CircularProgressIndicator +import androidx.compose.material3.ProgressIndicatorDefaults import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.Color import androidx.compose.ui.unit.dp /** @@ -15,11 +17,15 @@ import androidx.compose.ui.unit.dp * * One composable rather than a size and a stroke width repeated per site, so every one of them * reads as the same mark. + * + * [color] is for the one inside a button, where the mark belongs to that button's label and taking + * the scheme's own accent instead would read as a second thing going on beside it. */ @Composable -fun Working(modifier: Modifier = Modifier) { +fun Working(modifier: Modifier = Modifier, color: Color = ProgressIndicatorDefaults.circularColor) { CircularProgressIndicator( modifier = modifier.size(12.dp), + color = color, strokeWidth = 1.5.dp, ) } diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 0b478a9..1e48f41 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -60,6 +60,15 @@ pub enum Freshness { /// starts a build has the list and supplies this. pub type RecordBuilt = Arc; +/// What a move does once the working tree has moved: the acceptance gate +/// to ask on the tree it left, and where to record what got built. +/// +/// `None` is a move that builds nothing at all, which is every checkout -- +/// the two halves travel together because neither is any use without the +/// other, and an `Option` of the pair is what makes "build, but with +/// nowhere to record it" unsayable. +type BuildAfterMoving = Option<(Box bool + Send>, RecordBuilt)>; + /// The handles a build shares with the rest of the server, as opposed to /// the project configuration it is built from. /// @@ -713,37 +722,40 @@ impl BuildState { may_build: impl Fn() -> bool + Send + 'static, record: RecordBuilt, ) { - self.after_moving(move |this| this.pull(force), may_build, record); + self.after_moving( + move |this| this.pull(force), + Some((Box::new(may_build), record)), + ); } /// Moves this checkout onto `target` -- a branch or a commit -- and - /// then builds whatever that left behind. + /// builds nothing. /// /// Exclusive with everything, and deferred to the same machinery as a /// pull, because it is the same kind of act: there is one checkout, /// and this rewrites the files every component builds from. It /// reports through the project's own state for the same reason, so a /// card shows it exactly as it shows a pull. - pub fn checkout_and_build( - self: &Arc, - target: String, - may_build: impl Fn() -> bool + Send + 'static, - record: RecordBuilt, - ) { + /// + /// Where it deliberately differs from a pull is that nothing is built + /// afterwards. A pull is somebody taking new work, so building it is + /// the point; moving the checkout is somebody *looking* -- at another + /// branch, at last week's commit -- and making the cost of looking a + /// full build of every component means an accidental tap starts a + /// minute of work with nothing to cancel it, and replaces outputs + /// that were wanted. What that leaves behind is a component whose + /// build no longer matches the checkout, which the card already says + /// and already carries the button for. Iris asked for this on + /// 2026-09-02. + pub fn move_checkout(self: &Arc, target: String) { self.after_moving( move |this| { this.begin_project("checking out"); crate::git::checkout(&this.project_path, &target)?; tracing::info!("checked out {target} in {}", this.project_path.display()); - // Always "something moved": a checkout onto the commit - // that was already there is the one case this over-reports, - // and the cost of that is one build that had nothing to do. - // Under-reporting costs a checkout whose outputs stay from - // the commit before it, which is the silent kind. Ok(true) }, - may_build, - record, + None, ); } @@ -759,8 +771,7 @@ impl BuildState { fn after_moving( self: &Arc, move_it: impl FnOnce(&Arc) -> Result + Send + 'static, - may_build: impl Fn() -> bool + Send + 'static, - record: RecordBuilt, + then_build: BuildAfterMoving, ) { // Claimed before anything is spawned, and while nothing else is // running: one checkout, so a move is exclusive with every build @@ -783,14 +794,18 @@ impl BuildState { this.fail_pull(error); } Ok(pulled) => { - // Nothing configured to build, or nothing allowed to: - // the pull was the whole job. + // Nothing configured to build, nothing allowed to, or + // a move that builds nothing by definition: the move + // was the whole job. // `may_build()` is called here, with the pulled // declaration on disk, for the reason in the doc // comment above. Both it and `is_stale` take the lock // themselves, so they are asked before it is held. - let build = - may_build() && this.has_command() && (pulled || this.is_stale(None)); + let build = then_build + .as_ref() + .is_some_and(|(may_build, _)| may_build()) + && this.has_command() + && (pulled || this.is_stale(None)); // Handing the run over in one step, so nothing can // observe the moment between the pull ending and the // builds it decided on starting -- see `claim`. @@ -804,7 +819,9 @@ impl BuildState { inner.phase = None; claimed }; - this.run_claimed(claimed, &record); + if let Some((_, record)) = then_build { + this.run_claimed(claimed, &record); + } } } }); @@ -813,7 +830,10 @@ impl BuildState { /// Fetches and fast-forwards, reporting whether anything arrived. fn pull(&self, force: bool) -> Result { self.begin_project("fetching"); - crate::git::fetch(&self.project_path, self.git_ipv4)?; + // No prune: taking commits is not the moment to decide which + // remote branches still exist, and the Fetch button is what asks + // that question. + crate::git::fetch(&self.project_path, self.git_ipv4, false)?; // Counted after the fetch, from refs now on disk: this is the one // place that has the objects to count against. let behind = crate::git::behind(&self.project_path); @@ -2010,6 +2030,57 @@ mod tests { ); } + /// Moving the checkout runs git and nothing else. + /// + /// It used to build whatever the move left behind, which made looking + /// at another branch cost a full build of every component -- and there + /// is no way to call one back from a phone. What replaces it is the + /// card saying the component no longer matches the checkout, with its + /// own Build beside it. Asserted by giving the components a command + /// that leaves a file, so "it did not build" is a fact about the disk + /// rather than about a status word that could be read too early. + #[tokio::test] + async fn moving_the_checkout_builds_nothing() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let mut components = two_component_checkout(root); + for component in &mut components { + let marker = format!("{}-built", component.name()); + match component { + Component::Apk { build, .. } | Component::Server { build, .. } => { + *build = crate::config::ByMode::One(crate::config::Command::from_line( + &format!("touch ../{marker}"), + )); + } + } + } + run(root, &["git", "checkout", "-qb", "elsewhere"]); + std::fs::write(root.join("app/main.kt"), "two").expect("write"); + run(root, &["git", "commit", "-qam", "two"]); + run(root, &["git", "checkout", "-q", "main"]); + + let state = state_for(root, components); + state.move_checkout("elsewhere".to_string()); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while state.status().building { + assert!(std::time::Instant::now() < deadline, "the move never ended"); + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + + assert_eq!( + crate::git::status(root).expect("a checkout").branch, + "elsewhere", + "the checkout really moved, so the rest of this is about a move that happened", + ); + for marker in ["backend-built", "app-built"] { + assert!( + !root.join(marker).exists(), + "{marker} exists, so moving the checkout built something", + ); + } + } + /// The shape of bug a unit test that only calls `component_is_stale` /// cannot see: `status()` and `trigger_if_needed` hold the lock that /// the staleness check also wants. Taking it twice on one thread is a diff --git a/server/src/git.rs b/server/src/git.rs index bf79df6..3fba9fc 100644 --- a/server/src/git.rs +++ b/server/src/git.rs @@ -200,11 +200,22 @@ pub fn has_new_commits(project: &Path, ipv4: bool) -> Result { /// than wait for a person who isn't there. Then the whole thing is killed /// at [`REMOTE_HARD_TIMEOUT`] regardless, so nothing accumulates when a /// remote is merely very slow. -pub fn fetch(project: &Path, ipv4: bool) -> Result<(), String> { +/// +/// `prune` drops remote-tracking refs for branches the remote no longer +/// has. Only the Fetch button asks for it, because that is the one call +/// whose job is to make the branch picker say what the remote says: a +/// deleted branch left behind is one the picker offers forever, and +/// checking it out makes a local branch tracking something gone. A pull +/// does not, because pruning is not part of taking commits and a pull +/// should change as little as it can. +pub fn fetch(project: &Path, ipv4: bool, prune: bool) -> Result<(), String> { // `fetch` is one of the few git subcommands that takes the flag // itself, and that is what carries the preference to an https remote. // An ssh remote is covered by `ssh_command` either way. let mut args = vec!["fetch", "--quiet"]; + if prune { + args.push("--prune"); + } if ipv4 { args.insert(1, "-4"); } @@ -577,10 +588,47 @@ pub fn checkout(project: &Path, target: &str) -> Result<(), PullError> { // `--` so a branch and a path that share a name cannot be confused, // which is the one way a name from a phone could mean something other // than what it says. - git(project, &["checkout", target, "--"]).map_err(PullError::from)?; + let mut args: Vec<&str> = vec!["checkout"]; + let starting; + if let Some(local) = local_branch_for(project, target) { + starting = local; + args.extend(["-b", &starting, "--track"]); + } + args.extend([target, "--"]); + git(project, &args).map_err(PullError::from)?; Ok(()) } +/// The local branch to start when `target` names a remote-tracking ref +/// that has none, or `None` when it is anything else. +/// +/// `git checkout origin/topic` **detaches HEAD** at that commit: the DWIM +/// that creates a tracking branch fires on the bare name `topic` and +/// nothing else. So the branch picker's remote-only entries -- which are +/// named `origin/topic`, because that is what they are -- checked out to +/// no branch at all, with no upstream and so no Pull: exactly the parked +/// state picking a *commit* produces, from a control that says it is +/// picking a branch. Measured against a real clone rather than read; +/// `git checkout` prints its detached-HEAD advice and succeeds, so +/// nothing about the result looks like a failure. +/// +/// A local branch of the same name already existing is not this case -- +/// [`branches`] does not offer the remote one then -- but if it somehow +/// arrives, `-b` would fail on a name that is already taken, so it is +/// left to the plain checkout, where it means the same branch. +fn local_branch_for(project: &Path, target: &str) -> Option { + if !ref_exists(project, &format!("refs/remotes/{target}")) { + return None; + } + let (_remote, branch) = target.split_once('/')?; + (!ref_exists(project, &format!("refs/heads/{branch}"))).then(|| branch.to_string()) +} + +/// Whether a fully-qualified ref is in this checkout. +fn ref_exists(project: &Path, refname: &str) -> bool { + git(project, &["rev-parse", "--verify", "--quiet", refname]).is_ok_and(|sha| !sha.is_empty()) +} + /// Fast-forwards the current branch onto its upstream. /// /// `--ff-only` deliberately: a merge or a rebase can conflict, and @@ -860,6 +908,41 @@ mod tests { ); } + /// Picking a branch that only exists on the remote has to leave the + /// checkout *on a branch*, with an upstream. + /// + /// `git checkout origin/topic` detaches HEAD instead, which is the + /// same state picking a commit produces -- no branch on the card, no + /// Pull -- reached from the control that says it is picking a branch. + /// The picker names these entries `origin/topic` because that is what + /// they are, so this is the ordinary path rather than an edge. + #[test] + fn a_branch_only_on_the_remote_is_checked_out_as_a_branch() { + let dir = tempfile::tempdir().expect("tempdir"); + let (origin, clone) = origin_and_clone(dir.path()); + run(&origin, &["git", "branch", "topic"]); + run(&clone, &["git", "fetch", "-q"]); + + checkout(&clone, "origin/topic").expect("checking out a remote-only branch"); + + let moved = status(&clone).expect("a checkout"); + assert_eq!(moved.branch, "topic", "left on no branch: {moved:?}"); + assert_eq!( + moved.upstream.as_deref(), + Some("origin/topic"), + "a branch with no upstream cannot be pulled", + ); + + // And the ordinary cases still go through unchanged: a local + // branch by name, and a commit, which is the one that is *meant* + // to detach. + checkout(&clone, "main").expect("checking out a local branch"); + assert_eq!(status(&clone).expect("a checkout").branch, "main"); + let head = recent_commits(&clone, 1)[0].short.clone(); + checkout(&clone, &head).expect("checking out a commit"); + assert_eq!(status(&clone).expect("a checkout").branch, DETACHED); + } + /// What the settings sheet's two pickers are built from, and the one /// thing about them that is easy to get wrong: `origin/main` must not /// be offered beside `main`, because picking it would detach HEAD @@ -1143,7 +1226,7 @@ mod tests { std::fs::write(origin.join("file"), "two").expect("write"); run(&origin, &["git", "commit", "-qam", "two"]); - fetch(&clone, true).expect("fetch with ipv4 forced"); + fetch(&clone, true, false).expect("fetch with ipv4 forced"); pull(&clone, true, false).expect("pull with ipv4 forced"); assert!(!has_new_commits(&clone, true).expect("check")); } @@ -1193,7 +1276,7 @@ mod tests { ); // Pull is what actually takes them. - fetch(&clone, false).expect("fetch"); + fetch(&clone, false, false).expect("fetch"); assert_eq!(behind(&clone), 1); pull(&clone, false, false).expect("pull"); assert!(!has_new_commits(&clone, false).expect("check")); @@ -1221,7 +1304,7 @@ mod tests { std::fs::write(origin.join("file"), "two").expect("write"); run(&origin, &["git", "commit", "-qam", "two"]); - fetch(&clone, false).expect("fetch"); + fetch(&clone, false, false).expect("fetch"); assert!( has_new_commits(&clone, false).expect("check"), "fetched but not merged is still something to pull", @@ -1330,7 +1413,7 @@ mod tests { let (origin, clone) = origin_and_clone(dir.path()); std::fs::write(origin.join("file"), "two").expect("write"); run(&origin, &["git", "commit", "-qam", "two"]); - fetch(&clone, false).expect("fetch"); + fetch(&clone, false, false).expect("fetch"); pull(&clone, false, false).expect("pull"); assert_eq!(behind(&clone), 0); @@ -1348,7 +1431,7 @@ mod tests { let (origin, clone) = origin_and_clone(dir.path()); std::fs::write(origin.join("file"), "two").expect("write"); run(&origin, &["git", "commit", "-qam", "two"]); - fetch(&clone, false).expect("fetch"); + fetch(&clone, false, false).expect("fetch"); std::fs::write(clone.join("file"), "local edit").expect("write"); assert!(status(&clone).expect("status").dirty); @@ -1398,7 +1481,7 @@ mod tests { std::fs::write(origin.join("file"), "two").expect("write"); run(&origin, &["git", "commit", "-qam", "two"]); - fetch(&clone, false).expect("fetch"); + fetch(&clone, false, false).expect("fetch"); let err = pull(&clone, false, false).expect_err("no fast-forward exists"); assert!(err.unrelated_histories, "{}", err.message); @@ -1417,7 +1500,7 @@ mod tests { 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"); + fetch(&clone, false, false).expect("fetch"); let err = pull(&clone, false, false).expect_err("diverged"); assert!(!err.unrelated_histories, "{}", err.message); diff --git a/server/src/routes.rs b/server/src/routes.rs index 7efa544..65edb34 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -103,6 +103,10 @@ pub fn tls_router(state: Arc) -> Router { // both of these spawn git, and the manifest is fetched on every // open, resume and Refresh. .route("/apps/{key}/refs", get(checkout_refs)) + // Beside them, and the only one of the three that uses the + // network: it is what brings a branch pushed from somewhere else + // into the list the other two read. + .route("/apps/{key}/fetch", post(fetch_refs)) .route("/apps/{key}/checkout", post(build_checkout)) .route("/apps/{key}/prepare", post(build_prepare)) .route("/apps/{key}/build", post(build_now)) @@ -1607,13 +1611,60 @@ async fn checkout_refs( ) -> Result, ApiError> { let entry = lookup(&state, Some(key))?; let project = entry.project_path.clone(); - let refs = tokio::task::spawn_blocking(move || RefsResponse { - branches: crate::git::branches(&project), - commits: crate::git::recent_commits(&project, COMMIT_HISTORY), - head: crate::git::head_branch(&project), + let refs = tokio::task::spawn_blocking(move || read_refs(&project)) + .await + .context("reading the checkout's refs panicked")?; + Ok(Json(refs)) +} + +/// The three local reads behind [`checkout_refs`], in one place because +/// [`fetch_refs`] answers with the same thing and the two must not come +/// to describe a checkout differently. +fn read_refs(project: &std::path::Path) -> RefsResponse { + RefsResponse { + branches: crate::git::branches(project), + commits: crate::git::recent_commits(project, COMMIT_HISTORY), + head: crate::git::head_branch(project), + } +} + +/// Updates this checkout's remote-tracking refs, and answers with the +/// refs that leaves. +/// +/// The one thing here that reaches the network on somebody's press rather +/// than on a timer, and the only way a branch pushed from another machine +/// ever reaches the picker: nothing else this server runs writes into +/// `refs/remotes` except the fetch inside a pull, and a pull is only +/// reachable when the branch you are already on is behind. So a project +/// whose current branch is up to date could never be moved onto a new +/// branch at all -- the way out was a terminal on the build machine, +/// which is where the person holding the phone is not. +/// +/// Deliberately still not on the manifest, and still not on a timer. A +/// fetch mutates the checkout and pulls down objects, so it stays +/// something somebody asked for -- which also means the sheet's own read +/// (`checkout_refs`) keeps being a local one that cannot stall. +/// +/// It answers with the whole ref list rather than a bare acknowledgement, +/// so the pickers repopulate from *after* the fetch in the same round +/// trip. Two calls would leave a window where the button had finished and +/// the list was still the old one. +async fn fetch_refs( + State(state): State>, + key: UrlPath, +) -> Result, ApiError> { + let entry = lookup(&state, Some(key))?; + let project = entry.project_path.clone(); + let ipv4 = entry.git_ipv4; + let refs = tokio::task::spawn_blocking(move || { + crate::git::fetch(&project, ipv4, true).map(|()| read_refs(&project)) }) .await - .context("reading the checkout's refs panicked")?; + .context("fetching panicked")? + // Already shortened and explained by `git::remote_failure`, and a 500 + // here answers with its message -- so what reaches the card is git's + // first line plus whatever this process can see of the ssh agent. + .map_err(|err| ApiError::Internal(anyhow::anyhow!(err)))?; Ok(Json(refs)) } @@ -1626,15 +1677,14 @@ struct CheckoutBody { target: String, } -/// Moves the checkout onto a branch or a commit, then builds what that -/// left behind -- reported through the same status a pull is, because it -/// is the same kind of act on the same single checkout. +/// Moves the checkout onto a branch or a commit -- reported through the +/// same status a pull is, because it is the same kind of act on the same +/// single checkout. /// -/// Available while a declaration is waiting to be accepted, for the -/// reason a pull is: moving the checkout runs git rather than the -/// project's command, and it is how a different version of that request -/// arrives to be read. What it must not do is *build* in that state, -/// which is why the gate is asked afterwards, on the tree the move left. +/// It builds nothing; see [`BuildState::move_checkout`]. That is also +/// what makes the acceptance gate a pull needs irrelevant here: nothing +/// this runs is the project's own command, so there is nothing to ask +/// permission for. async fn build_checkout( State(state): State>, key: UrlPath, @@ -1651,12 +1701,7 @@ async fn build_checkout( .build .as_ref() .ok_or_else(|| ApiError::NoBuildStep(entry.label.clone()))?; - let gate = Arc::clone(&entry); - build.checkout_and_build( - target, - move || gate.pending_declaration().is_none(), - records_builds(&state, &entry.key), - ); + build.move_checkout(target); Ok(Json(build.status())) }