Code review comment for lp:~jelmer/bzr/merge-inner-require-tree-root

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Aaron,

On 12/07/11 19:39, Aaron Bentley wrote:
> Review: Disapprove
> This change is at the wrong level. A TreeTransform whose result lacks a tree root is valid, because it can be used to produce an empty PreviewTree.
>
> Instead, it should be done in TreeTransform.apply. That code already has functionality to ensure the root directory is not deleted, which makes your "if self.final_kind(self.root) is None: self.cancel_deletion(self.root)" redundant.
This already landed before your review, but I don't really understand
how TreeTransform.apply is better than TreeTransform.fix_tree_roots.

This particular code *is* necessary because without it the loom test
suite breaks (as discussed in Dublin). What alternative do you suggest
exactly?

Cheers,

Jelmer

« Back to merge proposal