[MERGE][0.13] Dynamic fulltext

[John Arbash Meinel]

Richard Wilbur wrote:

> John,
> 
> Your patch sounds like really good stuff!  As this has already been
> merged I thought I'd spare the mailing list this comment, in case it
> turns out to be noise.

No problem. I'll go ahead and reply on list, though.

> 
> I'm curious about the way the code reads in one spot as it seems
> counter-intuitive to me.
> 
> [...]     
>> +    def _check_should_delta(self, first_parents):
>> +        """Iterate back through the parent listing, looking for a fulltext.
>> +
>> +        This is used when we want to decide whether to add a delta or a new
>> +        fulltext. It searches for _max_delta_chain parents. When it finds a
>> +        fulltext parent, it sees if the total size of the deltas leading up to
>> +        it is large enough to indicate that we want a new full text anyway.
>> +
>> +        Return True if we should create a new delta, False if we should use a
>> +        full text.
>> +        """
> [...]
>> +            # The window was changed to a maximum of 200 deltas, but also added
>> +            # was a check that the total compressed size of the deltas is
>> +            # smaller than the compressed size of the fulltext.
>> +            if not self._check_should_delta([delta_parent]):
>> +                # We don't want a delta here, just do a normal insertion.
>>                  return super(KnitVersionedFile, self)._add_delta(version_id,
>>                                                                   parents,
>>                                                                   delta_parent,
>> @@ -669,17 +693,7 @@
> [...]
> 
> ^--- Why: if not _check_should_delta(), return _add_delta()?
> 
> The comment "# We don't want a delta here" seems to contradict the
> natural reading of the next line.  I haven't looked at the source code
> for _add_delta(), but maybe we should consider renaming it if it indeed
> can be used to not "add a delta".
> 
> Richard

1) The code was already written that way :)

2) _add_delta() is saying that I have a delta, and I want to add it. And
VersionedFile._add_delta() expands it to a fulltext, and adds it that way.

The comment here should probably be fixed, it is a remnant of my
refactoring process. But basically, there are 2 public functions in the
VersionedFile API: add_lines() and add_delta()

With the former, you have a complete text which you are asking to be
added. With the later, you presumably already have the delta.

VersionedFile.add_* calls self._add_* to get the work done. And the
default _add_delta() implementation just expands the text back to a full
text, and calls self.add_lines(). So it would go around the loop again,
but this time as a full text, rather than just a partial.


Now that I've said all that, there is one more point of clarification.
Nobody *uses* add_delta(). I think it was meant to handle merging of two
files, but instead we just use .join(), which figures it out for itself
(generally just copying the data over for knits).

I would be happy to deprecate and remove .add_delta() since it is not
being used.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061129/b112101b/attachment.pgp