Merge lp:~rls/bzr/transport-link-and-symlink-support into lp:bzr

Attempt to include support for hard- and symlinks into the Transport classes that advertise support for one or both them. Needed in the mainline to more cleanly support links for (in particular) the bzr-upload plugin.

I'd prefer this style:

+ if osutils.hardlinks_good()
+ def link(self, source, link_name):
...

Over the current:

+ def link(self, source, link_name):
...
+ except AttributeError:
+ raise errors.TransportNotPossible("Hardlinks are not supported on %s" % self)

I've been wondering whether I should implement link() for LocalTransport using osutil's link_or_copy(); I figured it's what it's there for. Is there any reason why I shouldn't do that?

For clarity (especially for non unix programmers) I might s/link/hardlink

On 26 February 2010 02:50, Neil Santos <email address hidden> wrote:
> Review: Needs Information
> I've been wondering whether I should implement link() for LocalTransport using osutil's link_or_copy(); I figured it's what it's there for.  Is there any reason why I shouldn't do that?

That seems a bit like an abstraction inversion. If it's desirable
(sometimes?) to copy when you can't hardlink, then that should be done
across all transports. I don't think you would want this
unconditionally, so it should be either a different transport method
or an option to .hardlink(). Making it a separate hardlink_or_copy()
would allow it to be defined in the Transport base class without
needing to be overridden per transport.

--
Martin <http://launchpad.net/~mbp/>

> I'd prefer this style:
>
> + if osutils.hardlinks_good()
> + def link(self, source, link_name):
> ...

Implemented. There's another problem I've ran into (and have bothered mbp with): the tests for SFTPTransport's symlink() implementation raises an IOError (Operation unsupported).

I think this is pretty close; thanks for posting it. Just a few comments:

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py 2010-02-23 07:43:11 +0000
+++ bzrlib/tests/per_transport.py 2010-02-26 04:50:30 +0000
@@ -1083,6 +1083,52 @@
         subdir.stat('./file')
         subdir.stat('.')

+ def test_link(self):
+ from stat import ST_NLINK
+
+ t = self.get_transport()
+
+ source_name = "original_target"
+ link_name = "target_link"
+
+ self.build_tree([source_name], transport=t)
+
+ try:
+ link_result = t.link(source_name, link_name)
+ self.failUnless(link_result)

Returning True for success is a bit strange in Python. Why not just
rely on exceptions to indicate errors?

+
+ self.failUnless(t.has(source_name))
+ self.failUnless(t.has(link_name))
+
+ 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.

+ except TransportNotPossible:
+ # This transport doesn't do hardlinks
+ return

Probably better to indicate it was skipped: raise TestSkipped(...).

+
+ def test_symlink(self):
+ from stat import S_ISLNK
+
+ t = self.get_transport()
+
+ source_name = "original_target"
+ link_name = "target_link"
+
+ self.build_tree([source_name], transport=t)
+
+ try:
+ link_result = t.symlink(source_name, link_name)
+ self.failUnless(link_result)
+

likewise

+ self.failUnless(t.has(source_name))
+ self.failUnless(t.has(link_name))
+
+ 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.

+ except TransportNotPossible:
+ # This transport doesn't do hardlinks
+ return
+
     def test_list_dir(self):
         # TODO: Test list_dir, just try once, and if it throws, stop testing
         t = self.get_transport()

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py 2010-02-23 07:43:11 +0000
+++ bzrlib/transport/__init__.py 2010-02-26 04:50:30 +0000
@@ -1202,6 +1202,20 @@
         count = self._iterate_over(relpaths, gather, pb, 'stat', expand=False)
         return stats

+ def link(self, source, link_name):
+ """Create a hardlink pointing to source named link_name.
+
+ Return True if successful.
+ """
+ raise errors.TransportNotPossible("Hard links are not supported on %s" % self)
+
+ def symlink(self, source, link_name):
+ """Create a symlink pointing to source named link_name.
+
+ Return True if successful.
+ """
+ raise errors.TransportNotPossible("Symlinks are not supported on %s" % self)
+
     def listable(self):
         """Return True if this store supports listing."""
         raise NotImplementedError(self.listable)

=== modified file 'bzrlib/transport/local.py'
--- bzrlib/transport/local.py 2010-02-23 07:43:11 +0000
+++ bzrlib/transport/local.py 2010-02-26 04:50:30 +0...

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

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

Hrm... I didn't find such a method per se, but I didn't find a couple of tests using osutils' readlink(), which should be good enough for LocalTransport, as well as Paramiko's SFTP's readlink(). I'll go ahead and add a readlink() to Transport; let me know how it looks.

> > I'd prefer this style:
> >
> > + if osutils.hardlinks_good()
> > + def link(self, source, link_name):
> > ...
>
> Implemented. ...

Ah, I also meant this to go for the `symlink` function on the same object, with `osutils.has_symlinks` as the definition-time check. The point being, you know what the module is loaded if the python implementation has a (?:sym)link function, so don't need to check within the call itself. You do still need to check for other errors, as the specific filesystem/path may not support the given operation.

Not sure what the main Martin meant with his code fragment on the same topic though.

> Ah, I also meant this to go for the `symlink` function on the same object,
> with `osutils.has_symlinks` as the definition-time check. The point being, you

Ah, LOL. I also figured I should do this; in fact, my last commit contains this, as well as a similar guard for the readlink() I'm implementing (using osutils.host_os_dereferences_symlink()).

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

> However it seems like paramiko normally translates errors into
> exceptions itself, so you may not need to worry about this?

Not sure about that part. I think it raises errors on blatant errors (disconnection, non-support-- stuff that the library can't handle without human intervention, I suppose). For the most part, it returns error codes. Crude, but hey, I just work with it. :P

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

Okay, I've reverted that, then.

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

Eh. Your suggestion conflicts with gz's. I do like the idea of letting the rest of bzrlib's guts tell me whether it's okay to declare a method, though, rather than handle an AttributeError myself. But you guys are more familiar with the gestalt of Bzr than I am, so perhaps I'm missing something?

On 3 March 2010 19:40, Neil Santos <email address hidden> wrote:
>> However it seems like paramiko normally translates errors into
>> exceptions itself, so you may not need to worry about this?
>
> Not sure about that part.  I think it raises errors on blatant errors (disconnection, non-support-- stuff that the library can't handle without human intervention, I suppose).  For the most part, it returns error codes.  Crude, but hey, I just work with it. :P

Heh, ok. So on the whole, I'd say if you get a non-ok return code,
just raise an error that includes that value; we could add a method
that specifically translates them if we think any are interesting.

>
>> 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.
>
> Okay, I've reverted that, then.
>
>> 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.
>
> Eh.  Your suggestion conflicts with gz's.  I do like the idea of letting the rest of bzrlib's guts tell me whether it's okay to declare a method, though, rather than handle an AttributeError myself.  But you guys are more familiar with the gestalt of Bzr than I am, so perhaps I'm missing something?

I think gz and I agree that there's no point in having redundant
checks. As he says, we have to handle the case that it turns out not
to be possible at runtime, and I guess it just seems a bit cleaner to
me to avoid conditionally declaring functions. Either is ok though,
so let me know when you're ready for this to be rereviewed/merged.

Thanks,
--
Martin <http://launchpad.net/~mbp/>

> > Eh.  Your suggestion conflicts with gz's.  I do like the idea of letting the
> rest of bzrlib's guts tell me whether it's okay to declare a method, though,
> rather than handle an AttributeError myself.  But you guys are more familiar
> with the gestalt of Bzr than I am, so perhaps I'm missing something?
>
> I think gz and I agree that there's no point in having redundant
> checks. As he says, we have to handle the case that it turns out not
> to be possible at runtime, and I guess it just seems a bit cleaner to
> me to avoid conditionally declaring functions. Either is ok though,
> so let me know when you're ready for this to be rereviewed/merged.

To be clear, I was trying to say the AttributeError handling is better written outside the method, but the functions still may throw EnvironmentError when called so that part had to be retained. It's a minor style point however.

Resubmitting for review.