Code review comment for lp:~spiv/bzr/per-file-merge-hook-491711

Revision history for this message
Andrew Bennetts (spiv) wrote :

John A Meinel wrote:
[...]
> Though it also means that calling:
> self.tt.tree_kind()
> self.tt.delete_contents()
>
> is redundant and double statting the file. So we should clean that up

Yeah, I'd noticed that, but wasn't sure if it was tasteful to clean that
up. Seeing as you think we should I've gone ahead and removed that line
:)

[...]
> So the comment looks correct to my understanding.

I've added a comment based on Aaron's wording.

[...]
> If I set up location.conf to say:
> [/home/jameinel/dev/bzr]
> news_merger_files = NEWS
> news_merge_files:policy = recurse

(Thanks for this, this is exactly the syntax I've used for the
news_merge plugin configuration)

> Will that match when 'bzr merge --preview' is invoked (what Branch is
> given to the PreviewTree, etc.) I'm not 100% sure that PreviewTree *has*
> a .branch property. (Given that RevisionTree does not, but has a
> _repository attribute. Even better it is passed in as 'branch'... :)

It turned out I had to pass the branch a little deeper in all cases, but
having done that 'merge --preview' works just as well as regular 'merge'
:)

[...]
> It is probably reasonable to merge soon. I'd probably like to see the
> news plugin in 'plugins' first, but that is not critical.

I've done that anyway. Please try it out! :)

-Andrew.

« Back to merge proposal