Ticket #4181 defect closed fixed

Opened 4 years ago

Last modified 15 months ago

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

4181-nlst-globbing-1.diff Download (5.5 KB) - added by adiroiban 21 months ago.
4181-nlst-globbing-2.diff Download (6.3 KB) - added by adiroiban 17 months ago.

Change History

1

Changed 4 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.

2

Changed 4 years ago by itamar

  • cc itamar added

3

Changed 4 years ago by glyph

  • owner changed from glyph to spiv
  • component changed from core to ftp

4

Changed 3 years ago by <automation>

  • owner spiv deleted

5

Changed 22 months 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!

6

Changed 22 months ago by adiroiban

  • cc adi@… added

7

Changed 22 months ago by exarkun

  • keywords review removed
  • owner set to adiroiban

Changed 21 months ago by adiroiban

8

Changed 21 months ago by adiroiban

  • owner adiroiban deleted
  • keywords review added

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

Thanks, Adi

9

Changed 18 months ago by tenth

  • branch set to branches/nlst-globbing-4181
  • branch_author set to tenth

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

10

Changed 18 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.)

11

Changed 18 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 17 months ago by adiroiban

12

Changed 17 months ago by adiroiban

  • owner changed from adiroiban to tenth
  • keywords review added

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.

13

Changed 15 months ago by exarkun

  • owner changed from tenth to exarkun

14

Changed 15 months ago by exarkun

  • branch changed from branches/nlst-globbing-4181 to branches/ftp-nlst-globs-4181
  • branch_author changed from tenth to exarkun, tenth

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

15

Changed 15 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.

16

Changed 15 months ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

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