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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/per-file-merge-hook-491711 into lp:bzr with lp:~spiv/bzr/merge-minor-refactoring as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #491711 hook to permit per file merges
> https://bugs.launchpad.net/bugs/491711
>
>
> This branch adds a hook point that allows a plugin to perform the merging of the contents of specific files (see bug 491711).
>
> The tests hopefully make the capabilities fairly clear. In short:
>
> * it is invoked for all changes where one side has changed the content, and the other side has changed the content or deleted the file
> * 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)
> * hook functions can explicitly do nothing (allowing the next registered function to try), or return a conflict, a successful merge, or a deletion of the file.
> * hook functions can set the contents of a file in both the success and conflict case
>
> Some open questions (could be addressed in future patches perhaps):
>
> * should merge of a file rename vs. a text change use this hook point?
> * 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?
> * what helpers can we provide to help people write hook functions?
> * 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 ideally this hook point would be marked experimental until we get some practical experience proving that it works well in practice (e.g. that someone can write a plugin to merge NEWS files), but that's what beta series are for ;)
>
>

+ 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'.

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.)

+ for hook in hooks:
+ hook_status, lines = hook(params)
+ if hook_status == 'not_applicable':
+ continue
+ elif hook_status == 'success':
+ self.tt.create_file(lines, trans_id)
+ elif hook_status == 'conflicted':
+ # XXX: perhaps the hook should be able to provide
+ # the BASE/THIS/OTHER files?
+ self.tt.create_file(lines, trans_id)
+ self._raw_conflicts.append(('text conflict', trans_id))
+ name = self.tt.final_name(trans_id)
+ parent_id = self.tt.final_parent(trans_id)
+ file_group = self._dump_conflicts(name, parent_id,
file_id)
+ file_group.append(trans_id)
+ elif hook_status == 'delete':
+ self.tt.unversion_file(trans_id)
+ return "deleted"
+ else:
+ raise AssertionError(
+ 'unknown hook_status: %r' % (hook_status,))
+ # This hook function applied, so don't try any other
+ # hook functions.
+ hook_ran = True
+ break

^- 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:

             # We have a hypothetical conflict, but if we have files,
then we
             # can try to merge the content
- - 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'.

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.

 review: needsinfo

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkstMUMACgkQJdeBCYSNAAN5UQCfZMMQaUAzzwg5ixDcQ/3Qz9zE
f4UAoK1USkiYCjywqJTqhrRFsIkaGywG
=7EsC
-----END PGP SIGNATURE-----