Comment 3 for bug 28850

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:
> - raise TransportError(orig_error=e)
> + if "no such file" in str(e).lower():
> + raise NoSuchFile(msg="Error storing %s: %s"
> + % (self.abspath(relpath), str(e)),
> + orig_error=e)
> + else:
> + raise FtpTransportError(orig_error=e)
>

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)

> 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?
The idea is that when you push to a directory which is group writable,
the subdirectories should stay group writable. This makes sharing
branches easier, since you only have to 'chmod' them once, and bzr will
maintain it.

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

John
=:->