Skip to content

fix: replace panic!() in tree.rs with proper error propagation #62

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
BryanFRD merged 1 commit into from
Apr 8, 2026
Merged

fix: replace panic!() in tree.rs with proper error propagation #62

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 18 additions & 15 deletions generator/src/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use anyhow::Result;
use anyhow::{bail, Result};
use git2::{FileMode, Oid, Repository, Signature, Time};
use std::collections::HashMap;

Expand All @@ -24,7 +24,7 @@ impl TreeNode {
self.cached_oid = None;
}

pub fn insert_blob(&mut self, path: &str, blob_oid: Oid) {
pub fn insert_blob(&mut self, path: &str, blob_oid: Oid) -> Result<()> {
self.invalidate();
if let Some(slash) = path.find('/') {
let dir = &path[..slash];
Comment on lines +27 to 30

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Expand All @@ -34,13 +34,16 @@ impl TreeNode {
.entry(dir.to_string())
.or_insert_with(|| TreeEntry::Tree(TreeNode::new()));
match child {
TreeEntry::Tree(node) => node.insert_blob(rest, blob_oid),
_ => panic!("path conflict: {dir} is a blob, not a tree"),
TreeEntry::Tree(node) => node.insert_blob(rest, blob_oid)?,
_ => {
bail!("path conflict: '{dir}' is a blob, not a tree (while inserting '{path}')")
}
Comment on lines +37 to +40

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
} else {
self.entries
.insert(path.to_string(), TreeEntry::Blob(blob_oid));
Comment on lines 43 to 44

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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));
}
}

Copilot uses AI. Check for mistakes.
}
Ok(())
}

pub fn write(&mut self, repo: &Repository) -> Result<Oid> {
Expand Down Expand Up @@ -80,15 +83,15 @@ impl BulkRepoBuilder {

pub fn set_file(&mut self, repo: &Repository, path: &str, content: &[u8]) -> Result<()> {
let blob_oid = repo.blob(content)?;
self.root.insert_blob(path, blob_oid);
self.root.insert_blob(path, blob_oid)?;
Ok(())
}

pub fn append_dummy(&mut self, repo: &Repository, path: &str) -> Result<()> {
let content = self.dummy_content.entry(path.to_string()).or_default();
content.extend_from_slice(b"change\n");
let blob_oid = repo.blob(content)?;
self.root.insert_blob(path, blob_oid);
self.root.insert_blob(path, blob_oid)?;
Ok(())
}

Expand Down Expand Up @@ -131,7 +134,7 @@ mod tests {
let blob = repo.blob(b"hello").unwrap();

let mut root = TreeNode::new();
root.insert_blob("file.txt", blob);
root.insert_blob("file.txt", blob).unwrap();

let tree_oid = root.write(&repo).unwrap();
let tree = repo.find_tree(tree_oid).unwrap();
Expand All @@ -144,7 +147,7 @@ mod tests {
let blob = repo.blob(b"data").unwrap();

let mut root = TreeNode::new();
root.insert_blob("a/b/c.txt", blob);
root.insert_blob("a/b/c.txt", blob).unwrap();

let tree_oid = root.write(&repo).unwrap();
let tree = repo.find_tree(tree_oid).unwrap();
Expand All @@ -163,8 +166,8 @@ mod tests {
let blob2 = repo.blob(b"v2").unwrap();

let mut root = TreeNode::new();
root.insert_blob("f.txt", blob1);
root.insert_blob("f.txt", blob2);
root.insert_blob("f.txt", blob1).unwrap();
root.insert_blob("f.txt", blob2).unwrap();

let tree_oid = root.write(&repo).unwrap();
let tree = repo.find_tree(tree_oid).unwrap();
Expand All @@ -180,25 +183,25 @@ mod tests {
let blob2 = repo.blob(b"second").unwrap();

let mut root = TreeNode::new();
root.insert_blob("a.txt", blob1);
root.insert_blob("a.txt", blob1).unwrap();
let oid1 = root.write(&repo).unwrap();

root.insert_blob("b.txt", blob2);
root.insert_blob("b.txt", blob2).unwrap();
let oid2 = root.write(&repo).unwrap();

assert_ne!(oid1, oid2);
}

#[test]
#[should_panic(expected = "path conflict")]
fn insert_blob_conflicts_with_existing_blob() {
let (_tmp, repo) = init_repo();
let blob = repo.blob(b"x").unwrap();

let mut root = TreeNode::new();
root.insert_blob("f", blob);
root.insert_blob("f", blob).unwrap();
// 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"));

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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"));

Copilot uses AI. Check for mistakes.
}

#[test]
Expand Down
Loading