[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