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

Reviewer	Date Requested	Status
John A Meinel	2010-01-13	Approve on 2010-01-18
Vincent Ladeuil		Approve on 2010-01-18
Review via email: mp+17279@code.launchpad.net

Revision history for this message

Andrew Bennetts (spiv) wrote on 2009-12-18: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

-----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: Posted in a previous version of this proposal

#

-----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: Posted in a previous version of this proposal

#

-----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: Posted in a previous version of this proposal

#

-----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: Posted in a previous version of this proposal

#

-----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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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: Posted in a previous version of this proposal

#

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

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-01-13:

#

This addresses most (all?) of the points raised in the original reviews.

Major changes:

  * Added 'news_merge' plugin to contrib/! This is an example plugin using the merge hook function so that you can see it in action, and so that users will have some inspiration for writing their own.
  * The built-in merge logic is now invoked the same way as user-registered hook functions. So in theory anything the built-in logic can do, a plugin can do too.
  * the hook point is invoked a bit earlier, so it can have a say in more kinds of conflicts by inspecting the 'winner' attribute.
  * added some convenience methods/properties to the params object.
  * generally simpler and more linear code path than lp:bzr, thanks to refactorings for above changes.

I think looking at the news_merge plugin the API is still a bit cumbersome (note the repetition of logic vs. the built-in text merge to see if this is an ordinary regular file, both sides changed merge). But having an example proves it works, and gives us good data for thinking of improvements.

Revision history for this message

Martin Pool (mbp) wrote on 2010-01-13:

#

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

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

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

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

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

Revision history for this message

Robert Collins (lifeless) wrote on 2010-01-13:

#

On Wed, 2010-01-13 at 07:54 +0000, Martin Pool wrote:
>
> Therefore maybe you should put this in bzrlib.plugins but arrange that
> it has no effect (and little load time overhead) unless it's
> configured on?

+1000

-Rob

Revision history for this message

John A Meinel (jameinel) wrote on 2010-01-13:

#

Download full text (6.1 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:
> John A Meinel (jameinel)
> Related bugs:
> #491711 hook to permit per file merges
> https://bugs.launchpad.net/bugs/491711
>
>

I think overall this is looking good. Looking over the actual diff
brought up a lot of questions about how various things hook together. I
don't know that everything needs to be addressed before merging, but I
think it would be good to at least understand what is going on.

review: needsfixing

+class MergeHookParams(object):
+ """Object holding parameters passed to merge_file_content hooks.
+
+ There are 3 fields hooks can access:
+

What about "this_pair", "other_pair", etc. I don't really understand
what the "pair" is. this_pair[0] looks to be a Kind. Also, you should
probably document "base_lines", "this_lines", and "other_lines" here as
well. The current documentation assumes you'll go to the merger, but
you've already provided helpers for that.

base_lines, this_lines and other_lines doesn't map very cleanly into
WeaveMerge... But maybe it is good enough.

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

^- Are we sure this is correct? If there is a contents_conflict we mark
the file as unversioned?

Also, you are using "self.this_tree.inventory" but later we use "file_id
in self.this_tree".

I'd probably rather use "self.this_tree.has_id(file_id)" in both cases.
Trees shouldn't really have a __contains__ method, and I'd like to avoid
using ".inventory" if we can. (Most Dirstate actions can be resolved
without building an inventory at all.)

...

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

^- Do you know why we are appending the trans_id here? file_group isn't
mentioned again. So is this mutating an internal structure behind the
scenes? (_dump_conflicts is creating a list and storing it in state
somewhere, and we mutate it to add the new id.) If so, a comment would
be nice. (Probably not your code, but certainly a "what is going on?"
moment.)

...

+ try:...

-----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:
>     John A Meinel (jameinel)
> Related bugs:
>   #491711 hook to permit per file merges
>   https://bugs.launchpad.net/bugs/491711
> 
>

I think overall this is looking good. Looking over the actual diff
brought up a lot of questions about how various things hook together. I
don't know that everything needs to be addressed before merging, but I
think it would be good to at least understand what is going on.

review: needsfixing

+class MergeHookParams(object):
+    """Object holding parameters passed to merge_file_content hooks.
+
+    There are 3 fields hooks can access:
+

What about "this_pair", "other_pair", etc. I don't really understand
what the "pair" is. this_pair[0] looks to be a Kind. Also, you should
probably document "base_lines", "this_lines", and "other_lines" here as
well. The current documentation assumes you'll go to the merger, but
you've already provided helpers for that.

base_lines, this_lines and other_lines doesn't map very cleanly into
WeaveMerge... But maybe it is good enough.

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

^- Are we sure this is correct? If there is a contents_conflict we mark
the file as unversioned?

Also, you are using "self.this_tree.inventory" but later we use "file_id
in self.this_tree".

I'd probably rather use "self.this_tree.has_id(file_id)" in both cases.
Trees shouldn't really have a __contains__ method, and I'd like to avoid
using ".inventory" if we can. (Most Dirstate actions can be resolved
without building an inventory at all.)

...

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

^- Do you know why we are appending the trans_id here? file_group isn't
mentioned again. So is this mutating an internal structure behind the
scenes? (_dump_conflicts is creating a list and storing it in state
somewhere, and we mutate it to add the new id.) If so, a comment would
be nice. (Probably not your code, but certainly a "what is going on?"
moment.)

...

+        try:
+            self.tt.tree_kind(trans_id)
+            self.tt.delete_contents(trans_id)
+        except errors.NoSuchFile:
+            pass

^- I don't quite understand the unconditional "delete_contents" if the
contents exist. Especially given that:

+        elif hook_status == 'success':
+            self.tt.create_file(lines, trans_id)

Is called *before* this.

Is it that TreeTransform.delete_contents() indicates we want to delete
the content in the current tree, and "create_file()" is creating
contents in the "next" tree?

A comment of this might clarify:
  # Now get rid of the contents in the working tree, so that any new
  # content (even conflicts) has a place to go.
...

+    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

^- Something seems missing if we are doing this only in this code path.
What about all the other code paths that will be creating file contents?
How do the MergeHooks interact with content filtering?

...

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

^- This is no longer true, since you changed it to "news_merge/__init__.py".

+from .parser import simple_parse

^- ????

I'm guessing ".parser" is python 2.6 syntax, but this certainly isn't
2.4 syntax.

Given the discussion, I would probably move this into bzrlib/plugins/*
and change the trigger to look at a config variable to determine whether
it is active.

Which makes me think that MergeHookParams is incomplete if we don't have
semi-easy access to what Branch/Tree this hook is being run on. I
suppose we can use "params.merger.this_tree.branch.get_config()" ?

How does this interact with "bzr merge --preview".

I would also move the bulk of the code into a separate file that is only
imported with the merge hook is actually triggered.

John
=:->

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

iEYEARECAAYFAktN2wkACgkQJdeBCYSNAAMgogCg0E9vADofwKNCGhQCP+2tYDoJ
rZ0An1N7fJmP95juTSb396tYwc8rOk4D
=uyn2
-----END PGP SIGNATURE-----

review: Needs Fixing

Revision history for this message

Martin Pool (mbp) wrote on 2010-01-14:

#

As a follow on (after merging) it would be good to

* announce this on udd
* see if there is some existing code for merging debian changelogs and merge that

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-01-15:

#

Download full text (9.4 KiB)

John A Meinel wrote:
> Review: Needs Fixing
> -----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:
> > John A Meinel (jameinel)
> > Related bugs:
> > #491711 hook to permit per file merges
> > https://bugs.launchpad.net/bugs/491711
> >
> >
>
> I think overall this is looking good. Looking over the actual diff
> brought up a lot of questions about how various things hook together. I
> don't know that everything needs to be addressed before merging, but I
> think it would be good to at least understand what is going on.

Fair enough. There are parts I don't fully understand, but those parts
I'm pretty sure are working the same as in trunk (and are equally
confusing in trunk).

Thanks for the review!

> +class MergeHookParams(object):
> + """Object holding parameters passed to merge_file_content hooks.
> +
> + There are 3 fields hooks can access:
> +
>
> What about "this_pair", "other_pair", etc. I don't really understand
> what the "pair" is. this_pair[0] looks to be a Kind. Also, you should

Yeah, it is a kind. This was a refactoring that I should have taken a
little further by just passing the kinds rather than the “pairs”. I've
done that now, and added this_kind and other_kind to the class
docstring.

> probably document "base_lines", "this_lines", and "other_lines" here as
> well. The current documentation assumes you'll go to the merger, but
> you've already provided helpers for that.

I've removed the outdated comment that suggests using the merger for
this.

The *_lines properties are already documented, though, they have their
own docstrings. Unless you think they should be mentioned in the class
docstring too, but I didn't think that was our usual practice?

> base_lines, this_lines and other_lines doesn't map very cleanly into
> WeaveMerge... But maybe it is good enough.

Yeah, I admit to being unsure about what to do with the non-default
Merger classes. I guess for now we leave it up to the plugins to check
the type of Merger if they are sensitive to it? I would like to give
plugins the chance to use their own hooks even in the case of
WeaveMerge.

> + 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))
>
> ^- Are we sure this is correct? If there is a contents_conflict we mark
> the file as unversioned?

I'm not really sure why, but this is what lp:bzr does in these cases.
So this isn't an issue introduced by this branch.

> Also, you are u...

John A Meinel wrote:
> Review: Needs Fixing
> -----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:
> >     John A Meinel (jameinel)
> > Related bugs:
> >   #491711 hook to permit per file merges
> >   https://bugs.launchpad.net/bugs/491711
> > 
> > 
> 
> I think overall this is looking good. Looking over the actual diff
> brought up a lot of questions about how various things hook together. I
> don't know that everything needs to be addressed before merging, but I
> think it would be good to at least understand what is going on.

Fair enough.  There are parts I don't fully understand, but those parts
I'm pretty sure are working the same as in trunk (and are equally
confusing in trunk).

Thanks for the review!

> +class MergeHookParams(object):
> +    """Object holding parameters passed to merge_file_content hooks.
> +
> +    There are 3 fields hooks can access:
> +
> 
> What about "this_pair", "other_pair", etc. I don't really understand
> what the "pair" is. this_pair[0] looks to be a Kind. Also, you should

Yeah, it is a kind.  This was a refactoring that I should have taken a
little further by just passing the kinds rather than the “pairs”.  I've
done that now, and added this_kind and other_kind to the class
docstring.

> probably document "base_lines", "this_lines", and "other_lines" here as
> well. The current documentation assumes you'll go to the merger, but
> you've already provided helpers for that.

I've removed the outdated comment that suggests using the merger for
this.

The *_lines properties are already documented, though, they have their
own docstrings.  Unless you think they should be mentioned in the class
docstring too, but I didn't think that was our usual practice?

> base_lines, this_lines and other_lines doesn't map very cleanly into
> WeaveMerge... But maybe it is good enough.

Yeah, I admit to being unsure about what to do with the non-default
Merger classes.  I guess for now we leave it up to the plugins to check
the type of Merger if they are sensitive to it?  I would like to give
plugins the chance to use their own hooks even in the case of
WeaveMerge.

> +        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))
> 
> ^- Are we sure this is correct? If there is a contents_conflict we mark
> the file as unversioned?

I'm not really sure why, but this is what lp:bzr does in these cases.
So this isn't an issue introduced by this branch.

> Also, you are using "self.this_tree.inventory" but later we use "file_id
> in self.this_tree".
> 
> I'd probably rather use "self.this_tree.has_id(file_id)" in both cases.
> Trees shouldn't really have a __contains__ method, and I'd like to avoid
> using ".inventory" if we can. (Most Dirstate actions can be resolved
> without building an inventory at all.)

Fair enough.  Again, I think this was probably an issue before this
branch, but this at least is something I can improve with confidence in
this one :)

I've gone ahead and changed this everywhere in this file, even places I
hadn't previously touched.

> +        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)
> 
> ^- Do you know why we are appending the trans_id here? file_group isn't
> mentioned again. So is this mutating an internal structure behind the
> scenes? (_dump_conflicts is creating a list and storing it in state
> somewhere, and we mutate it to add the new id.) If so, a comment would
> be nice. (Probably not your code, but certainly a "what is going on?"
> moment.)

Hmm.  Again, this is something that lp:bzr does that I've
preserved/copied.  On closer inspection, I don't think it has any effect
(either in lp:bzr or in my branch), so I'll just delete that.

> +        try:
> +            self.tt.tree_kind(trans_id)
> +            self.tt.delete_contents(trans_id)
> +        except errors.NoSuchFile:
> +            pass
> 
> ^- I don't quite understand the unconditional "delete_contents" if the
> contents exist. Especially given that:
> 
> +        elif hook_status == 'success':
> +            self.tt.create_file(lines, trans_id)
> 
> Is called *before* this.

Yes, this certainly puzzled me when I came across it in trunk!  (Again,
this oddity is definitely present in trunk, just that my patch has
perhaps made it more visible.)

> Is it that TreeTransform.delete_contents() indicates we want to delete
> the content in the current tree, and "create_file()" is creating
> contents in the "next" tree?

Exactly how TreeTransform isn't clearly documented that I could find.
If someone that knows precisely what the operations on it means and
could document that it would be a great help, I think.  AFAICS there
isn't even a description of what a trans_id is... is an abbreviation of
“transform id” or “transaction id”, are they file_ids, or something else
but like file_ids?  What's the difference between unversion_file()
and delete_contents()?  etc.

So, for this specific puzzle...

The code I'm guessing is there to ensure that the file is marked as
versioned, but that the temporary file used for the new contents is
removed?  Staring again at the treetransform.py code isn't bringing me
enlightenment.

> A comment of this might clarify:
>   # Now get rid of the contents in the working tree, so that any new
>   # content (even conflicts) has a place to go.
> ...

I really wish I knew if that was right.

All I'm certain of is that I'm doing the same as trunk in this case.

> +    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
> 
> ^- Something seems missing if we are doing this only in this code path.
> What about all the other code paths that will be creating file contents?
> How do the MergeHooks interact with content filtering?

That's a good question!  I don't know, but it's interesting that even in
trunk I only see content filtering involved in this case (OTHER is
winner) and when generating conflicts.  When both sides have modified
the file no content filtering appears to be involved, which doesn't
sound right.

So I *think* my patch doesn't make this worse, unless a plugin decides
to perform the merge for the winner == 'other' case.

[...]
> +"""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
> +"""
> 
> ^- This is no longer true, since you changed it to "news_merge/__init__.py".
> 
> +from .parser import simple_parse
> 
> ^- ????
> 
> I'm guessing ".parser" is python 2.6 syntax, but this certainly isn't
> 2.4 syntax.

Right.  Fixed.

> Given the discussion, I would probably move this into bzrlib/plugins/*
> and change the trigger to look at a config variable to determine whether
> it is active.

I haven't done this yet, but will soon.

> Which makes me think that MergeHookParams is incomplete if we don't have
> semi-easy access to what Branch/Tree this hook is being run on. I
> suppose we can use "params.merger.this_tree.branch.get_config()" ?

I suppose so... I'll find out when I move the plugin to bzrlib.plugins I
guess! :)

> How does this interact with "bzr merge --preview".

It seems to work as I expect: any registered hook, such as news_merge's,
is used.  Do you have some specific concerns in mind?

> I would also move the bulk of the code into a separate file that is only
> imported with the merge hook is actually triggered.

I haven't yet moved the code, although there isn't much of it, but I
have added some lazy_import magic.

-Andrew.

Revision history for this message

John A Meinel (jameinel) wrote on 2010-01-15:

#

Download full text (7.7 KiB)

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

...

>> + try:
>> + self.tt.tree_kind(trans_id)
>> + self.tt.delete_contents(trans_id)
>> + except errors.NoSuchFile:
>> + pass
>>
>> ^- I don't quite understand the unconditional "delete_contents" if the
>> contents exist. Especially given that:
>>
>> + elif hook_status == 'success':
>> + self.tt.create_file(lines, trans_id)
>>
>> Is called *before* this.
>
> Yes, this certainly puzzled me when I came across it in trunk! (Again,
> this oddity is definitely present in trunk, just that my patch has
> perhaps made it more visible.)
>
>> Is it that TreeTransform.delete_contents() indicates we want to delete
>> the content in the current tree, and "create_file()" is creating
>> contents in the "next" tree?
>
> Exactly how TreeTransform isn't clearly documented that I could find.
> If someone that knows precisely what the operations on it means and
> could document that it would be a great help, I think. AFAICS there
> isn't even a description of what a trans_id is... is an abbreviation of
> “transform id” or “transaction id”, are they file_ids, or something else
> but like file_ids? What's the difference between unversion_file()
> and delete_contents()? etc.

I can help with this.

A trans_id is a handle for the TreeTransform. Similar in concept to a
file_id mapping to a path in an inventory. However, TreeTransform can
talk about files that aren't versioned (have no trans_id), and multiple
copies of the same logical file (two people introduce the same file_id
at different locations, etc). As such, trans_id are an abstraction of a
specific file/directory content during the transaction. AFAICT they are
always of the form "new-%d".

unversion_file is saying that we need to remove the file_id from the
inventory.
delete_contents says we want to call 'os.remove()' on the object.

Note that you don't get a new trans_id for content being removed versus
content being added. I checked and _removed_contents indicates files
that are to be deleted from the wt, but you can *also* have the same
trans_id in _new_contents. So I think trans_id is like file_id, in that
it is how TT maps this file should be replacing that file.

I think part of it is that TT doesn't think about replacements. It only
has a remove phase and an add phase.

1) All new content is staged into .bzr/checkout/limbo, generally with
names like 'new-1'. Though we do try to create files in their final
directories with their final names if we know in advance. (So we don't
rename all files, just the containing dirs for 'bzr co'.)

2) All old files are renamed into .bzr/checkout/pending_deletion

3) All new files are renamed from .bzr/checkout/limbo into the working tree

4) All old files in .bzr/checkout/pending_deletion are removed

That is how we get atomicity of updates, so that if something fails in
(3) we can still revert back to the original tree.

>
> So, for this specific puzzle...
>
> The code I'm guessing is there to ensure that the file is marked as
> versioned, but that the temporary file used for the new contents is
> removed? Staring again at the treetransform.py...

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

...

>> +        try:
>> +            self.tt.tree_kind(trans_id)
>> +            self.tt.delete_contents(trans_id)
>> +        except errors.NoSuchFile:
>> +            pass
>>
>> ^- I don't quite understand the unconditional "delete_contents" if the
>> contents exist. Especially given that:
>>
>> +        elif hook_status == 'success':
>> +            self.tt.create_file(lines, trans_id)
>>
>> Is called *before* this.
> 
> Yes, this certainly puzzled me when I came across it in trunk!  (Again,
> this oddity is definitely present in trunk, just that my patch has
> perhaps made it more visible.)
> 
>> Is it that TreeTransform.delete_contents() indicates we want to delete
>> the content in the current tree, and "create_file()" is creating
>> contents in the "next" tree?
> 
> Exactly how TreeTransform isn't clearly documented that I could find.
> If someone that knows precisely what the operations on it means and
> could document that it would be a great help, I think.  AFAICS there
> isn't even a description of what a trans_id is... is an abbreviation of
> “transform id” or “transaction id”, are they file_ids, or something else
> but like file_ids?  What's the difference between unversion_file()
> and delete_contents()?  etc.