fix(TreeBuilder::build_conflict): resolve not-really conflicts (left and right agree) #188

Do we agree that it's clear that list_left can be of length more than outside of this if block, for any conflict that has more than a single element on the left-hand side?
Then, I think list_left can very well still be of length more than one inside the if. That would happen when the tree matching heuristic failed to match not just one pair of left/right elements, but multiple elements (consecutive children of the same parent).

Do we agree that it's clear that list_left can be of length more than outside of this if block, for any conflict that has more than a single element on the left-hand side?

I'm not sure I completely understand you. You probably meant "list_left can be of length more than one outside of this if block"? If yes, then... sure, that sounds right.

Then, I think list_left can very well still be of length more than one inside the if. That would happen when the tree matching heuristic failed to match not just one pair of left/right elements, but multiple elements (consecutive children of the same parent).

Ohh, I think I understand it now. Each list is, well, a list of PCS nodes, starting from a given divergence point (where we started to build the conflict) to a point where the two given revisions converge again?

As I said, as I was writing the comment, I didn't really understand what any of this meant. Funnily enough, now that I do get it, the comments in extract_conflict_side look pretty obvious. I guess that's just the curse of knowledge, and I'll try to rewrite the comment so that it's actually helpful to someone who isn't already familiar with the method.

But back to the question. I now agree that the lists may well be of length 1+ (but surely not 0 -- otherwise we wouldn't have called build_conflict in the first place, no?) EDIT: nevermind, both sides could've removed their node, so their corresponding lists can in fact both end up being empty. So I guess this is really similar to what we do at

after we get the conflict - we can either manage to resolve it, in which case we children.extend(resolved), or don't, in which case we children.push(conflict). But that's only done on conflicts that turn out to be commutative -- I think we should add similar logic for when it's not. Or rather, expand resolve_commutative_conflict so that it also checks for isomorphic left and right sides (and returns Vec<MergedTree> if they are) before continuing on with the rest of its logic. We could rename it to try_resolve_conflict to reflect the fact that it now handles all the cases where a conflict can be resolved.