Fix CR duplication in ASCII mode download #492
Conversation
|
How does this compare with what other FTP servers do in ASCII mode? |
pyftpdlib/handlers.py
Outdated
| pos = chunk.find(b'\n', pos) | ||
| if pos == -1: | ||
| break | ||
| if chunk[pos - 1] != 13: |
There was a problem hiding this comment.
It is ord('\r'). I guess you'll suggest to create a constant like CR = ord('\r'). Am I correct?
There was a problem hiding this comment.
Yep. I would define it at the top of this method.
There was a problem hiding this comment.
This way, ord('\r') will be executed each time the method is invoked.
I defined CR_BYTE at the top of handlers.py, which I believe is better. Is this ok?
There was a problem hiding this comment.
You're right. Yeah leave it there. =)
PureFTPd has the same issue. ProFTPd handles it correctly and does not convert CRLF to CRCRLF. |
pyftpdlib/test/__init__.py
Outdated
| try: | ||
| import sendfile | ||
| except ImportError: | ||
| sendfile = None |
There was a problem hiding this comment.
This was on purpose. I want tests to fail (and notice it) if pysendfile is not installed. (and make setup-dev-env will take care of that)
There was a problem hiding this comment.
Thank you for explanation. Reverted my changes.
| self.dummyfile.seek(0) | ||
| datafile = self.dummyfile.read() | ||
| self.assertEqual(len(data), len(datafile)) | ||
| self.assertEqual(hash(data), hash(datafile)) |
There was a problem hiding this comment.
does this test fail before applying the patch?
There was a problem hiding this comment.
Yes, it does.
|
Could you please update HISTORY file describing the problem? That aside this LGTM. |
|
...and CREDITS (add your name) |
|
Should I bump the version up to 1.5.5 in HISTORY.rst? |
|
Also, should I create an issue here on GitHub describing the problem in order to conform the HISTORY.rst format (#issue reference) or may I just refer to this PR? |
|
Thanks. |
|
Thank you! |
|
Given that the code no longer uses |
|
Regarding Also, I believe we should not rely on |
|
We support from 2.6 onwards. I'm not sure if there are systems other than OSX9 where os.sep == '\r'. |
|
Just to clarify - I don't need support for |
|
that goes without saying :)
thanks for pointing it out
On Wed, 27 Mar 2019 at 18:30, Andrew Scheller ***@***.***> wrote:
Just to clarify - I don't *need* support for \r line endings, I was just
pointing it out in case it was something that got overlooked... 😉
(i.e. I won't be upset if support doesn't get added)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#492 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAplLLaazLlP8o1RMT8rVkdiyitw4TpQks5va6q1gaJpZM4cMIjd>
.
--
Giampaolo - http://grodola.blogspot.com
|
|
I've added CR to CRLF converion support in #494. |
If a file has CRLF already, LF has been still replaced by CRLF, which resulted in CRCRLF line endings.