diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-src__ChildWithDuplicates.rbxm.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-src__ChildWithDuplicates.rbxm.snap deleted file mode 100644 index 7fe594f1..00000000 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-src__ChildWithDuplicates.rbxm.snap +++ /dev/null @@ -1,73 +0,0 @@ ---- -source: tests/rojo_test/syncback_util.rs -expression: src/ChildWithDuplicates.rbxm ---- -num_types: 1 -num_instances: 3 -chunks: - - Inst: - type_id: 0 - type_name: Folder - object_format: 0 - referents: - - 0 - - 1 - - 2 - - Prop: - type_id: 0 - prop_name: AttributesSerialize - prop_type: String - values: - - "" - - "" - - "" - - Prop: - type_id: 0 - prop_name: Capabilities - prop_type: SecurityCapabilities - values: - - 0 - - 0 - - 0 - - Prop: - type_id: 0 - prop_name: Name - prop_type: String - values: - - DuplicateChild - - DuplicateChild - - ChildWithDuplicates - - Prop: - type_id: 0 - prop_name: DefinesCapabilities - prop_type: Bool - values: - - false - - false - - false - - Prop: - type_id: 0 - prop_name: SourceAssetId - prop_type: Int64 - values: - - -1 - - -1 - - -1 - - Prop: - type_id: 0 - prop_name: Tags - prop_type: String - values: - - "" - - "" - - "" - - Prnt: - version: 0 - links: - - - 0 - - 2 - - - 1 - - 2 - - - 2 - - -1 - - End diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-stdout.snap index ecf33548..b0495943 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-stdout.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback-stdout.snap @@ -1,9 +1,12 @@ --- source: tests/rojo_test/syncback_util.rs +assertion_line: 101 expression: "String::from_utf8_lossy(&output.stdout)" --- -Writing src/ChildWithDuplicates.rbxm +Writing src/ChildWithDuplicates/DuplicateChild/.gitkeep +Writing src/ChildWithDuplicates/DuplicateChild1/.gitkeep Writing src/ChildWithoutDuplicates/Child/.gitkeep +Writing src/ChildWithDuplicates/DuplicateChild +Writing src/ChildWithDuplicates/DuplicateChild1 Writing src/ChildWithoutDuplicates Writing src/ChildWithoutDuplicates/Child -Removing src/ChildWithDuplicates diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__rbxm_fallback-src__ChildWithDuplicates__DuplicateChild1__.gitkeep.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__rbxm_fallback-src__ChildWithDuplicates__DuplicateChild1__.gitkeep.snap new file mode 100644 index 00000000..e0ec5e75 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__rbxm_fallback-src__ChildWithDuplicates__DuplicateChild1__.gitkeep.snap @@ -0,0 +1,6 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/ChildWithDuplicates/DuplicateChild1/.gitkeep +--- + diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__rbxm_fallback-src__ChildWithDuplicates__DuplicateChild__.gitkeep.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__rbxm_fallback-src__ChildWithDuplicates__DuplicateChild__.gitkeep.snap new file mode 100644 index 00000000..28b37886 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__rbxm_fallback-src__ChildWithDuplicates__DuplicateChild__.gitkeep.snap @@ -0,0 +1,6 @@ +--- +source: tests/tests/syncback.rs +assertion_line: 31 +expression: src/ChildWithDuplicates/DuplicateChild/.gitkeep +--- + diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index 48288a5b..3c3da14c 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -109,17 +109,12 @@ pub fn syncback_csv<'sync>( if !meta.is_empty() { let parent = snapshot.path.parent_err()?; - let instance_name = &new_inst.name; - let base = if crate::syncback::validate_file_name(instance_name).is_err() { - crate::syncback::slugify_name(instance_name) - } else { - instance_name.clone() - }; - let meta_stem = if base.to_lowercase() == "init" { - format!("_{base}") - } else { - base - }; + let file_name = snapshot + .path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(""); + let meta_stem = file_name.strip_suffix(".csv").unwrap_or(file_name); fs_snapshot.add_file( parent.join(format!("{meta_stem}.meta.json")), serde_json::to_vec_pretty(&meta).context("cannot serialize metadata")?, diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index 50e211fd..2696837f 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -8,10 +8,13 @@ use memofs::{DirEntry, Vfs}; use crate::{ snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource}, - syncback::{hash_instance, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, + syncback::{ + extension_for_middleware, hash_instance, FsSnapshot, SyncbackReturn, + SyncbackSnapshot, + }, }; -use super::{meta_file::DirectoryMetadata, snapshot_from_vfs}; +use super::{meta_file::DirectoryMetadata, snapshot_from_vfs, Middleware}; const EMPTY_DIR_KEEP_NAME: &str = ".gitkeep"; @@ -91,6 +94,22 @@ pub fn snapshot_dir_no_meta( Ok(Some(snapshot)) } +/// Splits a filesystem name into (stem, extension) based on middleware type. +/// For directory middleware, the extension is empty. For file middleware, +/// the extension comes from `extension_for_middleware`. +fn split_name_and_ext(name: &str, middleware: Middleware) -> (&str, &str) { + if middleware.is_dir() { + (name, "") + } else { + let ext = extension_for_middleware(middleware); + if let Some(stem) = name.strip_suffix(&format!(".{ext}")) { + (stem, ext) + } else { + (name, "") + } + } +} + pub fn syncback_dir<'sync>( snapshot: &SyncbackSnapshot<'sync>, ) -> anyhow::Result> { @@ -143,77 +162,119 @@ pub fn syncback_dir_no_meta<'sync>( } } - // Enforce unique filesystem names. Uses actual on-disk names for existing - // children and resolved names (with init-prefix) for new ones. - let mut fs_child_names = HashSet::with_capacity(new_inst.children().len()); - let mut duplicate_set = HashSet::new(); - for child_ref in new_inst.children() { - let child = snapshot.get_new_instance(*child_ref).unwrap(); - let fs_name = old_child_map - .get(child.name.as_str()) - .and_then(|old| old.metadata().relevant_paths.first()) - .and_then(|p| p.file_name()) - .and_then(|n| n.to_str()) - .map(|s| s.to_lowercase()) - .unwrap_or_else(|| { - let slug = slugify_name(&child.name); - let slug_lower = slug.to_lowercase(); - // Mirror name_for_inst's init-prefix. - if slug_lower == "init" { - format!("_{slug_lower}") - } else { - slug_lower - } - }); - - if !fs_child_names.insert(fs_name) { - duplicate_set.insert(child.name.as_str()); - } - } - if !duplicate_set.is_empty() { - if duplicate_set.len() <= 25 { - anyhow::bail!( - "Instance has children with duplicate name (case may not exactly match):\n {}", - duplicate_set.into_iter().collect::>().join(", ") - ); - } - anyhow::bail!("Instance has more than 25 children with duplicate names"); + // --- Two-pass collision resolution --- + // + // Pass 1: Collect each child's base filesystem name and old ref, applying + // skip conditions. Track which names are used (lowercased) so we can + // detect collisions. + struct ChildEntry { + new_ref: rbx_dom_weak::types::Ref, + old_ref: Option, + base_name: String, + middleware: Middleware, + skip: bool, } - if snapshot.old_inst().is_some() { - for new_child_ref in new_inst.children() { - let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); - if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) { - if old_child.metadata().relevant_paths.is_empty() { - log::debug!( - "Skipping instance {} because it doesn't exist on the disk", - old_child.name() - ); - continue; - } else if matches!( - old_child.metadata().instigating_source, - Some(InstigatingSource::ProjectNode { .. }) - ) { - log::debug!( - "Skipping instance {} because it originates in a project file", - old_child.name() - ); - continue; - } - // This child exists in both doms. Pass it on. - children.push(snapshot.with_joined_path(*new_child_ref, Some(old_child.id()))?); - } else { - // The child only exists in the the new dom - children.push(snapshot.with_joined_path(*new_child_ref, None)?); + let mut entries = Vec::with_capacity(new_inst.children().len()); + let mut used_names: HashSet = HashSet::with_capacity(new_inst.children().len()); + let mut collision_indices: Vec = Vec::new(); + + for new_child_ref in new_inst.children() { + let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); + + // Determine old_ref and apply skip conditions. + let old_child = if snapshot.old_inst().is_some() { + old_child_map.remove(new_child.name.as_str()) + } else { + None + }; + + let mut skip = false; + if let Some(ref old) = old_child { + if old.metadata().relevant_paths.is_empty() { + log::debug!( + "Skipping instance {} because it doesn't exist on the disk", + old.name() + ); + skip = true; + } else if matches!( + old.metadata().instigating_source, + Some(InstigatingSource::ProjectNode { .. }) + ) { + log::debug!( + "Skipping instance {} because it originates in a project file", + old.name() + ); + skip = true; } } - // Any children that are in the old dom but not the new one are removed. - removed_children.extend(old_child_map.into_values()); - } else { - // There is no old instance. Just add every child. - for new_child_ref in new_inst.children() { - children.push(snapshot.with_joined_path(*new_child_ref, None)?); + + let old_ref = old_child.as_ref().map(|o| o.id()); + + if skip { + entries.push(ChildEntry { + new_ref: *new_child_ref, + old_ref, + base_name: String::new(), + middleware: Middleware::Dir, + skip: true, + }); + continue; } + + let (middleware, base_name) = + snapshot.child_middleware_and_name(*new_child_ref, old_ref)?; + + let idx = entries.len(); + let lower = base_name.to_lowercase(); + if !used_names.insert(lower) { + // Name already claimed — needs resolution. + collision_indices.push(idx); + } + + entries.push(ChildEntry { + new_ref: *new_child_ref, + old_ref, + base_name, + middleware, + skip: false, + }); + } + + // Pass 2: Resolve collisions by appending incrementing suffixes. + for idx in collision_indices { + let entry = &entries[idx]; + let (stem, ext) = split_name_and_ext(&entry.base_name, entry.middleware); + let mut counter = 1u32; + loop { + let candidate = if ext.is_empty() { + format!("{stem}{counter}") + } else { + format!("{stem}{counter}.{ext}") + }; + let lower = candidate.to_lowercase(); + if used_names.insert(lower) { + // Safe to mutate — we only visit each collision index once. + let entry = &mut entries[idx]; + entry.base_name = candidate; + break; + } + counter += 1; + } + } + + // Create snapshots from resolved entries. + for entry in &entries { + if entry.skip { + continue; + } + let resolved_path = snapshot.path.join(&entry.base_name); + children.push(snapshot.with_new_path(resolved_path, entry.new_ref, entry.old_ref)); + } + + // Any children that are in the old dom but not the new one are removed. + if snapshot.old_inst().is_some() { + removed_children.extend(old_child_map.into_values()); } let mut fs_snapshot = FsSnapshot::new(); @@ -362,14 +423,15 @@ mod test { assert!( result.is_ok(), "should not error when two children have the same lowercased Roblox \ - name but map to distinct filesystem paths: {result:?}", + name but map to distinct filesystem paths: {:?}", + result.as_ref().err(), ); } - /// Two completely new children with the same non-init name would produce - /// the same filesystem entry and must be detected as a duplicate. + /// Two completely new children with the same name get resolved via + /// incrementing suffixes instead of erroring. #[test] - fn syncback_detects_sibling_duplicate_names() { + fn syncback_resolves_sibling_duplicate_names() { use rbx_dom_weak::{InstanceBuilder, WeakDom}; let old_parent = InstanceSnapshot::new() @@ -387,8 +449,6 @@ mod test { new_tree.root_ref(), InstanceBuilder::new("Folder").with_name("Parent"), ); - // "Foo" is not a reserved name but two siblings named "Foo" still - // collide on disk. new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo")); new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo")); @@ -405,9 +465,17 @@ mod test { let result = syncback_dir_no_meta(&snapshot); assert!( - result.is_err(), - "should error when two new children would produce the same filesystem name", + result.is_ok(), + "should resolve duplicate names with suffixes, not error: {:?}", + result.as_ref().err(), ); + let children = result.unwrap().children; + let mut names: Vec = children + .iter() + .map(|c| c.path.file_name().unwrap().to_string_lossy().into_owned()) + .collect(); + names.sort(); + assert_eq!(names, vec!["Foo", "Foo1"]); } /// A new child named "Init" (as a ModuleScript) would naively become @@ -452,7 +520,8 @@ mod test { let result = syncback_dir_no_meta(&snapshot); assert!( result.is_ok(), - "should resolve init-name conflict by prefixing '_', not error: {result:?}", + "should resolve init-name conflict by prefixing '_', not error: {:?}", + result.as_ref().err(), ); // The child should have been placed at "_Init.luau", not "Init.luau". let child_file_name = result @@ -518,7 +587,64 @@ mod test { assert!( result.is_ok(), "should allow a child whose filesystem name is slugified away from \ - the reserved 'init' stem: {result:?}", + the reserved 'init' stem: {:?}", + result.as_ref().err(), ); } + + /// Two new children both named "Init" (ModuleScripts) should get + /// "_Init.luau" and "_Init1.luau" respectively. + #[test] + fn syncback_resolves_multiple_init_conflicts() { + use rbx_dom_weak::{InstanceBuilder, WeakDom}; + + let old_parent = InstanceSnapshot::new() + .name("Parent") + .class_name("Folder") + .metadata( + InstanceMetadata::new() + .instigating_source(PathBuf::from("/root")) + .relevant_paths(vec![PathBuf::from("/root")]), + ); + let old_tree = RojoTree::new(old_parent); + + let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT")); + let new_parent = new_tree.insert( + new_tree.root_ref(), + InstanceBuilder::new("Folder").with_name("Parent"), + ); + new_tree.insert( + new_parent, + InstanceBuilder::new("ModuleScript").with_name("Init"), + ); + new_tree.insert( + new_parent, + InstanceBuilder::new("ModuleScript").with_name("Init"), + ); + + let vfs = make_vfs(); + let project = make_project(); + let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project); + let snapshot = SyncbackSnapshot { + data, + old: Some(old_tree.get_root_id()), + new: new_parent, + path: PathBuf::from("/root"), + middleware: None, + }; + + let result = syncback_dir_no_meta(&snapshot); + assert!( + result.is_ok(), + "should resolve multiple init conflicts with suffixes: {:?}", + result.as_ref().err(), + ); + let children = result.unwrap().children; + let mut names: Vec = children + .iter() + .map(|c| c.path.file_name().unwrap().to_string_lossy().into_owned()) + .collect(); + names.sort(); + assert_eq!(names, vec!["_Init.luau", "_Init1.luau"]); + } } diff --git a/src/snapshot_middleware/lua.rs b/src/snapshot_middleware/lua.rs index 560523b4..2dbcc21a 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -158,17 +158,21 @@ pub fn syncback_lua<'sync>( if !meta.is_empty() { let parent_location = snapshot.path.parent_err()?; - let instance_name = &snapshot.new_inst().name; - let base = if crate::syncback::validate_file_name(instance_name).is_err() { - crate::syncback::slugify_name(instance_name) - } else { - instance_name.clone() - }; - let meta_stem = if base.to_lowercase() == "init" { - format!("_{base}") - } else { - base - }; + let file_name = snapshot + .path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(""); + let meta_stem = file_name + .strip_suffix(".server.luau") + .or_else(|| file_name.strip_suffix(".server.lua")) + .or_else(|| file_name.strip_suffix(".client.luau")) + .or_else(|| file_name.strip_suffix(".client.lua")) + .or_else(|| file_name.strip_suffix(".plugin.luau")) + .or_else(|| file_name.strip_suffix(".plugin.lua")) + .or_else(|| file_name.strip_suffix(".luau")) + .or_else(|| file_name.strip_suffix(".lua")) + .unwrap_or(file_name); fs_snapshot.add_file( parent_location.join(format!("{meta_stem}.meta.json")), serde_json::to_vec_pretty(&meta).context("cannot serialize metadata")?, diff --git a/src/snapshot_middleware/txt.rs b/src/snapshot_middleware/txt.rs index 5b024a14..8f762087 100644 --- a/src/snapshot_middleware/txt.rs +++ b/src/snapshot_middleware/txt.rs @@ -58,17 +58,12 @@ pub fn syncback_txt<'sync>( if !meta.is_empty() { let parent = snapshot.path.parent_err()?; - let instance_name = &new_inst.name; - let base = if crate::syncback::validate_file_name(instance_name).is_err() { - crate::syncback::slugify_name(instance_name) - } else { - instance_name.clone() - }; - let meta_stem = if base.to_lowercase() == "init" { - format!("_{base}") - } else { - base - }; + let file_name = snapshot + .path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(""); + let meta_stem = file_name.strip_suffix(".txt").unwrap_or(file_name); fs_snapshot.add_file( parent.join(format!("{meta_stem}.meta.json")), serde_json::to_vec_pretty(&meta).context("could not serialize metadata")?, diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 9bc60fe5..61d26674 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -28,7 +28,7 @@ use crate::{ Project, }; -pub use file_names::{extension_for_middleware, name_for_inst, slugify_name, validate_file_name}; +pub use file_names::{extension_for_middleware, name_for_inst, validate_file_name}; pub use fs_snapshot::FsSnapshot; pub use hash::*; pub use property_filter::{filter_properties, filter_properties_preallocated}; diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index dfdf1a62..c36341c4 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -31,6 +31,25 @@ pub struct SyncbackSnapshot<'sync> { } impl<'sync> SyncbackSnapshot<'sync> { + /// Computes the middleware and filesystem name for a child without + /// creating a full snapshot. Uses the same logic as `with_joined_path`. + pub fn child_middleware_and_name( + &self, + new_ref: Ref, + old_ref: Option, + ) -> anyhow::Result<(Middleware, String)> { + let temp = Self { + data: self.data, + old: old_ref, + new: new_ref, + path: PathBuf::new(), + middleware: None, + }; + let middleware = get_best_middleware(&temp, self.data.force_json); + let name = name_for_inst(middleware, temp.new_inst(), temp.old_inst())?; + Ok((middleware, name.into_owned())) + } + /// Constructs a SyncbackSnapshot from the provided refs /// while inheriting this snapshot's path and data. This should be used for /// directories. @@ -251,6 +270,7 @@ impl<'sync> SyncbackData<'sync> { old_tree, new_tree, project, + force_json: false, } } } diff --git a/tests/tests/syncback.rs b/tests/tests/syncback.rs index 10be92a8..9cf17e99 100644 --- a/tests/tests/syncback.rs +++ b/tests/tests/syncback.rs @@ -60,8 +60,8 @@ syncback_tests! { // Ensures that projects can be reserialized by syncback and that // default.project.json doesn't change unexpectedly. project_reserialize => ["attribute_mismatch.luau", "property_mismatch.project.json"], - // Confirms that Instances that cannot serialize as directories serialize as rbxms - rbxm_fallback => ["src/ChildWithDuplicates.rbxm"], + // Confirms that duplicate children are resolved with incrementing suffixes + rbxm_fallback => ["src/ChildWithDuplicates/DuplicateChild/.gitkeep", "src/ChildWithDuplicates/DuplicateChild1/.gitkeep"], // Ensures that ref properties are linked properly on the file system ref_properties => ["src/pointer.model.json", "src/target.model.json"], // Ensures that ref properties are linked when no attributes are manually