Merge into bzr.dev : per-file-merge-hook-491711 : Code : Bazaar

Reviewer	Review Type	Date Requested	Status
John A Meinel		2009-12-18	Needs Information on 2009-12-19
Review via email: mp+16327@code.launchpad.net

Revision history for this message

Andrew Bennetts (spiv) wrote on 2009-12-18:

#

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

Revision history for this message

Robert Collins (lifeless) wrote on 2009-12-18:

#

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.

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

> * 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 don't think the ordering matters, except in the case that someone is
writing a new 'fallback' hook, to replace our three-way merge attempt.
Perhaps put a priority into the hook or something? I'd wait for signs we
need it.

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

You can just mark it experimental in the docs ;)

-Rob

Revision history for this message

James Westby (james-w) wrote on 2009-12-18:

#

On Fri, 18 Dec 2009 08:08:34 -0000, Andrew Bennetts <email address hidden> wrote:
> This branch adds a hook point that allows a plugin to perform the merging of the contents of specific files (see bug 491711).

Yay, thanks for working on this.

> The tests hopefully make the capabilities fairly clear. In short:
>
> * 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?

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.

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.

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?

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

> * 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'm not sure I see a use case for this.

> * what helpers can we provide to help people write hook functions?

Helpers for getting the paths, and other meta information.

Helpers for determining the cause of the conflict, text changes vs.
path changes etc.

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

Thanks,

James

On Fri, 18 Dec 2009 08:08:34 -0000, Andrew Bennetts <andrew.bennetts@canonical.com> wrote:
> This branch adds a hook point that allows a plugin to perform the merging of the contents of specific files (see bug 491711).

Yay, thanks for working on this.
 
> The tests hopefully make the capabilities fairly clear.  In short:
> 
>  * 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?

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.

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.

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?

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

>  * 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'm not sure I see a use case for this.

>  * what helpers can we provide to help people write hook functions?

Helpers for getting the paths, and other meta information.

Helpers for determining the cause of the conflict, text changes vs.
path changes etc.

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

Thanks,

James

Revision history for this message

Aaron Bentley (abentley) wrote on 2009-12-18:

#

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

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.

That would require us to pay attention to THIS-only changes, and that
would be a performance regression.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksrtrIACgkQ0F+nu1YWqI3a6wCeNdmME7ZY/xSZ7ma6snOd30fE
XUUAni6xZaHfi47GfpkTivlXF8uFDa5w
=uew/
-----END PGP SIGNATURE-----

Revision history for this message

Aaron Bentley (abentley) wrote on 2009-12-18:

#

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

James Westby wrote:
> On Fri, 18 Dec 2009 08:08:34 -0000, Andrew Bennetts <email address hidden> wrote:
>> This branch adds a hook point that allows a plugin to perform the merging of the contents of specific files (see bug 491711).
>> * 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'm not sure I see a use case for this.

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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksrt6oACgkQ0F+nu1YWqI26pgCdF24SIHw4I4pnYV1DpikUoPwT
3EgAn08cMSUKDneD39c0rBt4aKK+zYCh
=QYTu
-----END PGP SIGNATURE-----

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-18:

#

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

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.

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.

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

iEYEARECAAYFAksrw+oACgkQJdeBCYSNAANPTQCeLRj5ST7fT58nGOkIJldtyNis
9fsAoMnNoCWak28nr8shdECpIyLRbxtY
=lRgj
-----END PGP SIGNATURE-----

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-18:

#

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

...
>
> Is there any other information that is readily available?
>
> 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.
>
> 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.

Still O(changes), I don't see how you get O(tree).

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

>
> (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?
>
>> 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.
>
>> * 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'm not sure I see a use case for this.
>
>> * what helpers can we provide to help people write hook functions?
>
> Helpers for getting the paths, and other meta information.
>
> Helpers for determining the cause of the conflict, text changes vs.
> path changes etc.
>
>> * 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.
>
> Thanks,
>
> James

I think explicitly making them unordered forces people to think about
whether they can do what they need in an unordered fashion.

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

iEYEARECAAYFAksrxLwACgkQJdeBCYSNAANiYwCgpaTb3KqL65jqOHou/mpzrQwt
/3oAn1TyJC5/tS1+Xc0+3//PRxoai0lI
=Gavq
-----END PGP SIGNATURE-----

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

...
> 
> Is there any other information that is readily available?
> 
> 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.
> 
> 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.

Still O(changes), I don't see how you get O(tree).

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

> 
>   (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?
> 
>> 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.
> 
>>  * 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'm not sure I see a use case for this.
> 
>>  * what helpers can we provide to help people write hook functions?
> 
> Helpers for getting the paths, and other meta information.
> 
> Helpers for determining the cause of the conflict, text changes vs.
> path changes etc.
> 
>>  * 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.
> 
> Thanks,
> 
> James

I think explicitly making them unordered forces people to think about
whether they can do what they need in an unordered fashion.

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

iEYEARECAAYFAksrxLwACgkQJdeBCYSNAANiYwCgpaTb3KqL65jqOHou/mpzrQwt
/3oAn1TyJC5/tS1+Xc0+3//PRxoai0lI
=Gavq
-----END PGP SIGNATURE-----

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-18:

#

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

Aaron Bentley wrote:
> James Westby wrote:
>> On Fri, 18 Dec 2009 08:08:34 -0000, Andrew Bennetts <email address hidden> wrote:
>>> This branch adds a hook point that allows a plugin to perform the merging of the contents of specific files (see bug 491711).
>>> * 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'm not sure I see a use case for this.
>
> 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?

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

iEYEARECAAYFAksrxN0ACgkQJdeBCYSNAAMJ9gCgxngkhtNINZPw7iNuWPe8JUs3
6UIAn30zdkrAz6hhN2PoLlhgxsfy0vCm
=omK9
-----END PGP SIGNATURE-----

Revision history for this message

Robert Collins (lifeless) wrote on 2009-12-18:

#

On Fri, 2009-12-18 at 17:09 +0000, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> 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.
>
> That would require us to pay attention to THIS-only changes, and that
> would be a performance regression.

So there is a middle ground, where OTHER-only changes are examined.
There are a number of useful things one can do where OTHER-only changes
are examined (such as better merges of NEWS items by accounting for
changes after a release) which the current definition won't help with. I
see that checking for THIS-only changes would have an impact, and while
it might be nice, I don't see a need to do it at this point.

-Rob

Revision history for this message

John A Meinel (jameinel) wrote on 2009-12-19:

#

Download full text (6.4 KiB)

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

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

review: Needs Information

Revision history for this message

Andrew Bennetts (spiv) wrote on 2009-12-21:

#

Download full text (13.7 KiB)

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

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 <andrew.bennetts@canonical.com> 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.

Revision history for this message

James Westby (james-w) wrote on 2009-12-21:

#

On Mon, 21 Dec 2009 02:39:14 -0000, Andrew Bennetts <email address hidden> wrote:
> 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.

The way I imagine it working is that a hook will be registered by
bzr-builddeb that will merge debian/changelog files. The question
is to know what files to act on. We can try and parse the files
and just skip everything that doesn't parse as one of those files.
However, that will be both quite slow, plus hugely pessimistic.

Therefore I would want to short-circuit, and only bother trying
to parse files where the basename is "changelog". It would be
great if the basename was easily accesible to the hook so
that it could do this check without doing something like
trans_id->file_id->path.

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

Yep, I'm mainly concerned in these comments about the API that is
presented to plugin writers. Being able simply separate out the cases
without each plugin having to cargo cult code to detect what situation
it is being called for would be good.

I think this will partially come from APIs, which I imagine are there,
I just haven't looked, and from docs. We should docuement the things
people commonly want to do so that it is obvious to plugin writers.
My concern would be that you write a hook that assumes you are always
dealing with text vs. text, and then it crashes when you get text vs.
delete. Making it clear that you can detect the cases at least points
you towards considering which you want to handle, even if the API
doesn't force you to think about them all.

> Well what sort of priority system do you have in mind that would be
> better than ordering by name?

Nothing in particular. I was just making a plea for a defined order.
If e.g. the order was whatever dict.keys() returns then there is no
way to get your hook to run last if you need to.

Thanks,

James

On Mon, 21 Dec 2009 02:39:14 -0000, Andrew Bennetts <andrew.bennetts@canonical.com> wrote:
> 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.

The way I imagine it working is that a hook will be registered by
bzr-builddeb that will merge debian/changelog files. The question
is to know what files to act on. We can try and parse the files
and just skip everything that doesn't parse as one of those files.
However, that will be both quite slow, plus hugely pessimistic.

Therefore I would want to short-circuit, and only bother trying
to parse files where the basename is "changelog". It would be
great if the basename was easily accesible to the hook so
that it could do this check without doing something like
trans_id->file_id->path.

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

Yep, I'm mainly concerned in these comments about the API that is
presented to plugin writers. Being able simply separate out the cases
without each plugin having to cargo cult code to detect what situation
it is being called for would be good.

I think this will partially come from APIs, which I imagine are there,
I just haven't looked, and from docs. We should docuement the things
people commonly want to do so that it is obvious to plugin writers.
My concern would be that you write a hook that assumes you are always
dealing with text vs. text, and then it crashes when you get text vs.
delete. Making it clear that you can detect the cases at least points
you towards considering which you want to handle, even if the API
doesn't force you to think about them all.

> Well what sort of priority system do you have in mind that would be
> better than ordering by name?

Nothing in particular. I was just making a plea for a defined order.
If e.g. the order was whatever dict.keys() returns then there is no
way to get your hook to run last if you need to.

Thanks,

James

Revision history for this message

Robert Collins (lifeless) wrote on 2009-12-21:

#

On Mon, 2009-12-21 at 02:57 +0000, James Westby wrote:

> Nothing in particular. I was just making a plea for a defined order.
> If e.g. the order was whatever dict.keys() returns then there is no
> way to get your hook to run last if you need to.

You don't with names either.

'Run last' is very very hard to pin down.

'Topological order' is a bit easier, 'numeric order' is very simple.

Hooks don't have names so much as descriptions, I strongly urge /not/ to
use the labels as a sort order; ugh. We don't have orders elsewhere in
hook land and we haven't needed to - even though this discussion happens
every time.

Re: merging changelog, I'd lookup the full path and compare to
'debian/changelog' and 'changelog' - its simple and should be fast.

Andrew: are kind changes able to invoke this hook? If not please make
that clear.

I think passing in the Merger is fine; we pass in Branch in other hooks
and trust folk to use public bits and not make a mess. If someone does
fiddle, the conflict resolution phases will detect that pretty robustly
I think.

-Rob

Revision history for this message

Andrew Bennetts (spiv) wrote on 2009-12-21:

#

Robert Collins wrote:
> On Mon, 2009-12-21 at 02:57 +0000, James Westby wrote:
>
> > Nothing in particular. I was just making a plea for a defined order.
> > If e.g. the order was whatever dict.keys() returns then there is no
> > way to get your hook to run last if you need to.
>
> You don't with names either.
>
> 'Run last' is very very hard to pin down.
>
> 'Topological order' is a bit easier, 'numeric order' is very simple.
>
> Hooks don't have names so much as descriptions, I strongly urge /not/ to
> use the labels as a sort order; ugh. We don't have orders elsewhere in
> hook land and we haven't needed to - even though this discussion happens
> every time.

Yeah, I don't much like punning that. I'd like to make the order
defined and stable somehow rather than dependent on the somewhat
unstable order that plugins will get loaded, though.

> Re: merging changelog, I'd lookup the full path and compare to
> 'debian/changelog' and 'changelog' - its simple and should be fast.

I definitely think that's what I'd try to start with. Maybe that will
be a significant performance hit in which case we can rethink it, but
let's not prematurely optimise.

> Andrew: are kind changes able to invoke this hook? If not please make
> that clear.

I don't know off the top of my head. I'll find out and make it
explicit. My guess is no, but I'm not very confident about that.

> I think passing in the Merger is fine; we pass in Branch in other hooks
> and trust folk to use public bits and not make a mess. If someone does
> fiddle, the conflict resolution phases will detect that pretty robustly
> I think.

I'm not worried about people accessing non-public bits (they should
already know that _foo attributes are off-limits unless they're ready to
deal with the likely breakage), apart from ensuring that the public bits
are complete enough for hooks to do what they need.

However, I think it's important to decide if we pass the Merger with the
intent that the hook will use it only read-only (to retrieve file names,
file contents, maybe details about other things in the tree, etc), or if
we actually want to support the hook mutating things via the Merger. It
seems the answer is the latter, which is fine, but it's worth being
clear about that. That answer suggests that my original proposal that
the return value from the hook makes changes isn't quite right when the
hook can just do change(s) directly using the Merger.

-Andrew.

Robert Collins wrote:
> On Mon, 2009-12-21 at 02:57 +0000, James Westby wrote:
> 
> > Nothing in particular. I was just making a plea for a defined order.
> > If e.g. the order was whatever dict.keys() returns then there is no
> > way to get your hook to run last if you need to.
> 
> You don't with names either.
> 
> 'Run last' is very very hard to pin down.
> 
> 'Topological order' is a bit easier, 'numeric order' is very simple.
> 
> Hooks don't have names so much as descriptions, I strongly urge /not/ to
> use the labels as a sort order; ugh. We don't have orders elsewhere in
> hook land and we haven't needed to - even though this discussion happens
> every time.

Yeah, I don't much like punning that.  I'd like to make the order
defined and stable somehow rather than dependent on the somewhat
unstable order that plugins will get loaded, though.

> Re: merging changelog, I'd lookup the full path and compare to
> 'debian/changelog' and 'changelog' - its simple and should be fast.

I definitely think that's what I'd try to start with.  Maybe that will
be a significant performance hit in which case we can rethink it, but
let's not prematurely optimise.

> Andrew: are kind changes able to invoke this hook? If not please make
> that clear.

I don't know off the top of my head.  I'll find out and make it
explicit.  My guess is no, but I'm not very confident about that.

> I think passing in the Merger is fine; we pass in Branch in other hooks
> and trust folk to use public bits and not make a mess. If someone does
> fiddle, the conflict resolution phases will detect that pretty robustly
> I think.

I'm not worried about people accessing non-public bits (they should
already know that _foo attributes are off-limits unless they're ready to
deal with the likely breakage), apart from ensuring that the public bits
are complete enough for hooks to do what they need.

However, I think it's important to decide if we pass the Merger with the
intent that the hook will use it only read-only (to retrieve file names,
file contents, maybe details about other things in the tree, etc), or if
we actually want to support the hook mutating things via the Merger.  It
seems the answer is the latter, which is fine, but it's worth being
clear about that.  That answer suggests that my original proposal that
the return value from the hook makes changes isn't quite right when the
hook can just do change(s) directly using the Merger.

-Andrew.

Revision history for this message

Robert Collins (lifeless) wrote on 2009-12-21:

#

I'd suggest one of:
return value indicates 'handled' or 'not handled'
return value is parameter to pass to next hook or None-to-stop

and let changes that require calling fn's on Merger be done directly.

-Rob

Bazaar

Merge lp:~spiv/bzr/per-file-merge-hook-491711 into lp:bzr

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'bzrlib/decorators.py'
 --- bzrlib/decorators.py	2009-10-15 02:19:43 +0000
 +++ bzrlib/decorators.py	2010-01-13 07:31:30 +0000
@@ -253,3 +253,19 @@
      global needs_read_lock, needs_write_lock
      needs_read_lock = _pretty_needs_read_lock
      needs_write_lock = _pretty_needs_write_lock
++
++
++def cachedproperty(getter_func):
++    """Like property(getter_func), but caches the value.
++
++    Note: there's no way to clear or reset the cache.
++    """
++    memo = []
++    def f(self):
++        if not memo:
++            memo.append(getter_func(self))
++        return memo[0]
++    f.__doc__ = getter_func.__doc__
++    getter_func.__name__ = getter_func.__name__
++    return property(f)
++
 === modified file 'bzrlib/hooks.py'
 --- bzrlib/hooks.py	2010-01-03 03:49:07 +0000
 +++ bzrlib/hooks.py	2010-01-13 07:31:30 +0000
@@ -45,6 +45,8 @@
      'bzrlib.info', 'InfoHooks')
  known_hooks.register_lazy(('bzrlib.lock', 'Lock.hooks'), 'bzrlib.lock',
      'LockHooks')
++known_hooks.register_lazy(('bzrlib.merge', 'Merger.hooks'), 'bzrlib.merge',
++    'MergeHooks')
  known_hooks.register_lazy(('bzrlib.msgeditor', 'hooks'), 'bzrlib.msgeditor',
      'MessageEditorHooks')
  known_hooks.register_lazy(('bzrlib.mutabletree', 'MutableTree.hooks'),
 === modified file 'bzrlib/merge.py'
 --- bzrlib/merge.py	2009-12-18 08:22:42 +0000
 +++ bzrlib/merge.py	2010-01-13 07:31:30 +0000
@@ -19,8 +19,10 @@
      branch as _mod_branch,
      conflicts as _mod_conflicts,
      debug,
++    decorators,
      errors,
      graph as _mod_graph,
++    hooks,
      merge3,
      osutils,
      patiencediff,
@@ -50,7 +52,69 @@
          from_tree.unlock()
++class MergeHooks(hooks.Hooks):
++
++    def __init__(self):
++        hooks.Hooks.__init__(self)
++        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))
++
++
++class MergeHookParams(object):
++    """Object holding parameters passed to merge_file_content hooks.
++
++    There are 3 fields hooks can access:
++
++    :ivar merger: the Merger object
++    :ivar file_id: the file ID of the file being merged
++    :ivar trans_id: the transform ID for the merge of this file.
++
++    The lines of versions of the file being merged can be retrieved from the
++    merger, e.g.::
++
++        params.merger.get_lines(params.merger.this_tree, params.file_id)
++    """
++
++    def __init__(self, merger, file_id, trans_id, this_pair, other_pair,
++            winner):
++        self.merger = merger
++        self.file_id = file_id
++        self.trans_id = trans_id
++        self.this_pair = this_pair
++        self.other_pair = other_pair
++        self.winner = winner
++
++    def is_file_merge(self):
++        return self.this_pair[0] == 'file' and self.other_pair[0] == 'file'
++
++    @decorators.cachedproperty
++    def base_lines(self):
++        """The lines of the 'base' version of the file."""
++        return self.merger.get_lines(self.merger.base_tree, self.file_id)
++
++    @decorators.cachedproperty
++    def this_lines(self):
++        """The lines of the 'this' version of the file."""
++        return self.merger.get_lines(self.merger.this_tree, self.file_id)
++
++    @decorators.cachedproperty
++    def other_lines(self):
++        """The lines of the 'other' version of the file."""
++        return self.merger.get_lines(self.merger.other_tree, self.file_id)
++
++
  class Merger(object):
++
++    hooks = MergeHooks()
++
      def __init__(self, this_branch, other_tree=None, base_tree=None,
                   this_tree=None, pb=None, change_reporter=None,
                   recurse='down', revision_graph=None):
@@ -1107,18 +1171,6 @@
                  contents = None
              return kind, contents
--        def contents_conflict():
--            trans_id = self.tt.trans_id_file_id(file_id)
--            name = self.tt.final_name(trans_id)
--            parent_id = self.tt.final_parent(trans_id)
--            if file_id in self.this_tree.inventory:
--                self.tt.unversion_file(trans_id)
--                if file_id in self.this_tree:
--                    self.tt.delete_contents(trans_id)
--            file_group = self._dump_conflicts(name, parent_id, file_id,
--                                              set_version=True)
--            self._raw_conflicts.append(('contents conflict', file_group))
--
          # See SPOT run.  run, SPOT, run.
          # So we're not QUITE repeating ourselves; we do tricky things with
          # file kind...
@@ -1140,59 +1192,108 @@
          if winner == 'this':
              # No interesting changes introduced by OTHER
              return "unmodified"
++        # We have a hypothetical conflict, but if we have files, then we
++        # can try to merge the content
          trans_id = self.tt.trans_id_file_id(file_id)
--        if winner == 'other':
++        params = MergeHookParams(self, file_id, trans_id, this_pair,
++            other_pair, winner)
++        hooks = Merger.hooks['merge_file_content']
++        hooks = list(hooks) + [self.default_text_merge]
++        hook_status = 'not_applicable'
++        for hook in hooks:
++            hook_status, lines = hook(params)
++            if hook_status != 'not_applicable':
++                # Don't try any more hooks, this one applies.
++                break
++        result = "modified"
++        if hook_status == 'not_applicable':
++            # This is a contents conflict, because none of the available
++            # functions could merge it.
++            result = None
++            name = self.tt.final_name(trans_id)
++            parent_id = self.tt.final_parent(trans_id)
++            if file_id in self.this_tree.inventory:
++                self.tt.unversion_file(trans_id)
++            file_group = self._dump_conflicts(name, parent_id, file_id,
++                                              set_version=True)
++            self._raw_conflicts.append(('contents conflict', file_group))
++        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)
++            result = "deleted"
++        elif hook_status == 'done':
++            # The hook function did whatever it needs to do directly, no
++            # further action needed here.
++            pass
++        else:
++            raise AssertionError('unknown hook_status: %r' % (hook_status,))
++        if file_id not in self.this_tree and result == "modified":
++            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
++        return result
++
++    def _default_other_winner_merge(self, merge_hook_params):
++        """Replace this contents with other."""
++        file_id = merge_hook_params.file_id
++        trans_id = merge_hook_params.trans_id
++        file_in_this = file_id in self.this_tree
++        if file_id in self.other_tree:
++            # OTHER changed the file
++            wt = self.this_tree
++            if wt.supports_content_filtering():
++                # We get the path from the working tree if it exists.
++                # That fails though when OTHER is adding a file, so
++                # we fall back to the other tree to find the path if
++                # it doesn't exist locally.
++                try:
++                    filter_tree_path = wt.id2path(file_id)
++                except errors.NoSuchId:
++                    filter_tree_path = self.other_tree.id2path(file_id)
++            else:
++                # Skip the id2path lookup for older formats
++                filter_tree_path = None
++            transform.create_from_tree(self.tt, trans_id,
++                             self.other_tree, file_id,
++                             filter_tree_path=filter_tree_path)
++            return 'done', None
++        elif file_in_this:
++            # OTHER deleted the file
++            return 'delete', None
++        else:
++            raise AssertionError(
++                'winner is OTHER, but file_id %r not in THIS or OTHER tree'
++                % (file_id,))
++
++    def default_text_merge(self, merge_hook_params):
++        if merge_hook_params.winner == 'other':
              # OTHER is a straight winner, so replace this contents with other
--            file_in_this = file_id in self.this_tree
--            if file_in_this:
--                # Remove any existing contents
--                self.tt.delete_contents(trans_id)
--            if file_id in self.other_tree:
--                # OTHER changed the file
--                wt = self.this_tree
--                if wt.supports_content_filtering():
--                    # We get the path from the working tree if it exists.
--                    # That fails though when OTHER is adding a file, so
--                    # we fall back to the other tree to find the path if
--                    # it doesn't exist locally.
--                    try:
--                        filter_tree_path = wt.id2path(file_id)
--                    except errors.NoSuchId:
--                        filter_tree_path = self.other_tree.id2path(file_id)
--                else:
--                    # Skip the id2path lookup for older formats
--                    filter_tree_path = None
--                transform.create_from_tree(self.tt, trans_id,
--                                 self.other_tree, file_id,
--                                 filter_tree_path=filter_tree_path)
--                if not file_in_this:
--                    self.tt.version_file(file_id, trans_id)
--                return "modified"
--            elif file_in_this:
--                # OTHER deleted the file
--                self.tt.unversion_file(trans_id)
--                return "deleted"
++            return self._default_other_winner_merge(merge_hook_params)
++        elif merge_hook_params.is_file_merge():
++            # 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(merge_hook_params.file_id,
++                    merge_hook_params.trans_id)
++            except errors.BinaryFile:
++                return 'not_applicable', None
++            return 'done', None
          else:
--            # 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':
--                # 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)
--                except errors.BinaryFile:
--                    return contents_conflict()
--                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
--                return "modified"
--            else:
--                return contents_conflict()
++            return 'not_applicable', None
      def get_lines(self, tree, file_id):
          """Return the lines in a file, or an empty list."""
 === modified file 'bzrlib/tests/per_merger.py'
 --- bzrlib/tests/per_merger.py	2009-12-18 06:54:20 +0000
 +++ bzrlib/tests/per_merger.py	2010-01-13 07:31:30 +0000
@@ -18,6 +18,7 @@
  import os
++from bzrlib.conflicts import TextConflict
  from bzrlib import (
      errors,
      merge as _mod_merge,
@@ -28,6 +29,7 @@
      multiply_tests,
      TestCaseWithTransport,
+     )
++from bzrlib.tests.test_merge_core import MergeBuilder
  from bzrlib.transform import TreeTransform
@@ -188,4 +190,171 @@
          self.assertRaises(errors.LockError, wt.unlock)
++class TestHookMergeFileContent(TestCaseWithTransport):
++    """Tests that the 'merge_file_content' hook is invoked."""
++
++    def setUp(self):
++        TestCaseWithTransport.setUp(self)
++        self.hook_log = []
++
++    def install_hook_noop(self):
++        def hook_na(merge_params):
++            # This hook unconditionally does nothing.
++            self.hook_log.append(('no-op',))
++            return 'not_applicable', None
++        _mod_merge.Merger.hooks.install_named_hook(
++            'merge_file_content', hook_na, 'test hook (no-op)')
++
++    def install_hook_success(self):
++        def hook_success(merge_params):
++            self.hook_log.append(('success',))
++            if merge_params.file_id == '1':
++                return 'success', ['text-merged-by-hook']
++            return 'not_applicable', None
++        _mod_merge.Merger.hooks.install_named_hook(
++            'merge_file_content', hook_success, 'test hook (success)')
++
++    def install_hook_conflict(self):
++        def hook_conflict(merge_params):
++            self.hook_log.append(('conflict',))
++            if merge_params.file_id == '1':
++                return 'conflicted', ['text-with-conflict-markers-from-hook']
++            return 'not_applicable', None
++        _mod_merge.Merger.hooks.install_named_hook(
++            'merge_file_content', hook_conflict, 'test hook (delete)')
++
++    def install_hook_delete(self):
++        def hook_delete(merge_params):
++            self.hook_log.append(('delete',))
++            if merge_params.file_id == '1':
++                return 'delete', None
++            return 'not_applicable', None
++        _mod_merge.Merger.hooks.install_named_hook(
++            'merge_file_content', hook_delete, 'test hook (delete)')
++
++    def install_hook_log_lines(self):
++        """Install a hook that saves the get_lines for the this, base and other
++        versions of the file.
++        """
++        def hook_log_lines(merge_params):
++            self.hook_log.append((
++                'log_lines',
++                merge_params.this_lines,
++                merge_params.other_lines,
++                merge_params.base_lines,
++                ))
++            return 'not_applicable', None
++        _mod_merge.Merger.hooks.install_named_hook(
++            'merge_file_content', hook_log_lines, 'test hook (log_lines)')
++
++    def make_merge_builder(self):
++        builder = MergeBuilder(self.test_base_dir)
++        self.addCleanup(builder.cleanup)
++        return builder
++
++    def create_file_needing_contents_merge(self, builder, file_id):
++        builder.add_file(file_id, builder.tree_root, "name1", "text1", True)
++        builder.change_contents(file_id, other="text4", this="text3")
++
++    def test_change_vs_change(self):
++        """Hook is used for (changed, changed)"""
++        self.install_hook_success()
++        builder = self.make_merge_builder()
++        builder.add_file("1", builder.tree_root, "name1", "text1", True)
++        builder.change_contents("1", other="text4", this="text3")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual(conflicts, [])
++        self.assertEqual(
++            builder.this.get_file('1').read(), 'text-merged-by-hook')
++
++    def test_change_vs_deleted(self):
++        """Hook is used for (changed, deleted)"""
++        self.install_hook_success()
++        builder = self.make_merge_builder()
++        builder.add_file("1", builder.tree_root, "name1", "text1", True)
++        builder.change_contents("1", this="text2")
++        builder.remove_file("1", other=True)
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual(conflicts, [])
++        self.assertEqual(
++            builder.this.get_file('1').read(), 'text-merged-by-hook')
++
++    def test_result_can_be_delete(self):
++        """A hook's result can be the deletion of a file."""
++        self.install_hook_delete()
++        builder = self.make_merge_builder()
++        self.create_file_needing_contents_merge(builder, "1")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual(conflicts, [])
++        self.assertRaises(errors.NoSuchId, builder.this.id2path, '1')
++        self.assertEqual([], list(builder.this.list_files()))
++
++    def test_result_can_be_conflict(self):
++        """A hook's result can be a conflict."""
++        self.install_hook_conflict()
++        builder = self.make_merge_builder()
++        self.create_file_needing_contents_merge(builder, "1")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual(conflicts, [TextConflict('name1', file_id='1')])
++        # The hook still gets to set the file contents in this case, so that it
++        # can insert custom conflict markers.
++        self.assertEqual(
++            builder.this.get_file('1').read(),
++            'text-with-conflict-markers-from-hook')
++
++    def test_can_access_this_other_and_base_versions(self):
++        """The hook function can call params.merger.get_lines to access the
++        THIS/OTHER/BASE versions of the file.
++        """
++        self.install_hook_log_lines()
++        builder = self.make_merge_builder()
++        builder.add_file("1", builder.tree_root, "name1", "text1", True)
++        builder.change_contents("1", this="text2", other="text3")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual(
++            [('log_lines', ['text2'], ['text3'], ['text1'])], self.hook_log)
++
++    def test_chain_when_not_applicable(self):
++        """When a hook function returns not_applicable, the next function is
++        tried (when one exists).
++        """
++        self.install_hook_noop()
++        self.install_hook_success()
++        builder = self.make_merge_builder()
++        self.create_file_needing_contents_merge(builder, "1")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual(conflicts, [])
++        self.assertEqual(
++            builder.this.get_file('1').read(), 'text-merged-by-hook')
++        self.assertEqual([('no-op',), ('success',)], self.hook_log)
++
++    def test_chain_stops_after_success(self):
++        """When a hook function returns success, no later functions are tried.
++        """
++        self.install_hook_success()
++        self.install_hook_noop()
++        builder = self.make_merge_builder()
++        self.create_file_needing_contents_merge(builder, "1")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual([('success',)], self.hook_log)
++
++    def test_chain_stops_after_conflict(self):
++        """When a hook function returns conflict, no later functions are tried.
++        """
++        self.install_hook_conflict()
++        self.install_hook_noop()
++        builder = self.make_merge_builder()
++        self.create_file_needing_contents_merge(builder, "1")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual([('conflict',)], self.hook_log)
++
++    def test_chain_stops_after_delete(self):
++        """When a hook function returns delete, no later functions are tried.
++        """
++        self.install_hook_delete()
++        self.install_hook_noop()
++        builder = self.make_merge_builder()
++        self.create_file_needing_contents_merge(builder, "1")
++        conflicts = builder.merge(self.merge_type)
++        self.assertEqual([('delete',)], self.hook_log)
 === added directory 'contrib/news_merge'
 === added file 'contrib/news_merge/README'
 --- contrib/news_merge/README	1970-01-01 00:00:00 +0000
 +++ contrib/news_merge/README	2010-01-13 07:31:30 +0000
@@ -0,0 +1,9 @@
++A plugin for merging bzr's NEWS file.
++
++Install as usual (e.g. symlink this directory into ~/.bazaar/plugins), and any
++file with a path of NEWS that is in bzr's NEWS format will be merged with this
++hook.
++
++This hook can resolve conflicts where both sides add entries at the same place.
++If it encounters a more difficult conflict it gives up and bzr will fallback to
++the default merge algorithm.
 === added file 'contrib/news_merge/__init__.py'
 --- contrib/news_merge/__init__.py	1970-01-01 00:00:00 +0000
 +++ contrib/news_merge/__init__.py	2010-01-13 07:31:30 +0000
@@ -0,0 +1,90 @@
++# Copyright (C) 2010 Canonical Ltd
++#
++# This program is free software; you can redistribute it and/or modify
++# it under the terms of the GNU General Public License as published by
++# the Free Software Foundation; either version 2 of the License, or
++# (at your option) any later version.
++#
++# This program is distributed in the hope that it will be useful,
++# but WITHOUT ANY WARRANTY; without even the implied warranty of
++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++# GNU General Public License for more details.
++#
++# You should have received a copy of the GNU General Public License
++# along with this program; if not, write to the Free Software
++# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
++
++"""Merge hook for bzr's NEWS file.
++
++Install this as a plugin, e.g:
++
++    cp contrib/news-file-merge-hook.py ~/.bazaar/plugins/news_merge.py
++"""
++
++from .parser import simple_parse
++
++from bzrlib.merge import Merger
++from bzrlib.merge3 import Merge3
++
++
++def news_merge_hook(params):
++    if params.winner == 'other':
++        # OTHER is a straight winner, rely on default merge.
++        return 'not_applicable', None
++    elif params.is_file_merge():
++        # THIS and OTHER are both files.  That's a good start.
++        filename = params.merger.this_tree.id2path(params.file_id)
++        if filename != 'NEWS':
++            return 'not_applicable', None
++        return news_merger(params)
++    else:
++        return 'not_applicable', None
++
++magic_marker = '|NEWS-MERGE-MAGIC-MARKER|'
++
++def blocks_to_fakelines(blocks):
++    for kind, text in blocks:
++        yield '%s%s%s' % (kind, magic_marker, text)
++
++def fakelines_to_lines(fakelines):
++    for fakeline in fakelines:
++        yield fakeline.split(magic_marker, 1)[1] + '\n'
++        yield '\n'
++
++def sort_key(s):
++    return s.replace('`', '').lower()
++
++def news_merger(params):
++    def munge(lines):
++        return list(blocks_to_fakelines(simple_parse(''.join(lines))))
++    this_lines = munge(params.this_lines)
++    other_lines = munge(params.other_lines)
++    base_lines = munge(params.base_lines)
++    m3 = Merge3(base_lines, this_lines, other_lines)
++    result_lines = []
++    for group in m3.merge_groups():
++        if group[0] == 'conflict':
++            _, base, a, b = group
++            # are all the conflicting lines bullets?  If so, we can merge this.
++            for line_set in [base, a, b]:
++                for line in line_set:
++                    if not line.startswith('bullet'):
++                        # Something else :(
++                        # Maybe the default merge can cope.
++                        return 'not_applicable', None
++            new_in_a = set(a).difference(base)
++            new_in_b = set(b).difference(base)
++            all_new = new_in_a.union(new_in_b)
++            deleted_in_a = set(base).difference(a)
++            deleted_in_b = set(base).difference(b)
++            final = all_new.difference(deleted_in_a).difference(deleted_in_b)
++            final = sorted(final, key=sort_key)
++            result_lines.extend(final)
++        else:
++            result_lines.extend(group[1])
++    return 'success', list(fakelines_to_lines(result_lines))
++
++
++Merger.hooks.install_named_hook(
++    'merge_file_content', news_merge_hook, 'NEWS file merge')
++
 === added file 'contrib/news_merge/parser.py'
 --- contrib/news_merge/parser.py	1970-01-01 00:00:00 +0000
 +++ contrib/news_merge/parser.py	2010-01-13 07:31:30 +0000
@@ -0,0 +1,33 @@
++
++
++def simple_parse(content):
++    """Returns blocks, where each block is a 2-tuple (kind, text)."""
++    blocks = content.split('\n\n')
++    for block in blocks:
++        if block.startswith('###'):
++            # First line is ###...: Top heading
++            yield 'heading', block
++            continue
++        last_line = block.rsplit('\n', 1)[-1]
++        if last_line.startswith('###'):
++            # last line is ###...: 2nd-level heading
++            yield 'release', block
++        elif last_line.startswith('***'):
++            # last line is ***...: 3rd-level heading
++            yield 'section', block
++        elif block.startswith('* '):
++            # bullet
++            yield 'bullet', block
++        elif block.strip() == '':
++            # empty
++            yield 'empty', block
++        else:
++            # plain text
++            yield 'text', block
++
++
++if __name__ == '__main__':
++    import sys
++    content = open(sys.argv[1], 'rb').read()
++    for result in simple_parse(content):
++        print result