Merge lp:~ndurner/bzr/bzr-ftp into lp:~bzr/bzr/trunk-old

[Nils Durner]

[Nils Durner (ndurner)]

Fixes bug #294709.
Due to time constraints, I haven't looked into writing a testcase for it yet.

[Martin Pool (mbp)]

> Fixes bug #294709.
> Due to time constraints, I haven't looked into writing a testcase for it yet.

8 +
9 + self._has_append = True

OK it's fairly obvious but a comment here would be good, especially because it's "might have append", not necessarily "has append".

39 except ftplib.error_perm, e:
40 - self._translate_perm_error(e, abspath, extra='error appending',
41 - unknown_exc=errors.NoSuchFile)
42 + warning("FTP server does not support file appending natively. " \
43 + "Performance may be severely degraded!")
44 + self._has_append = False
45 + self._fallback_append(relpath, text, mode)
46 +

This seems to be treating *any* permission error on append as being failure to support append. What about if we just can't append to that one particular file because of say a permissions problem? I guess you need to either look for an error code or perhaps match the string.

Also to help debug this it might be good to include e in the message.

review: Needs Fixing

[Nils Durner (ndurner)]

> OK it's fairly obvious but a comment here would be good, especially because
> it's "might have append", not necessarily "has append".

Added in r4540.

> This seems to be treating *any* permission error on append

Any perm*anent* error, yes.
r4541 checks the actual reply code, in accordance to RFC 959 and 640.

> Also to help debug this it might be good to include e in the message.

r4542 has an extra mutter() in place, is that okay?

Thanks,

Nils

=== modified file 'bzrlib/transport/ftp/__init__.py'
--- bzrlib/transport/ftp/__init__.py	2009-04-27 16:10:10 +0000
+++ bzrlib/transport/ftp/__init__.py	2009-07-22 06:35:38 +0000
@@ -99,6 +99,9 @@
             self.is_active = True
         else:
             self.is_active = False
+        
+        # Most modern FTP servers support the APPE command. If ours doesn't, we (re)set this flag accordingly later.
+        self._has_append = True
 
     def _get_FTP(self):
         """Return the ftplib.FTP instance for this object."""
@@ -387,6 +390,8 @@
         """Append the text in the file-like object into the final
         location.
         """
+        text = f.read()
+        
         abspath = self._remote_path(relpath)
         if self.has(relpath):
             ftp = self._get_FTP()
@@ -394,8 +399,11 @@
         else:
             result = 0
 
-        mutter("FTP appe to %s", abspath)
-        self._try_append(relpath, f.read(), mode)
+        if self._has_append:
+            mutter("FTP appe to %s", abspath)
+            self._try_append(relpath, text, mode)
+        else:
+            self._fallback_append(relpath, text, mode)
 
         return result
 
@@ -416,8 +424,16 @@
             self._setmode(relpath, mode)
             ftp.getresp()
         except ftplib.error_perm, e:
-            self._translate_perm_error(e, abspath, extra='error appending',
-                unknown_exc=errors.NoSuchFile)
+            # Check whether the command is not supported (reply code 502)
+            if str(e).startswith('502 '):
+                warning("FTP server does not support file appending natively. " \
+                    "Performance may be severely degraded! (%s)", e)
+                self._has_append = False
+                self._fallback_append(relpath, text, mode)
+            else:
+                self._translate_perm_error(e, abspath, extra='error appending',
+                    unknown_exc=errors.NoSuchFile)
+            
         except ftplib.error_temp, e:
             if retries > _number_of_retries:
                 raise errors.TransportError("FTP temporary error during APPEND %s." \
@@ -427,6 +443,13 @@
                 self._reconnect()
                 self._try_append(relpath, text, mode, retries+1)
 
+    def _fallback_append(self, relpath, text, mode = None):
+        remote = self.get(relpath)
+        remote.seek(0, 2)
+        remote.write(text)
+        remote.seek(0, 0)
+        return self.put_file(relpath, remote, mode)
+
     def _setmode(self, relpath, mode):
         """Set permissions on a path.