Fix for issue #1010 - Merging always marks the database as dirty#1836
Fix for issue #1010 - Merging always marks the database as dirty#1836mbilloo wants to merge 3 commits intokeepassxreboot:developfrom
Conversation
…rrectly emits modified signal
| void TestMerge::testModifiedEmission() | ||
| { | ||
| Database* dbDestination = createTestDatabase(); | ||
| QSignalSpy spy(dbDestination, SIGNAL(modified())); |
There was a problem hiding this comment.
@mbilloo We should check that the modification date of the database has not changed. That would also remove the need to spy on modified.
There was a problem hiding this comment.
This particular test was created to verify that the modified() signal is not emitted when merging identical databases and emitted when merging different databases, to ensure that the changes made to the source are correct for all possible scenarios.
Any other conditions for which the databases should not be merged should be as separate issues and separate test cases.
|
@mbilloo I'm not convinced this is the way to go to fix #1010 What we want is a way to tell if merging the two databases is needed, not if the two databases are equal. Merging might be unnecessary even for database that are not equal (for example if an entry is different but newer in the destination database). I think we should detect if merging is necessary the same way we do in What do you think? |
|
I think the issue was very specific, in that the database was marked dirty even though two identical databases were merged. Tracing this issue back into the source resulted in observing that the modified() signal was blindly emitted in Database::merge. My approach, for this specific issue, was to not even attempt to merge the database (and hence not emit the modified() signal) if the databases were identical. If there are other conditions for which a database should not be merged into an existing database, I think they should be separate issues, or a single issue which outlines these conditions. |
The issue is that the database is marked as dirty even though nothing was performed during the merge operation. I can be that nothing was performed during the merge operation because both databases are exactly the same, but not necessarily. The merge operation uses the modification dates of the matching entries (using the UUID) to determine if merging is needed. I think a function that determines if merging is necessary should be consistent with how the changes are detected in the merge function. |
|
OK, maybe it just makes sense to check for equality of the destination database before and after the merge of the root group. If they're equal after the merge operation, then don't emit the modified() signal. This would rely on the logic in Group::merge() to determine whether or not to merge entries and subgroups. Thoughts? |
|
For what it's worth I think this PR has great value for other features that we may want to implement and check for equality. One thing that would be an enhancement for me is to not only emit a yes/no of equality, but create a small "response" list of what things are different. This could be a list of entries and some flags like title, password, etc. |
|
I will create another PR with what I think is the proper way to determine if the modified() signal should be emitted (in my last comment). |
|
I want this PR merged as well... Unless you are porting this work over to the new one? |
Description
Added appropriate overloaded comparison operators to determine whether two databases are equivalent, and to not merge them if they are equal.
Motivation and context
Fixes #1010
How has this been tested?
Tested using test case described on issue page as well as adding a test under the tests directory.
Screenshots (if appropriate):
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]