keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.29k stars 1.48k forks source link

Allow reviewing changes before/after merging #1152

Open phanimahesh opened 7 years ago

phanimahesh commented 7 years ago

Expected Behavior

I would like to see exactly what will happen when I'm merging databases. Added entries, removed/moved entries, modified entries. Just the path to entry (group/.../title), modification time and updated fields should be good enough.

Current Behavior

There is no feedback on changes happening during a merge.

Possible Solution

When merging, add an option to review and merge, which shows a summary of changes? Users who don't need this can use the other option, merge to skip reviewing.

Context

Each of my devices uses its own local copy and periodically I do a merge into and with a shared and synced copy, to avoid issues when sharing a database. If all devices operate on a shared version, and some devices have intermittent network connectivity or don't sync all the time it is possible to lose data since last update wins when syncing.

I'm worried about accidentally merging the wrong version and "losing" an update (not really sure if it is a real issue, given that entries track their history). Being able to review the diff will also tell me quickly if an entry I'm looking for is included in this merge, without having to merge everything.

Debug Info

eePassXC - Version 2.2.2 Revision: 6d46717

Libraries:

Operating system: Arch Linux CPU architecture: x86_64 Kernel: linux 4.13.7-1-ARCH

Enabled extensions:

ghostsquad commented 2 years ago

I recently found this tool: https://github.com/Narigo/keepass-diff

It would be really nice if a diff could be done. Even better if it was interactive like git, such that I could pick and choose what changes to keep.

erik78se commented 2 years ago

I too would like to see this feature as I'm constantly worried my entries are getting nuked by a merge.

droidmonkey commented 2 years ago

Worry not! The original entry state is pushed into the entry's history. You can always revert to the previous version if needed. Make sure history is enabled for your database in the database settings.

erik78se commented 2 years ago

I don't think we mean the same thing.

I'm looking for knowing what entries have been changed as part of a merge. Today - its just a message something like "Merged successfully". But I wouldn't know anything about "WHAT" has been changed. It leaves me in a state of worry about me possibly have merged something I didn't like to or some accidental change which would cause havoc.

A list of net changes to a database merge from old to new would be very helpful. It surprises me every time that I can't find that feature when I'm looking for it in the menus.

ghostsquad commented 2 years ago

100% what @erik78se wrote. Iteration 1: show me the diff. Iteration 2: act like git and let me choose what changes I'd like to persist.

schnerring commented 1 year ago

Could we at least log the merge operations somewhere?

I use Syncthing to sync my database between my workstation and phone. I'm aware of the limitations of this approach, but since my password DB is never mutated from both the workstation and phone simultaneously, it's fine. I have been doing it like this for many years.

Merge conflicts sometimes occur when I forget to connect my phone via WiFi for a while (days to weeks). I occasionally check for Syncthing merge conflicts, and if I find any, I merge the out-of-sync databases. However, it's hard to remember what I've changed when the merge conflict is already 2 weeks old. My DB is pretty big, so manually looking through histories is also unfeasible.

I'd love a merge tool, but I'd already be happy if the success dialog box would at least give me a text log of what's been changed.

droidmonkey commented 1 year ago

We offer that through the CLI tool right now, so if you have a major merge you can do that with the cli and see the log output.

You can also, post merge, search your database for * and sort by last modified to see which entries changed.

I absolutely want to introduce a merge log and conflict manager to v2.8.x.

taminob commented 10 months ago

I'd be interested in working on this issue since I also run into this issue quite often and I currently manually merge databases by sorting by modification time.

As an MVP I'd propose a simple dialog listing all new/modified entries by group/title and a button to confirm or abort the merge. In further iterations this could be refined into a separate list for modified entries showing the actual difference (e.g. in the style of the history view), the possibility to exclude certain entries or display "similar items" with e.g. same/similar group+title but different UUID. In the end, this could then also be extended to offer conflict resolution.

I already had a look at the code base and it should be possible to add a dry_run flag to Merger::merge() in src/core/Merger.cpp to get the necessary changelist without immediately applying the changes. The merge is done in DatabaseWidget::mergeDatabase() in src/gui/DatabaseWidget.cpp where it would be possible to display the dialog.

droidmonkey commented 10 months ago

I'm very open to an mvp for this. Keep it simple.

MathiasLui commented 8 months ago

I just changed a specific password a few (maybe 5) times on my phone in DX, on my already opened Desktop, which is synced via syncthing it asked me 5 times if I wanted to merge the changes and I clicked "yes" every time. The password that was there in the end, was actually the very first one, before any of the 5 changes...

taminob commented 8 months ago

I just changed a specific password a few (maybe 5) times on my phone in DX, on my already opened Desktop, which is synced via syncthing it asked me 5 times if I wanted to merge the changes and I clicked "yes" every time. The password that was there in the end, was actually the very first one, before any of the 5 changes...

Thanks for testing it that thoroughly, looks like there is some work to do. :) I'll need to rewrite the PR in the next days anyways and will try a different approach - but that approach will probably disable auto-reload while merging and it will still only keep the password that was in the database when initiating the merge (although it will also ask only once for the merge). Is that the behavior you would expect? Or should a changed database on disk abort the merge?

MathiasLui commented 8 months ago

Thanks for testing it that thoroughly, looks like there is some work to do. :) I'll need to rewrite the PR in the next days anyways and will try a different approach - but that approach will probably disable auto-reload while merging and it will still only keep the password that was in the database when initiating the merge (although it will also ask only once for the merge). Is that the behavior you would expect? Or should a changed database on disk abort the merge?

Actually - I have used the newest release build, it looks like you're assuming I have tested the PR, but I didn't notice it exists until now, and blindly commented, sorry 😅

I feel like a database change on disk during a merge of a previous change should add that new change to the merges to be resolved - is that the behaviour that was added? Does it also preview e.g. the password to be compared?

droidmonkey commented 8 months ago

Have you checked that entry's history?

taminob commented 8 months ago

Actually - I have used the newest release build, it looks like you're assuming I have tested the PR, but I didn't notice it exists until now, and blindly commented, sorry 😅

Ah, I thought your "clicking yes" meant the confirmation dialog. :)

I feel like a database change on disk during a merge of a previous change should add that new change to the merges to be resolved - is that the behaviour that was added? Does it also preview e.g. the password to be compared?

The exact changes can not be compared at the moment - that's something that should probably be handled in a separate PR (might be non-trivial regarding changes in e.g. an entry's history).

MathiasLui commented 8 months ago

Ah, I thought your "clicking yes" meant the confirmation dialog. :)

Yes, it simply asked if I wanted to merge several times in a row with the options "Yes" and "No"