From 7d0232c43026bdb84179adc87d566ed9db66c9de Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 19 Feb 2026 01:15:28 -0600 Subject: [PATCH] Add automatic dedup suffix stripping to enhance syncback functionality - Introduced a new plan to automatically strip `~N` dedup suffixes from filenames during forward sync, eliminating the need for redundant `.meta.json5` files. - Implemented the `strip_dedup_suffix()` utility in `dedup_suffix.rs` to handle suffix removal. - Updated `file_name_for_path()` and `get_dir_middleware()` to utilize the new stripping functionality. - Modified syncback logic to skip writing meta `name` fields for dedup-only cases, ensuring cleaner instance naming. - Enhanced existing tests and added new tests to cover the updated dedup behavior, ensuring robustness and correctness. These changes aim to streamline the handling of deduplicated instances, improving overall system efficiency and clarity in naming conventions. --- ...edup_suffix_automatically_3d35c847.plan.md | 117 ++++++++++++++++++ ...tests__build__dedup_suffix_auto_strip.snap | 32 +++++ ...d_to_end__tests__build__tilde_no_meta.snap | 4 +- .../default.project.json5 | 6 + .../dedup_suffix_auto_strip/src/Baz~0.luau | 1 + .../dedup_suffix_auto_strip/src/Foo.luau | 1 + .../dedup_suffix_auto_strip/src/Foo~2.luau | 1 + .../build-tests/tilde_no_meta/src/Foo~1.luau | 2 +- ...e__connected_dedup_no_meta_stem_patch.snap | 2 +- ...__syncback_util__rbxm_fallback-stdout.snap | 2 +- src/change_processor.rs | 9 +- src/snapshot/metadata.rs | 16 +-- src/snapshot_middleware/mod.rs | 3 +- src/syncback/dedup_suffix.rs | 26 ++++ src/syncback/file_names.rs | 97 ++++++++------- src/web/api.rs | 16 +-- tests/tests/build.rs | 1 + tests/tests/connected_mode.rs | 4 +- tests/tests/two_way_sync.rs | 13 +- 19 files changed, 269 insertions(+), 84 deletions(-) create mode 100644 .cursor/plans/strip_dedup_suffix_automatically_3d35c847.plan.md create mode 100644 rojo-test/build-test-snapshots/end_to_end__tests__build__dedup_suffix_auto_strip.snap create mode 100644 rojo-test/build-tests/dedup_suffix_auto_strip/default.project.json5 create mode 100644 rojo-test/build-tests/dedup_suffix_auto_strip/src/Baz~0.luau create mode 100644 rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo.luau create mode 100644 rojo-test/build-tests/dedup_suffix_auto_strip/src/Foo~2.luau 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