From 9c9fdad50432a856d716633aef73a1a1356cd4e0 Mon Sep 17 00:00:00 2001 From: Christoph Obermair Date: Wed, 29 Jul 2026 14:07:51 +0200 Subject: [PATCH] Fix case-only path reconciliation --- UserGuide.md | 13 +- src/analyse.rs | 4 +- src/db.rs | 440 +++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 400 insertions(+), 57 deletions(-) diff --git a/UserGuide.md b/UserGuide.md index b2d6d42..3d4434a 100644 --- a/UserGuide.md +++ b/UserGuide.md @@ -88,12 +88,15 @@ Command-line parameters * `-d` / `--db` Name and location of the database file. * `-l` / `--logging` Logging level; `trace`, `debug`, `info`, `warn`, `error`. Default is `info`. -* `-k` / `--keep-old` When analysing tracks, `bliss-analyser` will remove any -tracks specified in its database that are no-longer on the file-system. This -parameter is used to prevent this. +* `-k` / `--keep-old` When analysing tracks, `bliss-analyser` will remove database +rows for tracks that are no longer on the filesystem. It also reconciles stored +case-only path differences with the filesystem's exact spelling, preserving the +existing analysis, and removes stale case-only duplicates. This option prevents +all of those cleanup actions. * `-r` / `--dry-run` If this is supplied when analysing tracks, then no actual -analysis will be performed, instead the logging will inform you how many new -tracks are to be analysed and how many old tracks are left in the database. +analysis or database cleanup will be performed, instead the logging will inform +you how many new tracks are to be analysed, how many old tracks are left in the +database, and which stored paths would have their filename casing corrected. * `-i` / `--ignore` Name and location of the file containing items to ignore. * `-L` / `--lms` Hostname, or IP address, of your LMS server. * `-J` / `--json` JSONRPC port number of your LMS server. diff --git a/src/analyse.rs b/src/analyse.rs index 43e5d09..d4043fc 100644 --- a/src/analyse.rs +++ b/src/analyse.rs @@ -393,12 +393,12 @@ pub fn analyse_files(db_path: &str, mpaths: &Vec, dry_run: bool, keep_o db.init(); + let mut changes_made = false; if !keep_old { send_notif(&mut notifs, "Removing old files from DB"); - db.remove_old(mpaths, dry_run); + changes_made = db.remove_old(mpaths, dry_run); } - let mut changes_made = false; for path in mpaths { let mpath = path.clone(); let cur = path.clone(); diff --git a/src/db.rs b/src/db.rs index 7188c7f..4c6c395 100644 --- a/src/db.rs +++ b/src/db.rs @@ -10,10 +10,12 @@ use crate::tags; use bliss_audio::{Analysis, AnalysisIndex, FeaturesVersion}; use indicatif::{ProgressBar, ProgressStyle}; use rusqlite::{params, Connection}; +use std::collections::{HashMap, HashSet}; use std::convert::TryInto; -use std::fs::File; +use std::ffi::OsString; +use std::fs::{self, File}; use std::io::{BufRead, BufReader}; -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; use std::process; use std::thread; use std::thread::JoinHandle; @@ -76,6 +78,159 @@ pub struct Db { pub conn: Connection, } +#[derive(Debug, PartialEq)] +enum PathResolution { + Exact, + Corrected(String), + Missing, + Unknown, +} + +struct ExactPathResolver { + roots: Vec, + directory_entries: HashMap>, +} + +struct DirectoryEntries { + exact: HashSet, + case_insensitive: HashMap>, +} + +impl ExactPathResolver { + fn new(roots: &Vec) -> Self { + Self { + roots: roots.to_vec(), + directory_entries: HashMap::new(), + } + } + + fn entries(&mut self, directory: &Path) -> Option<&DirectoryEntries> { + if !self.directory_entries.contains_key(directory) { + let entries = match fs::read_dir(directory) { + Ok(entries) => { + let names = entries.map(|entry| entry.map(|entry| entry.file_name())).collect::>>(); + match names { + Ok(names) => { + let mut exact = HashSet::with_capacity(names.len()); + let mut case_insensitive = HashMap::with_capacity(names.len()); + + for name in names { + exact.insert(name.clone()); + if let Some(entry_text) = name.to_str() { + let folded = entry_text.to_lowercase(); + match case_insensitive.get_mut(&folded) { + Some(existing) => *existing = None, + None => { + case_insensitive.insert(folded, Some(name)); + } + } + } + } + + Some(DirectoryEntries { + exact, + case_insensitive, + }) + } + Err(_) => None, + } + } + Err(_) => None, + }; + self.directory_entries + .insert(directory.to_path_buf(), entries); + } + self.directory_entries + .get(directory) + .and_then(|entries| entries.as_ref()) + } + + fn resolve_under_root(&mut self, root: &Path, relative: &Path) -> PathResolution { + let mut current = root.to_path_buf(); + let mut resolved = PathBuf::new(); + let mut corrected = false; + for component in relative.components() { + let requested = match component { + Component::CurDir => continue, + Component::Normal(name) => name, + _ => return PathResolution::Unknown, + }; + let entries = match self.entries(¤t) { + Some(entries) => entries, + None => return PathResolution::Unknown, + }; + let actual = if entries.exact.contains(requested) { + requested.to_os_string() + } else { + let folded = match requested.to_str() { + Some(requested) => requested.to_lowercase(), + None => return PathResolution::Missing, + }; + let actual = match entries.case_insensitive.get(&folded) { + Some(Some(actual)) => actual.clone(), + Some(None) => return PathResolution::Unknown, + None => return PathResolution::Missing, + }; + corrected = true; + actual + }; + current.push(&actual); + resolved.push(actual); + } + if resolved.as_os_str().is_empty() { + PathResolution::Unknown + } else if corrected { + PathResolution::Corrected(normalize_db_path(&resolved)) + } else { + PathResolution::Exact + } + } + + fn resolve(&mut self, db_path: &str) -> PathResolution { + let (file_path, cue_suffix) = match db_path.find(CUE_MARKER) { + Some(index) => (&db_path[..index], &db_path[index..]), + None => (db_path, ""), + }; + let relative = PathBuf::from(file_path); + let mut correction: Option = None; + let mut unknown = false; + for root in self.roots.clone() { + match self.resolve_under_root(&root, &relative) { + PathResolution::Exact => return PathResolution::Exact, + PathResolution::Corrected(path) => { + let candidate = format!("{}{}", path, cue_suffix); + match correction.as_ref() { + Some(existing) if existing != &candidate => { + unknown = true; + } + _ => { + correction = Some(candidate); + } + } + } + PathResolution::Unknown => unknown = true, + PathResolution::Missing => {} + } + } + if unknown { + PathResolution::Unknown + } else if let Some(path) = correction { + PathResolution::Corrected(path) + } else { + PathResolution::Missing + } + } +} + +fn normalize_db_path(path: &Path) -> String { + let path = path.to_string_lossy().to_string(); + if cfg!(windows) { + path.replace("\\", "/") + } else { + path + } +} + impl Db { pub fn new(path: &String) -> Self { match Connection::open(path) { @@ -230,9 +385,11 @@ impl Db { let _ = self.conn.execute("DELETE FROM Failures WHERE File = ?;", params![path]); } - pub fn remove_old(&self, mpaths: &Vec, dry_run: bool) { - self.remove_old_from_table(mpaths, dry_run, "TracksV2"); - self.remove_old_from_table(mpaths, dry_run, "Failures"); + pub fn remove_old(&self, mpaths: &Vec, dry_run: bool) -> bool { + let mut resolver = ExactPathResolver::new(mpaths); + let tracks_changed = self.remove_old_from_table(dry_run, "TracksV2", &mut resolver); + let failures_changed = self.remove_old_from_table(dry_run, "Failures", &mut resolver); + tracks_changed || failures_changed } fn clear_failures(&self) { @@ -254,64 +411,85 @@ impl Db { ts } - fn remove_old_from_table(&self, mpaths: &Vec, dry_run: bool, table: &str) { - log::info!("Looking for non-existent tracks"); - let mut stmt = self.conn.prepare(format!("SELECT File FROM {};", table).as_str()).unwrap(); - let track_iter = stmt.query_map([], |row| Ok((row.get(0)?,))).unwrap(); + fn remove_old_from_table(&self, dry_run: bool, table: &str, resolver: &mut ExactPathResolver) -> bool { + log::info!("Looking for missing or case-mismatched paths in {}", table); + let mut stmt = self.conn.prepare(format!("SELECT File FROM {} ORDER BY File;", table).as_str()).unwrap(); + let file_iter = stmt.query_map([], |row| Ok((row.get(0)?,))).unwrap(); + let mut files: Vec = Vec::new(); + for file in file_iter { + files.push(file.unwrap().0); + } + + let known_paths: HashSet = files.iter().cloned().collect(); + let mut claimed_paths: HashSet = HashSet::new(); let mut to_remove: Vec = Vec::new(); - for tr in track_iter { - let mut db_path: String = tr.unwrap().0; - let orig_path = db_path.clone(); - match orig_path.find(CUE_MARKER) { - Some(s) => { - db_path.truncate(s); + let mut to_correct: Vec<(String, String)> = Vec::new(); + let mut unverified_count = 0; + + for original in files { + match resolver.resolve(&original) { + PathResolution::Exact => {} + PathResolution::Corrected(corrected) => { + if known_paths.contains(&corrected) + || !claimed_paths.insert(corrected.clone()) + { + to_remove.push(original); + } else { + to_correct.push((original, corrected)); + } } - None => {} - } - if cfg!(windows) { - db_path = db_path.replace("/", "\\"); - } - let mut exists = false; - - for mpath in mpaths { - let path = mpath.join(PathBuf::from(db_path.clone())); - //log::debug!("Check if '{}' exists.", path.to_string_lossy()); - - if path.exists() { - exists = true; - break; + PathResolution::Missing => to_remove.push(original), + PathResolution::Unknown => { + unverified_count += 1; } } - - if !exists { - to_remove.push(orig_path); - } } let num_to_remove = to_remove.len(); - log::info!("Num non-existent tracks: {}", num_to_remove); - if num_to_remove > 0 { - if dry_run { + let num_to_correct = to_correct.len(); + log::info!("Num missing/duplicate paths: {}", num_to_remove); + log::info!("Num case-only path corrections: {}", num_to_correct); + if unverified_count > 0 { + log::warn!( + "Could not verify exact filesystem spelling for {} path(s) in {}; keeping those database rows", + unverified_count, + table + ); + } + if dry_run { + if num_to_remove > 0 { log::info!("The following need to be removed from database:"); - for t in to_remove { - log::info!(" {}", t); + for path in &to_remove { + log::info!(" {}", path); } - } else { - let count_before = self.get_track_count(); - for t in to_remove { - //log::debug!("Remove '{}'", t); - let cmd = self.conn.execute(format!("DELETE FROM {} WHERE File = ?;", table).as_str(), params![t]); + } + if num_to_correct > 0 { + log::info!("The following stored path spellings need correction:"); + for (from, to) in &to_correct { + log::info!(" {} -> {}", from, to); + } + } + return false; + } - if let Err(e) = cmd { - log::error!("Failed to remove '{}' - {}", t, e) - } - } - let count_now = self.get_track_count(); - if (count_now + num_to_remove) != count_before { - log::error!("Failed to remove all tracks. Count before: {}, wanted to remove: {}, count now: {}", count_before, num_to_remove, count_now); + let mut changed = false; + for (from, to) in to_correct { + let cmd = self.conn.execute(format!("UPDATE {} SET File = ? WHERE File = ?;", table).as_str(), params![to, from]); + match cmd { + Ok(count) => changed |= count > 0, + Err(error) => { + log::error!("Failed to correct '{}' to '{}' - {}", from, to, error) } } } + for path in to_remove { + let cmd = self.conn.execute(format!("DELETE FROM {} WHERE File = ?;", table).as_str(), params![path]); + match cmd { + Ok(count) => changed |= count > 0, + Err(error) => log::error!("Failed to remove '{}' - {}", path, error), + } + } + changed } pub fn get_track_count(&self) -> usize { @@ -561,3 +739,165 @@ pub fn update_ignore(db_path: &str, ignore_path: &PathBuf) { db.close(); } + +#[cfg(test)] +mod tests { + use super::*; + use std::time::{SystemTime, UNIX_EPOCH}; + + struct TestMusicRoot { + path: PathBuf, + } + + impl TestMusicRoot { + fn new() -> Self { + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let path = std::env::temp_dir().join(format!( + "bliss-analyser-path-test-{}-{}", + process::id(), + unique + )); + fs::create_dir_all(&path).unwrap(); + Self { path } + } + + fn create_file(&self, relative: &str) { + let path = self.path.join(relative); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + File::create(path).unwrap(); + } + } + + impl Drop for TestMusicRoot { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.path); + } + } + + fn test_db() -> Db { + let db = Db { + conn: Connection::open_in_memory().unwrap(), + }; + db.init(); + db + } + + fn insert_track(db: &Db, path: &str) -> i64 { + db.conn + .execute("INSERT INTO TracksV2 (File) VALUES (?);", params![path]) + .unwrap(); + db.conn.last_insert_rowid() + } + + fn track_paths(db: &Db) -> Vec<(i64, String)> { + let mut statement = db + .conn + .prepare("SELECT rowid, File FROM TracksV2 ORDER BY File;") + .unwrap(); + statement + .query_map([], |row| Ok((row.get(0)?, row.get(1)?))) + .unwrap() + .map(|row| row.unwrap()) + .collect() + } + + #[test] + fn corrects_case_only_path_in_place() { + let music = TestMusicRoot::new(); + music.create_file("Artist/Album/Track.mp3"); + let db = test_db(); + let original_rowid = insert_track(&db, "artist/album/track.mp3"); + + assert!(db.remove_old(&vec![music.path.clone()], false)); + assert_eq!( + vec![(original_rowid, "Artist/Album/Track.mp3".to_string())], + track_paths(&db) + ); + } + + #[test] + fn removes_stale_case_variant_when_exact_row_exists() { + let music = TestMusicRoot::new(); + music.create_file("Artist/Album/Track.mp3"); + let db = test_db(); + insert_track(&db, "artist/album/track.mp3"); + let exact_rowid = insert_track(&db, "Artist/Album/Track.mp3"); + + assert!(db.remove_old(&vec![music.path.clone()], false)); + assert_eq!( + vec![(exact_rowid, "Artist/Album/Track.mp3".to_string())], + track_paths(&db) + ); + } + + #[test] + fn dry_run_reports_without_mutating() { + let music = TestMusicRoot::new(); + music.create_file("Artist/Album/Track.mp3"); + let db = test_db(); + let original_rowid = insert_track(&db, "artist/album/track.mp3"); + + assert!(!db.remove_old(&vec![music.path.clone()], true)); + assert_eq!( + vec![(original_rowid, "artist/album/track.mp3".to_string())], + track_paths(&db) + ); + } + + #[test] + fn corrects_case_without_losing_cue_track_suffix() { + let music = TestMusicRoot::new(); + music.create_file("Artist/Album/Image.flac"); + let db = test_db(); + let original_rowid = insert_track(&db, "artist/album/image.flac.CUE_TRACK.02"); + + assert!(db.remove_old(&vec![music.path.clone()], false)); + assert_eq!( + vec![( + original_rowid, + "Artist/Album/Image.flac.CUE_TRACK.02".to_string() + )], + track_paths(&db) + ); + } + + #[test] + fn removes_path_that_is_really_missing() { + let music = TestMusicRoot::new(); + let db = test_db(); + insert_track(&db, "Artist/Album/Missing.mp3"); + + assert!(db.remove_old(&vec![music.path.clone()], false)); + assert!(track_paths(&db).is_empty()); + } + + #[cfg(not(windows))] + #[test] + fn keeps_distinct_files_that_differ_only_by_case() { + let music = TestMusicRoot::new(); + music.create_file("Artist/Track.mp3"); + music.create_file("artist/track.mp3"); + let entries: HashSet = fs::read_dir(&music.path) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + if entries.len() != 2 { + return; + } + let db = test_db(); + let first_rowid = insert_track(&db, "Artist/Track.mp3"); + let second_rowid = insert_track(&db, "artist/track.mp3"); + + assert!(!db.remove_old(&vec![music.path.clone()], false)); + assert_eq!( + vec![ + (first_rowid, "Artist/Track.mp3".to_string()), + (second_rowid, "artist/track.mp3".to_string()) + ], + track_paths(&db) + ); + } +}