First, thanks everyone for the deluge of thoughtful comments!  It's
great to have so much enthusiasm about a patch :)

I'm going to reply to all the comments in this one jumbo reply, rather
than try to detangle the conversation threads this proposal has spawned.
I've skipped over some comments that are repeats of others, but I think
I've replied to every point.  Do let me know if I've inadvertantly
forgotten something you'd like me to consider.

Robert Collins wrote:
> On Fri, 2009-12-18 at 08:08 +0000, Andrew Bennetts wrote:
> > 
> >  * it is invoked for all changes where one side has changed the
> > content, and the other side has changed the content or deleted the fil
> 
> Why not 'all changes' ? I don't see why we should prevent hooks merging
> even one-side changes in a more sophisticated manner.

Hmm.  I guess because if there's a clear one-side winner then I don't
think of that as a “merge” (from the point of view of that file), and I
didn't have a use-case for it.  I was also trying to be mindful of
keeping the performance cost minimal, as Aaron mentioned.  You have
given a use-case in a follow-up (“better merges of NEWS items by
accounting for changes after a release”), and that would definitely be
nice to have.  I'll have a play and see what it would take to make
OTHER-only changes go via the hook, as your followup suggests.  It
sounds like no-one is strongly interested in THIS-only at this point, so
I'm happy to ignore that for this patch.

Also, I know it's a bit of a cop-out answer, but a plugin could always
implement a whole new Merger if the hook isn't powerful enough...

John's reply to you said:

] I assume this is because the merge code itself splits these cases. Files
] with clear winners don't get passed into the Merger code. Also, there is
] bidirectional issues.

Right, I definitely did want to keep the changes as minimal as possible
because I'm not deeply familiar with the structure of the merge code,
but I do get the sense that it would be easy to accidentally introduce
bugs if I rearranged things.

I'm not certain what you mean by bidirectional issues?  I think I can
make some educated guesses, but I'd rather you explained for me :)

> >  * should a hook function be able to have some control over the
> > emission of THIS/BASE/OTHER files when it results in a conflict, or
> > over their contents of those files?
> 
> I think yes, but that can come later.

Cool.

James Westby wrote:
> On Fri, 18 Dec 2009 08:08:34 -0000, Andrew Bennetts <email address hidden> wrote:
[...]
> >  * hook functions are passed the merger, file_id and trans_id (so can
> >  get access to filenames and file contents in the various
> >  this/other/base trees)
> 
> Is there any other information that is readily available?

As John says, the path in the various tree versions are fairly cheaply
available via the merger object.  I think it'll be worth adding
convenience methods to get at them.  I imagine some plugins might want
to just check the basename, others would like to check the full path,
some might just use the file_id.

> I am a little concerned about the performance issues. bzr-builddeb is
> going to be shipping one of these hooks, and I don't want that to
> destroy the performance of merge.

Me either!  I'm hoping to do some performance measurements at some
point.

> At minimum on every conflict it has to look up the path names. If
> we do as Rob suggests and call the hook on every modification that
> is going to be O(tree) lookups.

Well, O(changes), I think.  Files that are unchanged in all versions
won't trigger the hook (or much of the per-file merge logic at all, I
presume).

I'm interested in the “on every conflict it [bzr-builddeb?] has to look
up the path names” remark though — I don't think this hook as-is
provides a convenient way to implement “try the regular logic, if that
conflicts then try my custom plugin logic”.  Is that something you want?
I suspect the way to provide that nicely may be via making the regular
logic interact via the same interfaces as the hook (i.e. receiving the
same params object and returning the same values), and then providing a
convenient way to call that via the hook params.

This and some remarks from John (see below) suggest to me that we want
a better interface than my simple params_obj->(status,lines)...

> Some way to short-circuit this might be good. Either passing in the
> paths if they are already known (and are likely to be known even
> with refactoring), or even allowing the hook to declare what paths
> it is interested in (though that could be limiting).
> 
>   (e.g. if path_filter is not None and basename == path_filter:)
> 
> Have you written a very simple implementation of e.g. a NEWS merger
> and measured the impact?

I haven't yet, but I'd like to shortly — that's the big itch I'd love to
scratch with this patch.  The 'udd' tag on the bug was just the excuse
;)

Christmas is getting awfully close though so I'm not sure how soon that
will happen.

> > Some open questions (could be addressed in future patches perhaps):
> > 
> >  * should merge of a file rename vs. a text change use this hook
> >  point?
> 
> I imagine we would want to make it possible.
> 
> Some helpers would be useful for splitting out the cases, e.g. I imagine
> many will just want to act on two text changes.

Yeah, that's a good point.  Even many those that want to handle more than
plain text vs text changes might still want to punt on delete vs text,
for example.  So I need to take the knowledge about what sort of changes
are involved that's implicit in the code path now and turn that into
some data that the hook function can inspect.

> >  * what more can we do to help multiple hook registrations co-exist
> >  peacefully?  e.g. These functions already chain, but if multiple
> >  plugins register with this hook the ordering is not defined.
> 
> I think using a known ordering would be good. Even using sorted() on
> the hook names. This would allow you to place something at the end
> by calling it zzfallback for instance.
> 
> Otherwise changing the API to allow a priority could be good. However,
> these APIs are always tricky to deal with, as you are playing a game
> of rock-paper-scissors with and unbounded number of choices.
> 
> I bring it up now, as it may require API changes in the registration
> to do this.

Well what sort of priority system do you have in mind that would be
better than ordering by name?  A numeric system is no better (99fallback
vs. zzfallback).  I am inclined to sorting by hook names, as even if
it's only minimally useful for co-operating with other plugins it will
at least be deterministic and independent of platform-related issues
like plugin load order.

John A Meinel wrote:
> Aaron Bentley wrote:
[...]
> > merge3 and weave merge emit different things as their BASE, so if we
> > hooked merge to force a weave merge for certain files, we might want to
> > also use that style of BASE.
> > 
> > Can't see any use for controlling THIS or OTHER, though.
> > 
> > Aaron
> 
> Well, potentially a whitespace ignoring merger may want to pass that
> ability on to the output files?

Hmm, interesting!

It strikes me as a low-priority enhancement, but I think potentially
useful.

John Arbash Meinel wrote:
[...]
> > Some way to short-circuit this might be good. Either passing in the
> > paths if they are already known (and are likely to be known even
> > with refactoring), or even allowing the hook to declare what paths
> > it is interested in (though that could be limiting).
> 
> I think passing in the "path_in_this_tree, path_in_other_tree, and
> path_in_final_tree" as determined by the current merge logic would be
> reasonable.

Ok, I'll put some convenient cached properties on the MergeHookParams
for these.  (Properties so that if they aren't looked at the cost of
calculating them never occurs.)

I do worry a little that inspecting the path_in_final_tree in a hook
function is going to be a bit error prone, as couldn't that change
before the merge is completed if parent directory is renamed?  That's a
bit of an edge case I suppose.  Or are tree shape changes done before
merging contents?

John A Meinel wrote:
[...]
> +        self.create_hook(hooks.HookPoint('merge_file_content',
> +            "Called when file content needs to be merged (including when one "
> +            "side has deleted the file and the other has changed it)."
> +            "merge_file_content is called with a "
> +            "bzrlib.merge.MergeHookParams. The function should return a tuple "
> +            "of (status, lines), where status is one of 'not_applicable', "
> +            "'success', 'conflicted', or 'delete'.  If status is success or "
> +            "conflicted, then lines should be an iterable of the new lines "
> +            "for the file.",
> +            (2, 1), None))
> 
> 
> ^- I'm wondering if we wouldn't want something smarter than a tuple of
> (status, lines) being returned. It might give us a way to move to
> supporting returning THIS/BASE/OTHER more directly. It may also make it
> easier to handle path conflicts / updating the path as part of the hook.
> 
> I think that since the transform is being passed in (via the Merger
> object), you actually can do any renaming you want. You just tell the
> transform that the path for 'trans_id' should really be 'X'.

This is a really good point.  It is a bit weird to allow the hook
function access to the Merger, which would let it perform arbitrary
mutations, but also return values to do some mutations.  So I'm inclined
to try factor out the regular merge logic to a point that it and the
hook functions can interact with the Merger via the same interface.  Or
at least explicitly allow the hook to set file contents and conflicts
etc using the same API as the rest of merge.py... (although some of
those APIs are a bit messy for writing a simple hook, e.g. to register
conflicts, but that can be improved).

I'll need to be mindful of performance implications, of course, which
may get in the way of complete elegance.

> As james_w mentioned, we may want to make the api a little bit more
> verbose, and have some helper functions on MergeHookParams. For example,
> give me the final path for the file. It can be found via the transform
> (I believe), but it makes the hook api easier to use for implementors if
> some of the obvious questions are done for them. (old path, new path,
> base path, and old/new/base content are both things that would be
> obviously useful.)

Yep.

[...]
> ^- I find it odd that if the hook returns "success" or "conflicted" then
> you continue running other hooks, but if the hook claims "delete" then
> you just stop right there. Even further, though:

Ooh, that's not what I intended... ah, but that isn't what happens — if
we successfully get to the end of the loop body there's a break
statement there.  So anything other than not_applicable will terminate
the loop (and I even have some unit tests about this).  I'll see if I
can make the code clearer about this, as I can see that it is a bit
confusing.

> - -            if this_pair[0] == 'file' and other_pair[0] == 'file':
> +            if (this_pair[0] == 'file' and other_pair[0] == 'file') or
> hook_ran:
>                  # THIS and OTHER are both files, so text merge.  Either
>                  # BASE is a file, or both converted to files, so at
> least we
>                  # have agreement that output should be a file.
>                  try:
> - -                    self.text_merge(file_id, trans_id)
> +                    if not hook_ran:
> +                        # if no hook functions applied, do the default
> merge.
> +                        self.text_merge(file_id, trans_id)
>                  except errors.BinaryFile:
>                      return contents_conflict()
>                  if file_id not in self.this_tree:
> 
> ^- I don't quite understand why you enter this code path if the hook
> ran, but skip running the text merge if the hook ran.
> 
> I would guess you have a reasonable reason, but I would put the
> try/except under the 'if not hook ran'.

Actually, I don't understand exactly why either.  I wrote it like this
simply because it seemed to clearly have the least difference in
behaviour this way.  I do not understand the purpose of this logic:

                if file_id not in self.this_tree:
                    self.tt.version_file(file_id, trans_id)
                try:
                    self.tt.tree_kind(trans_id)
                    self.tt.delete_contents(trans_id)
                except errors.NoSuchFile:
                    pass

I suppose I can try disabling it and seeing what test(s) fail ;)

My best guess is it means something like “make sure file is present in
this_tree, and delete the temporary tree-transform version”.  I have no
idea what the call to self.tt.tree_kind achieves, unless it's an obscure
way to make sure that an absent file triggers the “except NoSuchFile”
block, rather than whatever delete_contents might raise in that case?
It's all horribly unclear to me, so I've been as conservative about
preserving the existing behaviour as I can.

> Hmm.. I wonder about not returning lines, but just calling the
> appropriate functionality on the TT itself. like 'tt.create_file', etc.
> Overall, I think this is a great step forward, though.

Yeah, that's a really interesting idea.  It's a much broader interface,
so maintaining compatibility might be harder if we're not careful about
what parts of TT and Merger are public, but it does mean hook functions
will have about as much power as the regular logic.  Another advantage
is it's one less custom interface.

On the other hand, “return 'conflicted', some_lines” is a much simpler
interface for a hook to use than the current alternative.  The best of
both worlds would be a single convenient API that both merge.py and
plugins could use...  Not something to do in a single leap, but this
might be nice incremental step towards that.

-Andrew.