Opened 8 years ago

Closed 5 years ago

#4181 defect closed fixed (fixed)

globbing in FTP.ftp_NLST may be incomplete

Reported by: jesstess Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: ftp Keywords:
Cc: jesstess, Itamar Turner-Trauring, Adi Roiban Branch: branches/ftp-nlst-globs-4181
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, tenth

Description


Attachments (2)

4181-nlst-globbing-1.diff (5.5 KB) - added by Adi Roiban 5 years ago.
4181-nlst-globbing-2.diff (6.3 KB) - added by Adi Roiban 5 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by Itamar Turner-Trauring

Unfortunately things like wildcards and "-a" are not part of FTP standard, but artifact of early FTP servers using /bin/ls to list files. Did you have a specific limitation in mind?

http://cr.yp.to/ftp/list.html is a handy reference to what is supported in practice (and of real world FTP in general). Looks like adding EPLF (a decent file listing command) may be worthy of another ticket.

comment:2 Changed 8 years ago by Itamar Turner-Trauring

Cc: Itamar Turner-Trauring added

comment:3 Changed 7 years ago by Glyph

Component: coreftp
Owner: changed from Glyph to spiv

comment:4 Changed 6 years ago by <automation>

Owner: spiv deleted

comment:5 Changed 5 years ago by Adi Roiban

Keywords: review added

Hi,

I have created an

The diff can be viewed here: https://github.com/adiroiban/twisted/compare/4181-nlst-globbing

Please let me know if you want me to attach a svn diff.


The changes include an utility function for checking if a segment contains a globbing expression.


I don't know if globbing is used in NLST so maybe you can chose to fix this ticket by removing globbing functionality from NLST command.


Please let me know what is the next step for having this ticket solved.

Thanks!

comment:6 Changed 5 years ago by Adi Roiban

Cc: Adi Roiban added

comment:7 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Adi Roiban

Changed 5 years ago by Adi Roiban

Attachment: 4181-nlst-globbing-1.diff added

comment:8 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have attached the diff file and I am not waiting for the review.

Thanks, Adi

comment:9 Changed 5 years ago by David Sturgis

Author: tenth
Branch: branches/nlst-globbing-4181

(In [36381]) Branching to 'nlst-globbing-4181'

comment:10 Changed 5 years ago by David Sturgis

Keywords: review removed

Thanks for the patch!

A few concerns:

  1. As per Twisted's Compatibility Policy, it's best to avoid public API additions unless they're necessary; isGlobbingExpression should probably be private.
  1. There seems to be some confusion between the code and the docstring: the argument to isGlobbingExpression is called "segments", but is referred to as "expression" in the docs. Ideally, the docstring should clearly state the type of the argument.
  1. It's not really clear what "globbing" is from the isGlobbingExpression docstring; even just a short explanation or link to additional docs would be helpful to future readers of the code. (I didn't really follow it until I read the test docstrings as well.)

comment:11 Changed 5 years ago by Glyph

Owner: set to Adi Roiban

Re-assigning so Adi can respond; thanks for all this work on FTP, we really appreciate your dedication :).

Changed 5 years ago by Adi Roiban

Attachment: 4181-nlst-globbing-2.diff added

comment:12 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to David Sturgis

Hi,

Many thanks for the review.

I have attached a new diff.

It renames isGlobbingExpression to _isGlobbingExpression

I hope that the new docstrings are helpful.

comment:13 Changed 5 years ago by Jean-Paul Calderone

Owner: changed from David Sturgis to Jean-Paul Calderone

comment:14 Changed 5 years ago by Jean-Paul Calderone

Author: tenthexarkun, tenth
Branch: branches/nlst-globbing-4181branches/ftp-nlst-globs-4181

(In [36884]) Branching to 'ftp-nlst-globs-4181'

comment:15 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed

Thanks. This looks pretty good. It needs a news file and a few doc adjustments to follow our doc conventions. I can make these simple changes and apply though. Thanks again.

comment:16 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [36888]) Merge ftp-nlst-globs-4181

Author: adiroiban Reviewer: tenth, exarkun Fixes: #4181

More completely support globbing expressions in the FTP server's handling of the NLST command by using fnmatch to detect globs, rather than having a custom, incomplete check for some subset of the globbing syntax.

Note: See TracTickets for help on using tickets.