Repository navigation
fix: replace panic!() in tree.rs with proper error propagation - #62
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces a panic!() in the tree-building logic with structured error propagation so path conflicts surface as actionable Result errors instead of crashing the generator.
Changes:
- Changed
TreeNode::insert_blobto returnResult<()>and useanyhow::bail!()on path conflicts - Updated
BulkRepoBuilder::{set_file,append_dummy}to propagateinsert_bloberrors - Updated unit tests to reflect the new fallible API and added an error-case assertion
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.entries | ||
| .insert(path.to_string(), TreeEntry::Blob(blob_oid)); |
There was a problem hiding this comment.
insert_blob still allows a tree/blob conflict in the opposite direction: if a directory already exists at path (i.e., entries contains TreeEntry::Tree for that key) and you later call insert_blob("a", ...) after inserting something under a/, the else branch will silently replace the tree with a blob and effectively discard the subtree. Consider detecting TreeEntry::Tree(_) here and returning a conflict error instead of overwriting.
| self.entries | |
| .insert(path.to_string(), TreeEntry::Blob(blob_oid)); | |
| match self.entries.get(path) { | |
| Some(TreeEntry::Tree(_)) => { | |
| bail!("path conflict: '{path}' is a tree, not a blob") | |
| } | |
| Some(TreeEntry::Blob(_)) | None => { | |
| self.entries | |
| .insert(path.to_string(), TreeEntry::Blob(blob_oid)); | |
| } | |
| } |
| TreeEntry::Tree(node) => node.insert_blob(rest, blob_oid)?, | ||
| _ => { | ||
| bail!("path conflict: '{dir}' is a blob, not a tree (while inserting '{path}')") | ||
| } |
There was a problem hiding this comment.
The conflict error message can lose the full path once recursion descends (e.g., a conflict under a/b/... will report inserting b/... because path is the per-node relative path). If this error is intended for end users, consider threading the original full path (or an accumulated prefix) through recursion so the message consistently includes the full conflicting path.
| pub fn insert_blob(&mut self, path: &str, blob_oid: Oid) -> Result<()> { | ||
| self.invalidate(); | ||
| if let Some(slash) = path.find('/') { | ||
| let dir = &path[..slash]; |
There was a problem hiding this comment.
self.invalidate() runs before any conflict checks. On a conflict this leaves the tree logically unchanged but clears cached_oid, causing unnecessary recomputation if the caller recovers and continues. Consider invalidating only after a successful insertion (or only on the branch where a mutation actually happens).
| // Try to insert a nested path where "f" is already a blob | ||
| root.insert_blob("f/g.txt", blob); | ||
| let err = root.insert_blob("f/g.txt", blob).unwrap_err(); | ||
| assert!(err.to_string().contains("path conflict")); |
There was a problem hiding this comment.
The new test only asserts the error contains the generic substring "path conflict". Since the PR goal is a descriptive message including the conflicting path, consider asserting that the error string also contains the conflicting segment (e.g. "f") and/or the attempted path (e.g. "f/g.txt") so regressions in the message content are caught.
| assert!(err.to_string().contains("path conflict")); | |
| let err_str = err.to_string(); | |
| assert!(err_str.contains("path conflict")); | |
| assert!(err_str.contains("f")); | |
| assert!(err_str.contains("f/g.txt")); |
Summary
TreeNode::insert_blobto returnResult<()>instead of panicking on path conflictsanyhow::bail!()with a descriptive error message including the conflicting pathBulkRepoBuilder::set_file,append_dummy) to propagate errorsunwrap()oninsert_blobcalls and test error case withunwrap_err()Closes #54