From fe2bf358e70ef9a225097e4736b7d6c692bdcc6a Mon Sep 17 00:00:00 2001 From: Joe Date: Sun, 1 Feb 2026 20:18:45 -0600 Subject: [PATCH] Enhance syncback logic for orphan file handling and improve test coverage - Updated the syncback process to prevent scanning the project root folder, ensuring that unrelated files like .gitignore and README.md are not deleted. - Introduced functionality to collect alternate file representations for paths, allowing orphan files to be identified and removed correctly. - Enhanced the logic for handling ignored paths, particularly for directories, to improve accuracy in clean mode operations. - Updated tests to reflect changes in orphan file handling, ensuring that only orphans within $path directories are removed, and clarified documentation regarding clean mode behavior. --- src/syncback/mod.rs | 129 ++++++++++++++++++++++++++----- src/syncback/snapshot.rs | 4 +- tests/tests/clean_mode_stress.rs | 22 +++--- 3 files changed, 124 insertions(+), 31 deletions(-) diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index aa617bfa..d7d07938 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -256,13 +256,60 @@ pub fn syncback_loop_with_stats( // Collect paths from the project tree definition collect_paths_from_project(&project.tree, project_path, &mut dirs_to_scan); - // Also scan the project folder itself - files like src.luau that exist - // alongside default.project.json5 should be considered for orphan removal - let project_path_buf = project_path.to_path_buf(); - if !dirs_to_scan.contains(&project_path_buf) { - log::trace!("Adding project folder to scan: {}", project_path.display()); - dirs_to_scan.push(project_path_buf); + // NOTE: We intentionally do NOT scan the project root folder itself. + // Only directories explicitly referenced via $path should be scanned. + // Scanning the root would delete unrelated files like .gitignore, README.md, etc. + // + // However, we DO need to collect alternate file representations of $path entries. + // For example, if $path: "src" expects a directory, but "src.luau" exists as a file + // (perhaps the user accidentally converted the directory to a file), that orphan + // file should be removed. We collect these as individual files to check, not as + // directories to scan recursively. + let mut orphan_files_to_check: Vec = Vec::new(); + + // Helper to find alternate file extensions for a path + fn collect_alternate_files( + node: &crate::project::ProjectNode, + base_path: &Path, + files: &mut Vec, + ) { + if let Some(ref path_node) = node.path { + let path_str = path_node.path(); + let resolved = base_path.join(path_str); + + // If the $path points to a directory, check for file alternates + if resolved.is_dir() { + // Common Rojo file extensions that could be alternates + let extensions = [ + ".luau", + ".lua", + ".server.luau", + ".server.lua", + ".client.luau", + ".client.lua", + ".local.luau", + ".local.lua", + ]; + for ext in extensions { + // Create alternate filename by appending extension to path name + let path_str_display = path_str.display().to_string(); + let alt_file = base_path.join(format!("{}{}", path_str_display, ext)); + if alt_file.exists() && alt_file.is_file() { + log::trace!( + "Found alternate file for $path '{}': {}", + path_str_display, + alt_file.display() + ); + files.push(alt_file); + } + } + } + } + for child in node.children.values() { + collect_alternate_files(child, base_path, files); + } } + collect_alternate_files(&project.tree, project_path, &mut orphan_files_to_check); // Also check instance metadata for paths that might not be in project // (e.g., from nested project references) @@ -356,6 +403,19 @@ pub fn syncback_loop_with_stats( // Note: We only add directories to dirs_to_scan now, so no else branch needed } + // Also add any alternate file representations we found earlier + // (e.g., src.luau when $path: "src" is a directory) + for file in &orphan_files_to_check { + if !is_valid_path(&ignore_patterns, project_path, file) { + continue; + } + log::trace!( + "Adding alternate file to orphan check: {}", + file.display() + ); + paths.insert(file.clone()); + } + log::debug!("Scanned {} existing paths from filesystem", paths.len()); paths } else { @@ -690,18 +750,51 @@ pub fn syncback_loop_with_stats( // e.g., if "ReplicatedStorage/KeepMe" is ignored, then "ReplicatedStorage" (src/) // should not be removed as it contains ignored content. if old_path_norm.is_dir() { - // Check if the pattern starts with the instance path (meaning it's inside this directory) - if pattern_str.starts_with(&inst_path_str) - && pattern_str.len() > inst_path_str.len() - && pattern_str.as_bytes().get(inst_path_str.len()) == Some(&b'/') - { - log::trace!( - "Skipping {} (ancestor of ignoreTrees path, inst_path: {})", - old_path.display(), - inst_path_str - ); - is_ignored = true; - break; + // For literal patterns (no wildcards), use string prefix check + if !pattern_str.contains('*') && !pattern_str.contains('?') { + if pattern_str.starts_with(&inst_path_str) + && pattern_str.len() > inst_path_str.len() + && pattern_str.as_bytes().get(inst_path_str.len()) == Some(&b'/') + { + log::trace!( + "Skipping {} (ancestor of literal ignoreTrees path, inst_path: {})", + old_path.display(), + inst_path_str + ); + is_ignored = true; + break; + } + } else { + // For glob patterns, check if ANY file inside this directory + // could match the pattern. We do this by checking all existing_paths + // that are inside this directory. + let dir_contains_match = existing_paths.iter().any(|child_path| { + // Only check children of this directory + if !child_path.starts_with(&old_path_norm) || child_path == &old_path_norm { + return false; + } + // Convert child path to instance path and check glob match + if let Some(child_inst_path) = fs_path_to_instance_path(child_path) { + if glob.is_match(&child_inst_path) { + log::trace!( + "Directory {} contains ignored file {} (inst: {})", + old_path.display(), + child_path.display(), + child_inst_path + ); + return true; + } + } + false + }); + if dir_contains_match { + log::trace!( + "Skipping {} (contains files matching glob pattern)", + old_path.display() + ); + is_ignored = true; + break; + } } } } diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index f77580d7..220caa1f 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -320,7 +320,7 @@ impl<'sync> SyncbackSnapshot<'sync> { } /// Returns whether Windows-invalid characters should be encoded in file - /// names during syncback. + /// names during syncback. Defaults to `true`. #[inline] pub fn encode_windows_invalid_chars(&self) -> bool { self.data @@ -328,7 +328,7 @@ impl<'sync> SyncbackSnapshot<'sync> { .syncback_rules .as_ref() .map(|rules| rules.encode_windows_invalid_chars()) - .unwrap_or(false) + .unwrap_or(true) } /// Returns whether to emit warnings when duplicate child names are diff --git a/tests/tests/clean_mode_stress.rs b/tests/tests/clean_mode_stress.rs index 73fd80fd..9db70d5e 100644 --- a/tests/tests/clean_mode_stress.rs +++ b/tests/tests/clean_mode_stress.rs @@ -396,13 +396,16 @@ fn clean_removes_multiple_duplicates() { // ============================================================================= /// Clean mode handles multiple different mutations at once. +/// +/// NOTE: All orphans are placed within $path directories (src/). +/// Orphan files at the project root are NOT removed by clean mode. #[test] fn clean_handles_multiple_mutations() { clean_equals_fresh( "deep_nesting", &[ Mutation::AddOrphanFile { - relative_path: "orphan1.luau", + relative_path: "src/orphan1.luau", content: "x", }, Mutation::AddOrphanFile { @@ -410,7 +413,7 @@ fn clean_handles_multiple_mutations() { content: "y", }, Mutation::AddOrphanDirectory { - relative_path: "fake_dir", + relative_path: "src/fake_dir", }, Mutation::AddNestedProjectFile { relative_path: "src/bad.project.json5", @@ -420,16 +423,16 @@ fn clean_handles_multiple_mutations() { } /// Clean mode handles extreme chaos with many mutations. +/// +/// NOTE: Clean mode only removes orphans within `$path` directories. +/// Orphan files at the project root (outside $path dirs) are NOT removed +/// because that would risk deleting legitimate files like .gitignore, README.md, etc. #[test] fn clean_handles_extreme_chaos() { clean_equals_fresh( "deep_nesting", &[ - // Orphan files everywhere - Mutation::AddOrphanFile { - relative_path: "orphan_root.luau", - content: "-- root level orphan", - }, + // Orphan files within $path directories (these WILL be removed) Mutation::AddOrphanFile { relative_path: "src/orphan_src.luau", content: "-- src level orphan", @@ -442,10 +445,7 @@ fn clean_handles_extreme_chaos() { relative_path: "src/level-1/level-2/orphan_l2.luau", content: "-- l2 orphan", }, - // Orphan directories - Mutation::AddOrphanDirectory { - relative_path: "fake_service", - }, + // Orphan directories within $path (these WILL be removed) Mutation::AddOrphanDirectory { relative_path: "src/fake_folder", },