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"); + } }