diff --git a/.cursor/plans/strip_dedup_suffix_automatically_3d35c847.plan.md b/.cursor/plans/strip_dedup_suffix_automatically_3d35c847.plan.md new file mode 100644 index 00000000..2deafdfa --- /dev/null +++ b/.cursor/plans/strip_dedup_suffix_automatically_3d35c847.plan.md @@ -0,0 +1,117 @@ +--- +name: Strip dedup suffix automatically +overview: Eliminate unnecessary `.meta.json5` files for dedup'd instances by having forward sync automatically strip `~N` dedup suffixes from filenames when deriving instance names, and having syncback skip writing meta `name` fields for dedup-only cases. +todos: + - id: strip-fn + content: Add `strip_dedup_suffix()` to `dedup_suffix.rs` and ensure it is importable from snapshot code + status: completed + - id: forward-files + content: Modify `file_name_for_path()` in `metadata.rs` to strip `~N` dedup suffixes + status: completed + - id: forward-dirs + content: Modify `get_dir_middleware()` in `snapshot_middleware/mod.rs` to strip `~N` from directory names + status: completed + - id: syncback-meta + content: Change `name_for_inst()` needs_meta condition to `needs_slugify` only (remove dedup check) + status: completed + - id: change-processor + content: Update file rename handler in `change_processor.rs` to use `slugified_new_name` for meta name conditions + status: completed + - id: update-tests + content: Update existing tests and fixtures for new dedup-only behavior (no meta name) + status: completed + - id: new-tests + content: "Add new tests: forward sync stripping, edge cases, syncback dedup-only, roundtrip without meta" + status: completed +isProject: false +--- + +# Auto-Strip Dedup Suffixes to Eliminate Redundant Meta Files + +## Problem + +When syncback encounters duplicate instance names (e.g., two instances named "LocalScript"), it creates: + +- `LocalScript.local.luau` (first) +- `LocalScript~2.local.luau` (second, with dedup suffix) +- `LocalScript~2.meta.json5` with `name: "LocalScript"` (meta file to recover real name) + +The meta file is redundant because `~N` is a well-defined system convention -- the `~` character is forbidden in instance names (always slugified to `_`), so `~N` can only come from the dedup system. Forward sync can strip it automatically. + +## Safety Argument + +- The `~` character is in `SLUGIFY_CHARS` (`[file_names.rs](src/syncback/file_names.rs)`), so any instance literally named `Foo~2` would be slugified to `Foo_2` during syncback. Natural tilde names cannot produce the `~N` pattern. +- Existing projects with meta files still work: meta `name` field takes priority over filename-derived name (`[meta_file.rs](src/snapshot_middleware/meta_file.rs)` `apply_name`). +- Non-dedup patterns like `~0`, `~abc`, `~` are NOT stripped (only `~N` where N > 0). + +## Changes + +### 1. Add `strip_dedup_suffix()` utility + +Add to `[src/syncback/dedup_suffix.rs](src/syncback/dedup_suffix.rs)` alongside `parse_dedup_suffix`: + +```rust +pub fn strip_dedup_suffix(name: &str) -> &str { + parse_dedup_suffix(name).map_or(name, |(base, _)| base) +} +``` + +### 2. Forward sync: strip dedup from file names + +In `[src/snapshot/metadata.rs](src/snapshot/metadata.rs)`, `SyncRule::file_name_for_path()` (line 330) -- after stripping the middleware extension, also strip `~N`: + +```rust +pub fn file_name_for_path<'a>(&self, path: &'a Path) -> anyhow::Result<&'a str> { + let name = /* existing logic to strip middleware extension */; + Ok(strip_dedup_suffix(name)) +} +``` + +The return type stays `&'a str` because stripping `~N` returns a sub-slice of the same string. + +### 3. Forward sync: strip dedup from directory names + +In `[src/snapshot_middleware/mod.rs](src/snapshot_middleware/mod.rs)`, `get_dir_middleware()` (line 116) -- strip `~N` from `dir_name` before returning: + +```rust +let dir_name = strip_dedup_suffix(dir_name); +``` + +Return type stays `&'path str` (sub-slice of the original path). + +### 4. Syncback: don't require meta name for dedup-only + +In `[src/syncback/file_names.rs](src/syncback/file_names.rs)`, `name_for_inst()` line 79 -- change: + +```rust +// Before: +let needs_meta = needs_slugify || deduped_slug != base; +// After: +let needs_meta = needs_slugify; +``` + +When only dedup happened (no slugification), forward sync will recover the correct name by stripping `~N`. + +### 5. Two-way sync: update meta name conditions in change_processor + +In `[src/change_processor.rs](src/change_processor.rs)`, the file rename handler (lines 1569, 1581) currently uses `deduped_new_name != *new_name` to decide whether meta name is needed. Change both to `slugified_new_name != *new_name`, matching the directory handler (line 1401) which already does this correctly. + +### 6. Module visibility + +Ensure `strip_dedup_suffix` is importable from snapshot code. May need to adjust `pub` visibility on the `syncback::dedup_suffix` module, or alternatively place the function in a shared location if cross-module import is problematic. + +### 7. Update existing tests and fixtures + +- `**src/syncback/file_names.rs` tests:** Update tests like `name_for_inst_dedup_collision` (line 826) and `nfi_clean_name_dedup_still_needs_meta_dir` (line 1175) -- these should now assert `needs_meta = false` for dedup-only cases. +- `**rojo-test/build-tests/dedup_suffix_with_meta/`:** Update fixture to remove now-unnecessary meta files for dedup-only cases. +- `**tests/tests/syncback_roundtrip.rs`:** The `dedup_suffix_with_meta` roundtrip test (line 299) and idempotency test (line 362) may need updated expected output. + +### 8. Add new tests + +- **Forward sync unit test:** File `Foo~2.luau` without meta -> instance name is `"Foo"` +- **Forward sync unit test:** Directory `Foo~2/` without meta -> instance name is `"Foo"` +- **Forward sync edge cases:** `Foo~0.luau` -> `"Foo~0"`, `Foo~abc.luau` -> `"Foo~abc"` (not valid dedup, kept as-is) +- **Syncback test:** Dedup-only case produces no meta `name` field +- **Syncback test:** Slugify + dedup still produces meta `name` field (e.g., `"Hey/Bro"` -> `Hey_Bro~2.luau` with meta name `"Hey/Bro"`) +- **Roundtrip test:** Dedup'd names without meta files survive build -> syncback -> rebuild + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_auto_strip.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_auto_strip.snap new file mode 100644 index 00000000..08eb1e11 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_auto_strip.snap @@ -0,0 +1,32 @@ +--- +source: tests/tests/build.rs +expression: contents +--- + + + + dedup_suffix_auto_strip + + + + Baz~0 + + + + + + Foo + + + + + + Foo + + + + + diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__tilde_no_meta.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__tilde_no_meta.snap index 4eabf6af..b25830d0 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__tilde_no_meta.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__tilde_no_meta.snap @@ -9,8 +9,8 @@ expression: contents - Foo~1 - Foo + diff --git a/rojo-test/build-tests/dedup_suffix_auto_strip/default.project.json5 b/rojo-test/build-tests/dedup_suffix_auto_strip/default.project.json5 new file mode 100644 index 00000000..225a5264 --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_auto_strip/default.project.json5 @@ -0,0 +1,6 @@ +{ + "name": "dedup_suffix_auto_strip", + "tree": { + "$path": "src" + } +} diff --git a/rojo-test/build-tests/dedup_suffix_auto_strip/src/Baz~0.luau b/rojo-test/build-tests/dedup_suffix_auto_strip/src/Baz~0.luau new file mode 100644 index 00000000..49cc226c --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_auto_strip/src/Baz~0.luau @@ -0,0 +1 @@ +return "not a dedup suffix" diff --git a/rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo.luau b/rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo.luau new file mode 100644 index 00000000..6bb0d45c --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo.luau @@ -0,0 +1 @@ +return "first" diff --git a/rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo~2.luau b/rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo~2.luau new file mode 100644 index 00000000..70a0843e --- /dev/null +++ b/rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo~2.luau @@ -0,0 +1 @@ +return "second" diff --git a/rojo-test/build-tests/tilde_no_meta/src/Foo~1.luau b/rojo-test/build-tests/tilde_no_meta/src/Foo~1.luau index 972b31c7..517b8840 100644 --- a/rojo-test/build-tests/tilde_no_meta/src/Foo~1.luau +++ b/rojo-test/build-tests/tilde_no_meta/src/Foo~1.luau @@ -1,2 +1,2 @@ --- Tilde in filename without meta should produce instance name "Foo~1" +-- Dedup suffix ~1: forward sync strips it to produce instance name "Foo" return nil diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__connected_mode__connected_dedup_no_meta_stem_patch.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__connected_mode__connected_dedup_no_meta_stem_patch.snap index c742866e..5c87e49b 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__connected_mode__connected_dedup_no_meta_stem_patch.snap +++ b/rojo-test/serve-test-snapshots/end_to_end__tests__connected_mode__connected_dedup_no_meta_stem_patch.snap @@ -12,7 +12,7 @@ body: Id: id-13 Metadata: ignoreUnknownInstances: false - Name: Foo~1 + Name: Foo Parent: id-3 Properties: Source: 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 d57c1961..20a658ad 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 @@ -3,7 +3,7 @@ source: tests/rojo_test/syncback_util.rs expression: normalized_stdout --- Writing src/ChildWithDuplicates/DuplicateChild/.gitkeep -Writing src/ChildWithDuplicates/DuplicateChild~2/init.meta.json5 +Writing src/ChildWithDuplicates/DuplicateChild~2/.gitkeep Writing src/ChildWithoutDuplicates/Child/.gitkeep Writing src/ChildWithDuplicates Writing src/ChildWithDuplicates/DuplicateChild diff --git a/src/change_processor.rs b/src/change_processor.rs index efe90f47..8c0e7a70 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -1556,8 +1556,9 @@ impl JobThreadContext { // Always update the name field when the // instance name changed, even if the path didn't // change (e.g. "Foo/Bar" → "Foo|Bar" both slugify - // to "Foo_Bar"). Use deduped_new_name because - // dedup may have appended ~N. + // to "Foo_Bar"). Only slugification requires a + // meta name -- dedup suffixes (~N) are stripped + // automatically by forward sync. // // For .model.json5/.model.json files, the name // field lives INSIDE the model file, not in @@ -1566,7 +1567,7 @@ impl JobThreadContext { let model_file = overridden_source_path .as_deref() .unwrap_or(path.as_path()); - if deduped_new_name != *new_name { + if slugified_new_name != *new_name { self.upsert_model_name_field( model_file, new_name, ); @@ -1578,7 +1579,7 @@ impl JobThreadContext { "{}.meta.json5", effective_meta_base )); - if deduped_new_name != *new_name { + if slugified_new_name != *new_name { self.upsert_meta_name_field( ¤t_meta, new_name, diff --git a/src/snapshot/metadata.rs b/src/snapshot/metadata.rs index 132769eb..82612f30 100644 --- a/src/snapshot/metadata.rs +++ b/src/snapshot/metadata.rs @@ -8,7 +8,8 @@ use anyhow::Context; use serde::{Deserialize, Serialize}; use crate::{ - glob::Glob, path_serializer, project::ProjectNode, snapshot_middleware::Middleware, RojoRef, + glob::Glob, path_serializer, project::ProjectNode, snapshot_middleware::Middleware, + syncback::dedup_suffix::strip_dedup_suffix, RojoRef, }; /// Rojo-specific metadata that can be associated with an instance or a snapshot @@ -328,23 +329,22 @@ impl SyncRule { } pub fn file_name_for_path<'a>(&self, path: &'a Path) -> anyhow::Result<&'a str> { - if let Some(suffix) = &self.suffix { + let name = if let Some(suffix) = &self.suffix { let file_name = path .file_name() .and_then(|s| s.to_str()) .with_context(|| format!("file name of {} is invalid", path.display()))?; if file_name.ends_with(suffix) { let end = file_name.len().saturating_sub(suffix.len()); - Ok(&file_name[..end]) + &file_name[..end] } else { - Ok(file_name) + file_name } } else { - // If the user doesn't specify a suffix, we assume they just want - // the name of the file (the file_stem) path.file_stem() .and_then(|s| s.to_str()) - .with_context(|| format!("file name of {} is invalid", path.display())) - } + .with_context(|| format!("file name of {} is invalid", path.display()))? + }; + Ok(strip_dedup_suffix(name)) } } diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index 7ea64881..f8ca439e 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -31,7 +31,7 @@ use serde::{Deserialize, Serialize}; use crate::{ glob::Glob, project::DEFAULT_PROJECT_NAMES, - syncback::{SyncbackReturn, SyncbackSnapshot}, + syncback::{dedup_suffix::strip_dedup_suffix, SyncbackReturn, SyncbackSnapshot}, }; use crate::{ snapshot::{InstanceContext, InstanceSnapshot, SyncRule}, @@ -118,6 +118,7 @@ fn get_dir_middleware<'path>( .expect("Could not extract directory name") .to_str() .ok_or_else(|| anyhow::anyhow!("File name was not valid UTF-8: {}", dir_path.display()))?; + let dir_name = strip_dedup_suffix(dir_name); static INIT_PATHS: OnceLock> = OnceLock::new(); let order = INIT_PATHS.get_or_init(|| { diff --git a/src/syncback/dedup_suffix.rs b/src/syncback/dedup_suffix.rs index 307e6192..d17471b6 100644 --- a/src/syncback/dedup_suffix.rs +++ b/src/syncback/dedup_suffix.rs @@ -58,6 +58,16 @@ pub fn parse_dedup_suffix(stem: &str) -> Option<(&str, u32)> { None } +/// Strips a `~N` dedup suffix from a filename stem, returning the base name. +/// +/// Given `"Foo~2"`, returns `"Foo"`. +/// Given `"Foo"`, returns `"Foo"` unchanged. +/// Given `"Foo~0"` or `"Foo~abc"`, returns the input unchanged (not valid +/// dedup suffixes). +pub fn strip_dedup_suffix(name: &str) -> &str { + parse_dedup_suffix(name).map_or(name, |(base, _)| base) +} + /// Builds a dedup'd filename from a base stem, optional suffix number, and /// extension. /// @@ -175,6 +185,22 @@ mod tests { assert_eq!(parse_dedup_suffix("My Script~1"), Some(("My Script", 1))); } + #[test] + fn strip_suffix_basic() { + assert_eq!(strip_dedup_suffix("Foo~1"), "Foo"); + assert_eq!(strip_dedup_suffix("Foo~2"), "Foo"); + assert_eq!(strip_dedup_suffix("Foo~10"), "Foo"); + } + + #[test] + fn strip_suffix_no_op() { + assert_eq!(strip_dedup_suffix("Foo"), "Foo"); + assert_eq!(strip_dedup_suffix("Foo~0"), "Foo~0"); + assert_eq!(strip_dedup_suffix("Foo~abc"), "Foo~abc"); + assert_eq!(strip_dedup_suffix("Foo~"), "Foo~"); + assert_eq!(strip_dedup_suffix(""), ""); + } + #[test] fn build_name_file() { assert_eq!( diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index 7636c049..076feef9 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -75,8 +75,9 @@ pub fn name_for_inst<'a>( Some(extension_for_middleware(middleware)) }; - let (deduped_slug, full_fs_name) = deduplicate_name_with_ext(&base, extension, taken_names); - let needs_meta = needs_slugify || deduped_slug != base; + let (_deduped_slug, full_fs_name) = + deduplicate_name_with_ext(&base, extension, taken_names); + let needs_meta = needs_slugify; let filename = full_fs_name.clone(); Ok((Cow::Owned(filename), needs_meta, full_fs_name)) @@ -832,7 +833,10 @@ mod tests { let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Foo~2.luau"); - assert!(needs_meta, "deduped name differs from original"); + assert!( + !needs_meta, + "dedup-only: forward sync strips ~N automatically" + ); } #[test] @@ -845,7 +849,10 @@ mod tests { let (filename, needs_meta, _dk) = name_for_inst(Middleware::Dir, child, None, &taken).unwrap(); assert_eq!(filename.as_ref(), "Stuff~2"); - assert!(needs_meta); + assert!( + !needs_meta, + "dedup-only: forward sync strips ~N automatically" + ); } #[test] @@ -1172,21 +1179,19 @@ mod tests { } #[test] - fn nfi_clean_name_dedup_still_needs_meta_dir() { - // Clean name "Foo", but "foo" is already taken → dedup adds ~2 → needs meta + fn nfi_clean_name_dedup_no_meta_dir() { + // Clean name "Foo", but "foo" is already taken → dedup adds ~2. + // Forward sync strips ~N, so no meta name needed. let (f, m) = nfi("Foo", Middleware::Dir, &["foo"]); assert_eq!(f, "Foo~2"); - assert!( - m, - "dedup suffix means filesystem name differs from instance name" - ); + assert!(!m, "dedup-only: forward sync strips ~N automatically"); } #[test] fn nfi_case_collision_dir() { let (f, m) = nfi("Assets", Middleware::Dir, &["assets"]); assert_eq!(f, "Assets~2"); - assert!(m); + assert!(!m, "dedup-only: forward sync strips ~N automatically"); } #[test] @@ -1223,7 +1228,7 @@ mod tests { assert_eq!(r[0].1, "Foo"); assert!(!r[0].2); assert_eq!(r[1].1, "Foo~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N automatically"); } #[test] @@ -1251,7 +1256,7 @@ mod tests { assert_eq!(r[0].1, "Hey_Bro"); assert!(r[0].2); assert_eq!(r[1].1, "Hey_Bro~2"); - assert!(r[1].2, "dedup suffix → needs meta even for natural name"); + assert!(!r[1].2, "dedup-only: natural name, forward sync strips ~N"); } #[test] @@ -1260,9 +1265,9 @@ mod tests { assert_eq!(r[0].1, "Script"); assert!(!r[0].2); assert_eq!(r[1].1, "script~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N automatically"); assert_eq!(r[2].1, "SCRIPT~3"); - assert!(r[2].2); + assert!(!r[2].2, "dedup-only: forward sync strips ~N automatically"); } #[test] @@ -1294,7 +1299,7 @@ mod tests { assert_eq!(r[0].1, "CON_"); assert!(r[0].2); assert_eq!(r[1].1, "CON_~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: natural name, forward sync strips ~N"); } #[test] @@ -1309,11 +1314,11 @@ mod tests { assert_eq!(r[0].1, "Utils"); assert!(!r[0].2); assert_eq!(r[1].1, "utils~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[2].1, "Utils~3"); - assert!(r[2].2); + assert!(!r[2].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[3].1, "UTILS~4"); - assert!(r[3].2); + assert!(!r[3].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[4].1, "Uti_ls"); assert!(r[4].2); } @@ -1351,9 +1356,9 @@ mod tests { assert_eq!(r[0].1, "Leading"); assert!(r[0].2, "leading space was stripped"); assert_eq!(r[1].1, "Leading~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: clean name, forward sync strips ~N"); assert_eq!(r[2].1, "Leading~3"); - assert!(r[2].2); + assert!(r[2].2, "trailing space stripped → slug differs"); } // ── Every middleware type with slugified name ───────────────────── @@ -1559,7 +1564,7 @@ mod tests { // must get "Utils~2". let r = process_new_children_with_disk_seed(&["utils"], &[("Utils", Middleware::Dir)]); assert_eq!(r[0].1, "Utils~2"); - assert!(r[0].2); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1567,7 +1572,7 @@ mod tests { // "SCRIPTS" directory on disk, new instance "Scripts" (dir) must dedup. let r = process_new_children_with_disk_seed(&["scripts"], &[("Scripts", Middleware::Dir)]); assert_eq!(r[0].1, "Scripts~2"); - assert!(r[0].2); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1592,9 +1597,9 @@ mod tests { ], ); assert_eq!(r[0].1, "Utils~2", "Utils collides with utils/ on disk"); - assert!(r[0].2); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[1].1, "Config~2", "Config collides with config/ on disk"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[2].1, "NewThing", "NewThing is clean"); assert!(!r[2].2); } @@ -1608,7 +1613,7 @@ mod tests { &[("Stuff", Middleware::Dir)], ); assert_eq!(r[0].1, "Stuff~2"); - assert!(r[0].2); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1621,7 +1626,7 @@ mod tests { &[("MyModule", Middleware::ModuleScript)], ); assert_eq!(r[0].1, "MyModule~2.luau"); - assert!(r[0].2, "dedup suffix means needs meta"); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1679,7 +1684,7 @@ mod tests { // New "Foo" dir must skip both and land on "Foo~2". let r = process_new_children_with_disk_seed(&["foo", "foo~1"], &[("Foo", Middleware::Dir)]); assert_eq!(r[0].1, "Foo~2"); - assert!(r[0].2); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1776,9 +1781,9 @@ mod tests { &[("Helper", Middleware::Dir), ("Helper", Middleware::Dir)], ); assert_eq!(r[0].1, "Helper~2"); - assert!(r[0].2); + assert!(!r[0].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[1].1, "Helper~3"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1796,11 +1801,11 @@ mod tests { assert_eq!(r[0].1, "Test"); assert!(!r[0].2); assert_eq!(r[1].1, "test~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[2].1, "TEST~3"); - assert!(r[2].2); + assert!(!r[2].2, "dedup-only: forward sync strips ~N"); assert_eq!(r[3].1, "tEsT~4"); - assert!(r[3].2); + assert!(!r[3].2, "dedup-only: forward sync strips ~N"); } // ══════════════════════════════════════════════════════════════════ @@ -1873,7 +1878,7 @@ mod tests { assert_eq!(r[0].1, "🎮 Games"); assert!(!r[0].2, "emoji name is clean"); assert_eq!(r[1].1, "🎮 Games~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: forward sync strips ~N"); } #[test] @@ -1884,7 +1889,7 @@ mod tests { assert_eq!(r[0].1, "CON_"); assert!(r[0].2); assert_eq!(r[1].1, "CON_~2"); - assert!(r[1].2, "natural CON_ collides with reserved CON's slug"); + assert!(!r[1].2, "dedup-only: natural CON_ needs no meta"); assert_eq!(r[2].1, "con_~3"); assert!(r[2].2); } @@ -1896,7 +1901,7 @@ mod tests { assert_eq!(r[0].1, "test"); assert!(r[0].2, "trailing dot stripped → slug differs"); assert_eq!(r[1].1, "test~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: clean name, forward sync strips ~N"); } #[test] @@ -1906,9 +1911,9 @@ mod tests { assert_eq!(r[0].1, "A"); assert!(r[0].2, "leading space stripped"); assert_eq!(r[1].1, "A~2"); - assert!(r[1].2); + assert!(!r[1].2, "dedup-only: clean name, forward sync strips ~N"); assert_eq!(r[2].1, "A~3"); - assert!(r[2].2); + assert!(r[2].2, "leading spaces stripped → slug differs"); } #[test] @@ -2067,7 +2072,7 @@ mod tests { assert_eq!(r[1].1, "a_project~2"); assert!(r[1].2); assert_eq!(r[2].1, "a_project~3"); - assert!(r[2].2, "natural name collides with slugified siblings"); + assert!(!r[2].2, "dedup-only: natural name, forward sync strips ~N"); } #[test] @@ -2079,7 +2084,7 @@ mod tests { assert!(!r[0].2); for (i, entry) in r.iter().enumerate().skip(1) { assert_eq!(entry.1, format!("Script~{}", i + 1)); - assert!(entry.2); + assert!(!entry.2, "dedup-only: forward sync strips ~N"); } // All filenames must be unique let unique: HashSet = r.iter().map(|(_, f, _)| f.to_lowercase()).collect(); @@ -2333,17 +2338,15 @@ mod tests { assert_eq!(name1.as_ref(), "A_B.luau"); assert!(meta1, "A/B was slugified, needs meta name"); assert_eq!(name2.as_ref(), "A_B~2.luau"); - assert!(meta2, "A_B was deduped to ~2, needs meta name"); + assert!(!meta2, "dedup-only: natural A_B, forward sync strips ~2"); } #[test] fn tilde_in_filename_not_parsed_as_dedup_marker() { - // A file named "Foo~1" should produce instance name "Foo~1", - // NOT be interpreted as "Foo" with dedup suffix ~1. - // This is verified by the fact that name_needs_slugify("Foo~1") - // returns true (tilde is in SLUGIFY_CHARS), so a file named - // "Foo~1.luau" can only exist if it was written by the dedup - // system. Forward sync reads the filename as-is. + // An instance literally named "Foo~1" gets slugified to "Foo_1" + // because tilde is in SLUGIFY_CHARS. This means a file named + // "Foo~1.luau" on disk can only have been produced by the dedup + // system, so forward sync correctly strips ~1 to derive "Foo". assert!( name_needs_slugify("Foo~1"), "tilde should trigger slugification" diff --git a/src/web/api.rs b/src/web/api.rs index 8a9f2f4d..eb2e839d 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -1929,7 +1929,7 @@ impl ApiService { // not filenames with extensions). This ensures file-format instances // are correctly detected as collisions. let encoded_name = deduplicate_name(&base_name, sibling_slugs); - let needs_meta_name = needs_slug || encoded_name != base_name; + let needs_meta_name = needs_slug; let meta_name_field: Option<&str> = if needs_meta_name { Some(&added.name) } else { @@ -6478,7 +6478,7 @@ mod tests { instance_name.to_string() }; let deduped = deduplicate_name(&base, &taken); - let needs_meta = needs_slug || deduped != base; + let needs_meta = needs_slug; fs::create_dir(temp.path().join(&deduped)).unwrap(); @@ -6516,7 +6516,7 @@ mod tests { name.to_string() }; let deduped = deduplicate_name(&base, &taken); - let needs_meta = needs_slug || deduped != base; + let needs_meta = needs_slug; taken.insert(deduped.to_lowercase()); let meta = if needs_meta { Some(name.to_string()) @@ -6541,7 +6541,7 @@ mod tests { fn dir_clean_name_collides_with_existing() { let (f, m) = simulate_dir_dedup(&["NewFolder"], "NewFolder"); assert_eq!(f, "NewFolder~2"); - assert_eq!(m.unwrap(), "NewFolder"); + assert!(m.is_none(), "dedup-only: forward sync strips ~N"); } #[test] @@ -6562,14 +6562,14 @@ mod tests { fn dir_natural_collides_with_slug() { let (f, m) = simulate_dir_dedup(&["A_B"], "A_B"); assert_eq!(f, "A_B~2"); - assert_eq!(m.unwrap(), "A_B"); + assert!(m.is_none(), "dedup-only: forward sync strips ~N"); } #[test] fn dir_case_insensitive_collision() { let (f, m) = simulate_dir_dedup(&["MyFolder"], "myfolder"); assert_eq!(f, "myfolder~2"); - assert_eq!(m.unwrap(), "myfolder"); + assert!(m.is_none(), "dedup-only: forward sync strips ~N"); } #[test] @@ -6628,7 +6628,7 @@ mod tests { assert_eq!(r[0].0, "Script"); assert!(r[0].1.is_none()); assert_eq!(r[1].0, "Script~2"); - assert_eq!(r[1].1.as_deref(), Some("Script")); + assert!(r[1].1.is_none(), "dedup-only: forward sync strips ~N"); assert_eq!(r[2].0, "Script~3"); } @@ -6663,7 +6663,7 @@ mod tests { assert_eq!(r[0].0, "CON_"); assert_eq!(r[0].1.as_deref(), Some("CON")); assert_eq!(r[1].0, "CON_~2"); - assert_eq!(r[1].1.as_deref(), Some("CON_")); + assert!(r[1].1.is_none(), "dedup-only: natural CON_ needs no meta"); assert_eq!(r[2].0, "con_~3"); assert_eq!(r[2].1.as_deref(), Some("con")); } diff --git a/tests/tests/build.rs b/tests/tests/build.rs index 26666f00..d648c39e 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -68,6 +68,7 @@ gen_build_tests! { meta_name_override, model_json_name_override, dedup_suffix_with_meta, + dedup_suffix_auto_strip, init_meta_name_override, reserved_name_override, } diff --git a/tests/tests/connected_mode.rs b/tests/tests/connected_mode.rs index 9d9a6177..91a90578 100644 --- a/tests/tests/connected_mode.rs +++ b/tests/tests/connected_mode.rs @@ -1067,8 +1067,8 @@ fn edit_dedup_file_with_meta() { }); } -/// Test 29: Create Foo~1.luau with NO meta. Instance name should be -/// literally "Foo~1" -- tilde is NOT parsed as dedup marker. +/// Test 29: Create Foo~1.luau with NO meta. Forward sync strips the ~1 +/// dedup suffix, so the instance name should be "Foo". #[test] fn dedup_file_without_meta_uses_stem() { run_serve_test("connected_slugify", |session, mut redactions| { diff --git a/tests/tests/two_way_sync.rs b/tests/tests/two_way_sync.rs index a44e822d..3011ac7a 100644 --- a/tests/tests/two_way_sync.rs +++ b/tests/tests/two_way_sync.rs @@ -7732,17 +7732,12 @@ fn add_same_named_folders_deduplicates() { data_dedup.display() ); - // The dedup'd directory should have a meta with name: "Data" + // Dedup-only: no meta file needed. Forward sync strips ~N automatically. let dedup_meta = data_dedup.join("init.meta.json5"); assert!( - dedup_meta.exists(), - "Data~2/init.meta.json5 should exist for name override" - ); - let meta_content = fs::read_to_string(&dedup_meta).unwrap(); - assert!( - meta_content.contains("\"Data\""), - "Dedup meta should contain name \"Data\", got: {}", - meta_content + !dedup_meta.exists(), + "Data~2/init.meta.json5 should NOT exist: forward sync strips ~N, got file at {}", + dedup_meta.display() ); // Each directory should have its own child script