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

2010/1/13 Andrew Bennetts <email address hidden>:
> This addresses most (all?) of the points raised in the original reviews.
>
> Major changes:
>
>  * Added 'news_merge' plugin to contrib/!  This is an example plugin using the merge hook function so that you can see it in action, and so that users will have some inspiration for writing their own.
>  * The built-in merge logic is now invoked the same way as user-registered hook functions.  So in theory anything the built-in logic can do, a plugin can do too.
>  * the hook point is invoked a bit earlier, so it can have a say in more kinds of conflicts by inspecting the 'winner' attribute.
>  * added some convenience methods/properties to the params object.
>  * generally simpler and more linear code path than lp:bzr, thanks to refactorings for above changes.
>
> I think looking at the news_merge plugin the API is still a bit cumbersome (note the repetition of logic vs. the built-in text merge to see if this is an ordinary regular file, both sides changed merge).  But having an example proves it works, and gives us good data for thinking of improvements.

(Without reading the diff) that sounds like a nice list of changes.

I don't want to add latency to these changes but I would say that
putting things in contrib/ is not a fabulous way to get it out to
people: it relies on them discovering that it's there and how to use
it. It is a bit confusing for packagers whether they should install
the things from in there or not: if they don't, it's more
inaccessible, and if they do then the packaged version is quite
different from the standard one. Empirically, the other things in
there have tended to rot. They are not tested.

Therefore maybe you should put this in bzrlib.plugins but arrange that
it has no effect (and little load time overhead) unless it's
configured on?

--
Martin <http://launchpad.net/~mbp/>