Skip to content

Commit

Permalink
merged_tree: allow to postpone resolution of intermediate trees
Browse files Browse the repository at this point in the history
This allows us to diff trees without fully resolving conflicts:

    let from_tree = merge_no_resolve(..);
    for (path, (from, to)) in from_tree.diff(to_tree, matcher) {
        let from = resolve_conflicts(from);
        if from == to {
            continue; // resolved file may be identical
        ...

I originally considered adding a matcher argument to merge() functions, but the
resulting API looked misleading. If merge() took a matcher, callers might expect
unmatched trees and files were omitted, not left unresolved. It's also slower
than diffing unresolved trees because merge(.., matcher) would have to write
partially resolved trees to the store.

Since "ancestor_tree" isn't resolved by itself, this patch has subtle behavior
change. For example, "jj diff -r9eaef582" in the "git" repository is no longer
empty. I think the new behavior is also technically correct, but I'm not pretty
sure.
  • Loading branch information
yuja committed Aug 11, 2024
1 parent 5d141be commit c9e147c
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 10 deletions.
6 changes: 4 additions & 2 deletions lib/src/default_index/revset_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1142,7 +1142,8 @@ fn has_diff_from_parent(
return Ok(false);
}
}
let from_tree = rewrite::merge_commit_trees_without_repo(store, &index, &parents)?;
let from_tree =
rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?.resolve()?;
let to_tree = commit.tree()?;
let mut tree_diff = from_tree.diff_stream(&to_tree, matcher);
async {
Expand All @@ -1163,7 +1164,8 @@ fn matches_diff_from_parent(
files_matcher: &dyn Matcher,
) -> BackendResult<bool> {
let parents: Vec<_> = commit.parents().try_collect()?;
let from_tree = rewrite::merge_commit_trees_without_repo(store, &index, &parents)?;
let from_tree =
rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?.resolve()?;
let to_tree = commit.tree()?;
let tree_diff = from_tree.diff_stream(&to_tree, files_matcher);
// Conflicts are compared in materialized form. Alternatively, conflict
Expand Down
14 changes: 10 additions & 4 deletions lib/src/merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,17 +301,23 @@ impl MergedTree {
}
}

/// Merges this tree with `other`, using `base` as base.
/// Merges this tree with `other`, using `base` as base. Any conflicts will
/// be resolved recursively if possible.
pub fn merge(&self, base: &MergedTree, other: &MergedTree) -> BackendResult<MergedTree> {
self.merge_no_resolve(base, other).resolve()
}

/// Merges this tree with `other`, using `base` as base, without attempting
/// to resolve file conflicts.
pub fn merge_no_resolve(&self, base: &MergedTree, other: &MergedTree) -> MergedTree {
let nested = Merge::from_vec(vec![
self.trees.clone(),
base.trees.clone(),
other.trees.clone(),
]);
let flattened = MergedTree {
MergedTree {
trees: nested.flatten().simplify(),
};
flattened.resolve()
}
}
}

Expand Down
15 changes: 11 additions & 4 deletions lib/src/rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,19 @@ use crate::repo_path::RepoPath;
use crate::settings::UserSettings;
use crate::store::Store;

/// Merges `commits` and tries to resolve any conflicts recursively.
#[instrument(skip(repo))]
pub fn merge_commit_trees(repo: &dyn Repo, commits: &[Commit]) -> BackendResult<MergedTree> {
merge_commit_trees_without_repo(repo.store(), repo.index(), commits)
if let [commit] = commits {
commit.tree()
} else {
merge_commit_trees_no_resolve_without_repo(repo.store(), repo.index(), commits)?.resolve()
}
}

/// Merges `commits` without attempting to resolve file conflicts.
#[instrument(skip(index))]
pub fn merge_commit_trees_without_repo(
pub fn merge_commit_trees_no_resolve_without_repo(
store: &Arc<Store>,
index: &dyn Index,
commits: &[Commit],
Expand All @@ -58,9 +64,10 @@ pub fn merge_commit_trees_without_repo(
.iter()
.map(|id| store.get_commit(id))
.try_collect()?;
let ancestor_tree = merge_commit_trees_without_repo(store, index, &ancestors)?;
let ancestor_tree =
merge_commit_trees_no_resolve_without_repo(store, index, &ancestors)?;
let other_tree = other_commit.tree()?;
new_tree = new_tree.merge(&ancestor_tree, &other_tree)?;
new_tree = new_tree.merge_no_resolve(&ancestor_tree, &other_tree);
}
Ok(new_tree)
}
Expand Down

0 comments on commit c9e147c

Please sign in to comment.