Opened 10 years ago

Closed 10 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 10 years ago.
5441.2.diff (1.7 KB) - added by Adi Roiban 10 years ago.
5441.3.diff (2.7 KB) - added by Adi Roiban 10 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 10 years ago by DefaultCC Plugin

Cc: itamarst added

comment:2 Changed 10 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 10 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 10 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 10 years ago by Adi Roiban

Attachment: 5441.diff added

comment:5 Changed 10 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 10 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 10 years ago by Thijs Triemstra

Owner: Adi Roiban deleted

comment:8 in reply to:  6 Changed 10 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 10 years ago by jesstess

Cc: jesstess added
Owner: set to jesstess

comment:10 Changed 10 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 10 years ago by Adi Roiban

Attachment: 5441.2.diff added

comment:11 Changed 10 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 10 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 10 years ago by Adi Roiban

Attachment: 5441.3.diff added

comment:13 Changed 10 years ago by Adi Roiban

Hi,

I have attached a new diff against the latest trunk.

Cheers, Adi

comment:14 Changed 10 years ago by jesstess

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

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

comment:15 Changed 10 years ago by jesstess

Owner: changed from Adi Roiban to jesstess

comment:16 Changed 10 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 10 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 10 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.