From dad5320aafd50512a6c4ea5c03571fdf2c66b57b Mon Sep 17 00:00:00 2001 From: Joe Date: Sat, 7 Mar 2026 13:25:49 -0600 Subject: [PATCH] Enhance patch computation to support glob-ignored children - Introduced `glob_ignore_rules` to manage instance removal based on glob patterns, allowing for more granular control over which instances are retained. - Updated the logic in `compute_patch_set_internal` and `compute_children_patches` to check against these rules before removing unmatched instances. - Refactored related tests to ensure correct behavior when instances are matched or ignored based on their paths, improving the robustness of instance synchronization. --- src/snapshot/patch_compute.rs | 164 ++++++++++++++++++++++++++++++---- 1 file changed, 145 insertions(+), 19 deletions(-) diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index a5df13ca..94c879ba 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -1,7 +1,7 @@ //! Defines the algorithm for computing a roughly-minimal patch set given an //! existing instance tree and an instance snapshot. -use std::{collections::HashMap, mem::take}; +use std::{collections::HashMap, mem::take, sync::Arc}; use rbx_dom_weak::{ types::{Ref, Variant}, @@ -15,7 +15,7 @@ use crate::{ use super::{ patch::{PatchAdd, PatchSet, PatchUpdate}, - InstanceSnapshot, InstanceWithMeta, RojoTree, + InstanceSnapshot, InstanceWithMeta, InstigatingSource, PathIgnoreRule, RojoTree, }; #[profiling::function] @@ -110,7 +110,11 @@ fn compute_patch_set_internal( .get_instance(id) .expect("Instance did not exist in tree"); - let suppress_removals = snapshot.metadata.glob_ignored_children; + let glob_ignore_rules = if snapshot.metadata.glob_ignored_children { + Some(Arc::clone(&snapshot.metadata.context.path_ignore_rules)) + } else { + None + }; compute_property_patches( &context.snapshot_specified_ids, @@ -126,7 +130,7 @@ fn compute_patch_set_internal( id, patch_set, session, - suppress_removals, + glob_ignore_rules.as_ref().map(|v| v.as_slice()), ); } @@ -277,7 +281,7 @@ fn compute_children_patches( id: Ref, patch_set: &mut PatchSet, session: &super::matching::MatchingSession, - suppress_removals: bool, + glob_ignore_rules: Option<&[PathIgnoreRule]>, ) { use super::matching::match_forward; @@ -321,14 +325,27 @@ fn compute_children_patches( }); } - // Unmatched tree children: instances to be removed, unless the parent - // directory has glob-ignored children. Standard $ignoreUnknownInstances - // (scripts-only mode) still allows server-side removals -- the plugin's - // shouldDeleteChild handles the selective keep logic for non-scripts. - if !suppress_removals { - for tree_child_id in match_result.unmatched_tree { - patch_set.removed_instances.push(tree_child_id); + // Unmatched tree children: instances to be removed. When the parent has + // glob-ignored children, each child is checked individually -- only + // children whose InstigatingSource path matches a glob rule are kept. + // Standard $ignoreUnknownInstances (scripts-only mode) still allows + // server-side removals; the plugin's shouldDeleteChild handles the + // selective keep logic for non-scripts. + for tree_child_id in match_result.unmatched_tree { + if let Some(rules) = glob_ignore_rules { + let is_glob_ignored = tree + .get_instance(tree_child_id) + .and_then(|inst| inst.metadata().instigating_source.as_ref()) + .map(|source| match source { + InstigatingSource::Path(path) => !rules.iter().all(|rule| rule.passes(path)), + _ => false, + }) + .unwrap_or(false); + if is_glob_ignored { + continue; + } } + patch_set.removed_instances.push(tree_child_id); } } @@ -443,8 +460,10 @@ mod test { } #[test] - fn glob_ignored_children_suppresses_removals() { + fn glob_ignored_children_suppresses_glob_matched_removals() { use super::super::patch_apply::apply_patch_set; + use crate::glob::Glob; + use std::path::PathBuf; let mut tree = RojoTree::new(InstanceSnapshot::new().name("root").class_name("Folder")); let root_id = tree.get_root_id(); @@ -453,11 +472,23 @@ mod test { added_instances: vec![ PatchAdd { parent_id: root_id, - instance: InstanceSnapshot::new().name("A").class_name("Folder"), + instance: InstanceSnapshot::new() + .name("A") + .class_name("Folder") + .metadata( + super::super::InstanceMetadata::new() + .instigating_source(PathBuf::from("/project/src/A")), + ), }, PatchAdd { parent_id: root_id, - instance: InstanceSnapshot::new().name("B").class_name("Folder"), + instance: InstanceSnapshot::new() + .name("B") + .class_name("Folder") + .metadata( + super::super::InstanceMetadata::new() + .instigating_source(PathBuf::from("/project/src/generated/B")), + ), }, ], updated_instances: Vec::new(), @@ -467,13 +498,20 @@ mod test { }; apply_patch_set(&mut tree, add_patch); - // Snapshot only has child A, with glob_ignored_children = true. - // Child B is "unknown" and should NOT be removed. + let mut context = super::super::InstanceContext::new(); + context.add_path_ignore_rules(vec![PathIgnoreRule { + base_path: PathBuf::from("/project"), + glob: Glob::new("src/generated/**").unwrap(), + }]); + + // Snapshot has only A. B is unmatched but its path matches the + // glob rule, so it should NOT be removed. let snapshot = InstanceSnapshot { snapshot_id: Ref::none(), metadata: super::super::InstanceMetadata::new() .ignore_unknown_instances(true) - .glob_ignored_children(true), + .glob_ignored_children(true) + .context(&context), properties: UstrMap::new(), name: Cow::Borrowed("root"), class_name: ustr("Folder"), @@ -484,7 +522,95 @@ mod test { assert!( patch_set.removed_instances.is_empty(), - "Unmatched tree children should NOT be removed when glob_ignored_children is true" + "Glob-matched child should NOT be removed" + ); + } + + #[test] + fn glob_ignored_children_per_child_matching() { + use super::super::patch_apply::apply_patch_set; + use crate::glob::Glob; + use std::path::PathBuf; + + let mut tree = RojoTree::new(InstanceSnapshot::new().name("root").class_name("Folder")); + let root_id = tree.get_root_id(); + + // A: will be in the snapshot (matched). + // B: deleted from filesystem, path does NOT match glob → removed. + // C: glob-ignored, path matches glob → NOT removed. + let add_patch = PatchSet { + added_instances: vec![ + PatchAdd { + parent_id: root_id, + instance: InstanceSnapshot::new() + .name("A") + .class_name("Folder") + .metadata( + super::super::InstanceMetadata::new() + .instigating_source(PathBuf::from("/project/src/A")), + ), + }, + PatchAdd { + parent_id: root_id, + instance: InstanceSnapshot::new() + .name("B") + .class_name("Folder") + .metadata( + super::super::InstanceMetadata::new() + .instigating_source(PathBuf::from("/project/src/B")), + ), + }, + PatchAdd { + parent_id: root_id, + instance: InstanceSnapshot::new() + .name("C") + .class_name("Folder") + .metadata( + super::super::InstanceMetadata::new() + .instigating_source(PathBuf::from("/project/src/generated/C")), + ), + }, + ], + updated_instances: Vec::new(), + removed_instances: Vec::new(), + stage_ids: std::collections::HashSet::new(), + stage_paths: Vec::new(), + }; + apply_patch_set(&mut tree, add_patch); + + let mut context = super::super::InstanceContext::new(); + context.add_path_ignore_rules(vec![PathIgnoreRule { + base_path: PathBuf::from("/project"), + glob: Glob::new("src/generated/**").unwrap(), + }]); + + let snapshot = InstanceSnapshot { + snapshot_id: Ref::none(), + metadata: super::super::InstanceMetadata::new() + .ignore_unknown_instances(true) + .glob_ignored_children(true) + .context(&context), + properties: UstrMap::new(), + name: Cow::Borrowed("root"), + class_name: ustr("Folder"), + children: vec![InstanceSnapshot::new().name("A").class_name("Folder")], + }; + + let patch_set = compute_patch_set(Some(snapshot), &tree, root_id); + + assert_eq!( + patch_set.removed_instances.len(), + 1, + "Only genuinely deleted child B should be removed, not glob-ignored child C" + ); + + let removed_inst = tree + .get_instance(patch_set.removed_instances[0]) + .expect("Removed instance should exist in tree"); + assert_eq!( + removed_inst.name(), + "B", + "The removed instance should be B (genuine deletion)" ); }