[MERGE] Add PreviewTree to the tree implementation tests

[Aaron Bentley]

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

Andrew Bennetts wrote:
> bb:tweak

Thanks for your review.

> Aaron Bentley wrote:
>> === modified file 'bzrlib/transform.py'
> [...]
>> +    def _content_change(self, file_id):
>> +        changes = self._changes(file_id)
>> +        return (changes is not None and changes[2])
> 
> This function is a bit too obscure.  A brief comment to remind the reader what
> “changes[2]” means here would be enough.

Okay.

>> +    def get_file_size(self, file_id):
>> +        if self.kind(file_id) == 'file':
>> +            return self._transform._tree.get_file_size(file_id)
> 
> Stylistically, I don't like a function that sometimes returns a value
> explicitly, and sometimes implicitly returns None by falling off the end.

Yeah, it's not my general habit, either.  I can only assume I was doing
TDD and forgot to update it for style once it worked.

> I'm not sure that returning None is the best thing to do here.  An exception
> might be the more Pythonic thing to do, but I'm willing to trust your judgement
> if you think this is a more practical behaviour.

I do feel that it's the practical choice.  When you may be calling this
on thousands of files, the exception handling can be a noticeable overhead.

> I know that another get_file_size uses
> os.path.getsize indiscriminately, and that returns an integer even for symlinks
> and directories, so just returning None for non-files here might surprise some
> people/code.

There was a test that for InventoryEntries, the size attribute should be
None for symlinks.  I felt that if Tree.get_file_size was mean to
replace the InventoryEntry.size attribute, it should be as similar as
possible.

There was already discrepancy among implementations-- the RevisionTree
implementation was returning None, the WorkingTree implementation was
returning an int.  I decided that returning None was correct and
WorkingTree was broken.

> The existing docstrings elsewhere for get_file_size don't say that None is a
> possible result though, so they should be changed to reflect the new reality.

Sure.

>>      def get_file_size(self, file_id):
>> -        return os.path.getsize(self.id2abspath(file_id))
>> +        try:
>> +            return os.path.getsize(self.id2abspath(file_id))
>> +        except OSError, e:
>> +            if e.errno != errno.ENOENT:
>> +                raise
>> +            else:
>> +                return None
> 
> Heh, speak of the devil... here's that os.path.getsize call I was referring to
> :)
> 
> I think the except block would be clearer without the unnecessary negation:
> 
>             if e.errno == errno.ENOENT:
>                 return None
>             else:
>                 raise
> 
> But I suspect I'm veering dangerously close to bikeshedding...

I like to handle the inapplicable cases first, which is generally a
single raise call.  Then I'll go into the main functionality, however
long it is.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIFfkV0F+nu1YWqI0RArCAAJ9rNmBT/91FCt4CA9vksAfiJYhPXACeKvvb
u8M3vYrJOCGqr07iVM0LyJE=
=Lyf+
-----END PGP SIGNATURE-----