From 212b90e4f859aaafb1a2a45106cd6cdc989de4cf Mon Sep 17 00:00:00 2001 From: Jochen Hunz Date: Tue, 15 Sep 2026 22:39:07 +0200 Subject: [PATCH] lore-revision: Only follow a parent the server renumbered A push carrying more than one local revision into a branch head that has moved drops the other side's work. The push succeeds, reports no conflict, and leaves the overwritten commit in the history, so nothing looks wrong afterwards. collect_fragments_and_push moved a revision's parent pointer onto the value the previous iteration left in current_latest, keeping its tree. That is correct for the case the TODO describes, where the server only rewrote the revision number. After a server-side fast-forward merge current_latest is a revision the server built from the other side's work, and the reparented revision then looks like a direct descendant: the server takes the ordinary fast-forward path with no three-way diff, and the branch becomes a tree that never held the other side's changes. Track the rename instead. Remember the pushed revision as (signature in local history, signature on the server) and follow the parent only to that revision under a new signature. Everything else is pushed as it stands, so the server merges it against a base it already holds. The guard !current_latest.is_zero() is why a single-revision push never hit this: the block cannot fire on the first revision. Regression test in scripts/test/test_push.py. Against 0.9.0 the second revision's tree replaces the branch, and the file the other client pushed is missing from a fresh clone. Signed-off-by: Jochen Hunz --- lore-revision/src/branch/push.rs | 22 +++++++++++---- scripts/test/test_push.py | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/lore-revision/src/branch/push.rs b/lore-revision/src/branch/push.rs index 4ed08d9f..8244b125 100644 --- a/lore-revision/src/branch/push.rs +++ b/lore-revision/src/branch/push.rs @@ -1021,8 +1021,12 @@ async fn collect_fragments_and_push( let mut current_latest = Hash::default(); let mut fast_forward_merged = false; - for current_revision in full_local_history.iter().rev() { - let mut current_revision = *current_revision; + // The revision pushed in the previous iteration, as (signature in local + // history, signature on the server). The next revision may follow its + // parent only to a revision the server stored under a new signature. + let mut renumbered_previous: Option<(Hash, Hash)> = None; + for original_revision in full_local_history.iter().rev() { + let mut current_revision = *original_revision; let state = State::deserialize(repository.clone(), current_revision) .await @@ -1030,7 +1034,10 @@ async fn collect_fragments_and_push( push_revision_links(&repository, token, &options, &state, branch).await?; - if !current_latest.is_zero() && state.parent_self() != current_latest { + if let Some((local, stored)) = renumbered_previous + && local != stored + && state.parent_self() == local + { // Rebase on new latest revision // TODO(mjansson): This only handles revision number rewrite for now, implement proper // automatic rebase if the push resulted in a clean rebase @@ -1040,12 +1047,12 @@ async fn collect_fragments_and_push( LoreBranchPushRevisionUpdateBeginEventData { revision: state.revision(), old_parent: state.parent_self(), - new_parent: current_latest, + new_parent: stored, }, ) .send(); - state.set_parent_self(current_latest); + state.set_parent_self(stored); current_revision = state .serialize(repository.clone(), token) .await @@ -1157,6 +1164,8 @@ async fn collect_fragments_and_push( remote_latest = response.revision; current_latest = response.revision; + // The server stored what we pushed as the merge's other parent. + renumbered_previous = Some((*original_revision, current_revision)); event::LoreEvent::BranchPushRevisionPushEnd( LoreBranchPushRevisionPushEndEventData { @@ -1201,8 +1210,11 @@ async fn collect_fragments_and_push( remote_latest = response.revision; current_latest = response.revision; + // The server rewrote the revision number, which changes the signature. + renumbered_previous = Some((*original_revision, response.revision)); } else { current_latest = current_revision; + renumbered_previous = Some((*original_revision, current_revision)); } let current_number = State::deserialize(repository.clone(), current_latest) diff --git a/scripts/test/test_push.py b/scripts/test/test_push.py index 3b24f896..7976199b 100644 --- a/scripts/test/test_push.py +++ b/scripts/test/test_push.py @@ -368,6 +368,53 @@ def test_push_fast_forward_merge_non_merge(new_lore_repo): ) +@pytest.mark.smoke +def test_push_fast_forward_merge_two_local_revisions(new_lore_repo): + """A fast-forward merge push that carries more than one local revision keeps + the work another client pushed in between. The client may only follow a + parent the server renumbered, never a head the server moved for another + reason, or the second revision's stale tree replaces the branch.""" + repo: Lore = new_lore_repo() + + with repo.open_file("seed.txt", "w+") as f: + f.write("seed\n") + repo.stage(scan=True) + repo.commit("Seed", offline=True) + repo.push() + + # Another client moves the branch head + other = repo.clone() + with other.open_file("from_other.txt", "w+") as f: + f.write("from the other client\n") + other.stage(scan=True, offline=True) + other.commit("Other client publishes", offline=True) + other.push() + + # This client is still on the old head, and commits twice before it pushes + # once. The second revision is the one whose parent used to be bent onto the + # head the server had moved to, which kept its tree and dropped the other + # client's file. + with repo.open_file("first.txt", "w+") as f: + f.write("first\n") + repo.stage(scan=True, offline=True) + repo.commit("First", offline=True) + with repo.open_file("second.txt", "w+") as f: + f.write("second\n") + repo.stage(scan=True, offline=True) + repo.commit("Second", offline=True) + repo.push(fast_forward_merge=True) + + expected_files = {"seed.txt", "from_other.txt", "first.txt", "second.txt"} + + verify = repo.clone() + clone_files = _collect_repo_files(verify) + assert clone_files == expected_files, ( + f"Files on the server differ from expected.\n" + f" Extra: {clone_files - expected_files}\n" + f" Missing: {expected_files - clone_files}" + ) + + @pytest.mark.smoke def test_push_non_current_branch_preserves_anchor(new_lore_repo): """Pushing a branch that is not the current branch must not corrupt the