DupFileManager: merge play and O history instead of discarding it - #759
Open
johnkattenhorn wants to merge 1 commit into
Open
DupFileManager: merge play and O history instead of discarding it#759johnkattenhorn wants to merge 1 commit into
johnkattenhorn wants to merge 1 commit into
Conversation
Merging a duplicate copied tags, performers, galleries, urls, studio, title, director, date, details, rating and code — but not the watch history. Play count, O count, total play duration and the organized flag were all left on the scene being deleted, so merge-then-delete silently destroyed them. Reported in issue stashapp#605. Setting play_count or o_counter through update_scene does not work: both are deprecated as unsupported on SceneUpdateInput and the values are dropped. sceneAddPlay and sceneAddO take a list of timestamps, so the fix merges the actual history rather than only bumping a counter — the merged scene keeps when each play happened, not just how many there were. Counts, history and play duration are cumulative, so they are summed across the two scenes. The organized flag follows the existing fill-the-blank rule used for the other fields: it is copied only when the destination does not already have it set. Two things were needed to make this work from the duplicate report, and both only showed up when running the real plugin task rather than calling merge directly: find_duplicate_scenes_diff returns a reduced scene fragment that carries none of the playback fields, and mergeItem indexed fields directly, so merging organized raised KeyError and every merge failed five retries deep while deletion carried on regardless. merge now re-reads both scenes when the playback fields are absent, and mergeItem skips fields the query did not return. Tested against Stash v0.31.1 in Docker with two real phash duplicates — one source encoded at two qualities — driven through the plugin's own Delete Duplicates task with Merge Duplicate Tags enabled. Empty destination: play_count 4, o_counter 2, play_duration 250 and both history lists carry onto the surviving scene with timestamps intact, and the duplicate is deleted. Destination with its own history: 4+1 plays, 2+1 Os, 250+50 duration all sum correctly, the history lists are the union of both, and the destination keeps its own title, so fill-the-blank semantics are unchanged. Applied to all three copies of StashPluginHelper.py, which were byte-identical in this region. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or 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
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.
Fixes #605.
The problem
mergeMetadata.merge()copies tags, performers, galleries, urls, studio, title, director, date, details, rating100 and code — but nothing about how the scene was watched. Play count, O count, total play duration and theorganizedflag are all left behind on the scene that is about to be deleted.So merge-then-delete silently destroys watch history. For a deduplication tool that is the one thing that must not happen: the file is replaceable, the history is not.
Why the obvious fix does not work
Setting
play_countoro_counterthroughupdate_scenehas no effect — both are@deprecated(reason: "Unsupported")onSceneUpdateInput, and the values are dropped server-side.sceneAddPlayandsceneAddOtake a list of timestamps. That is a better fix than the issue asks for: the merged scene keeps when each play happened, not merely how many there were.What this does
sceneAddPlay/sceneAddO.organized— follows the existing fill-the-blank rule used by every other scalar field: copied only when the destination does not already have it set.Counts and history are cumulative and therefore summed, not overwritten. Existing fill-the-blank behaviour for the other fields is untouched.
Two fixes that only appeared under end-to-end testing
Calling
mergeMetadatadirectly passes full scene dictionaries, and against that the change worked first time. Running the plugin's real Delete Duplicates task told a different story, twice:KeyError: 'organized'— every merge failed.find_duplicate_scenes_diffreturns a reduced scene fragment, andmergeItemindexes fields directly. Mergingorganizedraised on every pair, burned all five retries, and the merge never happened — while deletion carried on regardless.mergeItemnow skips fields the query did not return.The history merge silently did nothing. With the
KeyErrorguarded, merges appeared to succeed, but that reduced fragment carries none of the playback fields, so every history field was skipped.merge()now re-reads both scenes when the playback fields are absent.Worth flagging for maintainers: when the merge fails, deletion still proceeds. That is how a reported merge failure becomes lost data rather than a skipped merge.
Testing
Against a clean Stash v0.31.1 in Docker with two genuine phash duplicates (one source encoded at two qualities, distance 1), driven through the plugin's own Delete Duplicates task with Merge Duplicate Tags enabled.
Empty destination — the doomed copy carries
play_count 4,o_counter 2,play_duration 250, rating, title andorganized:Destination with its own history — the case where both copies were watched:
Also covered: the integer-id entry point used by the report path (
mergeMetadata(int, int)) — counts and duration sum correctly there too.Before this change, the same fixture produced
play_count 4 -> 0,o_counter 2 -> 0,organized true -> falseon the surviving scene.Scope
Applied to all three copies of
StashPluginHelper.py, which are byte-identical in this region. FileMonitor and RenameFile are covered by a compile check only — neither appears to exercise the merge class.Deliberately left for separate changes, to keep this reviewable:
stash_ids(StashDB links) and scene markers are still not merged.# ToDo: Figure out how to merge groupsat the top ofmerge()is untouched.resume_timeis intentionally not merged — unlike the others it is a playback position rather than an accumulated total, and there is no obviously correct answer when both scenes have one.Independent of #757 — different region of the same file, so the two apply in either order.
LLM-assisted contribution disclosure
Per the repository's contribution policy: this change was prepared with LLM assistance (Claude). The diff has been reviewed by me, the testing described above was carried out and its results are reproduced verbatim, and I take full responsibility for the change and its license compliance.