Opened 12 years ago

Closed 5 years ago

Last modified 5 years ago

#1333 enhancement closed fixed (fixed)

Survey existing FTP servers and clients to figure out how LIST works

Reported by: Jean-Paul Calderone Owned by:
Priority: low Milestone:
Component: ftp Keywords:
Cc: Jean-Paul Calderone, spiv, itamarst, jknight, jesstess, Adi Roiban Branch: branches/ftp-list-flags-1333-2
branch-diff, diff-cov, branch-cov, buildbot
Author: spiv, adiroiban

Description


Attachments (1)

1333-1.diff (2.0 KB) - added by Adi Roiban 5 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 12 years ago by Jean-Paul Calderone

Currently we special case a few options in ftp_LIST.  This sucks and we should
do something better.

comment:2 Changed 8 years ago by jesstess

Cc: jesstess added
Owner: changed from Jean-Paul Calderone to jesstess

comment:3 Changed 7 years ago by <automation>

Owner: jesstess deleted

comment:4 Changed 5 years ago by Adi Roiban

Cc: Adi Roiban added

Beside the current code:

        # bug in konqueror
        if path == "-a":
            path = ''
        # bug in gFTP 2.0.15
        if path == "-aL":
            path = ''
        # bug in Nautilus 2.10.0
        if path == "-L":
            path = ''
        # bug in ange-ftp
        if path == "-la":
            path = ''

I have also discovered the following clients:

        # bug in FireFTP
        if path == "-al":
            path = ''

        # bug in BareFTP
        if path == "-La":
            path = ''

Maybe we can use a regex or something simple like:

if path.lower() in ['-a', '-l', '-la', '-al']:
    path = ''

comment:5 Changed 5 years ago by Adi Roiban

Keywords: review added

comment:6 in reply to:  4 Changed 5 years ago by spiv

Keywords: review removed
Owner: set to spiv
Status: newassigned

Replying to adiroiban:

Maybe we can use a regex or something simple like:

if path.lower() in ['-a', '-l', '-la', '-al']:
    path = ''

I like this approach. Obviously it needs tests etc. I'll make a patch, if I don't fall asleep or otherwise get distracted first :)

comment:7 Changed 5 years ago by spiv

Author: spiv
Branch: branches/ftp-list-flags-1333

(In [34787]) Branch to ftp-list-flags-1333

comment:8 Changed 5 years ago by spiv

Keywords: review added
Owner: spiv deleted
Status: assignednew

comment:9 Changed 5 years ago by therve

Keywords: review removed
Owner: set to spiv
  1. Please name the test test_LISTWithBinLsFlags and add a docstring.
  1. This needs a NEWS fragment.

Thanks, merge once those points are fixed!

comment:10 Changed 5 years ago by therve

#5791 has been closed a duplicate of this bug.

comment:11 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: spiv deleted

I have attached a patch on top of branch 1333

I have renamed the test and add docstring. Also added docstring and renaming for neighbor tests.

It looks like the branch already has a news file.


Please switch to git/github. Working with SVN is so slow, Trac code navigation is slow and uploading patched to tickets is not fun.

Changed 5 years ago by Adi Roiban

Attachment: 1333-1.diff added

comment:12 Changed 5 years ago by Tom Prince

Author: spivspiv, tomprince
Branch: branches/ftp-list-flags-1333branches/ftp-list-flags-1333-2

(In [37410]) Branching to ftp-list-flags-1333-2.

comment:13 Changed 5 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37413]) Merge ftp-list-flags-1333-2: Handle bogus options to ftp's LIST command.

Author: spiv, adiroiban Reviewers: therve, tom.prince Fixes: #1333

The twisted.protocols.ftp.FTP server now treats "LIST -La", "LIST -al", and all other combinations of ordering and case of the "-l" and "-a" flags the same: by ignoring them rather than treating them as a pathname.

comment:14 Changed 5 years ago by Tom Prince

Author: spiv, tomprincespiv, adiroiban
Keywords: review removed

(I accidentally put my review on #5411 by mistake:)

Minor change: Prefer assertIn to assertTrue(... in ...).

I've fixed this up and merged forward.

Will merge once build results are in.

Note: See TracTickets for help on using tickets.