[MERGE] Add PreviewTree to the tree implementation tests

[Andrew Bennetts]

bb:tweak

I'm basically happy with this.  You've extended the interface of get_file_size a
little without updating any corresponding docstrings (see comments inline), but
no big complaints.  In particular, the test changes look fine to me now :)

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.

> +    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.  I'd
rather see an explicit “return None” to make it clear that it's intentional,
rather than just an oversight.  (The only reason I know it's intentional is that
elsewhere in this patch you add an explicit test for this behaviour.  Other
readers are unlikely to have that advantage.)

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.  (After all, “practicality
beats purity” is Pythonic too).  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.

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.

> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py	2008-03-15 14:07:55 +0000
> +++ bzrlib/workingtree.py	2008-04-12 19:43:13 +0000
> @@ -618,7 +618,13 @@
>      __contains__ = has_id
>  
>      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...

-Andrew.