Code review comment for lp:~rls/bzr/transport-link-and-symlink-support

> > Returning True for success is a bit strange in Python. Why not just
> > rely on exceptions to indicate errors?
>
> Agreed. I did it that way mostly because Paramiko's symlink() returns
> constants, and I wasn't entirely which way to go, and figured one way's as
> good as the other. I'm scouring bzrlib/errors.py for an appropriate exception
> to throw, and would be grateful if you could point out the one most useful in
> this case.

If paramiko returns SFTP_OK then just return (which implicitly returns None.)

If it returns an error you don't specifically know what to do with, it would be reasonable to raise just a generic TransportError with the error code and path. However it seems like paramiko normally translates errors into exceptions itself, so you may not need to worry about this?

> > This is in LocalTransport.stat, obviously changing it to behave more
> > like lstat that stat. Since transports never previously expected to
> > see symlinks perhaps it will not break anything, but on the other hand
> > perhaps it is a bit surprising that t.stat behaves like lstat. If we
> > ever actually wanted to follow symlinks, we'd need to add another
> > method for that.
> >
> > Perhaps if that behaviour is changed it should be tested.
> >
> > Alternatively you could add a t.lstat.
>
> I originally wanted to add an lstat(), though I figured I should just get it
> mostly working from the get-go, and check with reviewers to see which way it
> should go. I'll slap on an lstat() where appropriate and change stat() back
> to how it was, and see where that leads us.

Actually the thread seemed to conclude that it was better for Transport.stat() to call os.lstat, and then make the higher level code specifically deal with the symlink.

>
> > I posted an rfc. On the whole I think what you have is fine.
>
> Thanks! :)
>
> > Similarly to what gzlist said, I think here you should do
> >
> > try:
> > link = os.link
> > except AttributeError, ...
> > raise TransportNotPossible(...)
> >
> > link(...)
>
> Do you mean this:
>
> if osutils.hardlinks_good():
> def link(self, source, link_name):
> """See Transport.link."""
> try:
> link = os.link
> except AttributeError:
> return errors.TransportNotPossible("Hardlinks are not
> supported on %s" %self)
> except (IOError, OSError), e:
> self._translate_error(e, source)
>
> link(self._abspath(source), self._abspath(link_name))
>
> Or just straight up declaring a member named 'link' as an alias to os.link()?

Like that, except the osutils.hardlink_good() check seems redundant. Just declare the method always and let the AttributeError call handle the case that they're not supported.