All: Fixes #13782: When cleaning old revisions, ensure revisions are merged for all revision branches #13795
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.
Due to the distributed nature of the revision collection and cleaning in Joplin, the chain of revisions which exist for a note may not be linear. Currently, the revision cleaning logic assumes that the chain of revisions is linear, assuming only 1 revision needs to be updated to contain the merged contents of deleted revisions. This means that when the chain is not linear, part or all of the revisions for a note can become corrupted due to the 'surviving' revision for a particular branch within the revision list, not being merged before deleting its parents. For more details, see issue #13782 which this PR fixes.
This PR addresses the issue, by identifying the first child of all revisions to be deleted, and merging the diffs for children which are on or after the note history cut off date, before deleting the revisions. Additionally, I have added a new index to the db, to ensure that the new revision cleaning logic will not perform poorly with a high volume of data.
Testing
Using a backed up snapshot of my own Joplin profile, which I switched over to use file system sync, I did the following tests:
So basically I manually tested that the existing behaviour remains unchanged. For testing the actual scenarios of multiple surviving children needing to be merged, this is covered by unit tests on the PR, as it would be difficult to set up the data in the right state in the database, unless I manipulated the data manually.
I also verified in the sqlite db directly that the migration script correctly creates the new index: