rewrite: preserve conflict labels when restoring trees

[scott2000]

This implementation of rewrite::restore_tree preserves the conflict labels from the destination commit and the source commit by using MergedTree::merge to restore the trees instead of MergedTreeBuilder.

Suggested by @martinvonz in #8472 (comment).

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added/updated tests to cover my changes

[yuja]

nit: this function seems very specific to the current restore_tree() implementation. Maybe we can instead initialize MergedTreeBuilder with labeled empty tree?

[scott2000]

Was this comment meant to be on MergedTreeBuilder::write_tree_with_labels? I was able to remove that function following this suggestion. Otherwise, I believe we still need MergedTree::select_matching to keep the code simple for rewrite::restore_tree.

[yuja]

No. I just thought we wouldn't need a public MergedTree method if new tree could be build without using private helper functions.

[scott2000]

Ah ok, makes sense. I still think it's nice to add MergedTree::select_matching even if it doesn't use any private helper functions, since I think it makes the code easier to read and it also complements the other available methods on MergedTree (e.g. tree.conflicts_matching(matcher) could now be understood as an optimized version of tree.select_matching(matcher)?.conflicts()).

[yuja]

nit: I'm interested in how these labels would be rendered in real materialized conflicts. Are these labels included in addition to the original conflict labels? (I'm not familiar with conflict labeling, so feel free to ignore my comment.)

[scott2000]

These labels are only used when the corresponding tree is resolved (meaning it doesn't already have labels); if the tree being merged is conflicted, its existing labels are used instead.

I believe these labels currently would never be visible in materialized conflicts though, because they will always be eliminated during simplification. For instance, when restoring the first file from tree (A, B) into tree (C, D), the resulting conflict is (C, D) + ((A, _) - (C, _)) where _ represents an absent file. When materializing the first file, the conflict is C + (A - C), which is simplified to A, and when materializing the second file, the conflict is D + (_ - _), which is simplified to D.

We could therefore leave these labels empty, but I thought it would be good to have labels in case we have some command in the future which allows viewing the labels of all sides of a conflicted commit (e.g. #8377).

Edit: I added a comment in the code now to clarify this as well

[yuja]

My feeling is that users wouldn't think that restore is a merge operation, so these labels wouldn't make much sense.

BTW, maybe we can pad the destination tree with source labels + dummy base empty tree, and copy diffs from the source? This might be cheaper, but I'm not sure which one is technically more correct.

[scott2000]

My feeling is that users wouldn't think that restore is a merge operation, so these labels wouldn't make much sense.

Yeah, it could be unintuitive. But we use the "merge" operation in many places where we apply some kind of diff, and I think it's reasonable to look at restore as "applying a diff that restores the content of the file". Maybe we could try to make the wording clearer for the labels though? I'm open to any suggestions.

BTW, maybe we can pad the destination tree with source labels + dummy base empty tree, and copy diffs from the source? This might be cheaper, but I'm not sure which one is technically more correct.

I think we could do that, but we would need to be careful about ensuring all of our merges have the correct number of terms, which makes things more complicated. I think it would look something like this if I've understood you correctly:

let source_num_sides = source.tree_ids().num_sides();
let destination_num_sides = destination.tree_ids().num_sides();

let mut padded_destination_tree_ids = destination.tree_ids().clone();
padded_destination_tree_ids.pad_to(
    destination_num_sides * 2 - 1 + source_num_sides,
    &destination.store().empty_tree_id(),
);

let destination_labels = destination.labels_by_term("restore destination");
let base_labels = destination.labels_by_term("restore base");
let source_labels = source.labels_by_term("restore source");
let combined_labels = ConflictLabels::from_merge(
    Merge::from_vec(vec![destination_labels, base_labels, source_labels])
        .flatten()
        .map(|&label| label.to_owned()),
);

let mut tree_builder = MergedTreeBuilder::new(MergedTree::new(
    destination.store().clone(),
    padded_destination_tree_ids,
    combined_labels,
));
let mut diff_stream = source.diff_stream(destination, matcher);
while let Some(TreeDiffEntry {
    path: repo_path,
    values,
}) = diff_stream.next().await
{
    let Diff {
        before: source_value,
        after: destination_value,
    } = values?;
    let source_value = match source_value.into_resolved() {
        Ok(resolved) => Merge::repeated(resolved, source_num_sides),
        Err(conflict) => conflict,
    };
    let destination_value = match destination_value.into_resolved() {
        Ok(resolved) => Merge::repeated(resolved, destination_num_sides),
        Err(conflict) => conflict,
    };
    assert_eq!(source_value.num_sides(), source_num_sides);
    assert_eq!(destination_value.num_sides(), destination_num_sides);
    tree_builder.set_or_remove(
        repo_path,
        Merge::from_vec(vec![
            destination_value.clone(),
            destination_value,
            source_value,
        ])
        .flatten(),
    );
}
tree_builder.write_tree()

IMO using select_matching seems easier to read and understand, but maybe it would be worth writing the trees directly like this if performance is a concern.