Skip to content

Commit 3ca56d3

Browse files
Implemented correct fix:
- kept nodes having dirname/filename + the repo-relative path is reconstructed from the node names - paths are reconstructed by joining dir + child path when needed - write_commit_entries deletes removed dirs from `dir_hash_db` when they're removed + tree lookups used this so they'd return stale entries + **cache lookup bug**
1 parent e6cbb61 commit 3ca56d3

File tree

2 files changed

+48
-18
lines changed

2 files changed

+48
-18
lines changed

crates/lib/src/core/v_latest/rm.rs

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -727,19 +727,10 @@ fn r_process_remove_dir(
727727
match &node.node {
728728
// if node is a Directory, stage it for removal
729729
EMerkleTreeNode::Directory(_) => {
730-
// Update the dir name to the full relative path so that it
731-
// matches the naming convention used by `get_node_dir_children`
732-
// during commit (which joins `base_dir / name`). Without this
733-
// the remove entry has only the leaf name (e.g. "3") while the
734-
// existing child is keyed by the full path ("1/2/3"), causing
735-
// the HashSet::remove in `split_into_vnodes` to miss.
736-
let mut updated_node = node.clone();
737-
if let EMerkleTreeNode::Directory(ref mut dir_node) = updated_node.node {
738-
dir_node.set_name(path.to_string_lossy().as_ref());
739-
}
730+
// node has the correct relative path to the dir, so no need for updates
740731
let staged_entry = StagedMerkleTreeNode {
741732
status: StagedEntryStatus::Removed,
742-
node: updated_node,
733+
node: node.clone(),
743734
};
744735

745736
// Write removed node to staged db

crates/lib/src/repositories/commits/commit_writer.rs

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -646,29 +646,52 @@ fn split_into_vnodes(
646646
// Overwrite the existing child
647647
// if add or modify, replace the child
648648
// if remove, remove the child
649-
if let Ok(path) = child.node.maybe_path()
650-
&& path != Path::new("")
649+
if let Ok(child_path) = child.node.maybe_path()
650+
&& child_path != Path::new("")
651651
{
652+
// Staged directory entries keep only the leaf name (e.g. "3"),
653+
// but existing children from `node_data_to_staged_node` use the
654+
// full repo-relative path (e.g. "1/2/3"). Reconstruct the full
655+
// path so that HashSet lookups (which compare via maybe_path) match.
656+
let needs_prefix =
657+
!directory.as_os_str().is_empty() && !child_path.starts_with(directory);
658+
let child = if needs_prefix {
659+
let full_path = directory.join(&child_path);
660+
let mut prefixed = child.clone();
661+
match &mut prefixed.node.node {
662+
EMerkleTreeNode::Directory(dn) => {
663+
dn.set_name(full_path.to_str().unwrap());
664+
}
665+
EMerkleTreeNode::File(fn_) => {
666+
fn_.set_name(full_path.to_str().unwrap());
667+
}
668+
_ => {}
669+
}
670+
prefixed
671+
} else {
672+
child.clone()
673+
};
674+
652675
match child.status {
653676
StagedEntryStatus::Removed => {
654677
log::debug!(
655678
"removing child {:?} {:?} with {:?}",
656679
child.node.node.node_type(),
657-
path,
680+
child.node.maybe_path().unwrap(),
658681
child.node.maybe_path().unwrap()
659682
);
660-
children.remove(child);
661-
removed_children.insert(child.to_owned());
683+
children.remove(&child);
684+
removed_children.insert(child);
662685
}
663686
_ => {
664687
log::debug!(
665688
"replacing child {:?} {:?} with {:?}",
666689
child.node.node.node_type(),
667-
path,
690+
child.node.maybe_path().unwrap(),
668691
child.node.maybe_path().unwrap()
669692
);
670693
log::debug!("replaced child {}", child.node);
671-
children.replace(child.clone());
694+
children.replace(child);
672695
}
673696
}
674697
}
@@ -829,6 +852,22 @@ fn write_commit_entries(
829852
&mut total_written,
830853
)?;
831854

855+
// Remove dir_hash entries for directories that were removed.
856+
// The dir_hash_db was pre-populated from the previous commit, so
857+
// removed directories still have stale entries that must be deleted;
858+
// otherwise tree lookups will find the old subtree.
859+
for (_, (_, removed_children)) in entries {
860+
for child in removed_children {
861+
if let EMerkleTreeNode::Directory(_) = &child.node.node {
862+
if let Ok(child_path) = child.node.maybe_path() {
863+
let path_str = child_path.to_str().unwrap_or_default();
864+
log::debug!("deleting removed dir hash: {path_str:?}");
865+
str_val_db::delete(dir_hash_db, path_str)?;
866+
}
867+
}
868+
}
869+
}
870+
832871
Ok(())
833872
}
834873

0 commit comments

Comments
 (0)