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

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.

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

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

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

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

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

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

Andrew Bennetts wrote:
>> ^- Are we sure this is correct? If there is a contents_conflict we mark
>> the file as unversioned?

That's correct. For a contents conflict that is not a text conflict,
bzr does not leave a file in place with the original filename, therefore
we also unversion it, and version the .OTHER, .THIS or .BASE--
whatever's there.

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

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

This code doesn't appear to have ever made sense. It was introduced in
<email address hidden>. I
suspect it was meant to help with treating a file and its conflict files
as a group for conflict handling.

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

Operations on a TreeTransform simply schedule things to happen when you
call 'apply'. Before and after are irrelevant (except when you cancel
an operation you had previously scheduled). All that matters is that
the tree transform is consistent when you call apply.

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

delete_contents means that applying the transform should delete these
contents, and create_file means that applying the transform should
create a file with specified contents.

> Exactly how TreeTransform isn't clearly documented that I could find.

How it what? Works?

> If someone that knows precisely what the operations on it means and
> could document that it would be a great help, I think.

I've certainly tried in the past, and I'm happy to enhance the
documentation. Could you start by explaining what is confusing or
missing from the TreeTransform docstring?

> AFAICS there
> isn't even a description of what a trans_id is...

There are three paragraphs of description:

> Transform/Transaction ids
> -------------------------
> trans_ids are temporary ids assigned to all files involved in a transform.
> It's possible, even common, that not all ...

Aaron Bentley wrote:
[...]
> > Exactly how TreeTransform isn't clearly documented that I could find.
>
> How it what? Works?

Yes, that's the missing word. I'm glad your telepathy is functioning
well :)

> > If someone that knows precisely what the operations on it means and
> > could document that it would be a great help, I think.
>
> I've certainly tried in the past, and I'm happy to enhance the
> documentation. Could you start by explaining what is confusing or
> missing from the TreeTransform docstring?

Simply that I didn't find it when I looked for it, believe or not! I
looked for a class docstring on the class that implemented these methods
and instance variables (TreeTransformBase), and then looked for a module
docstring, and then tried grepping doc/developers/. It didn't occur to
me to try looking a couple of classes (and 1000 lines) further down
transform.py (and a couple of classes further down the inheritance
hierarchy too) to see if one of those had introductory documentation.

This is the sort of situation where I miss dedicated “interface”
documentation, I guess.

Thanks for pointing me to that, I thought I'd seen it before but several
minutes of solid searching failed to find it for me! I expect I'll
remember it for next time.

[...]
> is an abbreviation of
> > “transform id” or “transaction id”
>
> Both, as you can see above.

[Eep! Ambiguity of names doesn't help clarity. I realise it's basically
an arbitrary choice, but I suggest picking one. Actually, it's also
that neither name is particuarly satisfying: one of these IDs doesn't
correspond to a single tree transform (which is done as a single
transaction), it corresponds to an element of that transform. So, I'd
probably find “tentry_id” (short for “transform entry ID”) clear and
mnemonic... but it's a bit late to change now :)
]

> >, are they file_ids, or something else
> > but like file_ids?
>
>
> > What's the difference between unversion_file()
> > and delete_contents()?
>
> One schedules the deletion of the contents of a file. The other
> schedules removes the removal of file id of the file. The docstring for
> delete_contents is:
>
> > """Schedule the contents of a path entry for deletion"""
>
> The docstring for unversion_file is:
> > """Schedule a path entry to become unversioned"""
>
> If this is unclear, could you say what about it is unclear?

First I should say I had an intuition about the meanings of these, and I
think my intuition was basically correct. So it's not so bad.

It's unclear for a few reasons:

 * the docstrings don't really add information not already in the method
   name,
 * my confidence in my intuition about the names was low because the
   terms were so close to synonymous, and
 * the terminology is not memorably clear.

By “memorably clear”, I mean that terms like “delete” and “unversion”
are close to synonymous, so the distinction is hard to notice and
recall.

I don't mean say you necessarily should have found better names, maybe
there are no better names for these concepts... none spring to mind for
me! But (assuming I had found the TreeTransform docstring in the first
place) some explicit descri...

John A Meinel wrote:
[...]
> Though it also means that calling:
> self.tt.tree_kind()
> self.tt.delete_contents()
>
> is redundant and double statting the file. So we should clean that up

Yeah, I'd noticed that, but wasn't sure if it was tasteful to clean that
up. Seeing as you think we should I've gone ahead and removed that line
:)

[...]
> So the comment looks correct to my understanding.

I've added a comment based on Aaron's wording.

[...]
> If I set up location.conf to say:
> [/home/jameinel/dev/bzr]
> news_merger_files = NEWS
> news_merge_files:policy = recurse

(Thanks for this, this is exactly the syntax I've used for the
news_merge plugin configuration)

> Will that match when 'bzr merge --preview' is invoked (what Branch is
> given to the PreviewTree, etc.) I'm not 100% sure that PreviewTree *has*
> a .branch property. (Given that RevisionTree does not, but has a
> _repository attribute. Even better it is passed in as 'branch'... :)

It turned out I had to pass the branch a little deeper in all cases, but
having done that 'merge --preview' works just as well as regular 'merge'
:)

[...]
> It is probably reasonable to merge soon. I'd probably like to see the
> news plugin in 'plugins' first, but that is not critical.

I've done that anyway. Please try it out! :)

-Andrew.

I reviewed this chronologically (reading oldest revision first)
and I found it was far easier to understand what was going
on. I'm not sure that trick could be used for all reviews but it
definetly made that one a nice tour :)

Keep that in mind when reading the comments below as they may
seem out of context otherwise.

I had a moderately good understanding of the merge code and I
find that your refactoring makes it far clearer, thanks for that
!

32 === modified file 'bzrlib/decorators.py'
33 --- bzrlib/decorators.py 2009-10-15 02:19:43 +0000
34 +++ bzrlib/decorators.py 2010-01-18 08:00:37 +0000
35 @@ -253,3 +253,19 @@
36 global needs_read_lock, needs_write_lock
37 needs_read_lock = _pretty_needs_read_lock
38 needs_write_lock = _pretty_needs_write_lock
39 +
40 +
41 +def cachedproperty(getter_func):
42 + """Like property(getter_func), but caches the value.
43 +
44 + Note: there's no way to clear or reset the cache.
45 + """
46 + memo = []

AIUI, this is gc'ed when no more references exist on the cached
value. It's not obvious and may be worth documenting.

Also, this can be used in a lot of places in the codebase so
that's worth a NEWS entry isn't it ?

And last remark on that subject, can't use some unique object()
to initialize the cache instead of a list for each property ?
Using a list here is cute but a bit hackish, using a dedicated
object to represent the 'not cached yet' could make the code more
obvious to the casual reader.

The hook scope is a bit unclear regarding the tt updates (spotted
when reading revno 4887). Why don't you let the hooks do them (in
merge.merge_contents()) ? Shouldn't that be at least documented
in the hook docstring ?

I had to read the code to verify that is_file_merge() was indeed
is_text_merge(), is it just me or should it be renamed ? Ha,
fixed in revno 4890 :) Never mind then.

248 + params = MergeHookParams(self, file_id, trans_id, this_pair[0],
249 + other_pair[0], winner)

Using this_kind= this_pair[0] and other_kind=other_pair[0] will
add a final touch of clarity to revno 4894 :)

Overall, nice job and reading the history of how it was put together
was really a big help to understand it.

I'll be happy to land this for 2.1 unless John or Aaron have more objections
until tomorrow.

Addressing the points I raised above with be nice but not mandatory for landing.

Vincent Ladeuil wrote:
[...]
> 41 +def cachedproperty(getter_func):
> 42 + """Like property(getter_func), but caches the value.
> 43 +
> 44 + Note: there's no way to clear or reset the cache.
> 45 + """
> 46 + memo = []
>
>
> AIUI, this is gc'ed when no more references exist on the cached
> value. It's not obvious and may be worth documenting.

Erk, this is totally bogus. I'm not sure what I was thinking! I was
trying for a limited quick & dirty cachedproperty decorator without the
complexity of the one in Launchpad, but I think I should just borrow
Launchpad's, so I've now done that (including arranging for its doctests
to be run).

> Also, this can be used in a lot of places in the codebase so
> that's worth a NEWS entry isn't it ?

Good idea. Done.

> The hook scope is a bit unclear regarding the tt updates (spotted
> when reading revno 4887). Why don't you let the hooks do them (in
> merge.merge_contents()) ? Shouldn't that be at least documented
> in the hook docstring ?

I'm not sure what you mean here? You mean let the hook do the
self.tt.version_file and self.tt.delete_contents calls that happen at
the end of merge_contents?

I guess I'm still not certain exactly how much the hook should be
allowed to do... the more plugins people try to write for it, the more
we'll know :)

[...]
> I'll be happy to land this for 2.1 unless John or Aaron have more objections
> until tomorrow.
>
> Addressing the points I raised above with be nice but not mandatory for landing.

That's very much for the extra review.

-Andrew.

>>>>> "Andrew" == Andrew Bennetts <email address hidden> writes:

    Andrew> Vincent Ladeuil wrote:
    Andrew> [...]
    >> 41 +def cachedproperty(getter_func):
    >> 42 + """Like property(getter_func), but caches the value.
    >> 43 +
    >> 44 + Note: there's no way to clear or reset the cache.
    >> 45 + """
    >> 46 + memo = []
    >>
    >>
    >> AIUI, this is gc'ed when no more references exist on the cached
    >> value. It's not obvious and may be worth documenting.

    Andrew> Erk, this is totally bogus.

Really ? How so ?

<snip/>

    >> The hook scope is a bit unclear regarding the tt updates
    >> (spotted when reading revno 4887). Why don't you let the
    >> hooks do them (in merge.merge_contents()) ? Shouldn't
    >> that be at least documented in the hook docstring ?

    Andrew> I'm not sure what you mean here? You mean let the
    Andrew> hook do the self.tt.version_file and
    Andrew> self.tt.delete_contents calls that happen at the end
    Andrew> of merge_contents?

Yes or let the caller do it but document it anyway.

    Andrew> I guess I'm still not certain exactly how much the
    Andrew> hook should be allowed to do... the more plugins
    Andrew> people try to write for it, the more we'll know :)

Ok, no problem, I'm not sure either. It's just that it seems odd
to *forbid* the hook to do some stuff a bit different if
needed. But there is no point forcing people to do it
unconditionally either. Time will tell.

I'm happy enough with what is possible today anyway.

One problem I potentially see with the plugin is this:
585 +def filename_matches_config(params):
586 + config = params.merger.this_branch.get_config()
587 + affected_files = config.get_user_option_as_list('news_merge_files')
588 + if affected_files:
589 + filename = params.merger.this_tree.id2path(params.file_id)
590 + if filename in affected_files:
591 + return True
592 + return False

The issue is that "branch.get_config()" is not cached, and re-reads all config files. And you trigger this for *all* modified files. So if you are merging, say, bzr.dev after 20 revs, you'll get 50 changed files, and we will re-read branch.conf, bazaar.conf and locations.conf 50 times.

I wonder if we should have a place on MergeHookParams to store hook data...

Otherwise I like this revision.

On Mon, 2010-01-18 at 11:06 +0000, Andrew Bennetts wrote:

> > AIUI, this is gc'ed when no more references exist on the cached
> > value. It's not obvious and may be worth documenting.
>
> Erk, this is totally bogus. I'm not sure what I was thinking! I was
> trying for a limited quick & dirty cachedproperty decorator without the
> complexity of the one in Launchpad, but I think I should just borrow
> Launchpad's, so I've now done that (including arranging for its doctests
> to be run).

You'll need to get an ack to rerelease the LP code to do this: Launchpad
is AGPL, which is incompatible with GPL.

I agree with John that we need to avoid processing branch.conf per-file.

-Rob

> On Mon, 2010-01-18 at 11:06 +0000, Andrew Bennetts wrote:
>

> You'll need to get an ack to rerelease the LP code to do this: Launchpad
> is AGPL, which is incompatible with GPL.

I've got the ack from flacoste.

>
> I agree with John that we need to avoid processing branch.conf per-file.

spiv proposed to cache the config in the plugin, I'll tweak and merge.

We may want to discuss caching the config in the branch itself
(I can't find scenarios where a locked branch could have trouble with that)
but that's outside the scope of this plugin.