Fetch is a button, and moving the checkout builds nothing
Nothing here ever wrote a remote-tracking ref except the fetch 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 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.
POST /apps/{key}/fetch answers with the whole ref list, so the pickers
repopulate from after the fetch in one round trip. It is the only call
here that asks for --prune: making the picker say what the remote says is
its job, where a pull should change as little as it can. Still not on a
timer and not on opening the sheet, because a fetch mutates the checkout.
Picking a remote-only branch then had to work: `git checkout origin/topic`
detaches HEAD, since git's DWIM fires on the bare name, so the control
that says it is picking a branch produced the state picking a commit is
meant to produce -- and printed its detached-HEAD advice and succeeded.
Moving the checkout no longer builds. A pull is somebody taking new work;
a move is somebody looking, and charging a full build for a look starts a
minute of work an accidental tap cannot call back. What it leaves behind
is a component whose build no longer matches the checkout, so the build
button's word now follows the state: Rebuild only where every component
it covers is known current, Build otherwise.
The sheet loses its Checkout heading and its paragraph, both pickers sit
in a weighted row so a long branch name cannot wrap the label one letter
per line, and the failure text moved below them -- above, it shoved the
pickers down the screen as somebody reached for one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
f983db154d
commit
25c069909b
8 files changed
+507
-117
No files matched your search
+94
-23
@@ -60,6 +60,15 @@ pub enum Freshness {
|
||||
/// starts a build has the list and supplies this.
|
||||
pub type RecordBuilt = Arc<dyn Fn(&str, String) + Send + Sync>;
|
||||
|
||||
/// 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<dyn Fn() -> 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<Self>,
|
||||
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<Self>, 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<Self>,
|
||||
move_it: impl FnOnce(&Arc<Self>) -> Result<bool, crate::git::PullError> + 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<bool, crate::git::PullError> {
|
||||
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
|
||||
|
||||
Reference in new issue
Block a user