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.

> + st = t.stat(link_name)
> + self.failUnlessEqual(st[ST_NLINK], 2)
>
> Good; I think there might be a TestCase method to check they're linked
> to each other.

Okay; will go look for it.

> + except TransportNotPossible:
> + # This transport doesn't do hardlinks
> + return
>
> Probably better to indicate it was skipped: raise TestSkipped(...).

Changing; did it that way because that's how test_stat() did it. :P

> + st = t.stat(link_name)
> + self.failUnless(S_ISLNK(st.st_mode))
>
> I think you should actually read the link and check it had what you
> expected.

Understood.

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

> 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()?