Feature : Update entries across groups when merging #807
Conversation
This file contains 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
louib
force-pushed
the
feature_merge_enhancement
branch
4 times, most recently
from
875d27a to
027fe8e
Compare
This was referenced Jul 26, 2017
louib
force-pushed
the
feature_merge_enhancement
branch
3 times, most recently
from
3d45402 to
23ef0ab
Compare
louib
force-pushed
the
feature_merge_enhancement
branch
3 times, most recently
from
81cd9c6 to
4084639
Compare
TheZ3ro
reviewed
Aug 12, 2017
|
@TheZ3ro after a quick look at the KeePass code, I believe the MergeMode is configured at the database level, and not at the group level. I'm not even sure it's stored in the kdbx file. It looks like there's an intermediate form when merging to select the merge mode for the current merge operation. I believe that makes more sense than setting the merge mode at the group level. Would keeping the merge mode but setting it at the database level be acceptable? |
louib
force-pushed
the
feature_merge_enhancement
branch
2 times, most recently
from
fc46f06 to
217b92b
Compare
|
@TheZ3ro I readded the I think there is still a lot of work to do on the merge feature, but this is a path in the good direction I think. This PR is ready for a review. |
louib
force-pushed
the
feature_merge_enhancement
branch
from
217b92b to
d86a2b3
Compare
TheZ3ro
approved these changes
Sep 5, 2017
|
@TheZ3ro I think we should also match groups using UUIDs, but this is going to change the behaviour of the functionality. We could add an option to fallback on entry title / group name matching in the future. What do you think? |
louib
force-pushed
the
feature_merge_enhancement
branch
from
d615834 to
f343fb8
Compare
|
@louib it's ok I agree |
phoerious
added a commit
that referenced
this pull request
Feb 27, 2018
- 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]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary of changes:
shallowoption for cloning a groupmergetests to their own test file.qDebugstatements when merging.Description
Currently the merge functionality updates the entries by UUIDs, but only if they are in the same group. Moving an entry to another group would prevent that entry from updating, and would create a second entry with the same UUID in the database. This would cause other problems, like the duplicate entry not being written when saving.
This PR searches for entries using UUIDs across all the destination DB when merging.
I initially wanted to create a separate function for the synchronize feature (see #819). I don't think this is a good idea anymore. I think users will expect the merge functionality to detect identical entries and merge / update them. And if the databases are completely different, no UUIDs will match, so it will basically only be a merge.
One thing that I think is missing would be to detect entries using the title. I also think users will expect this when merging 2 databases that have identical / similar structure, but have been created differently (so the entries have different UUIDs). This is tricky though because there might be multiple entries with the same name, and some of those entries might even match with the UUID, so additional logic would need to be done for that. That would solve #818.
As the merge functionality gets more complex, we will add more unit tests for it, so I moved the merge tests to their own test file.
I added debug statements, because when I perform a merge/sync of my database (normally using the CLI), I want to have feedback on what was updated/created/moved.
Finally, I don't handle deleted entries and recycle bins yet, but I guess we should handle that in the future.
How has this been tested?
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]