[PATCH] knit pushing with ftp transport
John Arbash Meinel
john at arbash-meinel.com
Mon May 1 15:56:20 BST 2006
Alexandre Saint wrote:
> On Mon, May 01, 2006 at 07:51:48AM -0500, John Arbash Meinel wrote:
>> Alexandre Saint wrote:
>>> On Sun, Apr 30, 2006 at 03:34:02PM -0500, John Arbash Meinel wrote:
>>>> Alexandre Saint wrote:
>>>>> Hello,
>>>>>
>>>>> This patch adds support for knit format to the ftp transport.
>>>>>
>>>>> Added an append() method using FTP 'APPE' command.
>>>>> Modified copy() to deal well with relative paths (and not only with
>>>>> absolute ones).
>>>>>
> [...]
>> The interface for append() has to be a file-like object (as defined by
>> Transport).
>>
>> I was suggesting that append() should not be recursive, but should call
>> something like _append_helper() which would be recursive, and would
>> expect a string.
>>
>
> I misunderstood you. But I agree know.
>
>> We decided on a policy of "no magic interfaces", so we don't want to use
>> isinstance() switches if we can help it.
>>
>> Oh, and the correct way to determine it is "isinstance(x, str)". If you
>> want to compare for both regular strings and unicode strings, you need
>> to use "isinstance(x, basestring)", but in this case you don't want to
>> accept unicode strings.
>>
>> I would do something more like:
>>
>> def append(self, relpath, f, mode=None):
>> if self.has(relpath):
>> result = ftp.size(abspath)
>> else:
>> result = 0
>> self._append_helper(self._abspath(relpath), f.read(),
>> mode=mode, retries=0)
>>
>> return result
>>
>> def _append_helper(self, abspath, f, mode=None, retries=0):
>> """This is a recursive function that will ..."
>>
>>
>> I'm not sure if you need to check the file size each time, or just once.
>> But things that don't need to be repeated (like _abspath, and f.read())
>> would go into 'append', and the repeating stuff would go into
>> _append_helper().
>> Or maybe it should be called _try_append()
>>
>> John
>> =:->
>>
>
> In this patch, added _try_append(), which is recursive.
>
>
Looks pretty good. One issue is that you have embedded tab characters,
rather than only whitespace. PEP8 says space characters only.
>
> ------------------------------------------------------------------------
>
> === modified file 'a/bzrlib/transport/ftp.py'
> --- a/bzrlib/transport/ftp.py
> +++ b/bzrlib/transport/ftp.py
> @@ -303,11 +303,64 @@
> raise TransportError(msg="Cannot remove directory at %s" % \
> self._abspath(rel_path), extra=str(e))
>
> - def append(self, relpath, f):
> + def append(self, relpath, f, mode=None):
> """Append the text in the file-like object into the final
> location.
> """
> - raise TransportNotPossible('ftp does not support append()')
> + if self.has(relpath):
> + ftp = self._get_FTP()
> + result = ftp.size(self._abspath(relpath))
> + else:
> + result = 0
> +
> + self._try_append(relpath, f.read(), mode)
> +
> + return result
> +
> + def _try_append(self, relpath, text, mode=None, retries=0):
> + """This is a recursive function that will try to append the text
> + given as argument to the final location until the number of retries
> + exceeds the limit.
> + """
We try to have a single line summary at the beginning. Something like:
"""Try repeatedly to append the given text to the file at relpath.
More information here.
"""
> + try:
> + abspath = self._abspath(relpath)
> + mutter("FTP appe to %s" % abspath)
mutter() and all of the logging/debug functions are designed to take a
list of arguments, and format the string. Also, since this is in the
recursive portion, I would do:
mutter("FTP appe (%d) to %s", retries, abspath)
> + ftp = self._get_FTP()
> + ftp.voidcmd("TYPE I")
> + cmd = "APPE %s" % abspath
> + conn = ftp.transfercmd(cmd)
> + conn.sendall(text)
> + conn.close()
> + if mode is not None:
> + self._setmode(relpath, mode)
> + ftp.getresp()
> + except ftplib.error_perm, e:
> + FtpTransportError("Error appending data to %s" % abspath,
> + orig_error=e)
> + except ftplib.error_temp, e:
> + if retries > _number_of_retries:
> + raise TransportError("FTP temporary error during APPEND %s." \
> + "Aborting." % abspath, orig_error=e)
> + else:
> + warning("FTP temporary error: %s. Retrying." % str(e))
> + self._FTP_instance = None
> + self._try_append(relpath, text, mode, retries+1)
> +
I do believe warning() should be done the same:
warning("FTP temporary error: %s. Retrying.", str(e))
This is a small performance issue. Technically mutter() may not do
anything if the debug level isn't low enough. And in that case it
doesn't need to take the time to format the string, also it helps to
catch UnicodeErrors, since they happen inside mutter() instead of inside
your own code.
> +
> + def _setmode(self, relpath, mode):
> + """Set permissions on a path.
> +
> + Only set permissions if the FTP server support the 'SITE CHMOD'
> + extension.
> + """
> + try:
> + ftp = self._get_FTP()
> + cmd = "SITE CHMOD %s %s" % (self._abspath(relpath), str(mode))
> + ftp.sendcmd(cmd)
> + except ftplib.error_perm, e:
> + # Command probably not available on this server
> + pass
> +
>
> def copy(self, rel_from, rel_to):
> """Copy the item at rel_from to the location at rel_to"""
> @@ -347,11 +400,12 @@
> mutter("FTP nlst: %s" % self._abspath(relpath))
> f = self._get_FTP()
> basepath = self._abspath(relpath)
> - # FTP.nlst returns paths prefixed by relpath, strip 'em
> - the_list = f.nlst(basepath)
> - stripped = [path[len(basepath)+1:] for path in the_list]
> + paths = f.nlst(basepath)
> + # If FTP.nlst returns paths prefixed by relpath, strip 'em
> + if paths[0].startswith(basepath):
> + paths = [path[len(basepath)+1:] for path in paths]
> # Remove . and .. if present, and return
> - return [path for path in stripped if path not in (".", "..")]
> + return [path for path in paths if path not in (".", "..")]
> except ftplib.error_perm, e:
> raise TransportError(orig_error=e)
>
>
+1 with a couple of fixes. Someone else needs to review it before it can
be merged, and somehow I managed to upset the pqm so it isn't able to
merge from me right now. So someone else needs to submit it anyway.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060501/5392d7b8/attachment.pgp
More information about the bazaar
mailing list