diff --git a/core/library_reorganize.py b/core/library_reorganize.py index a7e4fa37..17cc323d 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -26,6 +26,7 @@ without a source ID are reported back to the caller and skipped entirely. """ +import errno import os import re import shutil @@ -34,7 +35,7 @@ import time import uuid from concurrent.futures import FIRST_COMPLETED, ThreadPoolExecutor, wait from dataclasses import dataclass, field -from typing import Any, Callable, Dict, List, Optional, Set +from typing import Any, Callable, Dict, List, Optional, Set, Tuple # Per-album track concurrency. Matches the download workers' per-batch # concurrency (3) so reorganize feels comparable to a fresh download. @@ -1101,6 +1102,12 @@ def preview_album_reorganize( 'track_number': track.get('track_number', 0), 'current_path': _trim_to_transfer(db_path, resolved, transfer_dir), 'new_path': '', + # Absolute on-disk paths (additive). `current_path`/`new_path` above are + # display-trimmed; these carry the real paths so the rename-only executor + # acts on EXACTLY what the preview computed — no separate path logic that + # could drift from what the user saw (#875). + 'current_path_abs': resolved or '', + 'new_path_abs': '', 'file_exists': resolved is not None, 'unchanged': False, 'collision': False, @@ -1148,6 +1155,7 @@ def preview_album_reorganize( new_full, _ok = build_final_path_fn( context, spotify_artist, album_info, file_ext, create_dirs=False ) + item['new_path_abs'] = new_full or '' item['new_path'] = ( os.path.relpath(new_full, transfer_dir) if transfer_dir and new_full and new_full.startswith(transfer_dir) @@ -1807,6 +1815,150 @@ def reorganize_album( return summary +def _rename_track_in_place(current_abs: str, new_abs: str) -> Tuple[bool, Optional[str]]: + """Move ONE file from ``current_abs`` to ``new_abs`` in place — no copy, no re-tag, + no post-processing. Creates the destination folder, carries sibling-format files + (e.g. a lossy ``.opus`` alongside the ``.flac``) along with the renamed stem, and + falls back to a cross-device move when the rename crosses a filesystem boundary. + + Refuses to overwrite a DIFFERENT existing file at the destination (returns an error + instead) — never silent data loss. Returns ``(ok, error_message)``. + """ + try: + if current_abs and not os.path.exists(current_abs): + return False, 'source file no longer on disk' + same = os.path.normpath(current_abs) == os.path.normpath(new_abs) + if os.path.exists(new_abs) and not same: + return False, 'destination already exists' + os.makedirs(os.path.dirname(new_abs), exist_ok=True) + # Carry sibling-format audio to the same destination with the renamed stem — + # mirrors _finalize_track so lossy-copy pairs don't get orphaned. + for sibling_src in _find_sibling_audio_files(current_abs): + _move_sibling_to_destination(sibling_src, new_abs) + try: + os.rename(current_abs, new_abs) + except OSError as e: + if getattr(e, 'errno', None) == errno.EXDEV: + shutil.move(current_abs, new_abs) # crosses a filesystem boundary + else: + raise + return True, None + except Exception as e: + return False, str(e) + + +def reorganize_album_rename_only( + *, + album_id: str, + db, + transfer_dir: str, + resolve_file_path_fn: Callable[[Optional[str]], Optional[str]], + build_final_path_fn: Callable, + update_track_path_fn: Optional[Callable[[object, str], None]] = None, + cleanup_empty_dir_fn: Optional[Callable[[str], None]] = None, + on_progress: Optional[Callable[[dict], None]] = None, + primary_source: Optional[str] = None, + strict_source: bool = False, + metadata_source: str = 'api', + stop_check: Optional[Callable[[], bool]] = None, + preview_fn: Optional[Callable] = None, +) -> dict: + """RENAME-ONLY reorganize (#875): move each track's file to the path the current + naming scheme dictates, and nothing else — no copy-to-staging, no re-tag, no + quality/AcoustID checks. + + It acts on EXACTLY what :func:`preview_album_reorganize` computed (injected via + ``preview_fn`` for testability), so the apply can never disagree with what the user + saw, and ONLY files whose path actually changes are touched — files marked + ``unchanged`` are skipped, which is what keeps a rename from rewriting the whole + album (the #875 complaint). Tags and audio are left byte-for-byte alone. + + Returns the same summary shape as :func:`reorganize_album`. + """ + preview_fn = preview_fn or preview_album_reorganize + summary = { + 'status': 'completed', 'source': None, 'total': 0, + 'moved': 0, 'skipped': 0, 'failed': 0, 'errors': [], + } + + def _emit(**updates): + if on_progress is None: + return + try: + on_progress(updates) + except Exception as e: + logger.debug("[Reorganize/rename] progress emit failed: %s", e) + + preview = preview_fn( + album_id=album_id, db=db, transfer_dir=transfer_dir, + resolve_file_path_fn=resolve_file_path_fn, + build_final_path_fn=build_final_path_fn, + primary_source=primary_source, strict_source=strict_source, + metadata_source=metadata_source, + ) + summary['source'] = preview.get('source') + if not preview.get('success'): + summary['status'] = preview.get('status', 'error') + return summary + + tracks = preview.get('tracks', []) + summary['total'] = len(tracks) + src_dirs_touched: Set[str] = set() + + for t in tracks: + if stop_check and stop_check(): + break + title = t.get('title', 'Unknown') + _emit(current_track=title) + + # Skip anything that isn't a real, changing move. `unchanged` is the key one — + # it's why a rename no longer rewrites files whose name didn't change. + if (not t.get('matched') or t.get('unchanged') + or t.get('collision') or not t.get('new_path_abs')): + summary['skipped'] += 1 + _emit(skipped=summary['skipped']) + continue + + current_abs = t.get('current_path_abs') + new_abs = t.get('new_path_abs') + ok, err = _rename_track_in_place(current_abs, new_abs) + if not ok: + summary['failed'] += 1 + summary['errors'].append({ + 'track_id': t.get('track_id'), 'title': title, + 'error': err or 'rename failed', + }) + _emit(failed=summary['failed'], errors=list(summary['errors'])) + continue + + # File is at its new home — update the DB directly (authoritative; no need to + # round-trip through a server scan to learn what we just did). On DB failure the + # file still moved; a library scan reconciles it, so we don't fail the track. + if update_track_path_fn: + try: + update_track_path_fn(t.get('track_id'), new_abs) + except Exception as db_err: + logger.warning( + "[Reorganize/rename] DB path update failed for %s: %s " + "(file moved to %s; a scan will reconcile)", + t.get('track_id'), db_err, new_abs, + ) + if current_abs: + src_dirs_touched.add(os.path.dirname(current_abs)) + summary['moved'] += 1 + _emit(moved=summary['moved'], + processed=summary['moved'] + summary['skipped'] + summary['failed']) + + if cleanup_empty_dir_fn: + for src_dir in src_dirs_touched: + try: + cleanup_empty_dir_fn(src_dir) + except Exception as e: + logger.debug("[Reorganize/rename] cleanup of %s failed: %s", src_dir, e) + + return summary + + def _find_artist_dir(dest_path: str, transfer_dir: str) -> Optional[str]: """Walk up from ``dest_path`` until the parent equals ``transfer_dir``; the directory at that point is the artist folder. Returns None if diff --git a/tests/test_reorganize_rename_only.py b/tests/test_reorganize_rename_only.py new file mode 100644 index 00000000..4bd3bb87 --- /dev/null +++ b/tests/test_reorganize_rename_only.py @@ -0,0 +1,168 @@ +"""Rename-only reorganize (#875): move files to the current naming scheme with NO +copy / re-tag / post-processing. The headline guarantee is that it acts on exactly +what the preview computed and ONLY touches files whose path actually changes — files +the preview marked `unchanged` are left alone (the "every file got modified" bug). +""" + +import os + +from core.library_reorganize import ( + _rename_track_in_place, + reorganize_album_rename_only, +) + + +# ── _rename_track_in_place ── + +def test_rename_moves_file_and_creates_dest_dir(tmp_path): + src = tmp_path / "old" / "01 - Song.flac" + src.parent.mkdir(parents=True) + src.write_bytes(b"audio") + dst = tmp_path / "new" / "Song - Artist.flac" + + ok, err = _rename_track_in_place(str(src), str(dst)) + assert ok and err is None + assert dst.exists() and dst.read_bytes() == b"audio" + assert not src.exists() + + +def test_rename_refuses_to_overwrite_a_different_file(tmp_path): + src = tmp_path / "a.flac" + src.write_bytes(b"source") + dst = tmp_path / "b.flac" + dst.write_bytes(b"someone else") # a DIFFERENT existing file + + ok, err = _rename_track_in_place(str(src), str(dst)) + assert not ok and "exists" in err + assert src.exists() and dst.read_bytes() == b"someone else" # nothing destroyed + + +def test_rename_missing_source_errors(tmp_path): + ok, err = _rename_track_in_place(str(tmp_path / "gone.flac"), str(tmp_path / "x.flac")) + assert not ok and "no longer on disk" in err + + +def test_rename_same_path_is_noop_ok(tmp_path): + f = tmp_path / "x.flac" + f.write_bytes(b"a") + ok, err = _rename_track_in_place(str(f), str(f)) + assert ok and f.exists() + + +def test_rename_carries_sibling_format_file(tmp_path): + # lossy-copy pair: canonical .flac + sibling .opus in the same folder + src = tmp_path / "old" / "01 - Song.flac" + src.parent.mkdir(parents=True) + src.write_bytes(b"flac") + sib = tmp_path / "old" / "01 - Song.opus" + sib.write_bytes(b"opus") + dst = tmp_path / "new" / "Song.flac" + + ok, _ = _rename_track_in_place(str(src), str(dst)) + assert ok + assert dst.exists() + assert (tmp_path / "new" / "Song.opus").exists() # sibling came along, renamed stem + + +# ── reorganize_album_rename_only (fake preview injected) ── + +def _fake_preview(tracks, *, success=True, status="planned", source="deezer"): + def _preview(**_kw): + return {"success": success, "status": status, "source": source, "tracks": tracks} + return _preview + + +def _run(tracks, *, update=None, cleanup=None, stop=None, **preview_kw): + return reorganize_album_rename_only( + album_id="A1", db=None, transfer_dir="/x", + resolve_file_path_fn=lambda p: p, + build_final_path_fn=lambda *a, **k: (None, True), + update_track_path_fn=update, + cleanup_empty_dir_fn=cleanup, + stop_check=stop, + preview_fn=_fake_preview(tracks, **preview_kw), + ) + + +def test_moves_changed_and_skips_unchanged(tmp_path): + """THE regression: a changed track moves + DB updates; an `unchanged` track is + left completely alone (not re-touched). This is the #875 fix in one test.""" + src = tmp_path / "old" / "01 - A.flac" + src.parent.mkdir(parents=True) + src.write_bytes(b"a") + new = tmp_path / "new" / "A - Artist.flac" + keep = tmp_path / "keep" / "B.flac" + keep.parent.mkdir(parents=True) + keep.write_bytes(b"b") + + updates = [] + summary = _run( + [ + {"track_id": "t1", "title": "A", "matched": True, "unchanged": False, + "collision": False, "current_path_abs": str(src), "new_path_abs": str(new)}, + {"track_id": "t2", "title": "B", "matched": True, "unchanged": True, + "collision": False, "current_path_abs": str(keep), "new_path_abs": str(keep)}, + ], + update=lambda tid, path: updates.append((tid, path)), + ) + + assert summary["moved"] == 1 and summary["skipped"] == 1 and summary["failed"] == 0 + assert new.exists() and not src.exists() # changed track moved + assert keep.exists() and keep.read_bytes() == b"b" # unchanged: untouched + assert updates == [("t1", str(new))] # DB updated ONLY for the moved one + + +def test_collision_and_unmatched_are_skipped(tmp_path): + summary = _run([ + {"track_id": "c", "title": "C", "matched": True, "unchanged": False, + "collision": True, "current_path_abs": "/a", "new_path_abs": "/b"}, + {"track_id": "u", "title": "U", "matched": False, "unchanged": False, + "collision": False, "current_path_abs": "/a", "new_path_abs": "/b"}, + ]) + assert summary["skipped"] == 2 and summary["moved"] == 0 and summary["failed"] == 0 + + +def test_failed_rename_is_counted_not_fatal(tmp_path): + src = tmp_path / "a.flac" + src.write_bytes(b"src") + dst = tmp_path / "taken.flac" + dst.write_bytes(b"occupied") # forces "destination already exists" + summary = _run([ + {"track_id": "t", "title": "T", "matched": True, "unchanged": False, + "collision": False, "current_path_abs": str(src), "new_path_abs": str(dst)}, + ]) + assert summary["failed"] == 1 and summary["moved"] == 0 + assert summary["errors"] and summary["errors"][0]["track_id"] == "t" + assert src.exists() and dst.read_bytes() == b"occupied" # nothing lost + + +def test_preview_failure_returns_its_status(): + summary = _run([], success=False, status="no_source_id") + assert summary["status"] == "no_source_id" + assert summary["moved"] == 0 + + +def test_stop_check_aborts_early(tmp_path): + src = tmp_path / "a.flac" + src.write_bytes(b"a") + summary = _run( + [{"track_id": "t", "title": "T", "matched": True, "unchanged": False, + "collision": False, "current_path_abs": str(src), "new_path_abs": str(tmp_path / "b.flac")}], + stop=lambda: True, + ) + assert summary["moved"] == 0 # aborted before processing + assert src.exists() + + +def test_cleanup_called_for_emptied_source_dirs(tmp_path): + src = tmp_path / "old" / "01 - A.flac" + src.parent.mkdir(parents=True) + src.write_bytes(b"a") + cleaned = [] + _run( + [{"track_id": "t1", "title": "A", "matched": True, "unchanged": False, + "collision": False, "current_path_abs": str(src), + "new_path_abs": str(tmp_path / "new" / "A.flac")}], + cleanup=lambda d: cleaned.append(d), + ) + assert str(tmp_path / "old") in cleaned