Shanty/org
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Stop organize from reshuffling and overwriting files

Browse Source 
Treat a file at a numbered variant of its target as already in place,
so tracks that render to the same path are not moved between the
numbered names on every run. Skip candidate paths another track holds
in the database, refuse to overwrite an existing file when moving or
copying, and move the file back if the DB update fails.
This commit is contained in:
connor committed 2026-10-26 12:51:53 -04:00
1 parent 9608a7215e
commit 44b15c9961
2 files changed
+305 -66
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

src/organizer.rs
+206 -66
View File
@@ -7,7 +7,7 @@ use walkdir::WalkDir;
use shanty_db::queries;
use crate::error::OrgResult;
use crate::error::{OrgError, OrgResult};
use crate::metadata::TrackMetadata;
use crate::template;
@@ -46,54 +46,142 @@ impl fmt::Display for OrgStats {
}
}
/// Resolve the target path, handling conflicts by appending a number.
fn resolve_target(target: &Path) -> PathBuf {
if !target.exists() {
return target.to_owned();
}
/// The paths `target` may be written to, in order: the path itself, then
/// `stem (2).ext` through `stem (999).ext`.
fn target_candidates(target: &Path) -> impl Iterator<Item = PathBuf> {
let stem = target
.file_stem()
.and_then(|s| s.to_str())
.unwrap_or("file");
let ext = target.extension().and_then(|s| s.to_str()).unwrap_or("");
let parent = target.parent().unwrap_or(Path::new("."));
.unwrap_or("file")
.to_owned();
let ext = target
.extension()
.and_then(|s| s.to_str())
.unwrap_or("")
.to_owned();
let parent = target.parent().unwrap_or(Path::new(".")).to_owned();
for i in 2..=999 {
std::iter::once(target.to_owned()).chain((2..=999).map(move |i| {
let new_name = if ext.is_empty() {
format!("{stem} ({i})")
} else {
format!("{stem} ({i}).{ext}")
};
let candidate = parent.join(new_name);
if !candidate.exists() {
return candidate;
}
}
// Extremely unlikely fallback
target.to_owned()
parent.join(new_name)
}))
}
/// Move or copy a file, creating parent directories as needed.
/// Resolve the target path, handling conflicts by appending a number.
fn resolve_target(target: &Path) -> PathBuf {
target_candidates(target)
.find(|candidate| !candidate.exists())
.unwrap_or_else(|| target.to_owned())
}
/// Like [`resolve_target`], but also skips paths another track holds in the
/// database. A file moved onto such a path leaves its own record unable to
/// follow it (`tracks.file_path` is unique).
async fn resolve_target_db(
conn: &DatabaseConnection,
target: &Path,
track_id: i32,
) -> OrgResult<PathBuf> {
for candidate in target_candidates(target) {
if candidate.exists() {
continue;
}
match queries::tracks::get_by_path(conn, &candidate.to_string_lossy()).await? {
Some(other) if other.id != track_id => continue,
_ => return Ok(candidate),
}
}
Err(OrgError::Other(format!(
"no free path for {}",
target.display()
)))
}
/// Whether `source` already sits at `target`, or at a numbered variant of it
/// (`stem (N).ext`) that [`resolve_target`] would have produced. Without the
/// second check, tracks whose metadata renders to the same path are moved
/// between the numbered names on every run.
fn already_in_place(source: &Path, target: &Path) -> bool {
let Some(target_parent) = target.parent().filter(|p| p.exists()) else {
return false;
};
let source_canon = source.canonicalize().unwrap_or_else(|_| source.to_owned());
let parent_canon = target_parent
.canonicalize()
.unwrap_or_else(|_| target_parent.to_owned());
if source_canon.parent() != Some(parent_canon.as_path()) {
return false;
}
let (Some(name), Some(target_name)) = (
source_canon.file_name().and_then(|s| s.to_str()),
target.file_name().and_then(|s| s.to_str()),
) else {
return false;
};
if name == target_name {
return true;
}
let target_name = Path::new(target_name);
let Some(stem) = target_name.file_stem().and_then(|s| s.to_str()) else {
return false;
};
let rest = match target_name.extension().and_then(|s| s.to_str()) {
Some(ext) => name.strip_suffix(&format!(".{ext}")),
None => Some(name),
};
rest.and_then(|r| r.strip_prefix(stem))
.and_then(|r| r.strip_prefix(" ("))
.and_then(|r| r.strip_suffix(')'))
.is_some_and(|n| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit()))
}
/// Copy `source` to `target`, failing if `target` already exists.
fn copy_no_clobber(source: &Path, target: &Path) -> std::io::Result<()> {
let mut src = std::fs::File::open(source)?;
let mut dst = std::fs::OpenOptions::new()
.write(true)
.create_new(true)
.open(target)?;
let result = std::io::copy(&mut src, &mut dst)
.and_then(|_| dst.set_permissions(src.metadata()?.permissions()));
if result.is_err() {
drop(dst);
let _ = std::fs::remove_file(target);
}
result
}
/// Move or copy a file, creating parent directories as needed. Fails if
/// `target` exists: `rename` would replace it, and concurrent organize workers
/// that picked the same free name lost audio that way.
fn move_or_copy(source: &Path, target: &Path, copy: bool) -> OrgResult<()> {
if let Some(parent) = target.parent() {
std::fs::create_dir_all(parent)?;
}
if copy {
std::fs::copy(source, target)?;
copy_no_clobber(source, target)?;
tracing::info!(
from = %source.display(),
to = %target.display(),
"copied"
);
} else {
// Try rename first (fast, same filesystem)
if std::fs::rename(source, target).is_err() {
// Cross-filesystem: copy then delete
std::fs::copy(source, target)?;
std::fs::remove_file(source)?;
// A hard link fails if the target exists, unlike rename
match std::fs::hard_link(source, target) {
Ok(()) => std::fs::remove_file(source)?,
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => return Err(e.into()),
// Cross-filesystem or no hard link support: copy then delete
Err(_) => {
copy_no_clobber(source, target)?;
std::fs::remove_file(source)?;
}
}
tracing::info!(
from = %source.display(),
@@ -149,21 +237,12 @@ pub async fn organize_track(
let relative = template::render(&config.format, &meta);
let target = config.target_dir.join(&relative);
// Canonicalize for comparison
let source_canon = source.canonicalize().unwrap_or_else(|_| source.to_owned());
let target_parent = target.parent().unwrap_or(Path::new("."));
if target_parent.exists() {
let target_canon = target_parent
.canonicalize()
.map(|p| p.join(target.file_name().unwrap_or_default()))
.unwrap_or_else(|_| target.clone());
if source_canon == target_canon {
tracing::debug!(path = %track.file_path, "already in place, skipping");
return Ok(false);
}
if already_in_place(source, &target) {
tracing::debug!(path = %track.file_path, "already in place, skipping");
return Ok(false);
}
let final_target = resolve_target(&target);
let final_target = resolve_target_db(conn, &target, track.id).await?;
if config.dry_run {
return Ok(true);
@@ -179,7 +258,23 @@ pub async fn organize_track(
file_path: Set(new_path),
..Default::default()
};
queries::tracks::update_metadata(conn, track.id, active).await?;
if let Err(e) = queries::tracks::update_metadata(conn, track.id, active).await {
// Undo the move so the record still points at the file
let undo = if config.copy {
std::fs::remove_file(&final_target).map_err(OrgError::from)
} else {
move_or_copy(&final_target, source, false)
};
if let Err(undo_err) = undo {
tracing::error!(
id = track.id,
path = %final_target.display(),
error = %undo_err,
"failed to undo organize after DB error"
);
}
return Err(e.into());
}
// Cleanup empty source directories up to the library root
if !config.copy
@@ -223,22 +318,20 @@ pub async fn organize_from_db(
let relative = template::render(&config.format, &meta);
let target = config.target_dir.join(&relative);
// Canonicalize for comparison
let source_canon = source.canonicalize().unwrap_or_else(|_| source.to_owned());
let target_parent = target.parent().unwrap_or(Path::new("."));
if target_parent.exists() {
let target_canon = target_parent
.canonicalize()
.map(|p| p.join(target.file_name().unwrap_or_default()))
.unwrap_or_else(|_| target.clone());
if source_canon == target_canon {
tracing::debug!(path = %track.file_path, "already in place, skipping");
stats.files_skipped += 1;
continue;
}
if already_in_place(source, &target) {
tracing::debug!(path = %track.file_path, "already in place, skipping");
stats.files_skipped += 1;
continue;
}
let final_target = resolve_target(&target);
let final_target = match resolve_target_db(conn, &target, track.id).await {
Ok(t) => t,
Err(e) => {
tracing::error!(path = %track.file_path, error = %e, "failed to resolve target");
stats.files_errored += 1;
continue;
}
};
if config.dry_run {
println!("{} → {}", source.display(), final_target.display());
@@ -336,18 +429,9 @@ pub async fn organize_from_directory(source_dir: &Path, config: &OrgConfig) -> O
let relative = template::render(&config.format, &meta);
let target = config.target_dir.join(&relative);
// Skip if already in place
let source_canon = source.canonicalize().unwrap_or_else(|_| source.clone());
let target_parent = target.parent().unwrap_or(Path::new("."));
if target_parent.exists() {
let target_canon = target_parent
.canonicalize()
.map(|p| p.join(target.file_name().unwrap_or_default()))
.unwrap_or_else(|_| target.clone());
if source_canon == target_canon {
stats.files_skipped += 1;
continue;
}
if already_in_place(source, &target) {
stats.files_skipped += 1;
continue;
}
let final_target = resolve_target(&target);
@@ -406,3 +490,59 @@ fn cleanup_empty_dirs_recursive(root: &Path) {
}
}
}
#[cfg(test)]
mod tests {
use super::*;
use tempfile::TempDir;
#[test]
fn test_already_in_place() {
let dir = TempDir::new().unwrap();
let target = dir.path().join("Unknown Title.opus");
let at = |name: &str| {
let p = dir.path().join(name);
std::fs::write(&p, b"x").unwrap();
p
};
assert!(already_in_place(&at("Unknown Title.opus"), &target));
assert!(already_in_place(&at("Unknown Title (7).opus"), &target));
assert!(already_in_place(&at("Unknown Title (12).opus"), &target));
assert!(!already_in_place(&at("Unknown Title ().opus"), &target));
assert!(!already_in_place(&at("Unknown Title (a).opus"), &target));
assert!(!already_in_place(&at("Unknown Title (7).mp3"), &target));
assert!(!already_in_place(&at("Other Title (7).opus"), &target));
let other = TempDir::new().unwrap();
let elsewhere = other.path().join("Unknown Title (7).opus");
std::fs::write(&elsewhere, b"x").unwrap();
assert!(!already_in_place(&elsewhere, &target));
}
#[test]
fn test_move_or_copy_does_not_overwrite() {
let dir = TempDir::new().unwrap();
let source = dir.path().join("a.opus");
let target = dir.path().join("b.opus");
std::fs::write(&source, b"source").unwrap();
std::fs::write(&target, b"target").unwrap();
assert!(move_or_copy(&source, &target, false).is_err());
assert!(move_or_copy(&source, &target, true).is_err());
assert_eq!(std::fs::read(&source).unwrap(), b"source");
assert_eq!(std::fs::read(&target).unwrap(), b"target");
}
#[test]
fn test_move_or_copy_moves() {
let dir = TempDir::new().unwrap();
let source = dir.path().join("a.opus");
let target = dir.path().join("sub/b.opus");
std::fs::write(&source, b"source").unwrap();
move_or_copy(&source, &target, false).unwrap();
assert!(!source.exists());
assert_eq!(std::fs::read(&target).unwrap(), b"source");
}
}
tests/integration.rs
+99
View File
@@ -257,3 +257,102 @@ async fn test_organize_missing_metadata() {
.exists()
);
}
/// Insert a track with no metadata pointing at a fresh untagged file.
async fn insert_untagged(db: &Database, path: &std::path::Path, content: &[u8]) -> i32 {
fs::write(path, content).unwrap();
let now = Utc::now().naive_utc();
let active = shanty_db::entities::track::ActiveModel {
file_path: Set(path.to_string_lossy().to_string()),
file_size: Set(content.len() as i64),
added_at: Set(now),
updated_at: Set(now),
..Default::default()
};
queries::tracks::upsert(db.conn(), active).await.unwrap().id
}
#[tokio::test]
async fn test_organize_colliding_tracks_stay_in_place() {
let db = test_db().await;
let source = TempDir::new().unwrap();
let target = TempDir::new().unwrap();
let mut ids = Vec::new();
for i in 0..3u8 {
let path = source.path().join(format!("untagged{i}.mp3"));
ids.push(insert_untagged(&db, &path, &[i; 16]).await);
}
let config = OrgConfig {
target_dir: target.path().to_owned(),
format: DEFAULT_FORMAT.to_string(),
dry_run: false,
copy: false,
};
let stats = organize_from_db(db.conn(), &config).await.unwrap();
assert_eq!(stats.files_organized, 3);
let mut first_paths = Vec::new();
for id in &ids {
first_paths.push(
queries::tracks::get_by_id(db.conn(), *id)
.await
.unwrap()
.file_path,
);
}
// A second run must not shuffle them between the numbered names
let stats = organize_from_db(db.conn(), &config).await.unwrap();
assert_eq!(stats.files_organized, 0);
assert_eq!(stats.files_skipped, 3);
for (i, (id, first)) in ids.iter().zip(&first_paths).enumerate() {
let track = queries::tracks::get_by_id(db.conn(), *id).await.unwrap();
assert_eq!(&track.file_path, first);
// Each record still points at its own file
assert_eq!(fs::read(&track.file_path).unwrap(), vec![i as u8; 16]);
}
}
#[tokio::test]
async fn test_organize_track_skips_path_held_in_db() {
let db = test_db().await;
let source = TempDir::new().unwrap();
let target = TempDir::new().unwrap();
let base = target
.path()
.join("Unknown Artist/Unknown Album/00 - Unknown Title.mp3");
// Another track's record holds the target path, but its file is gone
fs::create_dir_all(base.parent().unwrap()).unwrap();
let other = insert_untagged(&db, &base, b"other").await;
fs::remove_file(&base).unwrap();
let path = source.path().join("untagged.mp3");
let id = insert_untagged(&db, &path, b"mine").await;
let config = OrgConfig {
target_dir: target.path().to_owned(),
format: DEFAULT_FORMAT.to_string(),
dry_run: false,
copy: false,
};
assert!(
shanty_org::organize_track(db.conn(), id, &config)
.await
.unwrap()
);
let track = queries::tracks::get_by_id(db.conn(), id).await.unwrap();
let expected = target
.path()
.join("Unknown Artist/Unknown Album/00 - Unknown Title (2).mp3");
assert_eq!(track.file_path, expected.to_string_lossy());
assert_eq!(fs::read(&expected).unwrap(), b"mine");
let other = queries::tracks::get_by_id(db.conn(), other).await.unwrap();
assert_eq!(other.file_path, base.to_string_lossy());
}