Skip to content

Two bug fixes #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
congwang-mk merged 4 commits into from
Feb 18, 2026
Merged

Two bug fixes #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
106390c
Fix readdir to show inherited files from ancestor branches
congwang-mk Feb 18, 2026
f6a49b5
Fix stale reads after commit-to-main
congwang-mk Feb 18, 2026
35584fd
Add regression tests for readdir inheritance and commit propagation
congwang-mk Feb 18, 2026
1617cb2
Deduplicate file/dir removal logic in commit with remove_entry helper
congwang-mk Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 62 additions & 18 deletions src/branch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
Expand Down Expand Up @@ -396,6 +407,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<HashSet<String>> {
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<Option<PathBuf>> {
let branches = self.branches.read();

Expand Down Expand Up @@ -477,22 +524,13 @@ 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
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() {
Expand All @@ -503,8 +541,20 @@ 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.iter().chain(&child_tombstones) {
let main_delta = main_files_dir.join(rel_path.trim_start_matches('/'));
let _ = remove_entry(&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);
Expand Down Expand Up @@ -545,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());
}

Expand Down
106 changes: 43 additions & 63 deletions src/fs_helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand All @@ -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<String> = 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
Expand Down
Loading