diff --git a/AGENTS.md b/AGENTS.md index 958bb91..45c5c39 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -340,6 +340,58 @@ 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. + 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. + 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** + that is not a branch and cannot be checked out, so the list comes from + `for-each-ref refs/heads`; and `refs/remotes/origin/HEAD` abbreviates to + a bare **`origin`**, so the obvious filter for a name ending in `/HEAD` + matches nothing and a phantom branch called `origin` reaches the phone. + It is dropped by *being* a symref (`%(symref)` non-empty) instead. The + test that was meant to cover the second asserted the same wrong thing + and passed. + +- **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 + `git::pull` refuses on any dirt at all. With the strict check, moving + back to a commit from before the `.gitignore` that covers this + project's build output leaves that output sitting there untracked, the + tree is dirty, and every move afterwards is refused -- you can go back + and you cannot come forward, from a phone, with the way out being the + build machine. Nothing is given up by relaxing it, because git makes + the better check itself: `git checkout` refuses when an untracked file + would be overwritten and carries across the ones that would not, and + its refusal arrives as the error the card shows. So this guard covers + work somebody typed and git's covers the files it would clobber. + Found by moving a real checkout back and forth rather than by reading + the code; the first version passed its tests and trapped the checkout + on the second move. + +- **A detached HEAD is a state the card has to say out loud.** Picking a + commit leaves the checkout on no branch, `git::status` reports the + branch as the literal `HEAD`, and `can_pull` correctly goes false + because there is no upstream. The card draws that as **"no branch"** + rather than `HEAD`, which beside a branch icon reads as a branch + somebody named HEAD -- worth doing now that the sheet can produce the + state, where before it was only reachable on the build machine. + - **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 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 beb6b06..6fd4d67 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt @@ -164,6 +164,58 @@ fun setAppSettings(key: String, gitIpv4: Boolean) { ) {} } +/** One branch this project's checkout could be moved to. */ +data class GitBranch(val name: String, val current: Boolean, val remoteOnly: Boolean) + +/** One commit it could be moved to. [sha] is what gets sent; [short] is what gets read. */ +data class GitCommit( + val sha: String, + val short: String, + val subject: String, + val at: Long, + val current: Boolean, +) + +/** What the project settings sheet's two pickers are built from. */ +data class CheckoutRefs(val branches: List, val commits: List) + +/** + * The branches and recent commits of one project's checkout. + * + * Fetched when the settings sheet is opened rather than carried on the manifest: reading them + * spawns git on the build machine, and the manifest is fetched on every open, resume and Refresh. + * Local reads only there, so this cannot stall on a network round trip -- what the remote has and + * 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), + ) + }, + ) + } + /** One component's log, as the modal shows it. */ /** * Which of a component's two logs to read. 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 5f7d4fc..004e637 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt @@ -74,8 +74,12 @@ data class BuildProgressCount(val done: Long, val total: Long) // answers both routes immediately either way -- a rebuild it kicks off runs // in a background thread there, not inline with the request -- so neither // needs the multi-minute read timeout an actual build would. -private fun requestBuildStatus(path: String, method: String): BuildStatus = - requestFromServer(path, method) { connection -> +private fun requestBuildStatus( + path: String, + method: String, + jsonBody: String? = null, +): BuildStatus = + requestFromServer(path, method, jsonBody = jsonBody) { connection -> val json = JSONObject(connection.inputStream.bufferedReader().readText()) val components = json.optJSONArray("components") BuildStatus( @@ -119,6 +123,22 @@ private fun requestBuildStatus(path: String, method: String): BuildStatus = fun pullAndBuild(key: String, force: Boolean = false): BuildStatus = requestBuildStatus("/apps/$key/pull" + if (force) "?force=true" else "", "POST") +/** + * Moves the checkout onto a branch or a commit on the build machine, then builds what that left + * behind. + * + * Answers a build status like a pull does, and is here beside it rather than with the other + * management calls for exactly that reason: it is the same act on the same single checkout, so it + * reports through the one progress path the card already polls instead of a second one to keep in + * step. + */ +fun checkoutTarget(key: String, target: String): BuildStatus = + requestBuildStatus( + "/apps/$key/checkout", + "POST", + jsonBody = JSONObject().put("target", target).toString(), + ) + /** * Builds one component if its staleness rule says so, before it is downloaded. * 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 8c5d93d..db86484 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -881,6 +881,23 @@ private fun AppListScreen( } } + /** + * Moves this project's checkout onto a branch or a commit on the build machine, and builds what + * that leaves behind. + * + * 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. + */ + fun startCheckout(entry: ManifestEntry, target: String) { + scope.launch { + followBuild( + entry, + start = { checkoutTarget(entry.key, target) }, + progress = { ProjectState.Pulling(it) }, + ) + } + } + /** * Build because somebody asked, with nothing to pull and nothing looking stale -- which is the * only way a project already current with its checkout ever records what it was built from. @@ -1393,6 +1410,7 @@ private fun AppListScreen( onSettings = { gitIpv4 -> manage(entry) { setAppSettings(entry.key, gitIpv4) } }, + onCheckout = { target -> startCheckout(entry, target) }, onApprove = { manage(entry) { approveDeclaration(entry.key) } }, @@ -1553,6 +1571,8 @@ private fun AppCard( onRebuild: () -> Unit, onRefresh: () -> Unit, onSettings: (gitIpv4: Boolean) -> Unit, + /** Move this project's checkout onto a branch or commit, and build what that leaves behind. */ + onCheckout: (target: String) -> Unit, onApprove: () -> Unit, onRemove: () -> Unit, onSelectVariant: (component: String, ApkVariant?) -> Unit, @@ -1665,7 +1685,18 @@ private fun AppCard( Spacer(Modifier.width(4.dp)) Text( buildString { - append(git.branch) + // git answers `HEAD` when no branch + // is checked out, which beside a + // branch icon reads as a branch + // somebody named HEAD. Worth saying + // in words now that the settings + // sheet can put a checkout in that + // state -- before this it was only + // reachable on the build machine. + append( + if (git.branch == DETACHED_HEAD) "no branch" + else git.branch + ) // Not "new commits", which the colour // says. And not the lack of an // upstream either: plenty of @@ -2040,6 +2071,14 @@ private fun AppCard( if (settingsOpen) { ProjectSettingsDialog( + // Nothing can be moved while the checkout is being worked on: + // the server refuses it, and offering it anyway would be a + // control whose only answer is a refusal. + busy = projectState.busy, + onCheckout = { target -> + settingsOpen = false + onCheckout(target) + }, entry = entry, onDismiss = { settingsOpen = false }, onApply = { gitIpv4 -> @@ -2065,6 +2104,9 @@ private fun AppCard( @Composable private fun ProjectSettingsDialog( entry: ManifestEntry, + /** Something is already running on this checkout, so it must not be moved. */ + busy: Boolean, + onCheckout: (target: String) -> Unit, onDismiss: () -> Unit, onApply: (gitIpv4: Boolean) -> Unit, ) { @@ -2076,7 +2118,7 @@ private fun ProjectSettingsDialog( onDismissRequest = onDismiss, title = { Text(entry.label) }, text = { - Column { + Column(Modifier.verticalScroll(rememberScrollState())) { Row( verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth(), @@ -2089,6 +2131,8 @@ private fun ProjectSettingsDialog( Spacer(Modifier.width(12.dp)) Switch(checked = gitIpv4, onCheckedChange = { gitIpv4 = it }) } + Spacer(Modifier.height(16.dp)) + CheckoutSection(entry = entry, busy = busy, onCheckout = onCheckout) } }, confirmButton = { TextButton(onClick = { onApply(gitIpv4) }) { Text("Save") } }, @@ -2096,6 +2140,188 @@ private fun ProjectSettingsDialog( ) } +/** + * Where this project's checkout is, and the two ways to move it. + * + * Both pickers act on the press rather than waiting for Save, and the sheet closes behind them: + * moving the checkout is not a setting, it is a job that starts on the build machine and reports in + * the project's own row like a pull. Save is left meaning what it meant, which is the switch above. + * + * Reading the refs is a round trip, so it happens when the sheet opens rather than riding on the + * manifest -- listing branches and commits spawns git, and the manifest is fetched on every open, + * resume and Refresh. It is deliberately a *local* read on that side, so this cannot sit waiting on + * a network round trip before the sheet is usable. + * + * Every reason the pickers cannot be used is said rather than left to be inferred from a control + * 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. + */ +@Composable +private fun CheckoutSection( + entry: ManifestEntry, + busy: Boolean, + onCheckout: (target: String) -> Unit, +) { + var refs by remember(entry.key) { mutableStateOf(null) } + var failure by remember(entry.key) { mutableStateOf(null) } + val git = entry.git + + // Only for a project actually in a checkout; asking git about a + // directory that is not one would fail for a reason that is not a + // fault. + if (git != null) { + LaunchedEffect(entry.key) { + try { + refs = withContext(Dispatchers.IO) { checkoutRefs(entry.key) } + } catch (e: DownloadServerException) { + failure = e.message ?: "couldn't read this checkout's branches" + } + } + } + + SettingsHeading("Checkout") + when { + git == null -> { + SettingsNote("This project is not in a git repository, so there is nothing to move.") + return + } + // Said before the pickers are drawn, because it is the reason + // they will refuse rather than something to discover by pressing. + git.dirty -> + SettingsNote( + "The checkout has uncommitted changes. Moving it would go over them, so commit " + + "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 leaves it on no branch until a branch " + + "is picked again." + ) + } + + val enabled = !busy && !git.dirty && refs != null + 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)) + BranchPicker( + branches = loaded?.branches.orEmpty(), + // The branch line on the card is the same answer, and it is + // the one already on screen -- taking it from there rather + // than working one out keeps the two from disagreeing. + current = git.branch, + enabled = enabled, + onSelect = onCheckout, + ) + } + Row(verticalAlignment = Alignment.CenterVertically, modifier = Modifier.fillMaxWidth()) { + Text("Commit", style = MaterialTheme.typography.bodyLarge, modifier = Modifier.weight(1f)) + CommitPicker( + commits = loaded?.commits.orEmpty(), + enabled = enabled, + onSelect = onCheckout, + ) + } +} + +@Composable +private fun BranchPicker( + branches: List, + current: String, + enabled: Boolean, + onSelect: (String) -> Unit, +) { + var expanded by remember { mutableStateOf(false) } + Box { + TextButton(onClick = { expanded = true }, enabled = enabled && branches.isNotEmpty()) { + // "HEAD" is what git says when no branch is checked out. Said + // in words here, because a reader has no way to know that the + // literal string is a state rather than a branch somebody + // made. + Text( + if (current == DETACHED_HEAD) "none" else current, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + } + DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) { + branches.forEach { branch -> + DropdownMenuItem( + text = { + Text( + buildString { + append(branch.name) + if (branch.current) append(" ✓") + // Worth saying: moving to one of these + // starts a local branch rather than + // returning to one. + if (branch.remoteOnly) append(" (on the remote)") + } + ) + }, + onClick = { + expanded = false + if (!branch.current) onSelect(branch.name) + }, + ) + } + } + } +} + +@Composable +private fun CommitPicker(commits: List, enabled: Boolean, onSelect: (String) -> Unit) { + var expanded by remember { mutableStateOf(false) } + val here = commits.firstOrNull { it.current } + Box { + TextButton(onClick = { expanded = true }, enabled = enabled && commits.isNotEmpty()) { + Text(here?.short ?: "\u2014", maxLines = 1, overflow = TextOverflow.Ellipsis) + } + DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) { + commits.forEach { commit -> + DropdownMenuItem( + text = { + // The hash identifies it and the subject is what + // a person recognises, so both -- and the subject + // is the half that truncates, since a cut hash + // names nothing. + Row(verticalAlignment = Alignment.CenterVertically) { + Text( + commit.short, + style = MaterialTheme.typography.bodyMedium, + fontFamily = FontFamily.Monospace, + ) + Spacer(Modifier.width(8.dp)) + Text( + if (commit.current) "${commit.subject} ✓" else commit.subject, + style = MaterialTheme.typography.bodyMedium, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + } + }, + onClick = { + expanded = false + if (!commit.current) onSelect(commit.sha) + }, + ) + } + } + } +} + +/** What git reports as the branch when no branch is checked out. */ +private const val DETACHED_HEAD = "HEAD" + /** * How far along whatever is on its way to this phone is. * diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 1ffb7ab..d91cc2a 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -694,6 +694,58 @@ impl BuildState { may_build: impl Fn() -> bool + Send + 'static, record: RecordBuilt, ) { + self.after_moving(move |this| this.pull(force), may_build, record); + } + + /// Moves this checkout onto `target` -- a branch or a commit -- and + /// then builds whatever that left behind. + /// + /// 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, + ) { + 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, + ); + } + + /// The half a pull and a checkout share: run the thing that moves the + /// working tree, then build what it left behind. + /// + /// One copy, because the part that is easy to get wrong is not the + /// git command -- it is handing the run over under a single lock so + /// that nothing can observe the moment between the move ending and + /// the builds it decided on starting. A phone polling in that gap + /// sees a project that is neither pulling nor building and calls the + /// run finished. + fn after_moving( + self: &Arc, + move_it: impl FnOnce(&Arc) -> Result + Send + 'static, + may_build: impl Fn() -> bool + Send + 'static, + record: RecordBuilt, + ) { + // Claimed before anything is spawned, and while nothing else is + // running: one checkout, so a move is exclusive with every build + // as well as with another move. { let mut inner = self.inner.lock().unwrap(); if inner.anything_running() { @@ -703,13 +755,12 @@ impl BuildState { inner.error = None; inner.unrelated_histories = false; } - let this = Arc::clone(self); tokio::task::spawn_blocking(move || { - let outcome = this.pull(force); + let outcome = move_it(&this); match outcome { Err(error) => { - tracing::error!("pull failed: {}", error.message); + tracing::error!("moving the checkout failed: {}", error.message); this.fail_pull(error); } Ok(pulled) => { diff --git a/server/src/git.rs b/server/src/git.rs index 320c714..adfb205 100644 --- a/server/src/git.rs +++ b/server/src/git.rs @@ -300,6 +300,215 @@ impl From for PullError { } } +/// A branch this checkout could be moved to. +#[derive(Debug, Clone, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub struct Branch { + /// What to hand [`checkout`], and what a person reads: `main`, or + /// `origin/topic` for one that exists only on the remote. + pub name: String, + /// This is the branch that is checked out. False for every branch + /// while HEAD is detached, which is a state the sheet has to be able + /// to draw rather than pick a branch to call current. + pub current: bool, + /// It exists only as a remote-tracking ref, so moving to it creates a + /// local branch. Said on the phone because it is the difference + /// between returning to a branch and starting one. + pub remote_only: bool, +} + +/// One commit, as a line to pick from. +#[derive(Debug, Clone, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub struct CommitSummary { + /// The full hash, which is what [`checkout`] is given -- an + /// abbreviation is for reading, and can become ambiguous as the + /// repository grows. + pub sha: String, + /// The abbreviation to show, from git rather than by truncating the + /// hash here: git widens it when a repository needs more characters, + /// and a fixed slice would eventually name two commits. + pub short: String, + /// The first line of the message. + pub subject: String, + /// Author date, epoch seconds, for the phone to render in its own + /// locale rather than this machine's. + pub at: i64, + /// This is where HEAD is. What the sheet marks as selected, and the + /// reason it never has to guess from a position in the list. + pub current: bool, +} + +/// Every branch worth offering: the local ones, plus remote-tracking refs +/// with no local branch of the same name. +/// +/// A remote-only branch is included because "switch to the branch I +/// pushed from the other machine" is most of what this is for, and +/// leaving it out would mean the only way to reach it is a terminal on +/// the build machine -- which is exactly where the person holding the +/// phone is not. +/// +/// Never on the manifest path: this spawns git twice and is read when the +/// settings sheet is opened, which is somebody asking. +pub fn branches(project: &Path) -> Vec { + let head = status(project).map(|status| status.branch); + // Detached HEAD reports the literal `HEAD`, which is not a branch + // name, so nothing matches it and every branch reads as not current. + let current = head.filter(|branch| branch != "HEAD"); + + let mut branches: Vec = Vec::new(); + // `for-each-ref refs/heads` rather than `git branch`, which adds a + // pseudo-entry for the detached state -- `(HEAD detached at cfdce91)` + // reached the phone as a branch you could try to check out. Found by + // detaching a real checkout and looking at the list, which is the + // state the feature exists to produce and so the one to look at. + if let Ok(out) = git( + project, + &["for-each-ref", "--format=%(refname:short)", "refs/heads"], + ) { + for name in out.lines().map(str::trim).filter(|name| !name.is_empty()) { + branches.push(Branch { + current: current.as_deref() == Some(name), + name: name.to_string(), + remote_only: false, + }); + } + } + if let Ok(out) = git( + project, + &[ + "for-each-ref", + // A space, because `for-each-ref` does not expand `%xx` the + // way `log` does -- and a ref name cannot contain one, so + // there is nothing for it to split by mistake. + "--format=%(refname:short) %(symref)", + "refs/remotes", + ], + ) { + for line in out.lines().map(str::trim).filter(|line| !line.is_empty()) { + let (name, symref) = line.split_once(' ').unwrap_or((line, "")); + // `refs/remotes/origin/HEAD` is a symbolic ref at whatever the + // remote calls its default branch, and offering it would offer + // that branch a second time. Detected by *being* a symref + // rather than by its name, because `%(refname:short)` + // abbreviates it to a bare `origin` -- so the obvious check for + // a name ending in `/HEAD` matches nothing, and a phantom + // branch called `origin` is what reaches the phone. Found by + // running it against a real clone; the test that was supposed + // to cover it asserted the same wrong thing and passed. + if !symref.is_empty() { + continue; + } + // `origin/main` when `main` is already local is the same + // branch to a reader, and picking the remote one would detach + // from the branch they are on. + let local = name.split_once('/').map(|(_, rest)| rest).unwrap_or(name); + if branches.iter().any(|branch| branch.name == local) { + continue; + } + branches.push(Branch { + name: name.to_string(), + current: false, + remote_only: true, + }); + } + } + branches +} + +/// The most recent `limit` commits reachable from HEAD, newest first. +/// +/// Local history only. What the remote has and this checkout has not +/// fetched is not offered: Pull is what brings those in, and the card +/// already says when there are some -- so this cannot stall on a network +/// round trip, and opening the sheet cannot fail the way a remote check +/// can. +pub fn recent_commits(project: &Path, limit: usize) -> Vec { + // A record separator that cannot occur in a subject, so a message + // containing anything at all still parses. `%x1f` is the ASCII unit + // separator; git writes it literally. + let format = "--format=%H%x1f%h%x1f%at%x1f%s"; + let head = git(project, &["rev-parse", "HEAD"]) + .ok() + .map(|sha| sha.trim().to_string()); + let Ok(out) = git( + project, + &["log", &format!("-n{limit}"), format, "--no-color"], + ) else { + return Vec::new(); + }; + out.lines() + .filter_map(|line| { + let mut parts = line.splitn(4, '\u{1f}'); + let sha = parts.next()?.to_string(); + let short = parts.next()?.to_string(); + let at = parts.next()?.parse().unwrap_or(0); + // A commit with an empty subject is legal, so the last field + // is allowed to be missing rather than dropping the commit. + let subject = parts.next().unwrap_or_default().to_string(); + Some(CommitSummary { + current: head.as_deref() == Some(sha.as_str()), + sha, + short, + at, + subject, + }) + }) + .collect() +} + +/// Moves this checkout onto `target`, a branch name or a commit. +/// +/// Refuses a dirty tree for the same reason [`pull`] does, and it matters +/// more here: a checkout can move *backwards*, so the work it would +/// clobber may not exist anywhere else yet. The failure leaves the +/// working tree exactly as it was. +/// +/// A remote-only branch is checked out by name, which git resolves to a +/// new local branch tracking it -- the "switch to what I pushed from the +/// other machine" case. A commit detaches HEAD, which is a real state the +/// card already draws: `status` reports the branch as `HEAD`, `can_pull` +/// goes false because there is no upstream, and the Pull button correctly +/// stops being offered until a branch is chosen again. +pub fn checkout(project: &Path, target: &str) -> Result<(), PullError> { + if status(project).is_none() { + return Err("not a git repository".to_string().into()); + } + // **Tracked** modifications only, unlike the check in front of a pull. + // + // The difference is not a relaxation for its own sake; the strict + // version makes this feature a one-way door. Moving back to a commit + // from before a `.gitignore` existed -- or one that ignored a + // different directory -- leaves the build output this server just + // produced sitting there *untracked*, so the tree is dirty and every + // move afterwards is refused. You can go back and you cannot come + // forward, which is exactly the state there is no way out of from a + // phone. + // + // What is given up is nothing, because git makes the same check + // better: `git checkout` refuses when an untracked file would be + // overwritten, and carries across the ones that would not. So this + // guard covers uncommitted work somebody typed, and git's covers the + // files it would clobber -- with git's refusal arriving as the error + // below rather than as anything this has to predict. + let dirty = git(project, &["status", "--porcelain", "--untracked-files=no"]) + .map(|out| !out.trim().is_empty()) + .unwrap_or(false); + if dirty { + return Err(format!( + "{} has uncommitted changes -- refusing to move the checkout over them. Commit or \ + stash on the build machine first.", + project.display() + ) + .into()); + } + // `--` 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)?; + Ok(()) +} + /// Fast-forwards the current branch onto its upstream. /// /// `--ff-only` deliberately: a merge or a rebase can conflict, and @@ -579,6 +788,142 @@ mod tests { ); } + /// 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 + /// from the branch somebody is already on while looking like a way + /// back to it. + #[test] + fn the_pickers_offer_each_branch_once_and_mark_where_head_is() { + let dir = tempfile::tempdir().expect("tempdir"); + let (origin, clone) = origin_and_clone(dir.path()); + // A branch that exists only on the remote, which is most of what + // switching branches from a phone is for. + run(&origin, &["git", "branch", "topic"]); + run(&clone, &["git", "fetch", "-q"]); + + let branches = branches(&clone); + let named: Vec<&str> = branches.iter().map(|b| b.name.as_str()).collect(); + assert!(named.contains(&"main"), "{named:?}"); + assert!( + !named.iter().any(|name| name.ends_with("/main")), + "origin/main duplicates the local branch: {named:?}", + ); + // `refs/remotes/origin/HEAD` abbreviates to a bare `origin`, so + // this is what its leaking looks like -- not a name ending in + // `/HEAD`, which is what an earlier version of this test looked + // for and never found. + assert!( + !named.contains(&"origin"), + "the remote's default symref must not be offered as a branch: {named:?}", + ); + let topic = branches + .iter() + .find(|b| b.name == "origin/topic") + .expect("a remote-only branch is offered"); + assert!(topic.remote_only); + assert!(!topic.current); + assert!( + branches + .iter() + .find(|b| b.name == "main") + .expect("main") + .current, + "the branch HEAD is on is the one marked current", + ); + + let commits = recent_commits(&clone, 10); + assert_eq!(commits.len(), 1); + assert_eq!(commits[0].subject, "one"); + assert!(commits[0].current, "HEAD's own commit is marked"); + assert!(!commits[0].short.is_empty()); + } + + /// The trap the tracked-only check exists for: build output left + /// untracked by an older commit -- one from before the `.gitignore` + /// that covers it -- must not make the way back impossible. Refusing + /// on any dirt at all made this a one-way door, and the way out of it + /// is the build machine, which is where the person holding the phone + /// is not. + #[test] + fn untracked_build_output_does_not_trap_the_checkout() { + let dir = tempfile::tempdir().expect("tempdir"); + let (_origin, clone) = origin_and_clone(dir.path()); + let first = recent_commits(&clone, 10)[0].sha.clone(); + std::fs::write(clone.join(".gitignore"), "out/\n").expect("write"); + run(&clone, &["git", "add", "."]); + run(&clone, &["git", "commit", "-qm", "ignore out"]); + + // Back to before the .gitignore, then "build" into the directory + // it would have covered. + checkout(&clone, &first).expect("moving back"); + std::fs::create_dir_all(clone.join("out")).expect("mkdir"); + std::fs::write(clone.join("out/app.apk"), "built").expect("write"); + assert!( + status(&clone).expect("status").dirty, + "untracked output really does make the tree dirty", + ); + + checkout(&clone, "main").expect("and the way forward is still open"); + assert_eq!(status(&clone).expect("status").branch, "main"); + assert!( + clone.join("out/app.apk").exists(), + "git carries an untracked file across rather than discarding it", + ); + } + + /// Moving the checkout is refused over uncommitted work, and it + /// matters more here than for a pull: a checkout can go *backwards*, + /// so what it would clobber may exist nowhere else yet. + #[test] + fn a_checkout_refuses_a_dirty_tree_and_leaves_it_alone() { + let dir = tempfile::tempdir().expect("tempdir"); + let (_origin, clone) = origin_and_clone(dir.path()); + std::fs::write(clone.join("file"), "two").expect("write"); + run(&clone, &["git", "commit", "-qam", "two"]); + let first = recent_commits(&clone, 10) + .last() + .expect("the first commit") + .sha + .clone(); + + std::fs::write(clone.join("file"), "uncommitted").expect("write"); + let refused = checkout(&clone, &first).expect_err("a dirty tree is refused"); + assert!( + refused.message.contains("uncommitted"), + "{}", + refused.message + ); + assert_eq!( + std::fs::read_to_string(clone.join("file")).expect("read"), + "uncommitted", + "the working tree is left exactly as it was", + ); + + // Clean again, and the same move goes through and detaches HEAD -- + // which `status` reports as `HEAD` rather than inventing a branch. + run(&clone, &["git", "checkout", "-q", "--", "file"]); + checkout(&clone, &first).expect("a clean tree moves"); + assert_eq!( + std::fs::read_to_string(clone.join("file")).expect("read"), + "one", + ); + assert_eq!(status(&clone).expect("status").branch, "HEAD"); + let detached = branches(&clone); + assert!( + detached.iter().all(|b| !b.current), + "detached, so no branch is the current one", + ); + // `git branch` adds a `(HEAD detached at ...)` line here, which is + // not a branch and cannot be checked out. Asserted in the detached + // case because that is the only state it appears in. + assert!( + detached.iter().all(|b| !b.name.starts_with('(')), + "the detached-HEAD pseudo-entry is not a branch: {:?}", + detached.iter().map(|b| &b.name).collect::>(), + ); + } + /// An origin repo with one commit, and a clone of it. fn origin_and_clone(root: &Path) -> (PathBuf, PathBuf) { let origin = root.join("origin"); diff --git a/server/src/routes.rs b/server/src/routes.rs index 971d52e..823eefc 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -13,6 +13,10 @@ //! GET /apps/{key}/apk[?variant=] the payload itself (ranged) //! POST /apps/{key}/pull fetch, fast-forward, and build //! ?force=true resets onto the upstream +//! GET /apps/{key}/refs its branches and recent commits, for +//! the settings sheet's two pickers +//! POST /apps/{key}/checkout {target} +//! move the checkout there, then build //! POST /apps/{key}/prepare run the on-demand build step, if any //! ?component= restricts it to one //! POST /apps/{key}/build run it whether or not it looks stale @@ -95,6 +99,11 @@ pub fn tls_router(state: Arc) -> Router { .route("/apps/{key}", get(app).delete(remove_app)) .route("/apps/{key}/apk", get(serve_apk)) .route("/apps/{key}/pull", post(build_pull)) + // Read when the settings sheet is opened, never on the manifest: + // both of these spawn git, and the manifest is fetched on every + // open, resume and Refresh. + .route("/apps/{key}/refs", get(checkout_refs)) + .route("/apps/{key}/checkout", post(build_checkout)) .route("/apps/{key}/prepare", post(build_prepare)) .route("/apps/{key}/build", post(build_now)) .route("/apps/{key}/approve", post(approve_declaration)) @@ -1542,6 +1551,85 @@ async fn build_pull( Ok(Json(build.status())) } +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct RefsResponse { + branches: Vec, + commits: Vec, +} + +/// How much history the commit picker offers. +/// +/// Enough to reach back past a bad afternoon, and short enough that the +/// list is still something a person scrolls rather than searches. Going +/// further back is a job for the build machine, where the tools for +/// finding a particular commit actually are. +const COMMIT_HISTORY: usize = 50; + +/// The branches and commits this project could be moved to. +/// +/// Local reads only, so opening the sheet cannot stall on a network round +/// trip or fail the way a remote check can. What the remote has and this +/// checkout has not fetched is deliberately not here: Pull is what brings +/// those in, and the card already says when there are some. +async fn checkout_refs( + State(state): State>, + key: UrlPath, +) -> 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), + }) + .await + .context("reading the checkout's refs panicked")?; + Ok(Json(refs)) +} + +#[derive(Deserialize)] +struct CheckoutBody { + /// A branch name or a commit hash, as the phone read it back from + /// [`checkout_refs`]. Never checked against that list here: git is + /// the thing that decides whether a ref exists, and a second opinion + /// on this side could only ever disagree with it. + 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. +/// +/// 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. +async fn build_checkout( + State(state): State>, + key: UrlPath, + Json(body): Json, +) -> Result, ApiError> { + let entry = lookup(&state, Some(key))?; + let target = body.target.trim().to_string(); + if target.is_empty() { + return Err(ApiError::BadRequest( + "say which branch or commit to move to".to_string(), + )); + } + let build = entry + .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), + ); + Ok(Json(build.status())) +} + async fn build_status( State(state): State>, key: UrlPath,