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
1=== modified file 'bzrlib/transport/ftp/__init__.py'
2--- bzrlib/transport/ftp/__init__.py 2009-04-27 16:10:10 +0000
3+++ bzrlib/transport/ftp/__init__.py 2009-07-22 06:35:38 +0000
4@@ -99,6 +99,9 @@
5 self.is_active = True
6 else:
7 self.is_active = False
8+
9+ # Most modern FTP servers support the APPE command. If ours doesn't, we (re)set this flag accordingly later.
10+ self._has_append = True
11
12 def _get_FTP(self):
13 """Return the ftplib.FTP instance for this object."""
14@@ -387,6 +390,8 @@
15 """Append the text in the file-like object into the final
16 location.
17 """
18+ text = f.read()
19+
20 abspath = self._remote_path(relpath)
21 if self.has(relpath):
22 ftp = self._get_FTP()
23@@ -394,8 +399,11 @@
24 else:
25 result = 0
26
27- mutter("FTP appe to %s", abspath)
28- self._try_append(relpath, f.read(), mode)
29+ if self._has_append:
30+ mutter("FTP appe to %s", abspath)
31+ self._try_append(relpath, text, mode)
32+ else:
33+ self._fallback_append(relpath, text, mode)
34
35 return result
36
37@@ -416,8 +424,16 @@
38 self._setmode(relpath, mode)
39 ftp.getresp()
40 except ftplib.error_perm, e:
41- self._translate_perm_error(e, abspath, extra='error appending',
42- unknown_exc=errors.NoSuchFile)
43+ # Check whether the command is not supported (reply code 502)
44+ if str(e).startswith('502 '):
45+ warning("FTP server does not support file appending natively. " \
46+ "Performance may be severely degraded! (%s)", e)
47+ self._has_append = False
48+ self._fallback_append(relpath, text, mode)
49+ else:
50+ self._translate_perm_error(e, abspath, extra='error appending',
51+ unknown_exc=errors.NoSuchFile)
52+
53 except ftplib.error_temp, e:
54 if retries > _number_of_retries:
55 raise errors.TransportError("FTP temporary error during APPEND %s." \
56@@ -427,6 +443,13 @@
57 self._reconnect()
58 self._try_append(relpath, text, mode, retries+1)
59
60+ def _fallback_append(self, relpath, text, mode = None):
61+ remote = self.get(relpath)
62+ remote.seek(0, 2)
63+ remote.write(text)
64+ remote.seek(0, 0)
65+ return self.put_file(relpath, remote, mode)
66+
67 def _setmode(self, relpath, mode):
68 """Set permissions on a path.
69