Opened 9 years ago

Closed 9 years ago

#5441 defect closed fixed (fixed)

FTP.ftp_retr should check openForReading for IsADirectoryError

Reported by: Adi Roiban Owned by: jesstess
Priority: normal Milestone:
Component: ftp Keywords:
Cc: itamarst, Thijs Triemstra, jesstess Branch: branches/ftpretr-directory-5441
branch-diff, diff-cov, branch-cov, buildbot
Author: jesstess

Description (last modified by Thijs Triemstra)

Right now FTP.ftp_retr is checking openForReading call for IsNotADirectory, while it should check for IsADirectory as shell.openForReading is raising IsADirectory exception.

I will attach a branch with the fix.

Attachments (3)

5441.diff (2.0 KB) - added by Adi Roiban 9 years ago.
5441.2.diff (1.7 KB) - added by Adi Roiban 9 years ago.
5441.3.diff (2.7 KB) - added by Adi Roiban 9 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by DefaultCC Plugin

Cc: itamarst added

comment:2 Changed 9 years ago by Adi Roiban

Keywords: review added
Owner: set to Adi Roiban

code is here: https://code.launchpad.net/~adiroiban/twisted/5441 MP is here: https://code.launchpad.net/~adiroiban/twisted/5441/+merge/86916

Please let me know if you want to merge/land request in another format.

Cheers

comment:3 Changed 9 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Description: modified (diff)
Keywords: review removed

Thanks for your contribution. Our development process prefers that you supply a patch against trunk that we can review, see TwistedDevelopment#SubmittingaPatch for more information. You can also read the Coding standard and other policies for more information.

comment:4 Changed 9 years ago by Glyph

Keywords: review added

thijs,

Please don't reject legitimate contributions unless you've carefully inspected them.

A launchpad merge proposal is trivial to convert to a plain text diff, so we can accept them. (Some reviewers might actually prefer them.)

If you're not familiar with bzr and launchpad, there's a button, "diff against target". which will take you here: https://code.launchpad.net/~adiroiban/twisted/5441/+merge/86916#review-diff

and you can click "download diff" to get a plain-text diff that should apply to trunk.

Sorry for the delay, adiroiban! I'm putting this back into review, so hopefully someone will get to it soon...

Changed 9 years ago by Adi Roiban

Attachment: 5441.diff added

comment:5 Changed 9 years ago by Adi Roiban

Hi,

I have attached a diff generated using svn.

The Launchpad Merge Proposal page is done against twisted/trunk and contains a diff web view and a link for downloading a patch file. My previous changes were accepted based on bzr branches.

Cheers, Adi

comment:6 in reply to:  4 ; Changed 9 years ago by Thijs Triemstra

Replying to glyph:

thijs,

Please don't reject legitimate contributions unless you've carefully inspected them.

A launchpad merge proposal is trivial to convert to a plain text diff, so we can accept them. (Some reviewers might actually prefer them.)

If you're not familiar with bzr and launchpad, there's a button, "diff against target". which will take you here: https://code.launchpad.net/~adiroiban/twisted/5441/+merge/86916#review-diff

and you can click "download diff" to get a plain-text diff that should apply to trunk.

Alright, I wasn't aware of that. I guess the TwistedDevelopment page should point this out then, because I couldn't find anything official about the fact it's ok to use github/bitbucket/bzr/etc and let the core developers figure out how to generate a plain text diff (no matter how easy it is). The development page also suggest: "If you don't have commit access to Twisted, please submit changesets in diff -u format to the ticket tracker. "

comment:7 Changed 9 years ago by Thijs Triemstra

Owner: Adi Roiban deleted

comment:8 in reply to:  6 Changed 9 years ago by Glyph

Replying to thijs:

Alright, I wasn't aware of that. I guess the TwistedDevelopment page should point this out then, because I couldn't find anything official about the fact it's ok to use github/bitbucket/bzr/etc and let the core developers figure out how to generate a plain text diff (no matter how easy it is). The development page also suggest: "If you don't have commit access to Twisted, please submit changesets in diff -u format to the ticket tracker. "

This is true, and more or less correct; ideally, contributors would also provide a patch. It's just that saying that they should generate a patch doesn't really constitute a full review :). By all means, feel free to stick a comment on a ticket saying that a patch may accelerate the process of getting it reviewed; it should just remain in the review state in case another twisted core dev is comfortable with the contributor's selected method of contribution.

-glyph

comment:9 Changed 9 years ago by jesstess

Cc: jesstess added
Owner: set to jesstess

comment:10 Changed 9 years ago by jesstess

Keywords: review removed
Owner: changed from jesstess to Adi Roiban

Thanks for the bug report and patch, adiroiban! This patch looks good; I just have a couple of cosmetic/documentation comments:

  • Tests should start with "test_", followed by a camel-case name where the first word starts lower-cased, test_downloadFolder in this case. (test_ftp.py is quite old and has a bunch of code that hasn't been updated to meet the current testing standard)
  • test_downloadFolder needs a docstring.
  • "Downloading a file should not succeed" ==> "Downloading a folder should not succeed"
  • While you're touching ftp_RETR, can you give it a docstring?

Thanks again!

Changed 9 years ago by Adi Roiban

Attachment: 5441.2.diff added

comment:11 Changed 9 years ago by Adi Roiban

Keywords: review added

Thanks for the review!

I have attached a new patch and also updated the bzr branch. The patch is built on top of the previous patch. Please let me know if a full patch against the base or latest version is required.

Cheers, Adi

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

All things equal, I think most people would find a patch against trunk more useful than a patch against a bzr branch. Using the latter involves applying multiple patches or getting the bzr branch, and once you have the bzr branch you don't need the patch file anymore.

Changed 9 years ago by Adi Roiban

Attachment: 5441.3.diff added

comment:13 Changed 9 years ago by Adi Roiban

Hi,

I have attached a new diff against the latest trunk.

Cheers, Adi

comment:14 Changed 9 years ago by jesstess

Author: jesstess
Branch: branches/ftpretr-directory-5441

(In [33433]) Branching to 'ftpretr-directory-5441'

comment:15 Changed 9 years ago by jesstess

Owner: changed from Adi Roiban to jesstess

comment:16 Changed 9 years ago by jesstess

(In [33434]) Apply patch 5441.3.diff by adiroiban.

With some tiny cosmetic tweaks (docstring """ on lines by themselves).

refs #5441

comment:17 Changed 9 years ago by jesstess

Keywords: review removed

Thanks for the updated patch, adiroiban. Buildbot results look good, so I'll go ahead and merge.

comment:18 Changed 9 years ago by jesstess

Resolution: fixed
Status: newclosed

(In [33435]) Merge ftpretr-directory-5441

Author: adiroiban Reviewer: jesstess Fixes: #5441

Correct a check in FTP.ftp_retr from IsNotADirectoryError to IsADirectoryError. Now calling RETR on a directory won't be logged as an unexpected error.

Note: See TracTickets for help on using tickets.