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

[John Arbash Meinel]

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

Robert Collins wrote:
> 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.

When someone comes and says "I'm getting RevisionNotPresent" and gives the
traceback. We have practically 0 information about how to help them.
At least with the filename, we have an idea of what file it is looking in, and
where we might start looking for the problem.
The only way to get that information is to have the user run pdb. And I'm not
sure that I want to try and talk someone through using pdb over IRC. It is a
whole lot easier to have the initial report include a filename.

I understand we have some issues with layering, etc. And I won't block on this.
But if it is possible, having meaningful errors is a *huge* benefit when it
comes to working through problems. Just like Martin's recommendation to not use:

assert SOMETHING

Without putting a real message like

assert SOMETHING, "You tried to do X with Y, but it isn't possible"

When a user sees "AssertionError" they can only send it to the mailing list.
When they see:
AssertionError: "foo" was a directory but now it is a symlink, please add and
remove that entry.

It means that they can actually do something to resolve the problem.

> 
>> 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


Getting rid of the "from X import *" would be nice.

Having the error message include the filename would be nice.

I'll let you decide what is worth your time & effort before merging.

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

iD8DBQFG1vr7JdeBCYSNAAMRAkAxAKCw/8Xa8apX+FxRd3uUxGZEj13Y1ACgork3
LcG5/7IDXND01rFLpeuMEts=
=yEVw
-----END PGP SIGNATURE-----