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

Proposed by Nils Durner
Status: Merged
Approved by: Martin Pool
Approved revision: 4543
Merged at revision: not available
Proposed branch: lp:~ndurner/bzr/bzr-ftp
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 68 lines
To merge this branch: bzr merge lp:~ndurner/bzr/bzr-ftp
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Review via email: mp+8906@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Nils Durner (ndurner) wrote :

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

Revision history for this message
Martin Pool (mbp) wrote :

> 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
lp:~ndurner/bzr/bzr-ftp updated
4540. By Nils Durner <email address hidden>

Explain flag initialization

4541. By Nils Durner <email address hidden>

use the fallback mechanism only if the server really lacks the APPE command

4542. By Nils Durner <email address hidden>

add debug output if APPE failed

Revision history for this message
Nils Durner (ndurner) wrote :

> 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

lp:~ndurner/bzr/bzr-ftp updated
4543. By Nils Durner <email address hidden>

include FTP error message in warning

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
99 self.is_active = True99 self.is_active = True
100 else:100 else:
101 self.is_active = False101 self.is_active = False
102
103 # Most modern FTP servers support the APPE command. If ours doesn't, we (re)set this flag accordingly later.
104 self._has_append = True
102105
103 def _get_FTP(self):106 def _get_FTP(self):
104 """Return the ftplib.FTP instance for this object."""107 """Return the ftplib.FTP instance for this object."""
@@ -387,6 +390,8 @@
387 """Append the text in the file-like object into the final390 """Append the text in the file-like object into the final
388 location.391 location.
389 """392 """
393 text = f.read()
394
390 abspath = self._remote_path(relpath)395 abspath = self._remote_path(relpath)
391 if self.has(relpath):396 if self.has(relpath):
392 ftp = self._get_FTP()397 ftp = self._get_FTP()
@@ -394,8 +399,11 @@
394 else:399 else:
395 result = 0400 result = 0
396401
397 mutter("FTP appe to %s", abspath)402 if self._has_append:
398 self._try_append(relpath, f.read(), mode)403 mutter("FTP appe to %s", abspath)
404 self._try_append(relpath, text, mode)
405 else:
406 self._fallback_append(relpath, text, mode)
399407
400 return result408 return result
401409
@@ -416,8 +424,16 @@
416 self._setmode(relpath, mode)424 self._setmode(relpath, mode)
417 ftp.getresp()425 ftp.getresp()
418 except ftplib.error_perm, e:426 except ftplib.error_perm, e:
419 self._translate_perm_error(e, abspath, extra='error appending',427 # Check whether the command is not supported (reply code 502)
420 unknown_exc=errors.NoSuchFile)428 if str(e).startswith('502 '):
429 warning("FTP server does not support file appending natively. " \
430 "Performance may be severely degraded! (%s)", e)
431 self._has_append = False
432 self._fallback_append(relpath, text, mode)
433 else:
434 self._translate_perm_error(e, abspath, extra='error appending',
435 unknown_exc=errors.NoSuchFile)
436
421 except ftplib.error_temp, e:437 except ftplib.error_temp, e:
422 if retries > _number_of_retries:438 if retries > _number_of_retries:
423 raise errors.TransportError("FTP temporary error during APPEND %s." \439 raise errors.TransportError("FTP temporary error during APPEND %s." \
@@ -427,6 +443,13 @@
427 self._reconnect()443 self._reconnect()
428 self._try_append(relpath, text, mode, retries+1)444 self._try_append(relpath, text, mode, retries+1)
429445
446 def _fallback_append(self, relpath, text, mode = None):
447 remote = self.get(relpath)
448 remote.seek(0, 2)
449 remote.write(text)
450 remote.seek(0, 0)
451 return self.put_file(relpath, remote, mode)
452
430 def _setmode(self, relpath, mode):453 def _setmode(self, relpath, mode):
431 """Set permissions on a path.454 """Set permissions on a path.
432455