FTP transport: put: fails during push operation

[Alexander Belchenko]

I try to push my branch to site via ftp. I got permanent error like this:

TransportError: 553-Can't open that file: No such file or directory
553 Rename/move failure: No such file or directory

When I look into .bzr.log I see that this error occurs when bzr try to
push revision-store:

add store entry '<email address hidden>'
FTP has not:
/<email address hidden>

FTP has not:
/<email address hidden>

FTP put:
/<email address hidden>

[ 5244] Sun 04:02:27.217 ERROR: 553-Can't open that file: No such file
or directory
553 Rename/move failure: No such file or directory
Traceback (most recent call last):
...

John Meinel commentary (from mail list):

I think the bug is that FTPTransport is not properly raising a
NoSuchFile exception when a file doesn't exist. (as you can see, you are
getting a generic TransportError.

The reason is this bit of code in the TextStore.put()

def _try_put(self, fn, f):
    try:
        self._transport.put(fn, f, mode=self._file_mode)
    except NoSuchFile:
        if not self._prefixed:
            raise
        try:
            self._transport.mkdir(os.path.dirname(fn),
                                  mode=self._dir_mode)
        except FileExists:
            pass
        self._transport.put(fn, f, mode=self._file_mode)

Basically, we only create the directory if we fail to put a file there.
That way we don't have to attempt to create the directory *every* time
we put a file, only if it fails.

The big problem is that the FTP transport is not properly tested,
because we don't have an FTP server we can easily test with.

[Daniel Silverstone (dsilvers)]

I believe code to fix this is in my branch: https://launchpad.net/people/dsilvers/+branch/bzr/dsilvers-devel

However Martin is still deciding whether or not to integrate it.

[Alexander Belchenko (bialix)]

Daniel Silverstone пишет:
> Public bug report changed:
> https://launchpad.net/malone/bugs/28850
>
> Comment:
> I believe code to fix this is in my branch:
> https://launchpad.net/people/dsilvers/+branch/bzr/dsilvers-devel
>
> However Martin is still deciding whether or not to integrate it.

I think you could remind to him. Or just send to mailing list merge
request and someone (eg. John Meinel) will review your changes. I'd like
to have your fix in 0.7 release.

--
Alexander

[John A Meinel (jameinel)]

Alexander Belchenko wrote:
> Daniel Silverstone пишет:
>> Public bug report changed:
>> https://launchpad.net/malone/bugs/28850
>>
>> Comment:
>> I believe code to fix this is in my branch:
>> https://launchpad.net/people/dsilvers/+branch/bzr/dsilvers-devel
>>
>> However Martin is still deciding whether or not to integrate it.
>
> I think you could remind to him. Or just send to mailing list merge
> request and someone (eg. John Meinel) will review your changes. I'd like
> to have your fix in 0.7 release.
>
> --
> Alexander
>

Is there an explicit request for this? Daniel has quite a few changes in
his branch (the improvements to 'bzr missing', the ftp fixes, etc). I
can just take the FTP fixes to start with...

After resolving conflicts, this is the changes that I see on my
jam-integration branch versus Daniel's branch (revno 1458)

> === modified file 'bzrlib/transport/ftp.py'
> --- bzrlib/transport/ftp.py
> +++ bzrlib/transport/ftp.py
> @@ -38,6 +38,21 @@
> from bzrlib.errors import (TransportNotPossible, TransportError,
> NoSuchFile, FileExists)
> from bzrlib.trace import mutter
> +
> +
> +_FTP_cache = {}
> +def _find_FTP(hostname, username, password, is_active):
> + """Find an ftplib.FTP instance attached to this triplet."""
> + key = "%s|%s|%s|%s" % (hostname, username, password, is_active)
> + if key not in _FTP_cache:
> + mutter("Constructing FTP instance against %r" % key)
> + _FTP_cache[key] = ftplib.FTP(hostname, username, password)
> + _FTP_cache[key].set_pasv(not is_active)
> + return _FTP_cache[key]
> +

The cache seems about right. I'm a little concerned that the first
connection would be active, and subsequent connections would be passive,
but that is pretty unlikely to happen.

> +
> +class FtpTransportError(TransportError):
> + pass

I think we've gotten away from inheriting new errors in the transports.
I know we used to, but I checked, and none of the other transports do
anymore. (We really don't prefer to create exceptions inside main
classes anyway)

>
>
> class FtpStatResult(object):
> @@ -69,7 +84,6 @@
> self._query, self._fragment) = urlparse.urlparse(self.base)
> self._FTP_instance = _provided_instance
>
> -
> def _get_FTP(self):
> """Return the ftplib.FTP instance for this object."""
> if self._FTP_instance is not None:
> @@ -84,9 +98,8 @@
> if ':' in username:
> username, password = username.split(":", 1)
>
> - mutter("Constructing FTP instance")
> - self._FTP_instance = ftplib.FTP(hostname, username, password)
> - self._FTP_instance.set_pasv(not self.is_active)
> + self._FTP_instance = _find_FTP(hostname, username, password,
> + self.is_active)
> return self._FTP_instance
> except ftplib.error_perm, e:
> raise TransportError(msg="Error setting up connection: %s"
> @@ -194,7 +207,12 @@
> f = self._get_FTP()
> f.storbinary('STOR '+self._abspath(relpath), fp, 8192)
> except ftplib.error_perm, e:
> - ...

[Daniel Silverstone (dsilvers)]

On Wed, 2006-01-18 at 08:21 -0600, John A Meinel wrote:
> Is there an explicit request for this? Daniel has quite a few changes in
> his branch (the improvements to 'bzr missing', the ftp fixes, etc). I
> can just take the FTP fixes to start with...

IIRC all but the FTP stuff, a NEWS entry for bzr missing and a bizarre
patch to changeset.py has been merged into martin's mainline anyway.

Speaking with martin last week he said he'd merge it and undo the
changeset.py change. However if you think the exceptions are wrong then
I should fix those first.

> The cache seems about right. I'm a little concerned that the first
> connection would be active, and subsequent connections would be passive,
> but that is pretty unlikely to happen.

Umm, I don't quite see why this situation would ever arise since passive
vs. active is always chosen by the url scheme.

> > +
> > +class FtpTransportError(TransportError):
> > + pass
> I think we've gotten away from inheriting new errors in the transports.
> I know we used to, but I checked, and none of the other transports do
> anymore. (We really don't prefer to create exceptions inside main
> classes anyway)

Okay, so I should drop this in favour of raising pure TransportErrors ?
Or should I be declaring FtpTransportError somewhere else?

> NoSuchFile doesn't take a msg anymore, it takes a path, and an 'extra'
> value. So you want to raise:
> raise NoSuchFile(self.abspath(relpath), extra=e)
> else:
> raise TransportError(orig_error=e)

Okay, I can clean this up.

> > def mkdir(self, relpath, mode=None):
> > """Create a directory at the given path."""
> >
> I also noticed that you don't handle any of the mode=? stuff. Does FTP
> support chmod?

FTP itself supports it, but not ftplib (as you noted in some patches to
the ftp library you did)

> So with a little bit of cleanup, this seems to solve the ftp push issue.

Aye, It solved the problem back when I wrote it, but I'll admit I don't
use the FTP protocol very much (if ever) myself.

The way I see it, my branch needs a little tweaking to clean it up and
then it should be okay for merging -- I shall do this asap.

D.

--
Daniel Silverstone http://www.digital-scurf.org/
PGP mail accepted and encouraged. Key Id: 2BC8 4016 2068 7895

[Martin Pool (mbp)]

I think this is now merged & OK - please reopen if not.