From 4c1260aa2a3b9081983e81580b8ebf58ca3ff71a Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Sun, 8 Nov 2026 22:55:12 -0500 Subject: [PATCH 1/3] fix(locallibrary): let sync go ahead after a merge that took in the whole source A merge recorded only the target's new state, so the two drives still compared as diverged and the sync the docs suggest next was refused. A merge that leaves nothing of the source out now records the source's state in the target's lineage first, and its plan lists the tracks both drives have that differ. Undoing the merge takes that record back out. --- .../changes/loc-235-merge-records-source.md | 12 +++ apps/locallibrary/docs/commands.md | 6 +- apps/locallibrary/docs/rpc/locallibrary.d.ts | 13 +++ apps/locallibrary/docs/rpc/schema.json | 18 ++++ apps/locallibrary/src/render.rs | 47 ++++++--- crates/locallibrary/src/backup.rs | 3 + crates/locallibrary/src/commit.rs | 8 ++ crates/locallibrary/src/device.rs | 5 +- crates/locallibrary/src/merge.rs | 84 +++++++++++++--- crates/locallibrary/tests/it/merge.rs | 1 + .../locallibrary/tests/it/merge/diverged.rs | 99 +++++++++++++++++++ crates/loclib-model/src/model.rs | 3 +- 12 files changed, 271 insertions(+), 28 deletions(-) create mode 100644 apps/locallibrary/changes/loc-235-merge-records-source.md create mode 100644 crates/locallibrary/tests/it/merge/diverged.rs diff --git a/apps/locallibrary/changes/loc-235-merge-records-source.md b/apps/locallibrary/changes/loc-235-merge-records-source.md new file mode 100644 index 000000000..b4d714376 --- /dev/null +++ b/apps/locallibrary/changes/loc-235-merge-records-source.md @@ -0,0 +1,12 @@ +### Fixed + +- `sync` goes ahead after a `merge` of two drives that both changed, as the docs say: a merge + that takes in all of the source now counts the target as newer, so the sync copies the merged + library over the source. The merge plan lists tracks both drives have whose details, file or + analysis differ (the target's stay, and the sync copies them over), and says when a merge + leaves some of the source out (`--only`, `--playlists none`, history lists without + `--history`), which leaves the drives diverged. Undoing the merge leaves them diverged too. + +### Changed + +- The merge plan's list of new tracks shows every one with `--all`, as `compare`'s lists do. diff --git a/apps/locallibrary/docs/commands.md b/apps/locallibrary/docs/commands.md index c2ff1768e..2b6eda5b8 100644 --- a/apps/locallibrary/docs/commands.md +++ b/apps/locallibrary/docs/commands.md @@ -305,10 +305,14 @@ locallibrary library push /Volumes/NEW -l Home --snapshot 3f9a2c ```bash locallibrary diff REKORDBOX BACKUP --summary # "both changed since they were last in sync (2026-09-30 21:04)" -locallibrary merge BACKUP REKORDBOX # keep both sets of new tracks +locallibrary merge BACKUP REKORDBOX --history # keep both sets of new tracks locallibrary sync Home # then make them identical again ``` +The merge plan lists tracks both drives have whose details differ: REKORDBOX's stay, and the +sync copies them to BACKUP. A merge that leaves some of BACKUP out (`--only`, `--playlists none`, +history lists without `--history`) leaves the drives diverged, and so does undoing it. + **A new drive** ```bash diff --git a/apps/locallibrary/docs/rpc/locallibrary.d.ts b/apps/locallibrary/docs/rpc/locallibrary.d.ts index 137adcebf..67fae459c 100644 --- a/apps/locallibrary/docs/rpc/locallibrary.d.ts +++ b/apps/locallibrary/docs/rpc/locallibrary.d.ts @@ -2921,6 +2921,11 @@ export type LineageOp = | "export" /** Damaged files rewritten from a known-good copy (see [`crate::scrub`]). */ | "repair" + /** + * Another drive's state a merge took in whole, recorded before the merged state (see + * [`crate::merge`]). + */ + | "merged_in" /** An operation from a newer version of locallibrary. */ | "other"; @@ -3906,6 +3911,14 @@ export type MergePlan = { new_tracks: PlannedTrack[]; /** Source tracks already in the target. */ matched_tracks: number; + /** Those whose details, file or analysis differ: the target's stay as they are. */ + differing_tracks: TrackChange[]; + /** + * Whether the target holds everything the source has once merged (every track, its + * playlists and history), `differing_tracks` aside. Then the source's state goes into the + * target's lineage, so a `sync` afterwards copies the merged drive over the source. + */ + takes_in_source: boolean; playlists: PlannedPlaylist[]; artwork_files: number; copy_bytes: number; diff --git a/apps/locallibrary/docs/rpc/schema.json b/apps/locallibrary/docs/rpc/schema.json index bb909590f..78837db55 100644 --- a/apps/locallibrary/docs/rpc/schema.json +++ b/apps/locallibrary/docs/rpc/schema.json @@ -13794,6 +13794,11 @@ "const": "repair", "description": "Damaged files rewritten from a known-good copy (see [`crate::scrub`])." }, + { + "type": "string", + "const": "merged_in", + "description": "Another drive's state a merge took in whole, recorded before the merged state (see\n[`crate::merge`])." + }, { "type": "string", "const": "other", @@ -17058,6 +17063,17 @@ "minimum": 0, "description": "Source tracks already in the target." }, + "differing_tracks": { + "type": "array", + "items": { + "$ref": "#/$defs/TrackChange" + }, + "description": "Those whose details, file or analysis differ: the target's stay as they are." + }, + "takes_in_source": { + "type": "boolean", + "description": "Whether the target holds everything the source has once merged (every track, its\nplaylists and history), `differing_tracks` aside. Then the source's state goes into the\ntarget's lineage, so a `sync` afterwards copies the merged drive over the source." + }, "playlists": { "type": "array", "items": { @@ -17123,6 +17139,8 @@ "target", "new_tracks", "matched_tracks", + "differing_tracks", + "takes_in_source", "playlists", "artwork_files", "copy_bytes", diff --git a/apps/locallibrary/src/render.rs b/apps/locallibrary/src/render.rs index ac3e7f5c8..89f12f55e 100644 --- a/apps/locallibrary/src/render.rs +++ b/apps/locallibrary/src/render.rs @@ -574,12 +574,7 @@ pub fn comparison(c: &Comparison) { let list = |label: &str, items: Vec| { if !items.is_empty() { say!(" {label} ({}):", items.len()); - for i in items.iter().take(crate::output::cap(10)) { - say!(" {i}"); - } - if items.len() > crate::output::cap(10) { - say!(" {}", dim(format!("... and {} more", items.len() - 10))); - } + capped(&items, 10); } }; list("+ tracks", d.added_tracks.iter().map(|t| t.name.clone()).collect()); @@ -652,12 +647,29 @@ pub fn sync_report(r: &SyncReport) { pub fn merge_plan(p: &MergePlan) { say!("{} → {}", style(&p.source).bold(), p.target.display()); say!(" {} new track(s), {} already present, {}", p.new_tracks.len(), p.matched_tracks, bytes(p.copy_bytes)); - for t in p.new_tracks.iter().take(15) { - let note = if t.renamed { format!(" (as {})", t.audio) } else { String::new() }; - say!(" + {}{}", t.name, dim(note)); - } - if p.new_tracks.len() > 15 { - say!(" {}", dim(format!("... and {} more", p.new_tracks.len() - 15))); + let new: Vec = p + .new_tracks + .iter() + .map(|t| format!("+ {}{}", t.name, dim(if t.renamed { format!(" (as {})", t.audio) } else { String::new() }))) + .collect(); + capped(&new, 15); + if !p.differing_tracks.is_empty() { + say!(" {} already present differ; the target's stay as they are", p.differing_tracks.len()); + } + let differing: Vec = p + .differing_tracks + .iter() + .map(|t| { + let fields: Vec<&str> = t.fields.iter().map(|f| f.field.as_str()).collect(); + format!("~ {} {}", t.track.name, dim(format!("({})", fields.join(", ")))) + }) + .collect(); + capped(&differing, 15); + match p.takes_in_source { + true => say!(" {}", dim(format!("afterwards, sync copies the merged library to {}", p.source))), + false => { + say!(" {}", dim(format!("{} keeps what this merge leaves out, so sync still asks which way", p.source))) + } } for pl in &p.playlists { let verb = if pl.create { "create" } else { "extend" }; @@ -677,6 +689,17 @@ pub fn merge_plan(p: &MergePlan) { } } +/// The first `n` of `lines` (all of them with `--all`), indented, then how many more there are. +fn capped(lines: &[String], n: usize) { + let shown = crate::output::cap(n); + for l in lines.iter().take(shown) { + say!(" {l}"); + } + if lines.len() > shown { + say!(" {}", dim(format!("... and {} more", lines.len() - shown))); + } +} + pub fn merge_report(r: &MergeReport, target: &str) { ok(format!( "added {} track(s), {} playlist(s), {} playlist entries, {} artwork", diff --git a/crates/locallibrary/src/backup.rs b/crates/locallibrary/src/backup.rs index 47740522d..9069aa5ad 100644 --- a/crates/locallibrary/src/backup.rs +++ b/crates/locallibrary/src/backup.rs @@ -472,6 +472,9 @@ pub fn restore_with(device: &mut Device, id: Option<&str>, force: bool, ctx: &Ct let remembers = device.state().is_some_and(|s| s.player.is_some()) && carried(device, &info)?.is_none(); let shown = remembers.then(|| crate::player::db_identity(device.root())).flatten(); commit::commit(device, changes, opts, ctx)?; + // A merge it takes back no longer holds the other drive's state it took in. + device.state_mut()?.lineage.retain(|e| e.op != LineageOp::MergedIn || e.at < info.created); + device.save_state()?; put_back_carried(device, &info, shown)?; Ok(info) } diff --git a/crates/locallibrary/src/commit.rs b/crates/locallibrary/src/commit.rs index 5c999f7ea..e964ee73a 100644 --- a/crates/locallibrary/src/commit.rs +++ b/crates/locallibrary/src/commit.rs @@ -195,6 +195,9 @@ pub(crate) struct Options<'a> { pub lineage: bool, /// The drive the content came from (merge). pub source: Option, + /// That drive's state, when the write took all of it in (merge): recorded before the new + /// state, so the drive counts as having been through it. + pub held: Option, /// Edits to add to the drive's journal, with what they did. pub journal: Option<(&'a [Edit], &'a [String])>, /// Files it writes in place of ones the databases name (artwork replaced in place), for the @@ -211,6 +214,7 @@ impl Options<'_> { undoes: None, lineage: true, source: None, + held: None, journal: None, in_place: Vec::new(), } @@ -409,6 +413,10 @@ pub(crate) fn commit(device: &mut Device, changes: Changes<'_>, opts: Options<'_ backup::seal(device, &id)?; } if opts.lineage { + // Stamped now, after the backup: putting that back drops it (see `backup::restore_with`). + if let Some(held) = opts.held { + device.state_mut()?.record(LineageEntry { at: Utc::now(), ..held }); + } let library = device.library()?; let scanned = device.scan_fingerprinted(Some(&library), ctx)?; done.fingerprint = scanned.fingerprint; diff --git a/crates/locallibrary/src/device.rs b/crates/locallibrary/src/device.rs index 4b41c49a8..5082f8a09 100644 --- a/crates/locallibrary/src/device.rs +++ b/crates/locallibrary/src/device.rs @@ -109,6 +109,9 @@ pub enum LineageOp { Export, /// Damaged files rewritten from a known-good copy (see [`crate::scrub`]). Repair, + /// Another drive's state a merge took in whole, recorded before the merged state (see + /// [`crate::merge`]). + MergedIn, /// An operation from a newer version of locallibrary. #[serde(other)] Other, @@ -144,7 +147,7 @@ impl PendingOp { LineageOp::Artwork => Some("art"), LineageOp::Restore => Some("undo"), LineageOp::Repair => Some("drive verify --repair"), - LineageOp::Edit | LineageOp::Observed | LineageOp::Other => None, + LineageOp::Edit | LineageOp::Observed | LineageOp::MergedIn | LineageOp::Other => None, }; let what = command.map_or("a locallibrary operation".into(), |c| format!("a `locallibrary {c}`")); let about = if self.description.is_empty() { String::new() } else { format!(" ({})", self.description) }; diff --git a/crates/locallibrary/src/merge.rs b/crates/locallibrary/src/merge.rs index 07047b62b..d6eb77062 100644 --- a/crates/locallibrary/src/merge.rs +++ b/crates/locallibrary/src/merge.rs @@ -24,12 +24,19 @@ //! Paths that are taken by a *different* file get a ` (2)` suffix, and the analysis file's //! embedded path is rewritten to match. //! +//! A track already present keeps the target's details, analysis and file; the plan lists those +//! that differ. When the merge leaves nothing of the source out, the source's state goes into the +//! target's lineage, so the target counts as newer and `sync` copies it over the source, those +//! differences included. Undoing the merge takes that back out. +//! //! The target's database is backed up first (see [`crate::backup`]), so `locallibrary undo` takes back a //! merge. use rustc_hash::{FxHashMap, FxHashSet}; use std::path::PathBuf; +use chrono::Utc; + use rekordbox_pdb::{NewTrack, PdbEditor}; use crate::error::{Action, Reason}; @@ -38,11 +45,11 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use crate::commit::{self, Changes, NewDatabases, NewFile, Options}; -use crate::device::{Device, LineageOp}; +use crate::device::{Device, LineageEntry, LineageOp, Scanned}; use crate::error::IoContext; use crate::fsutil; use crate::hash::Hash; -use crate::model::{self, Library, Playlist, Track}; +use crate::model::{self, FieldChange, Library, Playlist, Track, TrackChange}; use crate::paths::{self, fold}; use crate::pdb; use crate::progress::{Ctx, Phase, WarningLimiter}; @@ -123,6 +130,12 @@ pub struct MergePlan { pub new_tracks: Vec, /// Source tracks already in the target. pub matched_tracks: usize, + /// Those whose details, file or analysis differ: the target's stay as they are. + pub differing_tracks: Vec, + /// Whether the target holds everything the source has once merged (every track, its + /// playlists and history), `differing_tracks` aside. Then the source's state goes into the + /// target's lineage, so a `sync` afterwards copies the merged drive over the source. + pub takes_in_source: bool, pub playlists: Vec, pub artwork_files: usize, pub copy_bytes: u64, @@ -143,6 +156,9 @@ pub struct MergePlan { /// The target's databases when planned (see [`crate::commit::lock`]). #[serde(skip)] state: Option, + /// The source's fingerprint when planned. + #[serde(skip)] + source_state: Option, /// source track id -> existing target track id #[serde(skip)] matches: FxHashMap, @@ -216,7 +232,7 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> ))); } let dst_lib = target.library()?; - let src_tree = source.scan(Some(&src_lib), ctx)?; + let Scanned { tree: src_tree, fingerprint: source_state, .. } = source.scan_fingerprinted(Some(&src_lib), ctx)?; let dst_tree = target.scan(Some(&dst_lib), ctx)?; let selected = selected_playlists(&src_lib, opts)?; @@ -259,6 +275,7 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> let mut new_tracks = Vec::new(); let mut unanalysed = Vec::new(); let mut matches = FxHashMap::default(); + let mut differing_tracks = Vec::new(); let mut copy_bytes = 0u64; let mut needed = 0u64; let mut artwork_ids = FxHashSet::default(); @@ -274,16 +291,16 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> warnings.push(format!("{}: unsafe file path {:?}; skipped", t.display_name(), t.file_path)); continue; } - if let Some(d) = dst_by_path.get(&fold(&src_key)) - && d.file_size == t.file_size - { - matches.insert(t.id, d.id); - continue; - } - if opts.match_mode == MatchMode::Metadata - && let Some(d) = dst_by_meta.get(&t.fingerprint_key()) - { + let matched = dst_by_path + .get(&fold(&src_key)) + .filter(|d| d.file_size == t.file_size) + .or_else(|| dst_by_meta.get(&t.fingerprint_key()).filter(|_| opts.match_mode == MatchMode::Metadata)); + if let Some(d) = matched { matches.insert(t.id, d.id); + let fields = differences(t, d, &src_tree, &dst_tree); + if !fields.is_empty() { + differing_tracks.push(TrackChange { track: t.into(), fields }); + } continue; } let Some(src_entry) = src_tree.resolve(&src_key) else { @@ -367,6 +384,12 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> } let new_ids: FxHashSet = new_tracks.iter().map(|t| t.source_id).collect(); let history_ids = if opts.history { plan_history(&src_lib, &dst_lib, &matches, &new_ids) } else { Vec::new() }; + // Nothing of the source's left out: no track skipped, no playlist or history list left behind. + let takes_in_source = source_state.is_some() + && wanted_tracks.is_none() + && opts.playlists != PlaylistMode::None + && new_tracks.len() + matches.len() == src_lib.tracks.len() + && (opts.history || plan_history(&src_lib, &dst_lib, &matches, &new_ids).is_empty()); let history: Vec = history_ids .iter() .filter_map(|id| src_lib.history.iter().find(|h| h.id == *id)) @@ -400,6 +423,8 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> source: source.name(), target: target.root().to_path_buf(), matched_tracks: matches.len(), + differing_tracks, + takes_in_source, new_tracks, playlists, artwork_files: artwork_ids.len(), @@ -416,9 +441,31 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> folder_name, again, state: Some(crate::commit::database_state(target)), + source_state, }) } +/// What a sync after the merge would change on the source for a track the target already has +/// (`d`, the source's `t`): its details, its file, and its analysis files (cues, beat grid). +fn differences(t: &Track, d: &Track, src_tree: &FileTree, dst_tree: &FileTree) -> Vec { + let mut fields = model::track_field_changes(t, d); + if paths::file_key(&t.file_path) != paths::file_key(&d.file_path) { + fields.push(FieldChange { field: "file".into(), old: t.file_path.clone(), new: d.file_path.clone() }); + } + let analysis = |tree: &FileTree, t: &Track| -> Vec> { + let key = paths::key(&t.analyze_path); + let stem = key.rsplit_once('.').map_or(key.as_str(), |(s, _)| s); + ["DAT", "EXT", "2EX"] + .iter() + .map(|ext| tree.resolve(&format!("{stem}.{ext}")).map(|e| (e.size, e.mtime))) + .collect() + }; + if analysis(src_tree, t) != analysis(dst_tree, d) { + fields.push(FieldChange { field: "analysis".into(), old: String::new(), new: String::new() }); + } + fields +} + /// A drive's My Tags, in each of its databases (missing or unreadable ones are `None`). struct DriveTags { ext: Option, @@ -782,7 +829,7 @@ pub fn apply( } let _lock = commit::lock(target, plan.state)?; let src_lib = source.library()?; - let src_tree = source.scan(Some(&src_lib), ctx)?; + let Scanned { tree: src_tree, fingerprint: source_state, .. } = source.scan_fingerprinted(Some(&src_lib), ctx)?; let mut report = MergeReport { tracks_matched: plan.matched_tracks, ..Default::default() }; // The source's own OneLibrary rows carry every column; read them if it has a database. let src_plus = if source.has_library_plus() { source.library_plus().ok() } else { None }; @@ -982,6 +1029,17 @@ pub fn apply( let mut opts = Options::new(LineageOp::Merge, format!("merge from {}", plan.source)) .backup(format!("before merging {}", plan.source)); opts.source = source.id(); + // All of the source as planned is in the merge: the target has been through its state. + if plan.takes_in_source && source_state == plan.source_state { + opts.held = source_state.map(|fingerprint| LineageEntry { + fingerprint, + at: Utc::now(), + op: LineageOp::MergedIn, + snapshot: None, + source: opts.source, + tracks: src_lib.tracks.len(), + }); + } let done = commit::commit(target, changes, opts, ctx); warnings.into_inner().finish(); let done = done?; diff --git a/crates/locallibrary/tests/it/merge.rs b/crates/locallibrary/tests/it/merge.rs index 5c471b8e7..eb1da3e34 100644 --- a/crates/locallibrary/tests/it/merge.rs +++ b/crates/locallibrary/tests/it/merge.rs @@ -1,3 +1,4 @@ +mod diverged; mod files; mod matching; mod playlist_trees; diff --git a/crates/locallibrary/tests/it/merge/diverged.rs b/crates/locallibrary/tests/it/merge/diverged.rs new file mode 100644 index 000000000..ed7cd070b --- /dev/null +++ b/crates/locallibrary/tests/it/merge/diverged.rs @@ -0,0 +1,99 @@ +//! Merging a drive into another when both changed since they were last in sync: once the merge +//! brings in everything the source has, the target counts as newer and `sync` copies it over the +//! source, tracks the plan listed as differing included. A merge only planned, one that leaves +//! some of the source out, or one taken back, leaves them diverged. + +use crate::common; + +use std::path::{Path, PathBuf}; + +use hegel::generators as gs; +use hegel::{HealthCheck, TestCase}; +use locallibrary::api::{self, ComparePair, UndoRequest}; +use locallibrary::compare::Relation; +use locallibrary::device::Device; +use locallibrary::error::Reason; +use locallibrary::merge::{self, MergeOptions, PlaylistMode}; +use locallibrary::paths; +use locallibrary::progress::Ctx; +use locallibrary::sync::{self, SyncOptions}; + +#[derive(Debug, Clone, Copy, PartialEq)] +enum How { + PlannedOnly, + Whole, + OnePlaylist, + TracksOnly, + Undone, +} + +fn relation(left: &Path, right: &Path) -> Relation { + let pair = ComparePair { left: common::path(left), right: common::path(right) }; + api::compare(&pair, &Ctx::silent()).unwrap().relation +} + +/// Drives A and B, A synced onto B and then both changed: a new export each, and with `edited`, +/// A's details for a track B has too. +fn diverged(dir: &Path, edited: bool) -> (PathBuf, PathBuf) { + let a_root = common::bigger_drive(dir, "A"); + common::adopt(None, &a_root, "A"); + let b_root = common::blank(dir, "B"); + let mut a = Device::open_export(&a_root).unwrap(); + sync::sync_drive_to_drive(&mut a, &mut Device::open(&b_root).unwrap(), &SyncOptions::default(), &Ctx::silent()) + .unwrap(); + common::export_new_track(&a_root, "Only", "On A"); + common::export_new_track(&b_root, "Only", "On B"); + if edited { + let mut ed = rekordbox_pdb::PdbEditor::from_file(a_root.join(paths::EXPORT_PDB)).unwrap(); + ed.set_track_strings(1, &[("comment", "edited on A")]).unwrap(); + ed.save(a_root.join(paths::EXPORT_PDB)).unwrap(); + } + assert_eq!(relation(&b_root, &a_root), Relation::Diverged); + (a_root, b_root) +} + +#[hegel::test(test_cases = 2, suppress_health_check = [HealthCheck::TooSlow])] +fn a_merge_that_takes_in_the_source_lets_sync_go_ahead(tc: TestCase) { + let edited = tc.draw(gs::booleans()); + for how in [How::PlannedOnly, How::Whole, How::OnePlaylist, How::TracksOnly, How::Undone] { + let dir = tempfile::tempdir().unwrap(); + let (a_root, b_root) = diverged(dir.path(), edited); + let a = Device::open_export(&a_root).unwrap(); + let mut b = Device::open_export(&b_root).unwrap(); + let lib = a.library().unwrap(); + let opts = match how { + How::OnePlaylist => MergeOptions { + only_playlists: vec![lib.playlist_path(lib.playlists.iter().find(|p| !p.is_folder).unwrap().id)], + ..Default::default() + }, + How::TracksOnly => MergeOptions { playlists: PlaylistMode::None, ..Default::default() }, + _ => MergeOptions::default(), + }; + let plan = merge::plan(&a, &b, &opts, &Ctx::silent()).unwrap(); + let whole = matches!(how, How::PlannedOnly | How::Whole | How::Undone); + assert_eq!(plan.takes_in_source, whole, "{how:?}"); + if whole { + let named: Vec<&str> = plan.differing_tracks.iter().map(|t| t.track.name.as_str()).collect(); + let track = lib.tracks.iter().find(|t| t.id == 1).unwrap().display_name(); + assert_eq!(named, if edited { vec![track.as_str()] } else { vec![] }); + } + if how != How::PlannedOnly { + merge::apply(&plan, &a, &mut b, &opts, &Ctx::silent()).unwrap(); + } + if how == How::Undone { + let undo = UndoRequest { drive: Some(common::path(&b_root)), ..Default::default() }; + api::undo_plan(&undo).unwrap().apply(None, &Ctx::silent()).unwrap(); + } + + let sync = api::sync_plan(&common::sync_pair(&b_root, &a_root), &Ctx::silent()); + if how != How::Whole { + assert_eq!(relation(&b_root, &a_root), Relation::Diverged, "{how:?}"); + assert_eq!(sync.err().and_then(|e| e.reason()), Some(Reason::Diverged), "{how:?}"); + continue; + } + assert_eq!(relation(&b_root, &a_root), Relation::LeftNewer); + sync.unwrap().apply(None, &Ctx::silent()).unwrap(); + assert_eq!(common::library_files(&a_root), common::library_files(&b_root), "A gets the merged library"); + assert_eq!(relation(&b_root, &a_root), Relation::Identical); + } +} diff --git a/crates/loclib-model/src/model.rs b/crates/loclib-model/src/model.rs index 40ce51c8e..b118e0753 100644 --- a/crates/loclib-model/src/model.rs +++ b/crates/loclib-model/src/model.rs @@ -668,7 +668,8 @@ fn playlist_contents(lib: &Library) -> IndexMap, (String, Vec Vec { +/// The details `b` holds differently from `a` (old: `a`'s, new: `b`'s). +pub fn track_field_changes(a: &Track, b: &Track) -> Vec { let mut out = Vec::new(); let mut cmp = |field: &str, x: &str, y: &str| { if x != y { From 4698dbc7bf6d9e092764c05997c1d5294c7f0e00 Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Mon, 9 Nov 2026 10:39:01 -0500 Subject: [PATCH 2/3] perf(locallibrary): keep the merge plan's track comparison within its allocation budget Planning a merge fingerprinted the source and compared every matched track by formatting each field and building analysis keys, which took the 500-track merge plan 25% over its allocation budget. Planning now stamps the source's database files from the scan it already makes, and the fingerprint is taken only when the merge is applied. Analysis files are compared by size, since their times differ between any two drives exported apart, and their keys are built in one reused buffer. track_field_changes formats only the fields that differ, which also cuts what comparing two 2,000-track libraries allocates by a quarter, so that budget is lowered to match. --- benchmarks/budgets/alloc.json | 4 +- crates/locallibrary/src/merge.rs | 69 ++++++++++++------- .../locallibrary/tests/it/merge/diverged.rs | 19 +++-- crates/loclib-model/src/model.rs | 33 +++++---- 4 files changed, 79 insertions(+), 46 deletions(-) diff --git a/benchmarks/budgets/alloc.json b/benchmarks/budgets/alloc.json index 87c38a67e..7017d6f89 100644 --- a/benchmarks/budgets/alloc.json +++ b/benchmarks/budgets/alloc.json @@ -24,12 +24,12 @@ "measures": "Bytes allocated filtering 2,000 tracks by artist, tempo and genre." }, "compare.2k.allocations": { - "limit": 69034, + "limit": 51034, "unit": "allocations", "measures": "Allocations made comparing two 2,000-track libraries." }, "compare.2k.bytes": { - "limit": 5690400, + "limit": 5589600, "unit": "bytes", "measures": "Bytes allocated comparing two 2,000-track libraries." }, diff --git a/crates/locallibrary/src/merge.rs b/crates/locallibrary/src/merge.rs index d6eb77062..8c25dd5cb 100644 --- a/crates/locallibrary/src/merge.rs +++ b/crates/locallibrary/src/merge.rs @@ -156,9 +156,9 @@ pub struct MergePlan { /// The target's databases when planned (see [`crate::commit::lock`]). #[serde(skip)] state: Option, - /// The source's fingerprint when planned. + /// The source's databases when planned (see [`databases_seen`]). #[serde(skip)] - source_state: Option, + source_state: Hash, /// source track id -> existing target track id #[serde(skip)] matches: FxHashMap, @@ -232,7 +232,7 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> ))); } let dst_lib = target.library()?; - let Scanned { tree: src_tree, fingerprint: source_state, .. } = source.scan_fingerprinted(Some(&src_lib), ctx)?; + let src_tree = source.scan(Some(&src_lib), ctx)?; let dst_tree = target.scan(Some(&dst_lib), ctx)?; let selected = selected_playlists(&src_lib, opts)?; @@ -276,6 +276,8 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> let mut unanalysed = Vec::new(); let mut matches = FxHashMap::default(); let mut differing_tracks = Vec::new(); + // Analysis file keys, written here one at a time (see `differences`). + let mut key = String::new(); let mut copy_bytes = 0u64; let mut needed = 0u64; let mut artwork_ids = FxHashSet::default(); @@ -291,13 +293,12 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> warnings.push(format!("{}: unsafe file path {:?}; skipped", t.display_name(), t.file_path)); continue; } - let matched = dst_by_path - .get(&fold(&src_key)) - .filter(|d| d.file_size == t.file_size) - .or_else(|| dst_by_meta.get(&t.fingerprint_key()).filter(|_| opts.match_mode == MatchMode::Metadata)); + let matched = dst_by_path.get(&fold(&src_key)).filter(|d| d.file_size == t.file_size).or_else(|| { + (opts.match_mode == MatchMode::Metadata).then(|| dst_by_meta.get(&t.fingerprint_key())).flatten() + }); if let Some(d) = matched { matches.insert(t.id, d.id); - let fields = differences(t, d, &src_tree, &dst_tree); + let fields = differences(t, d, &src_tree, &dst_tree, &mut key); if !fields.is_empty() { differing_tracks.push(TrackChange { track: t.into(), fields }); } @@ -385,8 +386,7 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> let new_ids: FxHashSet = new_tracks.iter().map(|t| t.source_id).collect(); let history_ids = if opts.history { plan_history(&src_lib, &dst_lib, &matches, &new_ids) } else { Vec::new() }; // Nothing of the source's left out: no track skipped, no playlist or history list left behind. - let takes_in_source = source_state.is_some() - && wanted_tracks.is_none() + let takes_in_source = wanted_tracks.is_none() && opts.playlists != PlaylistMode::None && new_tracks.len() + matches.len() == src_lib.tracks.len() && (opts.history || plan_history(&src_lib, &dst_lib, &matches, &new_ids).is_empty()); @@ -441,24 +441,45 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> folder_name, again, state: Some(crate::commit::database_state(target)), - source_state, + source_state: databases_seen(&src_tree), }) } +/// A drive's database files as `tree` found them, by size and modification time: a merge takes in +/// the source's state only if it's what was planned from. +fn databases_seen(tree: &FileTree) -> Hash { + let mut seen = blake3::Hasher::new(); + for key in paths::DATABASE_FILES { + let (size, mtime) = tree.resolve(key).map_or((0, 0), |e| (e.size, e.mtime)); + seen.update(&size.to_le_bytes()).update(&mtime.to_le_bytes()); + } + seen.finalize().into() +} + /// What a sync after the merge would change on the source for a track the target already has -/// (`d`, the source's `t`): its details, its file, and its analysis files (cues, beat grid). -fn differences(t: &Track, d: &Track, src_tree: &FileTree, dst_tree: &FileTree) -> Vec { +/// (`d`, the source's `t`): its details, its file, and its analysis files (cues, beat grid), by +/// size. Their times differ between any two drives exported apart, and their content isn't read +/// while planning, so a cue moved without changing a file's size isn't seen. Every matched track +/// is compared, so one that's the same costs no allocation: `key` holds each analysis file's key +/// in turn. +fn differences(t: &Track, d: &Track, src_tree: &FileTree, dst_tree: &FileTree, key: &mut String) -> Vec { let mut fields = model::track_field_changes(t, d); - if paths::file_key(&t.file_path) != paths::file_key(&d.file_path) { + if t.file_path != d.file_path && paths::file_key(&t.file_path) != paths::file_key(&d.file_path) { fields.push(FieldChange { field: "file".into(), old: t.file_path.clone(), new: d.file_path.clone() }); } - let analysis = |tree: &FileTree, t: &Track| -> Vec> { - let key = paths::key(&t.analyze_path); - let stem = key.rsplit_once('.').map_or(key.as_str(), |(s, _)| s); - ["DAT", "EXT", "2EX"] - .iter() - .map(|ext| tree.resolve(&format!("{stem}.{ext}")).map(|e| (e.size, e.mtime))) - .collect() + // rekordbox names analysis files in plain ASCII (`/PIONEER/USBANLZ/P016/0000875E/ANLZ0000.DAT`), + // so the path less its leading slash is the key. Looked up by exact key: a file whose name + // differs in case on one drive counts as differing. + let mut analysis = |tree: &FileTree, t: &Track| -> [Option; 3] { + let path = t.analyze_path.trim_start_matches('/'); + let stem = path.rsplit_once('.').map_or(path, |(s, _)| s); + ["DAT", "EXT", "2EX"].map(|ext| { + key.clear(); + key.push_str(stem); + key.push('.'); + key.push_str(ext); + tree.entries.get(key.as_str()).map(|e| e.size) + }) }; if analysis(src_tree, t) != analysis(dst_tree, d) { fields.push(FieldChange { field: "analysis".into(), old: String::new(), new: String::new() }); @@ -829,7 +850,7 @@ pub fn apply( } let _lock = commit::lock(target, plan.state)?; let src_lib = source.library()?; - let Scanned { tree: src_tree, fingerprint: source_state, .. } = source.scan_fingerprinted(Some(&src_lib), ctx)?; + let Scanned { tree: src_tree, fingerprint, .. } = source.scan_fingerprinted(Some(&src_lib), ctx)?; let mut report = MergeReport { tracks_matched: plan.matched_tracks, ..Default::default() }; // The source's own OneLibrary rows carry every column; read them if it has a database. let src_plus = if source.has_library_plus() { source.library_plus().ok() } else { None }; @@ -1030,8 +1051,8 @@ pub fn apply( .backup(format!("before merging {}", plan.source)); opts.source = source.id(); // All of the source as planned is in the merge: the target has been through its state. - if plan.takes_in_source && source_state == plan.source_state { - opts.held = source_state.map(|fingerprint| LineageEntry { + if plan.takes_in_source && databases_seen(&src_tree) == plan.source_state { + opts.held = fingerprint.map(|fingerprint| LineageEntry { fingerprint, at: Utc::now(), op: LineageOp::MergedIn, diff --git a/crates/locallibrary/tests/it/merge/diverged.rs b/crates/locallibrary/tests/it/merge/diverged.rs index ed7cd070b..fb29b1a22 100644 --- a/crates/locallibrary/tests/it/merge/diverged.rs +++ b/crates/locallibrary/tests/it/merge/diverged.rs @@ -33,10 +33,12 @@ fn relation(left: &Path, right: &Path) -> Relation { } /// Drives A and B, A synced onto B and then both changed: a new export each, and with `edited`, -/// A's details for a track B has too. +/// A's details and analysis (a beat grid of another size) for "Shared", a track B has too. fn diverged(dir: &Path, edited: bool) -> (PathBuf, PathBuf) { let a_root = common::bigger_drive(dir, "A"); common::adopt(None, &a_root, "A"); + // The fixture's own tracks keep their analysis in .PIONEER/, which no scan reads. + let shared = common::export_new_track(&a_root, "Both", "Shared"); let b_root = common::blank(dir, "B"); let mut a = Device::open_export(&a_root).unwrap(); sync::sync_drive_to_drive(&mut a, &mut Device::open(&b_root).unwrap(), &SyncOptions::default(), &Ctx::silent()) @@ -45,8 +47,12 @@ fn diverged(dir: &Path, edited: bool) -> (PathBuf, PathBuf) { common::export_new_track(&b_root, "Only", "On B"); if edited { let mut ed = rekordbox_pdb::PdbEditor::from_file(a_root.join(paths::EXPORT_PDB)).unwrap(); - ed.set_track_strings(1, &[("comment", "edited on A")]).unwrap(); + ed.set_track_strings(shared, &[("comment", "edited on A")]).unwrap(); ed.save(a_root.join(paths::EXPORT_PDB)).unwrap(); + let tracks = Device::open_export(&a_root).unwrap().library().unwrap().tracks; + let t = tracks.into_iter().find(|t| t.id == shared).unwrap(); + let regridded = common::synthetic_anlz(&format!("{}.longer", t.file_path)); + std::fs::write(paths::to_fs(&a_root, &paths::key(&t.analyze_path)), regridded).unwrap(); } assert_eq!(relation(&b_root, &a_root), Relation::Diverged); (a_root, b_root) @@ -73,9 +79,12 @@ fn a_merge_that_takes_in_the_source_lets_sync_go_ahead(tc: TestCase) { let whole = matches!(how, How::PlannedOnly | How::Whole | How::Undone); assert_eq!(plan.takes_in_source, whole, "{how:?}"); if whole { - let named: Vec<&str> = plan.differing_tracks.iter().map(|t| t.track.name.as_str()).collect(); - let track = lib.tracks.iter().find(|t| t.id == 1).unwrap().display_name(); - assert_eq!(named, if edited { vec![track.as_str()] } else { vec![] }); + let named: Vec<(&str, Vec<&str>)> = plan + .differing_tracks + .iter() + .map(|t| (t.track.name.as_str(), t.fields.iter().map(|f| f.field.as_str()).collect())) + .collect(); + assert_eq!(named, if edited { vec![("Both - Shared", vec!["comment", "analysis"])] } else { vec![] }); } if how != How::PlannedOnly { merge::apply(&plan, &a, &mut b, &opts, &Ctx::silent()).unwrap(); diff --git a/crates/loclib-model/src/model.rs b/crates/loclib-model/src/model.rs index b118e0753..0694183cc 100644 --- a/crates/loclib-model/src/model.rs +++ b/crates/loclib-model/src/model.rs @@ -669,25 +669,28 @@ fn playlist_contents(lib: &Library) -> IndexMap, (String, Vec Vec { let mut out = Vec::new(); - let mut cmp = |field: &str, x: &str, y: &str| { - if x != y { - out.push(FieldChange { field: field.into(), old: x.into(), new: y.into() }); + let mut cmp = |field: &str, same: bool, show: &dyn Fn(&Track) -> String| { + if !same { + out.push(FieldChange { field: field.into(), old: show(a), new: show(b) }); } }; - cmp("title", &a.title, &b.title); - cmp("artist", &a.artist, &b.artist); - cmp("album", &a.album, &b.album); - cmp("genre", &a.genre, &b.genre); - cmp("key", &a.key, &b.key); - cmp("label", &a.label, &b.label); - cmp("comment", &a.comment, &b.comment); - cmp("bpm", &format!("{:.2}", a.bpm), &format!("{:.2}", b.bpm)); - cmp("rating", &a.rating.to_string(), &b.rating.to_string()); - cmp("color", &format!("{:?}", a.color), &format!("{:?}", b.color)); - cmp("file_size", &a.file_size.to_string(), &b.file_size.to_string()); - cmp("artwork", &a.artwork_id.is_some().to_string(), &b.artwork_id.is_some().to_string()); + cmp("title", a.title == b.title, &|t| t.title.clone()); + cmp("artist", a.artist == b.artist, &|t| t.artist.clone()); + cmp("album", a.album == b.album, &|t| t.album.clone()); + cmp("genre", a.genre == b.genre, &|t| t.genre.clone()); + cmp("key", a.key == b.key, &|t| t.key.clone()); + cmp("label", a.label == b.label, &|t| t.label.clone()); + cmp("comment", a.comment == b.comment, &|t| t.comment.clone()); + // Two tempos that print the same to two places are the same. + let bpm = |t: &Track| format!("{:.2}", t.bpm); + cmp("bpm", a.bpm == b.bpm || bpm(a) == bpm(b), &bpm); + cmp("rating", a.rating == b.rating, &|t| t.rating.to_string()); + cmp("color", a.color == b.color, &|t| format!("{:?}", t.color)); + cmp("file_size", a.file_size == b.file_size, &|t| t.file_size.to_string()); + cmp("artwork", a.artwork_id.is_some() == b.artwork_id.is_some(), &|t| t.artwork_id.is_some().to_string()); out } From eacf138d5c386da4126ef8b3ac064960ed70e086 Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:56:47 -0500 Subject: [PATCH 3/3] perf(locallibrary): lower check.500's allocation limit to what it now costs Comparing tracks formats only the fields that differ, so checking two 500-track drives allocates 2.1% less: 205,609 on CI against a limit of 210,117, past the 1% tolerance the ratchet allows. --- benchmarks/budgets/alloc.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/budgets/alloc.json b/benchmarks/budgets/alloc.json index 7017d6f89..cc99bed2a 100644 --- a/benchmarks/budgets/alloc.json +++ b/benchmarks/budgets/alloc.json @@ -54,7 +54,7 @@ "measures": "Bytes allocated planning a sync of two 500-track drives." }, "check.500.allocations": { - "limit": 210117, + "limit": 205609, "unit": "allocations", "measures": "Allocations made checking two 500-track drives." },