Skip to content

Commit

Permalink
refactor: remove unused merge methods
Browse files Browse the repository at this point in the history
  • Loading branch information
@louib @droidmonkey
louib authored and droidmonkey committed Dec 10, 2023
1 parent cc0530b commit f7fd388
Show file tree
Hide file tree
Showing 6 changed files with 3 additions and 219 deletions.
16 changes: 0 additions & 16 deletions share/translations/keepassxc_en.ts
View file
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
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
};
Expand Down
102 changes: 3 additions & 99 deletions src/core/Merger.cpp
View file
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
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
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
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

0 comments on commit f7fd388

Please sign in to comment.