feat(merge): add Database::merge_with_ancestor for three-way merging#342
Open
christhomas wants to merge 3 commits into
Open
feat(merge): add Database::merge_with_ancestor for three-way merging#342christhomas wants to merge 3 commits into
christhomas wants to merge 3 commits into
Conversation
christhomas
requested review from
droidmonkey,
louib,
sseemayer and
varjolintu
as code owners
Contributor
Author
|
I don't have any files which have conflicts, so can somebody try this and see whether it gives the right output, or instruct me on how I could create a test case for it and I'll see how to implement it. |
Contributor
Author
|
The CI failure is unrelated to this PR — the 'Test Suite on x86_64' job failed at the 'Install cross-tool test dependencies' step because |
Collaborator
|
Can you add a test for entries that are in different groups between Source and Destination. Ideally the last modified entry would win the location battle. |
Contributor
Author
|
@droidmonkey there you go |
droidmonkey
previously approved these changes
May 26, 2026
Owner
|
Thanks for the contribution. I am surprised that the changeset is this small. Does this really cover all of the cases of three-way merging? Have you spent the time thinking how the merge logic should work, or is this just a direct implementation of #337 ? I want to spend some time, probably this weekend to properly think through the merge logic from first principles. |
Contributor
Author
|
you give me a clear direction on the gap you think that we have and I'll go away and fix it. I have to admit I'm not an expert at keepassxc, so I have no choice but to delegate some level of direction to you. But I like the project and I use it everyday, so I'm using my free time for good use :) |
Contributor
Author
|
I added some extra situations which might be useful, do you want to look into it please? |
merge_with_ancestor(other, ancestor) threads the common ancestor through merge_groups and merge_entries to distinguish one-sided changes from true conflicts: - Only self changed since ancestor → keep self silently - Only other changed since ancestor → take other silently - Both changed identically → trivially resolved - Both changed differently → warn via MergeLog::warnings, resolve newest-wins Database::merge is now a shim: merge_with_ancestor(other, &Database::new()). Using an empty ancestor means every object appears changed on both sides, which degrades to the existing two-way (newest-wins) semantics with no new warnings — fully backward-compatible. Also adds five targeted tests: - test_ancestor_one_sided_change_in_other - test_ancestor_one_sided_change_in_self - test_ancestor_true_conflict_warns_and_newest_wins - test_ancestor_idempotence - test_two_way_shim_no_warning_on_conflict Closes sseemayer#337
Two tests requested in review: - test_ancestor_entry_moved_to_different_groups_other_wins: both sides move the same entry to different groups and edit its content; other is newer so its location (group2) and content win, and a conflict warning is emitted. - test_ancestor_entry_moved_in_other_only_no_conflict: only other moves the entry (self unchanged since ancestor); location and content are taken silently with no warning.
…groups_diverged Wire base_db through merge_icons for ancestor-aware conflict detection, matching the treatment already applied to merge_entries and merge_groups. Fix have_groups_diverged to exclude previous_parent_group from the content divergence check. That field records location history, not content state. The prior behaviour caused a spurious GroupModificationTimeNotUpdated error when both sides relocated the same group: the merge's intermediate move_to(root) step wrote to previous_parent_group, making the equal-timestamp groups appear content-diverged even though only their location had changed. Add nine new three-way tests covering the previously untested scenarios: Entry - test_ancestor_both_changed_identically: same final value, no warning Group - test_ancestor_group_one_sided_change_in_other - test_ancestor_group_one_sided_change_in_self - test_ancestor_group_both_changed_conflict - test_ancestor_group_moved_in_other_only - test_ancestor_group_moved_in_both_other_wins Icon (new — merge_icons was two-way only before this commit) - test_ancestor_icon_one_sided_change_in_other - test_ancestor_icon_one_sided_change_in_self - test_ancestor_icon_both_changed_conflict
christhomas
force-pushed
the
feat/merge-with-ancestor
branch
from
59561f9 to
5a43c08
Compare
Merged
davidroeca
added a commit
to davidroeca/keepass-rs
that referenced
this pull request
Jun 8, 2026
A runnable example that merges two versions of a database and inspects the returned MergeLog by matching on the public event enums, demonstrating why the result types are exposed. Gated behind the _merge feature. Notes the upcoming three-way merge (sseemayer#342).
davidroeca
added a commit
to davidroeca/keepass-rs
that referenced
this pull request
Jun 8, 2026
A runnable example that merges two versions of a database and inspects the returned MergeLog by matching on the public event enums, demonstrating why the result types are exposed. Gated behind the _merge feature. Notes the upcoming three-way merge (sseemayer#342).
louib
pushed a commit
that referenced
this pull request
Jun 10, 2026
* test: assert merge result types are publicly reachable Add a `_merge`-gated integration test that performs a real merge and inspects the returned `MergeLog` by naming `MergeError` as the return type and matching the public `MergeEvent` / `MergeEventType` / `MergeEventTarget` enums. Compiled as an external crate, it fails with `error[E0603]: module 'merge' is private` until the module is re-exported. * feat(db): re-export merge module as pub mod merge `Database::merge` is callable downstream but its result types (`MergeLog`, `MergeError`, `MergeEvent`, `MergeEventType`, `MergeEventTarget`) live in the private `merge` module and cannot be named outside the crate, even with the `_merge` feature enabled. Mirror the existing `pub mod fields;` and expose the module. The change is gated behind `_merge`, so default builds are unaffected. * docs(merge): document public merge result types This fixex CI which fails on missing doc warnings. * docs(example): add merge example showcasing MergeLog inspection A runnable example that merges two versions of a database and inspects the returned MergeLog by matching on the public event enums, demonstrating why the result types are exposed. Gated behind the _merge feature. Notes the upcoming three-way merge (#342). * test: remove merge reexport test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #337.
What this adds
Database::merge_with_ancestor(other, ancestor)— a three-way merge that uses a common ancestor to distinguish one-sided changes from true conflicts:selfchanged since ancestorselfsilentlyotherchanged since ancestorothersilentlyMergeLog::warnings, resolve by newest-winsDatabase::mergeis now a shim overmerge_with_ancestor(other, &Database::new()). An empty ancestor makes every object appear changed on both sides, which degrades to the existing two-way semantics with no new warnings — fully backward-compatible.Implementation
The ancestor is threaded through the existing
merge_groups/merge_entrieshelpers as a third&Databaseparameter. In the "both exist" case for entries and groups, both sides are compared against the ancestor'slast_modificationtimestamp before falling through to the existing newest-wins logic.Tests
Five new tests alongside the existing 31:
test_ancestor_one_sided_change_in_other— no warning, other's value takentest_ancestor_one_sided_change_in_self— no warning, self's value kepttest_ancestor_true_conflict_warns_and_newest_wins— one warning, newest wins, history preservedtest_ancestor_idempotencetest_two_way_shim_no_warning_on_conflict— backward-compat:merge()produces no new warningsRun with
cargo test --lib --features _merge.