Code review comment for lp:~jameinel/bzr/1.17-rework-annotate

Review: Approve

Good to see more tests in that area !

There is little to comment on given the detailed cover letter, I
like the cleanup (to come ? :) in annotate and the introduction
of the Annotator class, but since you intend to build policy
classes as front-end, why not make the class private until you
feel more confident about the overall API ? Like you, I'm not
sure the GUIs will really need to access that class...

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> You have been requested to review the proposed merge of
    jam> lp:~jameinel/bzr/1.17-rework-annotate into lp:bzr.

    jam> This is a fairly major overhaul of the annotate code,
    jam> with an eye on improving annotations overall. In the
    jam> short term, it just makes it faster (~9s => ~7s for
    jam> NEWS)

Which is always good to take (for interested readers that's still
with using --show-ids).

    jam> Overview of changes

    jam> 1) Some small tweaks to BranchBuilder that I needed to
    jam> write some test cases.

Good, obviously the tests you modified are clearer too.

    jam> 2) Changing from a bunch of loose functions in
    jam> bzrlib.annotate.reannotate*' to a class Annotator.

Good step forward.

A couple of comments on the class API:

- _update_needed_children() and _get_needed_keys() sounds like
  good candidates for Graph() or some specialization of it.

- _update_from_one_parent() the doc string says first parent, why
  not call it _update_from_first_parent() then ? Unless you envision
  some other possible usage...

- add_special_text(), hmm, what's that ? The doc string doesn't
  help a lot :-) Does that need to be public ?

    jam> 4) Implement a pyrex version of Annotator, to handle some
    jam> inner-loop functionality. (Nicely enough, you can still subclass
    jam> from it.)

It would be nice to defines in pyrex *only* those inner-loops,
not a requirement to land that patch tough.

<snip/>

    jam> Still to do:
    jam> 1) I'd like to break it up a bit more, and allow you to pass some
    jam> sort of Policy object into the code. That would let you do things
    jam> like ignore whitespace changes, only annotate based on mainline
    jam> changes, etc.

    jam> 2) Support _break_annotation_tie again. This is really something
    jam> I'd like to fold into Policy, but I guess MySQL wanted a custom
    jam> implementation. I don't really like the current api, as it is
    jam> probably fixed at 2 revisions, and it passes in the lines along
    jam> with the revisions. But I can certainly adapt what I need to the
    jam> old api. Note that technically the api supports > 2, but I doubt
    jam> that is actually supported anywhere, but I haven't seen the MySQL
    jam> implementation as a comparison point.

Pretty much:

- extract the date from the revids of the annotations (only the
  first two ones),
- return the oldest

It would be very appreciated to not break the actual result or at
the very least provides a way to get the same functionality
before 1.17 is out.

    jam> 3) GUI support. I don't really know how to expose the Annotator
    jam> functionality to a GUI, or if the api really works well there
    jam> until I've actually written some code. However, this patch has
    jam> gotten too big already, so I'd like to get it reviewed as is.

There is already work to be done in the two major GUIs (bzr-gtk
and qbzr) regarding annotations, the API we provide is not enough
to keep both code bases as simple as they should. So I'd say, as
long as you don't break the actual one, feel free to propose a
new enhanced one ;).

I'd be happy to test any proposal in bzr-gtk (which I know better
than qbzr) and I had discussions on that precise point with Gary
regarding qbzr so I think he'll be interested too.

A couple of nits below.

    jam> === added file 'bzrlib/_annotator_py.py'
...
    jam> + def _record_annotation(self, key, parent_keys, annotations):
...
    jam> + the_heads = heads(annotation)
    jam> + if len(the_heads) == 1:
    jam> + for head in the_heads:
    jam> + break

That's a really funny way to write head = heads[0]... Do I miss
something ?

<snip/>

    jam> === added file 'bzrlib/_annotator_pyx.pyx'
...
    jam> +class Annotator:
...
    jam> + def _update_needed_children(self, key, parent_keys):
    jam> + for parent_key in parent_keys:
    jam> + if parent_key in self._num_needed_children:
    jam> + self._num_needed_children[parent_key] += 1

+=1 ? Hmm, I like that pyrex version you're using, send me some :)