Merge : Synchronising groups. #1165

louib · 2017-11-03T20:28:06Z

This changes the merge function to match the groups using the UUIDs, the same way we match the entries. This allows us to do basic conflict resolution on the groups, so we can update the basic information such as the name and the location.

I also removed the shallow parameter to Group::clone in favor of CloneFlags, just like for Entry::clone.

I think the next step for the merge function would be to separate the change detection (new entries, new groups, entries or groups changed) and the actual modification of the destination database. This would allow us to fix a couple of issues with the merge function. For example, we could easily implement a dry-run functionality, and we could detect when the database was really modified by a merge operation. This would also allow us to do the conflict resolution interactively.

How has this been tested?

Added new unit tests.
Tested locally

Screenshots (if appropriate):

Types of changes

✅ New feature (non-breaking change which adds functionality)
✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ I have added tests to cover my changes.

phoerious · 2017-11-20T23:46:35Z

src/core/Group.cpp

+    const QDateTime timeExisting = existingGroup->timeInfo().lastModificationTime();
+    const QDateTime timeOther = otherGroup->timeInfo().lastModificationTime();
+
+    // only if the other group newer, update the existing one.


missing "is" 😉

phoerious · 2017-11-20T23:47:35Z

src/core/Group.cpp

+        existingGroup->setName(otherGroup->name());
+        existingGroup->setNotes(otherGroup->notes());
+        existingGroup->setIcon(otherGroup->iconNumber());
+        existingGroup->setIcon(otherGroup->iconUuid());


This will either reset the previously set iconNumber or trigger an assertion for !uuid.isNull(). You need to check which one is set: icon number or UUID.

@phoerious you're right, the tests we're indeed failing when building in debug mode. Which makes me think, should we not build in debug mode in travis?

We are building both in Debug and Release mode. I have no idea why the tests passed.

phoerious · 2017-11-20T23:58:13Z

src/core/Group.h

@@ -126,7 +135,8 @@ class Group : public QObject
     * new group into another database.
     */
    Group* clone(Entry::CloneFlags entryFlags = Entry::CloneNewUuid | Entry::CloneResetTimeInfo,
-                 bool shallow = false) const;
+                 CloneFlags groupFlags = static_cast<CloneFlags>(Group::CloneNewUuid | Group::CloneResetTimeInfo |


Wrong indentation

That's actually the formatting done by clang-format. How do you think it should be indented?

CloneFlags groupFlags should be on the same level as Group*

@phoerious still not sure what you mean. CloneFlags groupFlags, so it should be aligned with the other parameters.

Anyway, I extracted the default flags for the function, so the signature is much smaller.

I just got owned by the inline diff view. Ignore what I said. But not having a static_cast there is a lot cleaner, now, anyway.

louib · 2017-11-21T14:56:07Z

@phoerious thanks for the review, back to you!

louib · 2017-11-21T19:25:09Z

@phoerious changed formatting + added unit tests for the new flags of Group::clone

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

louib requested a review from droidmonkey November 3, 2017 20:28

louib force-pushed the feature_merge_enhancements branch from 4ab9691 to 7243dfd Compare November 3, 2017 20:29

louib requested review from a team and removed request for droidmonkey November 4, 2017 20:46

louib force-pushed the feature_merge_enhancements branch from 7243dfd to 89ea06e Compare November 10, 2017 21:32

louib added this to the v2.3.0 milestone Nov 20, 2017

phoerious requested changes Nov 21, 2017

View reviewed changes

Louis-Bertrand Varin added 4 commits November 21, 2017 14:06

Merge : Synchronising groups.

9603c91

Only updating custom group icon if set.

2ff56fe

Extracting default clone flags.

94ea323

Formatting

2cd5006

louib force-pushed the feature_merge_enhancements branch from 19e8817 to 2cd5006 Compare November 21, 2017 19:06

Adding tests for Group::clone

f25c23c

phoerious approved these changes Nov 21, 2017

View reviewed changes

phoerious merged commit b0e6bcf into keepassxreboot:develop Nov 21, 2017

phoerious added the new feature label Nov 21, 2017

phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge : Synchronising groups. #1165

Merge : Synchronising groups. #1165

louib commented Nov 3, 2017 •

edited

Loading

phoerious Nov 20, 2017

phoerious Nov 20, 2017

louib Nov 21, 2017

phoerious Nov 21, 2017

phoerious Nov 20, 2017

louib Nov 21, 2017

phoerious Nov 21, 2017

louib Nov 21, 2017

phoerious Nov 21, 2017

louib commented Nov 21, 2017

louib commented Nov 21, 2017

Merge : Synchronising groups. #1165

Merge : Synchronising groups. #1165

Conversation

louib commented Nov 3, 2017 • edited Loading

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louib commented Nov 21, 2017

louib commented Nov 21, 2017

louib commented Nov 3, 2017 •

edited

Loading