[MERGE] Implement Tree.plan_merge, deprecate RevisionTree.get_weave

Aaron Bentley abentley at panoramicfeedback.com
Thu Jul 19 13:40:14 BST 2007


Robert Collins wrote:
> Robert Collins has voted +1 (conditional).
> Status is now: Conditionally approved
> Comment:
> 
> +            # Disable pending merges, to avoid affecting merge behavior
> +            tree.set_parent_ids(parents[:1])
> 
> Could you explain more why this is needed?

plan_merge is affected by the list of ancestors.  If OTHER is listed as
an ancestor of THIS, that means that all of the changes in OTHER have
already been considered and rejected, so the merge doesn't need to do
anything.

> It seems prone to losing a
> users data e.g. during a power-off or kill -9 situation as well as
> requiring more work at the tree layer to discard and recreate the cached
> parent data.

Yeah, the question is whether we care enough in these situations:

1. remerge is rarely used
2. user files will also be a mess in this situation, so it's only
   incrementaly worse
3. if we do lose the parent data, users can just do "revert" and "merge"
   instead.

> with our current API
> 
> +    def _get_ancestors(self, default_revision):
> +        ancestors = set([default_revision])
> +        for parent_id in self.get_parent_ids():
> +            ancestors.update(self.branch.repository.get_ancestry(
> +                             parent_id, topo_sorted=False))
> +        return ancestors
> 
> will be faster as
> +        ancestors = set([default_revision])
> +
> ancestors.update(self.branch.repository.get_revision_graph(self.get_parent_ids()))
> 
> +        return ancestors
> 
> because that will do just one index traversal.

True, but it's exceptional to perform a merge while there is more than
one ancestor, and it seems clearer to use get_ancestry.

> Its interesting that we grab full ancestry here. This doesn't affect the
> merge, but I wonder about giving a callback here, or even using the file
> merge graph which will be much smaller to reduce the amount of index
> looked at.

Not really necessary.  We really just need graph difference.

> With the previously discussed plan_merge -> plan_file_merge I'm happy to
> see this come in.

Thanks.

Aaron



More information about the bazaar mailing list