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

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.