From 106390c16de196dbbc9541162d119e091649abd6 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 18 Feb 2026 15:04:35 -0800 Subject: [PATCH 1/4] Fix readdir to show inherited files from ancestor branches Signed-off-by: Cong Wang --- src/branch.rs | 36 ++++++++++++++++ src/fs_helpers.rs | 106 +++++++++++++++++++--------------------------- 2 files changed, 79 insertions(+), 63 deletions(-) diff --git a/src/branch.rs b/src/branch.rs index 47379e8..5f10c68 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -396,6 +396,42 @@ impl BranchManager { } } + /// Collect all candidate file/directory names visible in a directory + /// by walking the full branch ancestor chain and base directory. + /// Used by readdir to enumerate all possible entries before filtering. + pub fn collect_dir_names(&self, branch_name: &str, rel_path: &str) -> Result> { + let branches = self.branches.read(); + let mut names = HashSet::new(); + + let mut current = branch_name; + loop { + let branch = branches + .get(current) + .ok_or_else(|| BranchError::NotFound(current.to_string()))?; + + let delta_dir = branch.files_dir.join(rel_path.trim_start_matches('/')); + if let Ok(dir) = fs::read_dir(&delta_dir) { + for entry in dir.flatten() { + names.insert(entry.file_name().to_string_lossy().to_string()); + } + } + + match &branch.parent { + Some(parent) => current = parent, + None => break, + } + } + + let base_dir = self.base_path.join(rel_path.trim_start_matches('/')); + if let Ok(dir) = fs::read_dir(&base_dir) { + for entry in dir.flatten() { + names.insert(entry.file_name().to_string_lossy().to_string()); + } + } + + Ok(names) + } + pub fn resolve_path(&self, branch_name: &str, rel_path: &str) -> Result> { let branches = self.branches.read(); diff --git a/src/fs_helpers.rs b/src/fs_helpers.rs index 9ecf73e..2159f37 100644 --- a/src/fs_helpers.rs +++ b/src/fs_helpers.rs @@ -142,6 +142,10 @@ impl BranchFs { /// Collect readdir entries for a directory resolved via a specific branch. /// + /// Walks the full ancestor chain (branch → parent → … → main → base) to + /// collect all candidate names, then resolves each via `resolve_path` to + /// respect tombstones and determine the correct file type. + /// /// `inode_prefix` controls how child inode paths are formed: /// - `"/@branch"` for branch subtrees (produces `/@branch/child`) /// - `""` for root-level paths (produces `/child`) @@ -157,69 +161,45 @@ impl BranchFs { (ino, FileType::Directory, "..".to_string()), ]; - let mut seen = std::collections::HashSet::new(); - - // Collect from base directory - let base_dir = self - .manager - .base_path - .join(rel_path.trim_start_matches('/')); - if let Ok(dir) = std::fs::read_dir(&base_dir) { - for entry in dir.flatten() { - let name = entry.file_name().to_string_lossy().to_string(); - if seen.insert(name.clone()) { - let child_rel = if rel_path == "/" { - format!("/{}", name) - } else { - format!("{}/{}", rel_path, name) - }; - let inode_path = format!("{}{}", inode_prefix, child_rel); - let ft = entry.file_type(); - let is_symlink = ft.as_ref().map(|t| t.is_symlink()).unwrap_or(false); - let is_dir = !is_symlink && ft.as_ref().map(|t| t.is_dir()).unwrap_or(false); - let child_ino = self.inodes.get_or_create(&inode_path, is_dir); - let kind = if is_symlink { - FileType::Symlink - } else if is_dir { - FileType::Directory - } else { - FileType::RegularFile - }; - entries.push((child_ino, kind, name)); - } - } - } - - // Collect from branch deltas - if let Some(resolved) = self.resolve_for_branch(branch, rel_path) { - if resolved != base_dir { - if let Ok(dir) = std::fs::read_dir(&resolved) { - for entry in dir.flatten() { - let name = entry.file_name().to_string_lossy().to_string(); - if seen.insert(name.clone()) { - let child_rel = if rel_path == "/" { - format!("/{}", name) - } else { - format!("{}/{}", rel_path, name) - }; - let inode_path = format!("{}{}", inode_prefix, child_rel); - let ft = entry.file_type(); - let is_symlink = ft.as_ref().map(|t| t.is_symlink()).unwrap_or(false); - let is_dir = - !is_symlink && ft.as_ref().map(|t| t.is_dir()).unwrap_or(false); - let child_ino = self.inodes.get_or_create(&inode_path, is_dir); - let kind = if is_symlink { - FileType::Symlink - } else if is_dir { - FileType::Directory - } else { - FileType::RegularFile - }; - entries.push((child_ino, kind, name)); - } - } - } - } + // Collect all candidate names from the full branch ancestor chain + base. + let mut candidates: Vec = match self.manager.collect_dir_names(branch, rel_path) { + Ok(names) => names.into_iter().collect(), + Err(_) => return entries, + }; + // Sort for deterministic readdir ordering across paged calls. + candidates.sort(); + + // For each candidate, resolve via the branch chain to check visibility + // (handles tombstones) and determine the actual file type. + for name in candidates { + let child_rel = if rel_path == "/" { + format!("/{}", name) + } else { + format!("{}/{}", rel_path, name) + }; + + // resolve_path handles tombstone filtering — returns None for deleted files. + let resolved = match self.resolve_for_branch(branch, &child_rel) { + Some(p) => p, + None => continue, + }; + + let inode_path = format!("{}{}", inode_prefix, child_rel); + let ft = resolved.symlink_metadata(); + let is_symlink = ft + .as_ref() + .map(|m| m.file_type().is_symlink()) + .unwrap_or(false); + let is_dir = !is_symlink && ft.as_ref().map(|m| m.is_dir()).unwrap_or(false); + let child_ino = self.inodes.get_or_create(&inode_path, is_dir); + let kind = if is_symlink { + FileType::Symlink + } else if is_dir { + FileType::Directory + } else { + FileType::RegularFile + }; + entries.push((child_ino, kind, name)); } entries From f6a49b53b1d009c92bf74d8ac7fa56f505c05589 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 18 Feb 2026 15:08:41 -0800 Subject: [PATCH 2/4] Fix stale reads after commit-to-main Signed-off-by: Cong Wang --- src/branch.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/branch.rs b/src/branch.rs index 5f10c68..933ca34 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -529,6 +529,7 @@ impl BranchManager { // Copy delta files to base let mut num_files = 0u64; let mut total_bytes = 0u64; + let mut committed_paths = Vec::new(); self.walk_files(&child_files_dir, "", &mut |rel_path, src_path| { let dest = self.base_path.join(rel_path.trim_start_matches('/')); if let Some(parent_dir) = dest.parent() { @@ -539,8 +540,44 @@ impl BranchManager { } let _ = storage::copy_entry(src_path, &dest); num_files += 1; + committed_paths.push(rel_path.to_string()); })?; + // Remove main's delta for committed/tombstoned paths so base + // takes precedence. Without this, main's pre-existing delta + // (written before branching) would overshadow the updated base. + if let Some(main_branch) = branches.get("main") { + let main_files_dir = &main_branch.files_dir; + for rel_path in &committed_paths { + let main_delta = main_files_dir.join(rel_path.trim_start_matches('/')); + if main_delta.symlink_metadata().is_ok() { + if main_delta + .symlink_metadata() + .map(|m| m.file_type().is_dir()) + .unwrap_or(false) + { + let _ = fs::remove_dir_all(&main_delta); + } else { + let _ = fs::remove_file(&main_delta); + } + } + } + for path in &child_tombstones { + let main_delta = main_files_dir.join(path.trim_start_matches('/')); + if main_delta.symlink_metadata().is_ok() { + if main_delta + .symlink_metadata() + .map(|m| m.file_type().is_dir()) + .unwrap_or(false) + { + let _ = fs::remove_dir_all(&main_delta); + } else { + let _ = fs::remove_file(&main_delta); + } + } + } + } + // Increment parent's commit_count (first-wins bookkeeping) if let Some(main_branch) = branches.get("main") { main_branch.commit_count.fetch_add(1, Ordering::SeqCst); From 35584fd33a77559120abecc84b0095c1ed92c551 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 18 Feb 2026 15:14:14 -0800 Subject: [PATCH 3/4] Add regression tests for readdir inheritance and commit propagation Signed-off-by: Cong Wang --- tests/test_integration.rs | 248 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 248 insertions(+) diff --git a/tests/test_integration.rs b/tests/test_integration.rs index fb6b604..5c21801 100644 --- a/tests/test_integration.rs +++ b/tests/test_integration.rs @@ -654,3 +654,251 @@ fn test_symlink_isolation_between_branches() { assert!(dir_b.join("link_b").symlink_metadata().is_ok()); assert!(dir_b.join("link_a").symlink_metadata().is_err()); } + +// ── Readdir inheritance tests ─────────────────────────────────────── +// +// Regression tests for: readdir on branches not showing inherited files. +// collect_readdir_entries() only read from base + one resolved directory, +// missing files in parent branch deltas. + +/// Files written through the mount root (stored in main's delta) should +/// appear in a child branch's directory listing, not just via direct access. +#[test] +#[ignore] +fn test_readdir_branch_inherits_main_delta_files() { + let fix = TestFixture::new("readdir_inherit"); + fix.mount(); + + // Write a file through the mount root — goes to main's delta + fs::write(fix.mnt.join("dynamic.txt"), "written via mount\n").unwrap(); + + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + + // Direct access should work (resolve_path walks the chain) + assert!( + bdir.join("dynamic.txt").exists(), + "branch should see main's file via direct access" + ); + + // readdir should ALSO list it + let entries: Vec = fs::read_dir(&bdir) + .expect("read_dir on branch") + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().to_string()) + .collect(); + + assert!( + entries.contains(&"dynamic.txt".to_string()), + "readdir should list main's delta file; got: {:?}", + entries + ); + // Should also list base files + assert!( + entries.contains(&"file1.txt".to_string()), + "readdir should list base files; got: {:?}", + entries + ); + assert!( + entries.contains(&"file2.txt".to_string()), + "readdir should list base files; got: {:?}", + entries + ); +} + +/// readdir on a nested (grandchild) branch should list files from the +/// parent branch's delta, the grandparent's delta, and base. +#[test] +#[ignore] +fn test_readdir_nested_branch_inherits_parent_delta() { + let fix = TestFixture::new("readdir_nested"); + fix.mount(); + let ctl = fix.open_ctl(); + + // Create parent branch, write a file + let parent = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE parent"); + let pdir = fix.branch_dir(&parent); + fs::write(pdir.join("parent_only.txt"), "from parent\n").unwrap(); + + // Create child branch from parent + let pctl = fix.open_branch_ctl(&parent); + let child = unsafe { ioctl_create(pctl.as_raw_fd()) }.expect("CREATE child"); + let cdir = fix.branch_dir(&child); + + // readdir on child should list parent's file + base files + let entries: Vec = fs::read_dir(&cdir) + .expect("read_dir on child branch") + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().to_string()) + .collect(); + + assert!( + entries.contains(&"parent_only.txt".to_string()), + "child readdir should list parent's file; got: {:?}", + entries + ); + assert!( + entries.contains(&"file1.txt".to_string()), + "child readdir should list base files; got: {:?}", + entries + ); +} + +/// readdir should respect tombstones: a file deleted in a branch should +/// not appear in that branch's directory listing. +#[test] +#[ignore] +fn test_readdir_respects_tombstones() { + let fix = TestFixture::new("readdir_tomb"); + fix.mount(); + let ctl = fix.open_ctl(); + + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + + // Delete a base file in the branch + fs::remove_file(bdir.join("file1.txt")).unwrap(); + + let entries: Vec = fs::read_dir(&bdir) + .expect("read_dir") + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().to_string()) + .collect(); + + assert!( + !entries.contains(&"file1.txt".to_string()), + "deleted file should not appear in readdir; got: {:?}", + entries + ); + // Other base files should still be there + assert!( + entries.contains(&"file2.txt".to_string()), + "undeleted file should still appear; got: {:?}", + entries + ); +} + +// ── Commit propagation tests ──────────────────────────────────────── +// +// Regression tests for: committed changes not visible at mount root. +// When committing to main, delta files were copied to base but main's +// pre-existing delta was left untouched, overshadowing the updated base. + +/// After committing a branch that modified a file originally written +/// through the mount root, reading from the mount root should return +/// the committed version. +#[test] +#[ignore] +fn test_commit_changes_visible_at_mount_root() { + let fix = TestFixture::new("commit_visible"); + fix.mount(); + + // Write a file through the mount root — goes to main's delta + fs::write(fix.mnt.join("evolve.txt"), "version 1\n").unwrap(); + assert_eq!( + fs::read_to_string(fix.mnt.join("evolve.txt")).unwrap(), + "version 1\n" + ); + + // Create a branch and modify the file + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + fs::write(bdir.join("evolve.txt"), "version 2\n").unwrap(); + assert_eq!( + fs::read_to_string(bdir.join("evolve.txt")).unwrap(), + "version 2\n" + ); + + // Mount root still sees version 1 (isolation) + assert_eq!( + fs::read_to_string(fix.mnt.join("evolve.txt")).unwrap(), + "version 1\n" + ); + + // Commit + let bctl = fix.open_branch_ctl(&branch); + let ret = unsafe { ioctl_commit(bctl.as_raw_fd()) }; + assert_eq!(ret, 0, "commit should succeed"); + + // Mount root should now see version 2 + assert_eq!( + fs::read_to_string(fix.mnt.join("evolve.txt")).unwrap(), + "version 2\n", + "mount root should show committed content, not stale main delta" + ); +} + +/// After committing a branch that deletes a file originally written +/// through the mount root, the file should be gone at the mount root. +#[test] +#[ignore] +fn test_commit_deletion_visible_at_mount_root() { + let fix = TestFixture::new("commit_delete"); + fix.mount(); + + // Write a file through the mount root + fs::write(fix.mnt.join("doomed.txt"), "will be deleted\n").unwrap(); + assert!(fix.mnt.join("doomed.txt").exists()); + + // Create a branch and delete the file + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + fs::remove_file(bdir.join("doomed.txt")).unwrap(); + assert!(!bdir.join("doomed.txt").exists()); + + // Mount root still sees it (isolation) + assert!(fix.mnt.join("doomed.txt").exists()); + + // Commit + let bctl = fix.open_branch_ctl(&branch); + let ret = unsafe { ioctl_commit(bctl.as_raw_fd()) }; + assert_eq!(ret, 0, "commit should succeed"); + + // Mount root should no longer see the file + assert!( + !fix.mnt.join("doomed.txt").exists(), + "file deleted in branch should be gone at mount root after commit" + ); +} + +/// Modifying a base file (not main's delta) in a branch and committing +/// should update the file visible at the mount root. +#[test] +#[ignore] +fn test_commit_base_file_modification_visible_at_mount_root() { + let fix = TestFixture::new("commit_base_mod"); + fix.mount(); + + // file1.txt is seeded in base (not via FUSE write, so no main delta) + assert_eq!( + fs::read_to_string(fix.mnt.join("file1.txt")).unwrap(), + "base content\n" + ); + + // Create a branch and modify the base file + let ctl = fix.open_ctl(); + let branch = unsafe { ioctl_create(ctl.as_raw_fd()) }.expect("CREATE"); + let bdir = fix.branch_dir(&branch); + fs::write(bdir.join("file1.txt"), "updated in branch\n").unwrap(); + + // Commit + let bctl = fix.open_branch_ctl(&branch); + let ret = unsafe { ioctl_commit(bctl.as_raw_fd()) }; + assert_eq!(ret, 0, "commit should succeed"); + + // Mount root should see the update + assert_eq!( + fs::read_to_string(fix.mnt.join("file1.txt")).unwrap(), + "updated in branch\n", + "mount root should show committed content" + ); + + // Base should also be updated + assert_eq!( + fs::read_to_string(fix.base.join("file1.txt")).unwrap(), + "updated in branch\n", + ); +} From 1617cb20a8c33fb219da404a4371431e2f61ccdb Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Wed, 18 Feb 2026 15:23:22 -0800 Subject: [PATCH 4/4] Deduplicate file/dir removal logic in commit with remove_entry helper Signed-off-by: Cong Wang --- src/branch.rs | 59 +++++++++++++-------------------------------------- 1 file changed, 15 insertions(+), 44 deletions(-) diff --git a/src/branch.rs b/src/branch.rs index 933ca34..5ccfb52 100644 --- a/src/branch.rs +++ b/src/branch.rs @@ -13,6 +13,17 @@ use crate::error::{BranchError, Result}; use crate::inode::ROOT_INO; use crate::storage; +/// Remove a file or directory at `path`, following symlinks for the type check. +/// Returns `Ok(())` even if the path doesn't exist; propagates real I/O errors. +fn remove_entry(path: &Path) -> std::io::Result<()> { + match path.symlink_metadata() { + Ok(m) if m.file_type().is_dir() => fs::remove_dir_all(path), + Ok(_) => fs::remove_file(path), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e), + } +} + pub struct Branch { pub name: String, pub parent: Option, @@ -513,17 +524,7 @@ impl BranchManager { // Apply tombstones as deletions for path in &child_tombstones { let full_path = self.base_path.join(path.trim_start_matches('/')); - if full_path.symlink_metadata().is_ok() { - if full_path - .symlink_metadata() - .map(|m| m.file_type().is_dir()) - .unwrap_or(false) - { - fs::remove_dir_all(&full_path)?; - } else { - fs::remove_file(&full_path)?; - } - } + remove_entry(&full_path)?; } // Copy delta files to base @@ -548,33 +549,9 @@ impl BranchManager { // (written before branching) would overshadow the updated base. if let Some(main_branch) = branches.get("main") { let main_files_dir = &main_branch.files_dir; - for rel_path in &committed_paths { + for rel_path in committed_paths.iter().chain(&child_tombstones) { let main_delta = main_files_dir.join(rel_path.trim_start_matches('/')); - if main_delta.symlink_metadata().is_ok() { - if main_delta - .symlink_metadata() - .map(|m| m.file_type().is_dir()) - .unwrap_or(false) - { - let _ = fs::remove_dir_all(&main_delta); - } else { - let _ = fs::remove_file(&main_delta); - } - } - } - for path in &child_tombstones { - let main_delta = main_files_dir.join(path.trim_start_matches('/')); - if main_delta.symlink_metadata().is_ok() { - if main_delta - .symlink_metadata() - .map(|m| m.file_type().is_dir()) - .unwrap_or(false) - { - let _ = fs::remove_dir_all(&main_delta); - } else { - let _ = fs::remove_file(&main_delta); - } - } + let _ = remove_entry(&main_delta); } } @@ -618,13 +595,7 @@ impl BranchManager { // and add tombstone to parent for tombstone in &child_tombstones { let parent_delta = parent_files_dir.join(tombstone.trim_start_matches('/')); - if parent_delta.exists() { - if parent_delta.is_dir() { - let _ = fs::remove_dir_all(&parent_delta); - } else { - let _ = fs::remove_file(&parent_delta); - } - } + let _ = remove_entry(&parent_delta); parent_tombstones.insert(tombstone.clone()); }