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.
This commit is contained in:
Joe 2026-02-01 20:18:45 -06:00
parent 411f79ee81
commit fe2bf358e7
3 changed files with 124 additions and 31 deletions

View file

@ -256,13 +256,60 @@ pub fn syncback_loop_with_stats(
// Collect paths from the project tree definition // Collect paths from the project tree definition
collect_paths_from_project(&project.tree, project_path, &mut dirs_to_scan); collect_paths_from_project(&project.tree, project_path, &mut dirs_to_scan);
// Also scan the project folder itself - files like src.luau that exist // NOTE: We intentionally do NOT scan the project root folder itself.
// alongside default.project.json5 should be considered for orphan removal // Only directories explicitly referenced via $path should be scanned.
let project_path_buf = project_path.to_path_buf(); // Scanning the root would delete unrelated files like .gitignore, README.md, etc.
if !dirs_to_scan.contains(&project_path_buf) { //
log::trace!("Adding project folder to scan: {}", project_path.display()); // However, we DO need to collect alternate file representations of $path entries.
dirs_to_scan.push(project_path_buf); // 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<PathBuf> = 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<PathBuf>,
) {
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 // Also check instance metadata for paths that might not be in project
// (e.g., from nested project references) // (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 // 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()); log::debug!("Scanned {} existing paths from filesystem", paths.len());
paths paths
} else { } else {
@ -690,18 +750,51 @@ pub fn syncback_loop_with_stats(
// e.g., if "ReplicatedStorage/KeepMe" is ignored, then "ReplicatedStorage" (src/) // e.g., if "ReplicatedStorage/KeepMe" is ignored, then "ReplicatedStorage" (src/)
// should not be removed as it contains ignored content. // should not be removed as it contains ignored content.
if old_path_norm.is_dir() { if old_path_norm.is_dir() {
// Check if the pattern starts with the instance path (meaning it's inside this directory) // For literal patterns (no wildcards), use string prefix check
if pattern_str.starts_with(&inst_path_str) if !pattern_str.contains('*') && !pattern_str.contains('?') {
&& pattern_str.len() > inst_path_str.len() if pattern_str.starts_with(&inst_path_str)
&& pattern_str.as_bytes().get(inst_path_str.len()) == Some(&b'/') && 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: {})", log::trace!(
old_path.display(), "Skipping {} (ancestor of literal ignoreTrees path, inst_path: {})",
inst_path_str old_path.display(),
); inst_path_str
is_ignored = true; );
break; 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;
}
} }
} }
} }

View file

@ -320,7 +320,7 @@ impl<'sync> SyncbackSnapshot<'sync> {
} }
/// Returns whether Windows-invalid characters should be encoded in file /// Returns whether Windows-invalid characters should be encoded in file
/// names during syncback. /// names during syncback. Defaults to `true`.
#[inline] #[inline]
pub fn encode_windows_invalid_chars(&self) -> bool { pub fn encode_windows_invalid_chars(&self) -> bool {
self.data self.data
@ -328,7 +328,7 @@ impl<'sync> SyncbackSnapshot<'sync> {
.syncback_rules .syncback_rules
.as_ref() .as_ref()
.map(|rules| rules.encode_windows_invalid_chars()) .map(|rules| rules.encode_windows_invalid_chars())
.unwrap_or(false) .unwrap_or(true)
} }
/// Returns whether to emit warnings when duplicate child names are /// Returns whether to emit warnings when duplicate child names are

View file

@ -396,13 +396,16 @@ fn clean_removes_multiple_duplicates() {
// ============================================================================= // =============================================================================
/// Clean mode handles multiple different mutations at once. /// 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] #[test]
fn clean_handles_multiple_mutations() { fn clean_handles_multiple_mutations() {
clean_equals_fresh( clean_equals_fresh(
"deep_nesting", "deep_nesting",
&[ &[
Mutation::AddOrphanFile { Mutation::AddOrphanFile {
relative_path: "orphan1.luau", relative_path: "src/orphan1.luau",
content: "x", content: "x",
}, },
Mutation::AddOrphanFile { Mutation::AddOrphanFile {
@ -410,7 +413,7 @@ fn clean_handles_multiple_mutations() {
content: "y", content: "y",
}, },
Mutation::AddOrphanDirectory { Mutation::AddOrphanDirectory {
relative_path: "fake_dir", relative_path: "src/fake_dir",
}, },
Mutation::AddNestedProjectFile { Mutation::AddNestedProjectFile {
relative_path: "src/bad.project.json5", relative_path: "src/bad.project.json5",
@ -420,16 +423,16 @@ fn clean_handles_multiple_mutations() {
} }
/// Clean mode handles extreme chaos with many 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] #[test]
fn clean_handles_extreme_chaos() { fn clean_handles_extreme_chaos() {
clean_equals_fresh( clean_equals_fresh(
"deep_nesting", "deep_nesting",
&[ &[
// Orphan files everywhere // Orphan files within $path directories (these WILL be removed)
Mutation::AddOrphanFile {
relative_path: "orphan_root.luau",
content: "-- root level orphan",
},
Mutation::AddOrphanFile { Mutation::AddOrphanFile {
relative_path: "src/orphan_src.luau", relative_path: "src/orphan_src.luau",
content: "-- src level orphan", content: "-- src level orphan",
@ -442,10 +445,7 @@ fn clean_handles_extreme_chaos() {
relative_path: "src/level-1/level-2/orphan_l2.luau", relative_path: "src/level-1/level-2/orphan_l2.luau",
content: "-- l2 orphan", content: "-- l2 orphan",
}, },
// Orphan directories // Orphan directories within $path (these WILL be removed)
Mutation::AddOrphanDirectory {
relative_path: "fake_service",
},
Mutation::AddOrphanDirectory { Mutation::AddOrphanDirectory {
relative_path: "src/fake_folder", relative_path: "src/fake_folder",
}, },