From 487beb50b45aed988666aaa5cf308219357b7406 Mon Sep 17 00:00:00 2001 From: Jochen Hunz Date: Wed, 16 Sep 2026 12:54:12 +0200 Subject: [PATCH] Let a commit land when it already carries the branch latest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sync that has to merge cannot finish: it stages a merge of the remote target with the local revision, commits it, and the commit is refused for the very divergence the merge resolves. The message tells the user to sync while they are inside a sync: Synchronizing with local changes failed to merge with remote revision: Branch has been advanced by another instance, sync and re-stage to commit The merge itself is fine — computed, staged, correct. Only landing it is blocked, and the only way out is `lore commit --force` by hand. The check is written as a pointer comparison: if !globals.force() && !branch_latest.is_zero() && branch_latest != current_revision { return Err(BranchAdvanced.into()); } but the rule it stands for is "do not drop what another instance added". An anchor equal to the latest is the cheapest way to satisfy that, not the only one: a staged state that already has the latest in its ancestry publishes that work rather than replacing it. Git draws the same line in its push path — a merge commit containing the remote tip is accepted, because reachability, not pointer equality, is the safety property. So when the cheap comparison fails, ask the real question before refusing. The two parents answer it outright; otherwise walk first parents back from the staged state's own parent, stopping once revision numbers fall below the latest's. The shape this exists for — sync staging a merge of a target that descends from the latest — resolves in one or two steps, and a plain commit on a stale anchor aborts immediately. Nothing changes for a commit whose anchor is the latest: the walk is reached only where the code previously returned an error. It fails closed. An unreadable state, a walk past its bound, a history this clone only partly holds — all answer "no" and the commit is refused. A gap costs a refusal the user resolves by syncing; the opposite mistake costs somebody's work. Two tests define the boundary, with identical pointers and only the staged state differing: one whose ancestry reaches the latest two steps back (the sync-merge shape) now commits, one built on the stale anchor is still refused. Reverting the check turns the first red and leaves the second green. Verified end to end on a repository that was actually stuck this way: the sync now completes on its own, anchor and branch latest meet on the new merge revision, nothing staged is left behind, and both sides' files are present — without `--force`. cargo test -p lore-revision: 1061 passed. Smoke suite against release binaries: 1009 passed, 0 failed. clippy and nightly fmt clean. Signed-off-by: Jochen Hunz --- lore-revision/src/commit.rs | 91 ++++++++++++- lore-revision/tests/commit.rs | 248 ++++++++++++++++++++++++++++++++++ 2 files changed, 338 insertions(+), 1 deletion(-) diff --git a/lore-revision/src/commit.rs b/lore-revision/src/commit.rs index 256f58b1..e69c050b 100644 --- a/lore-revision/src/commit.rs +++ b/lore-revision/src/commit.rs @@ -637,7 +637,34 @@ pub async fn commit_impl( .await .unwrap_or_default(); if !globals.force() && !branch_latest.is_zero() && branch_latest != current_revision { - return Err(BranchAdvanced.into()); + // The rule this check exists for is "do not drop what another instance + // added", and an anchor that equals the latest is only the cheapest way + // of satisfying it. A staged state that already has the latest in its + // ancestry satisfies it too: committing it publishes that work rather + // than replacing it. + // + // Sync's own merge is exactly that state, and without this it cannot + // land: sync stages a merge of the remote target with the local + // revision, then commits it — and the commit was refused for the very + // divergence the merge resolves, telling the user to sync while they + // were inside a sync. + // + // Reached only when the cheap comparison already failed, so the + // ordinary commit path is unchanged. + if !incorporates_branch_latest( + repository.clone(), + current_branch, + staged_revision, + branch_latest, + ) + .await + { + return Err(BranchAdvanced.into()); + } + lore_debug!( + "Branch latest {branch_latest} is already part of staged revision {staged_revision}, \ + committing on top of it" + ); } let state_staged = State::deserialize(repository.clone(), staged_revision) @@ -1615,6 +1642,68 @@ async fn commit_staged_revision( } } +/// How far back the reachability walk below is willing to look for the branch +/// latest. Generous for the shapes it exists to answer — a sync merge finds it +/// one step in — and bounded so a pathological history cannot turn a commit +/// into a long walk. +const BRANCH_LATEST_SEARCH_LIMIT: usize = 512; + +/// Whether committing `staged` would publish `branch_latest` rather than +/// replace it — i.e. whether the latest is already part of the staged state's +/// ancestry. +/// +/// Answers the question [`commit`]'s branch-advanced check actually cares +/// about. The cheap cases are the two parents: a staged state built directly on +/// the latest, and a merge that took it as its second parent. Otherwise walk +/// first parents back from the staged state's own parent, stopping as soon as +/// the revision numbers fall below the latest's, since nothing older can be it. +/// In the shape this exists for — sync staging a merge of a remote target that +/// descends from the latest — the walk ends on the first or second step, and +/// for a plain commit on a stale anchor it stops immediately. +/// +/// **Fails closed.** Anything that cannot be established — an unreadable state, +/// a walk that runs past its bound, a history this clone only partly holds — +/// answers `false` and the commit is refused. A gap here costs a refusal the +/// user can resolve by syncing; the opposite mistake costs somebody's work. +async fn incorporates_branch_latest( + repository: Arc, + branch: BranchId, + staged: Hash, + branch_latest: Hash, +) -> bool { + let Ok(state_staged) = State::deserialize(repository.clone(), staged).await else { + return false; + }; + if state_staged.parent_self() == branch_latest || state_staged.parent_other() == branch_latest { + return true; + } + + let Ok(state_latest) = State::deserialize(repository.clone(), branch_latest).await else { + return false; + }; + let latest_number = state_latest.revision_number(); + + crate::find::find_revision( + repository, + branch, + state_staged.parent_self(), + false, + Some(BRANCH_LATEST_SEARCH_LIMIT), + |state, _| { + if state.revision() == branch_latest { + crate::find::FindMatchResult::Match + } else if state.revision_number() < latest_number { + // Walked past the point the latest could still appear. + crate::find::FindMatchResult::Abort + } else { + crate::find::FindMatchResult::Continue + } + }, + ) + .await + .is_ok() +} + /// Publish `signature` as `branch`'s tip and anchor it as the current revision. /// /// `previous` is the tip the caller observed when it decided to commit; the diff --git a/lore-revision/tests/commit.rs b/lore-revision/tests/commit.rs index c3f7020a..e7f48e71 100644 --- a/lore-revision/tests/commit.rs +++ b/lore-revision/tests/commit.rs @@ -600,4 +600,252 @@ mod tests { "expected NO RevisionCommitRevision event (discriminant {commit_discriminant}) in captured {captured:?}" ); } + + // --------------------------------------------------------------------- + // Branch-advanced gate: the check is "does this commit drop what another + // instance added", not "is your anchor the branch latest". These two + // tests are the pair that defines the difference — one staged state that + // already carries the latest, one that does not, with identical pointers. + // + // The pointers are set directly rather than produced by a race, because + // the gate only ever reads them: how the anchor came to differ from the + // latest (a server-side fast-forward merge, a forced commit) is not + // something it can see. + // --------------------------------------------------------------------- + + /// Build a repository with three linear revisions and return + /// `(repository, write_token, [r1, r2, r3])`, leaving the anchor and the + /// branch latest both on `r3`. + async fn three_revision_chain( + path: &std::path::Path, + immutable_store: Arc, + mutable_store: Arc, + ) -> ( + Arc, + repository::RepositoryWriteToken, + BranchId, + Vec, + ) { + std::fs::create_dir_all(path).expect("Create directory failed"); + let repository_id = RepositoryId::from(uuid::Uuid::now_v7()); + let default_branch_id = BranchId::from(uuid::Uuid::now_v7()); + let write_token = repository::RepositoryWriteToken::acquire(path).await; + let created_repo = repository::create_local( + path, + &write_token, + repository_id, + default_branch_id, + branch::DEFAULT_DEFAULT_NAME.to_string(), + repository::RepositoryConfig::default(), + false, + ) + .await + .expect("Failed to initialize repository"); + + let repository = Arc::new( + RepositoryContext::new( + default_repository_creation_args(immutable_store, mutable_store) + .with_path(path) + .with_id(repository_id) + .with_instance_id(created_repo.instance_id), + ) + .with_write_token(write_token.share()), + ); + lore_revision::instance::store_current_anchor_branch(&repository, default_branch_id) + .await + .expect("Failed to store anchor branch"); + + let mut revisions = Vec::new(); + for i in 0..3u8 { + let file_path = path.join(format!("file{i}.bin")); + { + let mut file = std::fs::File::options() + .create(true) + .truncate(true) + .write(true) + .open(&file_path) + .expect("Failed to create test file"); + file.write_all(&[i, i, i]) + .expect("Failed to write test file"); + } + file::stage::stage( + repository.clone(), + &write_token, + LoreArray::from_vec(vec![LoreString::from(&file_path)]), + StageOptions { + case_change: stage::StageCaseChange::Error, + node_flags: NodeFlags::NoFlags, + file_id: None, + no_children: false, + scan: true, + }, + ) + .await + .expect("Failed to stage file"); + + let signature = Box::pin(commit::commit( + repository.clone(), + &write_token, + CommitOptions { + message: format!("r{i}"), + link_messages: std::collections::HashMap::new(), + link: None, + layer_messages: std::collections::HashMap::new(), + layer: None, + }, + )) + .await + .expect("Failed to commit revision"); + revisions.push(signature); + } + + (repository, write_token, default_branch_id, revisions) + } + + /// Stage a state with a chosen ancestry, the way a merge leaves one behind: + /// a serialized state stored as the staged anchor. `parent_other` set makes + /// it a merge, which is the shape sync stages — `parent_self` the remote + /// target it is synchronizing to, `parent_other` the local revision. + async fn stage_state_on( + repository: &Arc, + write_token: &repository::RepositoryWriteToken, + parent_self: lore_base::types::Hash, + parent_other: lore_base::types::Hash, + ) -> lore_base::types::Hash { + let state = state::State::deserialize(repository.clone(), parent_self) + .await + .expect("deserialize parent state"); + state.set_parent_self(parent_self); + state.set_parent_other(parent_other); + state.set_revision_number(0); + state.set_metadata_hash(Default::default()); + state.mark_dirty(); + let signature = state + .serialize(repository.clone(), write_token) + .await + .expect("serialize staged state"); + lore_revision::instance::store_staged_anchor(repository, signature) + .await + .expect("store staged anchor"); + signature + } + + /// The branch latest is already in the staged state's ancestry — two steps + /// back, the shape a sync's own merge commit has. Committing it publishes + /// that work rather than replacing it, so it is allowed even though the + /// anchor is not the latest. + #[tokio::test] + async fn commit_allowed_when_staged_state_already_carries_the_branch_latest() { + let (immutable_store, mutable_store, execution) = + test_store_create().await.expect("Failed to create stores"); + + #[allow(clippy::disallowed_methods)] + runtime() + .spawn(LORE_CONTEXT.scope(execution.clone(), async move { + let tempdir = generate_tempdir(); + let (repository, write_token, branch_id, revisions) = + three_revision_chain(tempdir.to_path_buf().as_path(), immutable_store, mutable_store) + .await; + let (r1, r2, r3) = (revisions[0], revisions[1], revisions[2]); + + // Anchor behind the latest, and the latest itself behind the + // revision the staged state is built on: r1 <- r2 <- r3. + lore_revision::instance::store_current_anchor(&repository, r1) + .await + .expect("store anchor"); + branch::store_latest( + repository.clone(), + branch_id, + r3, + r2, + branch::BranchLatestStatus::Divergent, + ) + .await + .expect("store latest"); + + // The shape sync stages: parent_self the target it is + // synchronizing to, parent_other the revision it is on. + let staged = stage_state_on(&repository, &write_token, r3, r1).await; + assert_ne!(staged, r3, "staged state must be its own revision"); + + let result = Box::pin(commit::commit( + repository.clone(), + &write_token, + CommitOptions { + message: "carries the latest".to_string(), + link_messages: std::collections::HashMap::new(), + link: None, + layer_messages: std::collections::HashMap::new(), + layer: None, + }, + )) + .await; + + assert!( + result.is_ok(), + "a staged state whose ancestry contains the branch latest must commit, got {result:?}" + ); + })) + .await + .expect("Test task failed"); + } + + /// The counterpart, and the reason the check still earns its place: the + /// staged state hangs off the stale anchor and never reaches the latest, so + /// committing it would drop whatever advanced the branch. Still refused. + #[tokio::test] + async fn commit_refused_when_staged_state_cannot_reach_the_branch_latest() { + let (immutable_store, mutable_store, execution) = + test_store_create().await.expect("Failed to create stores"); + + #[allow(clippy::disallowed_methods)] + runtime() + .spawn(LORE_CONTEXT.scope(execution.clone(), async move { + let tempdir = generate_tempdir(); + let (repository, write_token, branch_id, revisions) = three_revision_chain( + tempdir.to_path_buf().as_path(), + immutable_store, + mutable_store, + ) + .await; + let (r1, r2, r3) = (revisions[0], revisions[1], revisions[2]); + + // Same pointers as the test above — only the staged state + // differs: it is built on r1, which cannot reach r2. + lore_revision::instance::store_current_anchor(&repository, r1) + .await + .expect("store anchor"); + branch::store_latest( + repository.clone(), + branch_id, + r3, + r2, + branch::BranchLatestStatus::Divergent, + ) + .await + .expect("store latest"); + + stage_state_on(&repository, &write_token, r1, Default::default()).await; + + let result = Box::pin(commit::commit( + repository.clone(), + &write_token, + CommitOptions { + message: "would drop the advance".to_string(), + link_messages: std::collections::HashMap::new(), + link: None, + layer_messages: std::collections::HashMap::new(), + layer: None, + }, + )) + .await; + + assert!( + result.is_err(), + "a staged state that cannot reach the branch latest must still be refused" + ); + })) + .await + .expect("Test task failed"); + } }