Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused merge methods #9702

Merged
merged 1 commit into from
Dec 10, 2023
Merged

Remove unused merge methods #9702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions share/translations/keepassxc_en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5615,22 +5615,6 @@ We recommend you use the AppImage available on our downloads page.</source>
<source>older entry merged from database &quot;%1&quot;</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Adding backup for older target %1 [%2]</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Adding backup for older source %1 [%2]</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Reapplying older target entry on top of newer source %1 [%2]</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Reapplying older source entry on top of newer target %1 [%2]</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Synchronizing from newer source %1 [%2]</source>
<translation type="unfinished"></translation>
Expand Down
3 changes: 0 additions & 3 deletions src/core/Group.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ class Group : public ModifiableObject
enum MergeMode
{
Default, // Determine merge strategy from parent or fallback (Synchronize)
Duplicate, // lossy strategy regarding deletions, duplicate older changes in a new entry
KeepLocal, // merge history forcing local as top regardless of age
KeepRemote, // merge history forcing remote as top regardless of age
KeepNewer, // merge history
Synchronize, // merge history keeping most recent as top entry and applying deletions
Comment on lines 42 to 43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is actually the difference between these two?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only difference. In a future PR, I think I will remove the MergeMode completely, and instead add a Merger option called mergeDeletions or synchronizeDeletions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree!

};
Expand Down
102 changes: 3 additions & 99 deletions src/core/Merger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,76 +257,6 @@ void Merger::eraseGroup(Group* group)
database->setDeletedObjects(deletions);
}

Merger::ChangeList
Merger::resolveEntryConflict_Duplicate(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry)
{
ChangeList changes;
const int comparison = compare(targetEntry->timeInfo().lastModificationTime(),
sourceEntry->timeInfo().lastModificationTime(),
CompareItemIgnoreMilliseconds);
// if one entry is newer, create a clone and add it to the group
if (comparison < 0) {
Entry* clonedEntry = sourceEntry->clone(Entry::CloneNewUuid | Entry::CloneIncludeHistory);
moveEntry(clonedEntry, context.m_targetGroup);
markOlderEntry(targetEntry);
changes << tr("Adding backup for older target %1 [%2]").arg(targetEntry->title(), targetEntry->uuidToHex());
} else if (comparison > 0) {
Entry* clonedEntry = sourceEntry->clone(Entry::CloneNewUuid | Entry::CloneIncludeHistory);
moveEntry(clonedEntry, context.m_targetGroup);
markOlderEntry(clonedEntry);
changes << tr("Adding backup for older source %1 [%2]").arg(sourceEntry->title(), sourceEntry->uuidToHex());
}
return changes;
}

Merger::ChangeList
Merger::resolveEntryConflict_KeepLocal(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry)
{
Q_UNUSED(context);
ChangeList changes;
const int comparison = compare(targetEntry->timeInfo().lastModificationTime(),
sourceEntry->timeInfo().lastModificationTime(),
CompareItemIgnoreMilliseconds);
if (comparison < 0) {
// we need to make our older entry "newer" than the new entry - therefore
// we just create a new history entry without any changes - this preserves
// the old state before merging the new state and updates the timestamp
// the merge takes care, that the newer entry is sorted inbetween both entries
// this type of merge changes the database timestamp since reapplying the
// old entry is an active change of the database!
changes << tr("Reapplying older target entry on top of newer source %1 [%2]")
.arg(targetEntry->title(), targetEntry->uuidToHex());
Entry* agedTargetEntry = targetEntry->clone(Entry::CloneNoFlags);
targetEntry->addHistoryItem(agedTargetEntry);
}
return changes;
}

Merger::ChangeList
Merger::resolveEntryConflict_KeepRemote(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry)
{
Q_UNUSED(context);
ChangeList changes;
const int comparison = compare(targetEntry->timeInfo().lastModificationTime(),
sourceEntry->timeInfo().lastModificationTime(),
CompareItemIgnoreMilliseconds);
if (comparison > 0) {
// we need to make our older entry "newer" than the new entry - therefore
// we just create a new history entry without any changes - this preserves
// the old state before merging the new state and updates the timestamp
// the merge takes care, that the newer entry is sorted inbetween both entries
// this type of merge changes the database timestamp since reapplying the
// old entry is an active change of the database!
changes << tr("Reapplying older source entry on top of newer target %1 [%2]")
.arg(targetEntry->title(), targetEntry->uuidToHex());
targetEntry->beginUpdate();
targetEntry->copyDataFrom(sourceEntry);
targetEntry->endUpdate();
// History item is created by endUpdate since we should have changes
}
return changes;
}

Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContext& context,
const Entry* sourceEntry,
Entry* targetEntry,
Expand Down Expand Up @@ -366,38 +296,12 @@ Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContex
Merger::ChangeList
Merger::resolveEntryConflict(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry)
{
ChangeList changes;
// We need to cut off the milliseconds since the persistent format only supports times down to seconds
// so when we import data from a remote source, it may represent the (or even some msec newer) data
// which may be discarded due to higher runtime precision

Group::MergeMode mergeMode = m_mode == Group::Default ? context.m_targetGroup->mergeMode() : m_mode;
switch (mergeMode) {
case Group::Duplicate:
changes << resolveEntryConflict_Duplicate(context, sourceEntry, targetEntry);
break;

case Group::KeepLocal:
changes << resolveEntryConflict_KeepLocal(context, sourceEntry, targetEntry);
changes << resolveEntryConflict_MergeHistories(context, sourceEntry, targetEntry, mergeMode);
break;

case Group::KeepRemote:
changes << resolveEntryConflict_KeepRemote(context, sourceEntry, targetEntry);
changes << resolveEntryConflict_MergeHistories(context, sourceEntry, targetEntry, mergeMode);
break;

case Group::Synchronize:
case Group::KeepNewer:
// nothing special to do since resolveEntryConflictMergeHistories takes care to use the newest entry
changes << resolveEntryConflict_MergeHistories(context, sourceEntry, targetEntry, mergeMode);
break;

default:
// do nothing
break;
}
return changes;
return resolveEntryConflict_MergeHistories(context, sourceEntry, targetEntry, mergeMode);
}

bool Merger::mergeHistory(const Entry* sourceEntry, Entry* targetEntry, Group::MergeMode mergeMethod)
Expand All @@ -408,8 +312,8 @@ bool Merger::mergeHistory(const Entry* sourceEntry, Entry* targetEntry, Group::M
const int comparison = compare(sourceEntry->timeInfo().lastModificationTime(),
targetEntry->timeInfo().lastModificationTime(),
CompareItemIgnoreMilliseconds);
const bool preferLocal = mergeMethod == Group::KeepLocal || comparison < 0;
const bool preferRemote = mergeMethod == Group::KeepRemote || comparison > 0;
const bool preferLocal = comparison < 0;
const bool preferRemote = comparison > 0;

QMap<QDateTime, Entry*> merged;
for (Entry* historyItem : targetHistoryItems) {
Expand Down
6 changes: 0 additions & 6 deletions src/core/Merger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,6 @@ class Merger : public QObject
void eraseGroup(Group* group);
ChangeList resolveEntryConflict(const MergeContext& context, const Entry* existingEntry, Entry* otherEntry);
ChangeList resolveGroupConflict(const MergeContext& context, const Group* existingGroup, Group* otherGroup);
Merger::ChangeList
resolveEntryConflict_Duplicate(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry);
Merger::ChangeList
resolveEntryConflict_KeepLocal(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry);
Merger::ChangeList
resolveEntryConflict_KeepRemote(const MergeContext& context, const Entry* sourceEntry, Entry* targetEntry);
Merger::ChangeList resolveEntryConflict_MergeHistories(const MergeContext& context,
const Entry* sourceEntry,
Entry* targetEntry,
Expand Down
89 changes: 0 additions & 89 deletions tests/TestMerge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,6 @@ QTEST_GUILESS_MAIN(TestMerge)

namespace
{
TimeInfo modificationTime(TimeInfo timeInfo, int years, int months, int days)
{
const QDateTime time = timeInfo.lastModificationTime();
timeInfo.setLastModificationTime(time.addYears(years).addMonths(months).addDays(days));
return timeInfo;
}

MockClock* m_clock = nullptr;
} // namespace

Expand Down Expand Up @@ -258,50 +251,6 @@ void TestMerge::testResolveConflictExisting()
}
}

/**
* Tests the KeepBoth merge mode.
*/
void TestMerge::testResolveConflictDuplicate()
{
QScopedPointer<Database> dbDestination(createTestDatabase());
QScopedPointer<Database> dbSource(
createTestDatabaseStructureClone(dbDestination.data(), Entry::CloneIncludeHistory, Group::CloneIncludeEntries));

// sanity check
QCOMPARE(dbDestination->rootGroup()->children().at(0)->entries().size(), 2);

// make this entry newer than in original db
QPointer<Entry> updatedDestinationEntry = dbDestination->rootGroup()->children().at(0)->entries().at(0);
const TimeInfo initialEntryTimeInfo = updatedDestinationEntry->timeInfo();
const TimeInfo updatedEntryTimeInfo = modificationTime(initialEntryTimeInfo, 1, 0, 0);

updatedDestinationEntry->setTimeInfo(updatedEntryTimeInfo);

dbDestination->rootGroup()->setMergeMode(Group::MergeMode::Duplicate);

// Make sure the merge changes have a different timestamp.
m_clock->advanceSecond(1);

Merger merger(dbSource.data(), dbDestination.data());
merger.merge();

// one entry is duplicated because of mode
QCOMPARE(dbDestination->rootGroup()->children().at(0)->entries().size(), 3);
QCOMPARE(dbDestination->rootGroup()->children().at(0)->entries().at(0)->historyItems().isEmpty(), false);
// the older entry was merged from the other db as last in the group
QPointer<Entry> newerEntry = dbDestination->rootGroup()->children().at(0)->entries().at(0);
QPointer<Entry> olderEntry = dbDestination->rootGroup()->children().at(0)->entries().at(2);
QVERIFY(newerEntry->title() == olderEntry->title());
QVERIFY2(!newerEntry->attributes()->hasKey("merged"), "newer entry is not marked with an attribute \"merged\"");
QVERIFY2(olderEntry->attributes()->hasKey("merged"), "older entry is marked with an attribute \"merged\"");
QCOMPARE(olderEntry->historyItems().isEmpty(), false);
QCOMPARE(newerEntry->timeInfo(), updatedEntryTimeInfo);
// TODO HNH: this may be subject to discussions since the entry itself is newer but represents an older one
// QCOMPARE(olderEntry->timeInfo(), initialEntryTimeInfo);
QVERIFY2(olderEntry->uuidToHex() != updatedDestinationEntry->uuidToHex(),
"KeepBoth should not reuse the UUIDs when cloning.");
}

void TestMerge::testResolveConflictTemplate(
int mergeMode,
std::function<void(Database*, const QMap<const char*, QDateTime>&)> verification)
Expand Down Expand Up @@ -733,26 +682,11 @@ void TestMerge::testDeletionConflictEntry_Synchronized()
testDeletionConflictTemplate(Group::Synchronize, &TestMerge::assertDeletionNewerOnly);
}

void TestMerge::testDeletionConflictEntry_KeepLocal()
{
testDeletionConflictTemplate(Group::KeepLocal, &TestMerge::assertDeletionLocalOnly);
}

void TestMerge::testDeletionConflictEntry_KeepRemote()
{
testDeletionConflictTemplate(Group::KeepRemote, &TestMerge::assertDeletionLocalOnly);
}

void TestMerge::testDeletionConflictEntry_KeepNewer()
{
testDeletionConflictTemplate(Group::KeepNewer, &TestMerge::assertDeletionLocalOnly);
}

void TestMerge::testDeletionConflictEntry_Duplicate()
{
testDeletionConflictTemplate(Group::Duplicate, &TestMerge::assertDeletionLocalOnly);
}

/**
* Tests the KeepNewer mode concerning history.
*/
Expand All @@ -766,29 +700,6 @@ void TestMerge::testResolveConflictEntry_Synchronize()
});
}

/**
* Tests the KeepExisting mode concerning history.
*/
void TestMerge::testResolveConflictEntry_KeepLocal()
{
testResolveConflictTemplate(Group::KeepLocal, [](Database* db, const QMap<const char*, QDateTime>& timestamps) {
QPointer<Group> mergedRootGroup = db->rootGroup();
QPointer<Group> mergedGroup1 = mergedRootGroup->children().at(0);
TestMerge::assertUpdateMergedEntry1(mergedGroup1->entries().at(0), timestamps);
TestMerge::assertUpdateReappliedEntry2(mergedGroup1->entries().at(1), timestamps);
});
}

void TestMerge::testResolveConflictEntry_KeepRemote()
{
testResolveConflictTemplate(Group::KeepRemote, [](Database* db, const QMap<const char*, QDateTime>& timestamps) {
QPointer<Group> mergedRootGroup = db->rootGroup();
QPointer<Group> mergedGroup1 = mergedRootGroup->children().at(0);
TestMerge::assertUpdateReappliedEntry1(mergedGroup1->entries().at(0), timestamps);
TestMerge::assertUpdateMergedEntry2(mergedGroup1->entries().at(1), timestamps);
});
}

void TestMerge::testResolveConflictEntry_KeepNewer()
{
testResolveConflictTemplate(Group::KeepNewer, [](Database* db, const QMap<const char*, QDateTime>& timestamps) {
Expand Down
6 changes: 0 additions & 6 deletions tests/TestMerge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,9 @@ private slots:
void testResolveGroupConflictOlder();
void testMergeNotModified();
void testMergeModified();
void testResolveConflictDuplicate();
void testResolveConflictEntry_Synchronize();
void testResolveConflictEntry_KeepLocal();
void testResolveConflictEntry_KeepRemote();
void testResolveConflictEntry_KeepNewer();
void testDeletionConflictEntry_Duplicate();
void testDeletionConflictEntry_Synchronized();
void testDeletionConflictEntry_KeepLocal();
void testDeletionConflictEntry_KeepRemote();
void testDeletionConflictEntry_KeepNewer();
void testMoveEntry();
void testMoveEntryPreserveChanges();
Expand Down