Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#4986 defect closed fixed (fixed)

FTPFileListProtocol does not support files with spaces in them

Reported by: jrydberg Owned by: David Sturgis
Priority: normal Milestone:
Component: ftp Keywords:
Cc: itamarst, Adi Roiban Branch: branches/ftp-filename-spaces-4986
branch-diff, diff-cov, branch-cov, buildbot
Author: tenth

Description

Have a directory with a few files with spaces in their name, and they do not show up in my listing when I use twisted.protocols.ftp.FTPFileListProtocol.

from twisted.protocols.ftp import FTPFileListProtocol
p = FTPFileListProtocol()
data1 = "-rwxr-xr-x    1 1003     1003     186352836 Mar 17 09:55 Big Buck Bunny_2436000.ismv"
data2 = "-rwxr-xr-x    1 1003     1003     186352836 Mar 17 09:55 Big_Buck_Bunny_2436000.ismv"
p.lineReceived(data1)
p.lineReceived(data2)
assert len(p.files) == 2

Twisted version 10.2

Attachments (3)

4986-file-with-space-1.diff (1.9 KB) - added by Adi Roiban 6 years ago.
4986-file-with-space-2.diff (2.5 KB) - added by Adi Roiban 6 years ago.
4986-file-with-space-3.diff (3.7 KB) - added by Adi Roiban 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by DefaultCC Plugin

Cc: itamarst added

Changed 6 years ago by Adi Roiban

Attachment: 4986-file-with-space-1.diff added

comment:2 Changed 6 years ago by Adi Roiban

Cc: Adi Roiban added
Keywords: review added

Hi,

I have attached a patch that attempts to fix this issue.

It replaces:

(?P<filename>([^ ]|\\ )*?)

with

(?P<filename>(.){1,}?)

I don't know why this expression was used: ([^ ]|\\ )* and why space characters were explicitly denied.

Maybe I am not understanding something right and my patch is not valid.

Please let me know if you spot any errors.

Adi

Changed 6 years ago by Adi Roiban

Attachment: 4986-file-with-space-2.diff added

comment:3 in reply to:  2 Changed 5 years ago by spiv

Keywords: review removed
Owner: set to Adi Roiban

Replying to adiroiban:

I don't know why this expression was used: ([^ ]|\\ )* and why space characters were explicitly denied.

That appears to be intended to only match backslash-escaped spaces. Note that parseDirectoryLine will unescape those.

I think it would be reasonable to attempt to accept unescaped spaces too, with appropriate care taken to ensure the existing unescaping functionality is preserved. That means adding unit tests for it, as there don't appear to be any yet.

Aside from that, your patch looks sound. I'm a bit hesitant to clutter the docstring with examples of various cases it handles, but I'd rather more documentation than less, so it's ok.

This patch will also need a news entry (i.e. add twisted/topfiles/4986.bug, as per http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles).

Changed 5 years ago by Adi Roiban

Attachment: 4986-file-with-space-3.diff added

comment:4 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to spiv

Hi,

Many thanks for the review.

I have added the required news file together with a test for escaped strings.

Please let me know if other changes are required. Adi

comment:5 Changed 5 years ago by David Sturgis

Owner: spiv deleted

Hi adiroiban, just wanted to suggest that when a ticket is ready for re-review, you should probably leave it as unassigned, with the review keyword, to give it the best chance of being picked up by a reviewer.

comment:6 Changed 5 years ago by David Sturgis

Author: tenth
Branch: branches/ftp-filename-spaces-4986

(In [36274]) Branching to 'ftp-filename-spaces-4986'

comment:7 Changed 5 years ago by David Sturgis

Build Results for patch 4986-file-with-space-3.diff

comment:8 Changed 5 years ago by David Sturgis

Keywords: review removed
Owner: set to David Sturgis

Hi adiroiban,

Unfortunately, the doc changes in twisted.protocols.ftp aren't properly formatted for epytext, and are currently causing a build error:

Please take a look at the Tools For Development page for more information on how to get pydoctor and epytext, which will allow you to test your doc changes. And some documentation for epytext markup itself can be found here.

Otherwise this patch looks good; I'm going to make a quick fix to the docs (to mark the examples starting at line 2874 as a literal block) and then land this branch.

comment:9 Changed 5 years ago by David Sturgis

(In [36378]) Made the examples a literal block: refs #4986

comment:10 Changed 5 years ago by David Sturgis

Resolution: fixed
Status: newclosed

(In [36380]) Merge ftp-filename-spaces-4986: FTPFileListProtocol does not support files with spaces in them

Author: adiroiban Reviewer: spiv, tenth Fixes: #4986

Fixes a bug wherein twisted.protocols.ftp.FTPFileListProtocol does not list filenames containing spaces.

comment:11 Changed 5 years ago by Adi Roiban

Thanks for the review and for the link to Development Tools.

Until now I was only using "trial" to validate the code. I will try to compile epydoc before requesting a review.

Cheers, Adi

Note: See TracTickets for help on using tickets.