ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

tree: propagate errors from sub_tree()/path_value()

Browse source
This commit is contained in:
Martin von Zweigbergk 2024-05-21 06:55:33 -07:00 committed by Martin von Zweigbergk
parent facfb71f7b
commit 1970ddef15
12 changed files with 146 additions and 93 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
cli/src/cli_util.rs
View file

@ -1967,7 +1967,8 @@ pub fn print_unmatched_explicit_paths<'a>(
) -> io::Result<()> {
let mut explicit_paths = expression.explicit_paths().collect_vec();
for tree in trees {
explicit_paths.retain(|&path| tree.path_value(path).is_absent());
// TODO: propagate errors
explicit_paths.retain(|&path| tree.path_value(path).unwrap().is_absent());
if explicit_paths.is_empty() {
return Ok(());
}

2
cli/src/commands/cat.rs
View file

@ -58,7 +58,7 @@ pub(crate) fn cmd_cat(
// Try fast path for single file entry
if let Some(path) = get_single_path(&fileset_expression) {
let value = tree.path_value(path);
let value = tree.path_value(path)?;
if value.is_absent() {
let ui_path = workspace_command.format_file_path(path);
return Err(user_error(format!("No such path: {ui_path}")));

2
cli/src/commands/git.rs
View file

@ -1262,7 +1262,7 @@ fn cmd_git_submodule_print_gitmodules(
let commit = workspace_command.resolve_single_rev(&args.revisions)?;
let tree = commit.tree()?;
let gitmodules_path = RepoPath::from_internal_string(".gitmodules");
let mut gitmodules_file = match tree.path_value(gitmodules_path).into_resolved() {
let mut gitmodules_file = match tree.path_value(gitmodules_path)?.into_resolved() {
Ok(None) => {
writeln!(ui.status(), "No submodules!")?;
return Ok(());

12
cli/src/merge_tools/builtin.rs
View file

@ -128,7 +128,7 @@ fn read_file_contents(
tree: &MergedTree,
path: &RepoPath,
) -> Result<FileInfo, BuiltinToolError> {
let value = tree.path_value(path);
let value = tree.path_value(path)?;
let materialized_value = materialize_tree_value(store, path, value)
.map_err(BuiltinToolError::BackendError)
.block_on()?;
@ -444,7 +444,7 @@ pub fn apply_diff_builtin(
let file_deleted = file_existed_previously && !file_exists_now;
if new_empty_file {
let value = right_tree.path_value(&path);
let value = right_tree.path_value(&path)?;
tree_builder.set_or_remove(path, value);
} else if file_deleted {
tree_builder.set_or_remove(path, Merge::absent());
@ -458,7 +458,7 @@ pub fn apply_diff_builtin(
old_description: _,
new_description: _,
} => {
let value = right_tree.path_value(&path);
let value = right_tree.path_value(&path)?;
tree_builder.set_or_remove(path, value);
}
scm_record::SelectedContents::Present { contents } => {
@ -1028,9 +1028,9 @@ mod tests {
}
}
let merge = Merge::from_vec(vec![
to_file_id(left_tree.path_value(path)),
to_file_id(base_tree.path_value(path)),
to_file_id(right_tree.path_value(path)),
to_file_id(left_tree.path_value(path).unwrap()),
to_file_id(base_tree.path_value(path).unwrap()),
to_file_id(right_tree.path_value(path).unwrap()),
]);
let content = extract_as_single_hunk(&merge, store, path).block_on();
let slices = content.map(|ContentHunk(buf)| buf.as_slice());

2
cli/src/merge_tools/mod.rs
View file

@ -283,7 +283,7 @@ impl MergeEditor {
tree: &MergedTree,
repo_path: &RepoPath,
) -> Result<MergedTreeId, ConflictResolveError> {
let conflict = match tree.path_value(repo_path).into_resolved() {
let conflict = match tree.path_value(repo_path)?.into_resolved() {
Err(conflict) => conflict,
Ok(Some(_)) => return Err(ConflictResolveError::NotAConflict(repo_path.to_owned())),
Ok(None) => return Err(ConflictResolveError::PathNotFound(repo_path.to_owned())),

2
lib/src/local_working_copy.rs
View file

@ -1081,7 +1081,7 @@ impl TreeState {
if clean {
Ok(None)
} else {
let current_tree_values = current_tree.path_value(repo_path);
let current_tree_values = current_tree.path_value(repo_path)?;
let new_file_type = if !self.symlink_support {
let mut new_file_type = new_file_state.file_type.clone();
if matches!(new_file_type, FileType::Normal { .. })

54
lib/src/merged_tree.rs
View file

@ -211,30 +211,30 @@ impl MergedTree {
/// Gets the `MergeTree` in a subdirectory of the current tree. If the path
/// doesn't correspond to a tree in any of the inputs to the merge, then
/// that entry will be replace by an empty tree in the result.
pub fn sub_tree(&self, name: &RepoPathComponent) -> Option<MergedTree> {
pub fn sub_tree(&self, name: &RepoPathComponent) -> BackendResult<Option<MergedTree>> {
if let MergedTree::Legacy(tree) = self {
tree.sub_tree(name).map(MergedTree::Legacy)
Ok(tree.sub_tree(name)?.map(MergedTree::Legacy))
} else {
match self.value(name) {
MergedTreeVal::Resolved(Some(TreeValue::Tree(sub_tree_id))) => {
let subdir = self.dir().join(name);
Some(MergedTree::resolved(
self.store().get_tree(&subdir, sub_tree_id).unwrap(),
))
Ok(Some(MergedTree::resolved(
self.store().get_tree(&subdir, sub_tree_id)?,
)))
}
MergedTreeVal::Resolved(_) => None,
MergedTreeVal::Resolved(_) => Ok(None),
MergedTreeVal::Conflict(merge) => {
let merged_trees = merge.map(|value| match value {
let merged_trees = merge.try_map(|value| match value {
Some(TreeValue::Tree(sub_tree_id)) => {
let subdir = self.dir().join(name);
self.store().get_tree(&subdir, sub_tree_id).unwrap()
self.store().get_tree(&subdir, sub_tree_id)
}
_ => {
let subdir = self.dir().join(name);
Tree::null(self.store().clone(), subdir.clone())
Ok(Tree::null(self.store().clone(), subdir.clone()))
}
});
Some(MergedTree::Merge(merged_trees))
})?;
Ok(Some(MergedTree::Merge(merged_trees)))
}
}
}
@ -243,17 +243,17 @@ impl MergedTree {
/// The value at the given path. The value can be `Resolved` even if
/// `self` is a `Conflict`, which happens if the value at the path can be
/// trivially merged.
pub fn path_value(&self, path: &RepoPath) -> MergedTreeValue {
pub fn path_value(&self, path: &RepoPath) -> BackendResult<MergedTreeValue> {
assert_eq!(self.dir(), RepoPath::root());
match path.split() {
Some((dir, basename)) => match self.sub_tree_recursive(dir.components()) {
None => Merge::absent(),
Some(tree) => tree.value(basename).to_merge(),
Some((dir, basename)) => match self.sub_tree_recursive(dir.components())? {
None => Ok(Merge::absent()),
Some(tree) => Ok(tree.value(basename).to_merge()),
},
None => match self {
MergedTree::Legacy(tree) => Merge::normal(TreeValue::Tree(tree.id().clone())),
MergedTree::Legacy(tree) => Ok(Merge::normal(TreeValue::Tree(tree.id().clone()))),
MergedTree::Merge(trees) => {
trees.map(|tree| Some(TreeValue::Tree(tree.id().clone())))
Ok(trees.map(|tree| Some(TreeValue::Tree(tree.id().clone()))))
}
},
}
@ -267,12 +267,22 @@ impl MergedTree {
}
}
fn sub_tree_recursive(&self, mut components: RepoPathComponentsIter) -> Option<MergedTree> {
if let Some(first) = components.next() {
components.try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name))
} else {
Some(self.clone())
fn sub_tree_recursive(
&self,
components: RepoPathComponentsIter,
) -> BackendResult<Option<MergedTree>> {
let mut current_tree = self.clone();
for name in components {
match current_tree.sub_tree(name)? {
None => {
return Ok(None);
}
Some(sub_tree) => {
current_tree = sub_tree;
}
}
}
Ok(Some(current_tree))
}
/// Iterator over the entries matching the given matcher. Subtrees are

56
lib/src/tree.rs
View file

@ -124,39 +124,55 @@ impl Tree {
self.data.value(basename)
}
pub fn path_value(&self, path: &RepoPath) -> Option<TreeValue> {
pub fn path_value(&self, path: &RepoPath) -> BackendResult<Option<TreeValue>> {
assert_eq!(self.dir(), RepoPath::root());
match path.split() {
Some((dir, basename)) => self
.sub_tree_recursive(dir.components())
.and_then(|tree| tree.data.value(basename).cloned()),
None => Some(TreeValue::Tree(self.id.clone())),
Some((dir, basename)) => {
let tree = self.sub_tree_recursive(dir.components())?;
Ok(tree.and_then(|tree| tree.data.value(basename).cloned()))
}
None => Ok(Some(TreeValue::Tree(self.id.clone()))),
}
}
pub fn sub_tree(&self, name: &RepoPathComponent) -> Option<Tree> {
self.data.value(name).and_then(|sub_tree| match sub_tree {
TreeValue::Tree(sub_tree_id) => {
let subdir = self.dir.join(name);
Some(self.store.get_tree(&subdir, sub_tree_id).unwrap())
pub fn sub_tree(&self, name: &RepoPathComponent) -> BackendResult<Option<Tree>> {
if let Some(sub_tree) = self.data.value(name) {
match sub_tree {
TreeValue::Tree(sub_tree_id) => {
let subdir = self.dir.join(name);
let sub_tree = self.store.get_tree(&subdir, sub_tree_id)?;
Ok(Some(sub_tree))
}
_ => Ok(None),
}
_ => None,
})
} else {
Ok(None)
}
}
fn known_sub_tree(&self, subdir: &RepoPath, id: &TreeId) -> Tree {
self.store.get_tree(subdir, id).unwrap()
}
fn sub_tree_recursive(&self, mut components: RepoPathComponentsIter) -> Option<Tree> {
if let Some(first) = components.next() {
components.try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name))
} else {
// TODO: It would be nice to be able to return a reference here, but
// then we would have to figure out how to share Tree instances
// across threads.
Some(self.clone())
fn sub_tree_recursive(
&self,
components: RepoPathComponentsIter,
) -> BackendResult<Option<Tree>> {
let mut current_tree = self.clone();
for name in components {
match current_tree.sub_tree(name)? {
None => {
return Ok(None);
}
Some(sub_tree) => {
current_tree = sub_tree;
}
}
}
// TODO: It would be nice to be able to return a reference here, but
// then we would have to figure out how to share Tree instances
// across threads.
Ok(Some(current_tree))
}
pub fn conflicts_matching(&self, matcher: &dyn Matcher) -> Vec<(RepoPathBuf, ConflictId)> {

2
lib/src/tree_builder.rs
View file

@ -134,8 +134,10 @@ impl TreeBuilder {
return tree_cache.get(dir).unwrap();
}
let (parent, basename) = dir.split().expect("root must be populated");
// TODO: Propagate errors
let tree = populate_trees(tree_cache, store, parent)
.sub_tree(basename)
.unwrap()
.unwrap_or_else(|| Tree::null(store.clone(), dir.to_owned()));
tree_cache.entry(dir.to_owned()).or_insert(tree)
}

6
lib/tests/test_merge_trees.rs
View file

@ -599,8 +599,8 @@ fn test_simplify_conflict_after_resolving_parent() {
// Test the setup: Both B and C should have conflicts.
let tree_b2 = commit_b2.tree().unwrap();
let tree_c2 = commit_b2.tree().unwrap();
assert!(!tree_b2.path_value(path).is_resolved());
assert!(!tree_c2.path_value(path).is_resolved());
assert!(!tree_b2.path_value(path).unwrap().is_resolved());
assert!(!tree_c2.path_value(path).unwrap().is_resolved());
// Create the resolved B and rebase C on top.
let tree_b3 = create_tree(repo, &[(path, "AbC\ndef\nghi\n")]);
@ -622,7 +622,7 @@ fn test_simplify_conflict_after_resolving_parent() {
// The conflict should now be resolved.
let tree_c2 = commit_c3.tree().unwrap();
let resolved_value = tree_c2.path_value(path);
let resolved_value = tree_c2.path_value(path).unwrap();
match resolved_value.into_resolved() {
Ok(Some(TreeValue::File {
id,

83
lib/tests/test_merged_tree.rs
View file

@ -301,7 +301,7 @@ fn test_path_value_and_entries() {
// Get the root tree
assert_eq!(
merged_tree.path_value(RepoPath::root()),
merged_tree.path_value(RepoPath::root()).unwrap(),
Merge::from_removes_adds(
vec![Some(TreeValue::Tree(tree1.id().clone()))],
vec![
@ -312,40 +312,43 @@ fn test_path_value_and_entries() {
);
// Get file path without conflict
assert_eq!(
merged_tree.path_value(resolved_file_path),
Merge::resolved(tree1.path_value(resolved_file_path)),
merged_tree.path_value(resolved_file_path).unwrap(),
Merge::resolved(tree1.path_value(resolved_file_path).unwrap()),
);
// Get directory path without conflict
assert_eq!(
merged_tree.path_value(resolved_dir_path),
Merge::resolved(tree1.path_value(resolved_dir_path)),
merged_tree.path_value(resolved_dir_path).unwrap(),
Merge::resolved(tree1.path_value(resolved_dir_path).unwrap()),
);
// Get missing path
assert_eq!(merged_tree.path_value(missing_path), Merge::absent());
assert_eq!(
merged_tree.path_value(missing_path).unwrap(),
Merge::absent()
);
// Get modify/delete conflict (some None values)
assert_eq!(
merged_tree.path_value(modify_delete_path),
merged_tree.path_value(modify_delete_path).unwrap(),
Merge::from_removes_adds(
vec![tree1.path_value(modify_delete_path)],
vec![tree2.path_value(modify_delete_path), None]
vec![tree1.path_value(modify_delete_path).unwrap()],
vec![tree2.path_value(modify_delete_path).unwrap(), None]
),
);
// Get file/dir conflict path
assert_eq!(
merged_tree.path_value(file_dir_conflict_path),
merged_tree.path_value(file_dir_conflict_path).unwrap(),
Merge::from_removes_adds(
vec![tree1.path_value(file_dir_conflict_path)],
vec![tree1.path_value(file_dir_conflict_path).unwrap()],
vec![
tree2.path_value(file_dir_conflict_path),
tree3.path_value(file_dir_conflict_path)
tree2.path_value(file_dir_conflict_path).unwrap(),
tree3.path_value(file_dir_conflict_path).unwrap()
]
),
);
// Get file inside file/dir conflict
// There is a conflict in the parent directory, but this file is still resolved
assert_eq!(
merged_tree.path_value(file_dir_conflict_sub_path),
Merge::resolved(tree3.path_value(file_dir_conflict_sub_path)),
merged_tree.path_value(file_dir_conflict_sub_path).unwrap(),
Merge::resolved(tree3.path_value(file_dir_conflict_sub_path).unwrap()),
);
// Test entries()
@ -363,7 +366,7 @@ fn test_path_value_and_entries() {
]
.iter()
.sorted()
.map(|&path| (path.to_owned(), merged_tree.path_value(path)))
.map(|&path| (path.to_owned(), merged_tree.path_value(path).unwrap()))
.collect_vec();
assert_eq!(actual_entries, expected_entries);
@ -378,7 +381,7 @@ fn test_path_value_and_entries() {
let expected_entries = [resolved_file_path, modify_delete_path]
.iter()
.sorted()
.map(|&path| (path.to_owned(), merged_tree.path_value(path)))
.map(|&path| (path.to_owned(), merged_tree.path_value(path).unwrap()))
.collect_vec();
assert_eq!(actual_entries, expected_entries);
}
@ -592,8 +595,11 @@ fn test_conflict_iterator() {
let conflicts = tree.conflicts().collect_vec();
let conflict_at = |path: &RepoPath| {
Merge::from_removes_adds(
vec![base1.path_value(path)],
vec![side1.path_value(path), side2.path_value(path)],
vec![base1.path_value(path).unwrap()],
vec![
side1.path_value(path).unwrap(),
side2.path_value(path).unwrap(),
],
)
};
// We initially also get a conflict in trivial_hunk_path because we had
@ -681,11 +687,14 @@ fn test_conflict_iterator_higher_arity() {
let conflicts = tree.conflicts().collect_vec();
let conflict_at = |path: &RepoPath| {
Merge::from_removes_adds(
vec![base1.path_value(path), base2.path_value(path)],
vec![
side1.path_value(path),
side2.path_value(path),
side3.path_value(path),
base1.path_value(path).unwrap(),
base2.path_value(path).unwrap(),
],
vec![
side1.path_value(path).unwrap(),
side2.path_value(path).unwrap(),
side3.path_value(path).unwrap(),
],
)
};
@ -710,10 +719,10 @@ fn test_conflict_iterator_higher_arity() {
(
two_sided_path.to_owned(),
Merge::from_removes_adds(
vec![base2.path_value(two_sided_path)],
vec![base2.path_value(two_sided_path).unwrap()],
vec![
side1.path_value(two_sided_path),
side3.path_value(two_sided_path),
side1.path_value(two_sided_path).unwrap(),
side3.path_value(two_sided_path).unwrap(),
],
)
),
@ -761,8 +770,8 @@ fn test_diff_resolved() {
(
modified_path.to_owned(),
(
Merge::resolved(before.path_value(modified_path)),
Merge::resolved(after.path_value(modified_path))
Merge::resolved(before.path_value(modified_path).unwrap()),
Merge::resolved(after.path_value(modified_path).unwrap())
),
)
);
@ -771,7 +780,7 @@ fn test_diff_resolved() {
(
removed_path.to_owned(),
(
Merge::resolved(before.path_value(removed_path)),
Merge::resolved(before.path_value(removed_path).unwrap()),
Merge::absent()
),
)
@ -782,7 +791,7 @@ fn test_diff_resolved() {
added_path.to_owned(),
(
Merge::absent(),
Merge::resolved(after.path_value(added_path))
Merge::resolved(after.path_value(added_path).unwrap())
),
)
);
@ -869,7 +878,10 @@ fn test_diff_conflicted() {
.map(|&path| {
(
path.to_owned(),
(left_merged.path_value(path), right_merged.path_value(path)),
(
left_merged.path_value(path).unwrap(),
right_merged.path_value(path).unwrap(),
),
)
})
.collect_vec();
@ -885,7 +897,10 @@ fn test_diff_conflicted() {
.map(|&path| {
(
path.to_owned(),
(right_merged.path_value(path), left_merged.path_value(path)),
(
right_merged.path_value(path).unwrap(),
left_merged.path_value(path).unwrap(),
),
)
})
.collect_vec();
@ -985,8 +1000,8 @@ fn test_diff_dir_file() {
vec![right_base],
vec![right_side1, right_side2],
));
let left_value = |path: &RepoPath| left_merged.path_value(path);
let right_value = |path: &RepoPath| right_merged.path_value(path);
let left_value = |path: &RepoPath| left_merged.path_value(path).unwrap();
let right_value = |path: &RepoPath| right_merged.path_value(path).unwrap();
// Test the forwards diff
{

15
lib/tests/test_rewrite.rs
View file

@ -947,9 +947,18 @@ fn test_rebase_descendants_contents() {
let tree_c = commit_c.tree().unwrap();
let tree_d = commit_d.tree().unwrap();
let new_tree_c = new_commit_c.tree().unwrap();
assert_eq!(new_tree_c.path_value(path3), tree_c.path_value(path3));
assert_eq!(new_tree_c.path_value(path4), tree_d.path_value(path4));
assert_ne!(new_tree_c.path_value(path2), tree_b.path_value(path2));
assert_eq!(
new_tree_c.path_value(path3).unwrap(),
tree_c.path_value(path3).unwrap()
);
assert_eq!(
new_tree_c.path_value(path4).unwrap(),
tree_d.path_value(path4).unwrap()
);
assert_ne!(
new_tree_c.path_value(path2).unwrap(),
tree_b.path_value(path2).unwrap()
);
}
#[test]