[MERGE] Make log revision filtering pluggable.

[John Arbash Meinel]

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

Robert Collins wrote:
> On Tue, 2008-08-26 at 16:38 -0400, John Arbash Meinel wrote:
>> John Arbash Meinel has voted comment.
>> Status is now: Semi-approved
>> Comment:
>> I'm a bit concerned about your api. You seem to have an iterator of 
>> iterators, which is a bit confusing in its nesting.
> 
> This is for performance, to allow the current batching of revisions to
> be presented to the filters, rather than them all reinventing it, etc.

I do understand that. I also know that I'm fairly comfortable with the logging
code, generators and iterators and it still took me quite a while to figure
out what was going on. Especially stuff like the _convert one snippet.

Fundamentally, my concern is that this is rather hard to understand. A
"pipeline" isn't specifically tricky, but way everything is implemented is
pretty hard to follow.

For starters, the fundamental object is a nested tuple of:
  ((revision_id, revno, merge_depth), Revision_or_None, TreeDelta_or_None)

Which is already a bit odd. What is x[0][1]? We have LogRevision explicitly
because dealing in parameter tuples is difficult and hard to maintain
backwards compatibility. We *might* need to use tuples like this for
performance reasons, but I'm not strictly convinced we do. (Getting rid of
O(ancestry) merge_sorted would do far more for us than switching to tuples.)

Second you use terms like: "log_rev_iterator" but it isn't iterating
revisions. It is iterating iterators of tuples which may have a "rev" in them
somewhere. The fact that the previous sentence is difficult signals to me that
the api is difficult to understand.



...

>>
>> So... I really like it being a chain of filters. But I wonder if we 
>> would be a lot better served by having a chain of Classes rather than 
>> plain functions. At least at the moment, I have difficulty following 
>> what is really going on. I can sort of tease it out, but it doesn't 
>> really pass my "simplicity" filter.
> 
> How would a chain of classes work any different? This is quite literally
> exactly what we had for log before (though it was a bit inconsistent
> before - some levels were single rev, some were multiple).

Classes have significantly more obvious state than generators. You could
define them as:

class Filter(object):

  def __init__(self, **kwargs):
    ...

  def filter(self, log_revisions_iterators):
    """..."""
    for log_rev_iterator in log_revisions_iterators:
      for log_rev in log_rev_iterator:
	yield log_rev


Which would allow you to put things like "generate_deltas" in as a "**kwargs"
parameter, rather than as parameters for the function call itself. Arguably
you could make the extra arguments "**kwargs" in the same fashion.

I'm not strictly against the model, but it feels brittle and complex.

> 
>> Oh, and if "log_adapters" is going to be non-stable, I think it should 
>> be "_log_adapters" for now.
> 
> _log_adapters would be private. This is public, but not stable.
> 
> -Rob
> 

I know we are at a bit of a lack of having enough ways to decorate a symbol to
indicate its public/private status versus its stable/unstable status. But
generally we say that "public apis are stable and will be deprecated, etc." So
I think making it "_log_adapters" is perfectly valid, as we aren't
guaranteeing to plugins not to change the api without deprecating.

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

iD8DBQFItH+BJdeBCYSNAAMRAtJ4AJ9YqT7VBP/NxsSjpV4vi8cIbkq+9wCdFoLj
bw8lKOfbznqF9ZOoX24U71k=
=BTBw
-----END PGP SIGNATURE-----