Opened 5 years ago

Closed 19 months ago

#4181 defect closed fixed (fixed)

globbing in FTP.ftp_NLST may be incomplete

Reported by: jesstess Owned by: exarkun
Priority: normal Milestone:
Component: ftp Keywords:
Cc: jesstess, itamar, adi@… Branch: branches/ftp-nlst-globs-4181
(diff, github, buildbot, log)
Author: exarkun, tenth Launchpad Bug:

Description


Attachments (2)

4181-nlst-globbing-1.diff (5.5 KB) - added by adiroiban 2 years ago.
4181-nlst-globbing-2.diff (6.3 KB) - added by adiroiban 22 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by itamar

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 5 years ago by itamar

  • Cc itamar added

comment:3 Changed 4 years ago by glyph

  • Component changed from core to ftp
  • Owner changed from glyph to spiv

comment:4 Changed 4 years ago by <automation>

  • Owner spiv deleted

comment:5 Changed 2 years ago by adiroiban

  • 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 2 years ago by adiroiban

  • Cc adi@… added

comment:7 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to adiroiban

Changed 2 years ago by adiroiban

comment:8 Changed 2 years ago by adiroiban

  • Keywords review added
  • Owner adiroiban deleted

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

Thanks,
Adi

comment:9 Changed 22 months ago by tenth

  • Author set to tenth
  • Branch set to branches/nlst-globbing-4181

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

comment:10 Changed 22 months ago by tenth

  • 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 22 months ago by glyph

  • Owner set to adiroiban

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

Changed 22 months ago by adiroiban

comment:12 Changed 22 months ago by adiroiban

  • Keywords review added
  • Owner changed from adiroiban to tenth

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 19 months ago by exarkun

  • Owner changed from tenth to exarkun

comment:14 Changed 19 months ago by exarkun

  • Author changed from tenth to exarkun, tenth
  • Branch changed from branches/nlst-globbing-4181 to branches/ftp-nlst-globs-4181

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

comment:15 Changed 19 months ago by exarkun

  • 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 19 months ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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.