[MERGE] updated index bisection

Robert Collins robertc at robertcollins.net
Fri Oct 12 08:22:16 BST 2007


On Thu, 2007-10-11 at 05:47 -0400, Martin Pool wrote: 
> Martin Pool has voted comment.
> Status is now: Semi-approved
> Comment:
> I think you need a little bit of commentary at the Index level 
> explaining the concepts which are added:
> 
>   key references
>   that the index can be partially parsed into memory, and
>   we remember both what keys we've seen and what bytes they were
> 
>           self._nodes = None
> 
> Since you're adding comments about the others it would be good to add 
> one for
> _nodes, particularly since its function is less clear if the index is
> partially parsed.

Added in the attached.

> +    def _resolve_references(self, references):
> +        """Return the resolved key references for references."""
> 
> The docstring doesn't tell me much more than the method name...

Fixed too.

> +    def lookup_keys_via_location(self, location_keys):
> +        """Public interface for implementing bisection.
> 
> Is this intended to be a public interface?  It looks more like a special
> callback for bisection and so it shouldn't be public.

Well, I really prefer to be testing public interfaces, and as the
bisection interface is a defined one, this is in my mind public. I'm
happy to make it non public if you still think thats better after
reading this.

> This method is really a bit long.  It looks like it should be possible
> to split up into its several sub-tasks of searching the cache, getting
> the headers,

I'd like to come back to this; I strongly suspect I'll be changing it
non-trivially, in the next round of performance work for this. 

> +        :param location_keys: A list of location, key tuples.
> +        :return: A list of (location_key, result) tuples as expected by
> +            bzrlib.bisect_multi.bisect_multi_bytes.
> 
> And location is a byte offset?

Tweaked.

> You have some cases of missing spaces around = for assignment.

I couldn't find any.

> It looks like the index has three almost disjoint states and behaviors: 
> entirely loaded, partially loaded, and not loaded at all.  I wonder if 
> it would be simpler to factor out an inner object with two classes for 
> the different strategies.

Thats an interesting idea. One possibility to consider is the parsing of
the unparsed data when we decide that a full read is taking place; then
there is some advantage in not having to pass state to the full reader.

> s/ajust/adjust

Done.

> to be continued...

Please let me know what you think of this updated patch.

Cheers,
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: index.patch
Type: text/x-patch
Size: 94661 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071012/436f3cc1/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071012/436f3cc1/attachment-0001.pgp 


More information about the bazaar mailing list