Repository navigation
Add "Rescan directory" import prompt choice (v2) - #6983
Closed
barrettclark wants to merge 3 commits into
Closed
barrettclark wants to merge 3 commits into
barrettclark wants to merge 3 commits into
Conversation
When beets can't confidently match an album, the interactive import prompt now offers a "Rescan directory" option alongside Skip/Use as-is/ etc. It re-reads the task's directories from disk and re-runs the match, so files can be cleaned up (duplicates, junk) mid-import without restarting the whole beet import run. - New Action.RESCAN enum member (actions.py) and set_choice wiring (tasks.py). - New "r" PromptChoice, shown for album tasks with a toppath, added to both the "no candidates" and "pick a candidate" prompt variants since they share the same choice list (session.py). - New rescan_tasks() generator and a RESCAN branch in the user_query pipeline stage that re-extends the pipeline with freshly-discovered tasks, mirroring the existing TRACKS/ALBUMS branches (stages.py). - Handles multi-disc albums (nested and flattened/sibling), the "Group albums" choice (no directory scope to rescan, so the choice is withheld for it), and a plugin forcing Action.RESCAN onto a task with no valid rescan scope (singleton, toppath-less, or grouped). - Tests covering added/removed/all files between the initial scan and the rescan, multi-disc grouping, toppath preservation for move pruning and resume state, and the plugin-forced-choice guards (test_importer.py). - Documents the new choice in the auto-tagger guide (docs/guides/tagger.rst). - Changelog entry. Fixes beetbox#166 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NmPv7JDYZi9UqeKE6z312
snejus reported that rescanning a flattened (sibling) multi-disc album could wrongly reimport a loose, already-processed album sharing the same parent directory: the parent is walked to reconstruct the disc grouping, and a post-hoc ancestor/subdirectory filter let the parent itself match as a legitimate "merged subdirectories" target, since nothing excluded the walked root itself from that branch. Drop that ancestor-matching branch entirely rather than special-case around it: since albums_in_dir's collapsing only ever extends a group one walked directory at a time, any real ancestor of an original directory other than their shared walked ancestor would already have been collapsed into the group and so would already be one of the original directories, matched directly by the filter's remaining identity/descendant checks. The shared ancestor is the sole exception, and dropping the branch is what excludes it -- which also means a user merging every original directory's files directly into that ancestor isn't reconstructed; rare, and just as ambiguous as the bug being fixed, so the rescan finds nothing there instead of guessing. That shared ancestor also isn't always simply the first directory's parent: a flattened group's directories aren't necessarily all at the same depth, since the collapse only checks a walked directory's basename, not how deep it is. Use the lowest common ancestor of every original directory instead, so the walk is scoped correctly regardless of which of them happens to be first. Adds regression tests for: the reported bug; a flattened group spanning more than one directory depth in each direction (a disc nested deeper than its sibling, and a disc sitting shallower than its sibling); a rescan of the merge-into-shared-ancestor case that's intentionally not reconstructed; and rescanning the same task twice in a row. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NmPv7JDYZi9UqeKE6z312
An album directory can already hold its own plain, non-disc-named subdirectory with its own music (a bonus-tracks folder, for example). Unlike a disc subdirectory, that isn't collapsed into the parent album's group -- albums_in_dir discovers it as a separate task in its own right, queued right behind the parent's. Rescanning the parent walks its own directory unrestricted (nothing outside it is in scope, so rescan_tasks doesn't filter what that walk finds), which rediscovers the subdirectory too and emits a fresh task for it. The original, still-pending task for that same subdirectory is untouched by any of this, so it goes on to be processed normally -- and the subdirectory gets imported twice. Give ImportSession a small record of directories a "Rescan directory" choice has already re-read from disk, tagged with a "generation" number that increments on every rescan. rescan_tasks marks the exact directories (task.paths) it's about to authoritatively re-read. user_query drops any other task whose paths fall inside an already-marked directory more recently than that task's own `rescan_generation`, defaulting to 0 for a task straight from the initial scan. Any task built fresh in user_query by splitting or rebuilding another task's items -- duplicate-merge, as-Tracks, Group albums -- needs a `rescan_generation` too, or it looks stale by the very check meant to catch everything else and gets silently dropped instead of imported. Two different mistakes showed up here in turn: stamping the pre- replacement task instead of what `ImportTask.handle_created` actually returns (a plugin listening for `import_task_created` can swap in different objects, and only the originals were getting stamped); and, for duplicate-merge specifically, inheriting the *source* task's own generation, when a merge's paths can include library paths pulled in from anywhere -- not just the source task's directory scope -- so an entirely unrelated rescan elsewhere in the same session could still make it look stale. Both are fixed by a single small helper, `_inherit_rescan_generation`, that stamps the session's current generation (not any particular task's) onto whatever a task actually continues through the pipeline as. rescan_tasks also no longer branches on whether task.paths all nest under their first entry: os.path.commonpath(task.paths) gives the correct walk root either way (it's provably the same value as the first entry alone in the nested case), so the separate "walk it directly, no filtering needed" path was always doing exactly what the general path already does, just with more code to say so. Also strengthens several guards' test coverage, found via mutation testing to be silently load-bearing without a test that would actually fail if they were removed: `group_albums` must re-apply `is_grouped` to what `handle_created` returns for the same plugin-replacement reason as `rescan_generation` above; the plugin-forced-Rescan-on-a- grouped-task guard's existing test asserted only "nothing got imported", a symptom just as consistent with the guard being entirely absent (a grouped task's item-file paths can never match a directory walk regardless) as with it firing correctly; and the choice's withholding for a toppath-less task (e.g. produced by a library query) had no test of its own, only the analogous Group-albums case. Removes an `Action.RESCAN` entry added to `choose_match`'s pass-through tuple that turned out to be dead code following an already-dead pattern: `choose_candidate` never returns a bare `importer.Action`, only `AlbumMatch`/`TrackMatch`/`PromptChoice`, so Rescan (like the pre-existing Skip/As-is entries there) actually reaches `user_query` through the `PromptChoice.callback` branch a few lines down, not this one. Adds regression tests for: the reported duplicate-import case; that same case surviving a second rescan without leaving the first rescan's output stranded; a sibling-disc rescan not dropping an unrelated album that happens to sort after the disc directories under their shared parent; duplicate-merge, as-Tracks, and Group albums each actually taking effect when chosen right after a rescan, including with a task-replacing plugin listening on `import_task_created` for the latter two; a duplicate-merge against an unrelated album that an earlier, unconnected rescan in the same session had already marked; and the coverage gaps above. Clarifies in rescan_tasks's docstring that discovery itself is never reimplemented here -- every group comes from the same factory logic the original scan uses -- and that what rescan_tasks does reconstruct is only the narrowest directory that logic needs to be re-run from, as a bounded alternative to always re-walking the task's entire toppath. Also notes in the changelog that the new choice's `r` shortcut is now reserved by the default prompt, displacing any plugin-provided choice bound to the same letter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011NmPv7JDYZi9UqeKE6z312
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6983 +/- ##
==========================================
+ Coverage 76.85% 76.96% +0.10%
==========================================
Files 163 163
Lines 21627 21680 +53
Branches 3344 3354 +10
==========================================
+ Hits 16621 16685 +64
+ Misses 4190 4179 -11
Partials 816 816
🚀 New features to boost your workflow:
|
Contributor
Author
|
Superseded — pushed straight to #6945 instead (draft PRs don't get Copilot review, so this route wasn't buying anything). Closing. |
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.
Description
Draft companion to #6945 ("Add 'Rescan directory' import prompt choice"). This branch redoes the "Rescan directory" scoping/reconstruction logic that #6945's review has been iterating on, in response to snejus's feedback that rescanning shouldn't reimplement directory-grouping logic separately from the normal scan.
Opening this as a separate draft (rather than pushing straight to #6945) to work through Copilot's automated review here first, since the review thread on #6945 has already been long. Once this settles, I'll bring it back into #6945 (or this can simply replace it, whichever's easier on your end).
What changed relative to #6945's current state
rescan_tasksnow reuses the exact same discovery/grouping logic (ImportTaskFactory.paths()/albums_in_dir) the normal scan uses, rather than a separate reconstruction — it only ever has to figure out the smallest directory that logic needs to be re-run from (os.path.commonpath(task.paths)), then filters the walk's output back down to the groups related to the original task.ImportSessionnow tracks which directories a rescan has authoritatively re-read, with a small generation counter so a task built fresh from a rescanned task's items (duplicate-merge, "as Tracks", "Group albums") isn't mistaken for stale output.import_task_created.Known items
uv.lockhas a one-line version bump (2.13.0→2.13.1) picked up incidentally:pyproject.tomlon master already says2.13.1, but master'suv.lockdoesn't, and this repo's local pre-commit hook re-syncs it on every commit. Happy to drop it if you'd rather keep that separate.To Do