[MERGE] Fix a case of look before you leap in knit.py

[Robert Collins]

On Wed, 2007-08-22 at 13:27 -0400, John Arbash Meinel wrote:
> John Arbash Meinel has voted tweak.
> Status is now: Conditionally approved
> Comment:
>       def _get_node(self, version_id):
> -        return 
> list(self._get_entries(self._version_ids_to_keys([version_id])))[0]
> +        try:
> +            return 
> list(self._get_entries(self._version_ids_to_keys([version_id])))[0]
> +        except IndexError:
> +            raise RevisionNotPresent(version_id, self)
> 
> ^- I think you are actually supposed to be raising a filename here, not 
> an object.

There is no filename to raise; and RevisionNotPresent just wants a
'weave' object - the format string is 'Revision %s not present in %s'.

> At the very least, your object should have a __str__ that shows what 
> path it is at. Otherwise debugging things when we get a bad revision id 
> is really hard.
> It would seem that KnitGraphIndex doesn't have any idea what file it is 
> associated with, though. Which is a real shame, because we *have* 
> encountered this sort of thing. (Especially with data corruption, etc)

Right, it doesn't. I guess I can parameterise it at construction with a
file_id, but its another piece of data that could potentially skew, and
one that will only be used for __str__, so it seems ugly to do that.

> So, in the short term, this is okay. But really KGI is a bit hard to use 
> for post-mortem understanding. I think part of that is the abstraction, 
> because you are overlying a 'graph_index' object, which is also a bit 
> abstract.

Right. I want to reduce the amount of calls going through these deep
layers. For clarity the layers used by a current pack repo are:
Knit: KnitGraphIndex, _KnitData
KnitGraphIndex: GraphIndexPrefixAdapter
_KnitData: _KnitAccess
GraphIndexPrefixAdapter: CombinedGraphIndex
CombinedGraphIndex: [GraphIndex ...]
during a write there is also an InMemoryGraphIndex added to the
CombinedGraphIndex and wired up to the KnitGraphIndex callback, as well
as a ContainerWriter which is hooked up to the _KnitAccess object.

> And the fact that KGI is only used in your repository branch, and not by 
> any "live" code (just the test suite) makes it difficult to find. (And I 
> have to track InMemoryGraphIndex back through an 'import *' which means 
> I can't just simply '*' to search for the import occurance.)

Is that in the test file? I'll change that.

I'm not really clear what you want changed though.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- 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/20070823/4ec976b1/attachment.pgp