Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? Dawit Alemayehu wrote: It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. Dawit Alemayehu wrote: dfaure: any further questions on this? I still think this is wrong. By contract the kioslave must emit a mimetype. Instead, ftpSendMimeType should be fixed, to detect the case where totalSize is 0, and skip the loop in that case since we have nothing to read. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? Dawit Alemayehu wrote: It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. Dawit Alemayehu wrote: dfaure: any further questions on this? David Faure wrote: I still think this is wrong. By contract the kioslave must emit a mimetype. Instead, ftpSendMimeType should be fixed, to detect the case where totalSize is 0, and skip the loop in that case since we have nothing to read. Ahh... I forgot that the mimeType signal MUST be emitted. But even if we avoid the while loop the if statement also prevents the emission of that signal. What mime-type should be emitted in case of zero sized files would the better question I guess. KMimeType::defaultMimeType()? - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? Dawit Alemayehu wrote: It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. Dawit Alemayehu wrote: dfaure: any further questions on this? David Faure wrote: I still think this is wrong. By contract the kioslave must emit a mimetype. Instead, ftpSendMimeType should be fixed, to detect the case where totalSize is 0, and skip the loop in that case since we have nothing to read. Dawit Alemayehu wrote: Ahh... I forgot that the mimeType signal MUST be emitted. But even if we avoid the while loop the if statement also prevents the emission of that signal. What mime-type should be emitted in case of zero sized files would the better question I guess. KMimeType::defaultMimeType()? No, application/x-zerosize. (defaultMimeType means unknown) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 11:35 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 11:39 a.m.) Review request for kdelibs and David Faure. Changes --- Reverted incorrect patch update. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 12:47 p.m.) Review request for kdelibs and David Faure. Changes --- Updated patch to send the proper mimetype for completely empty files (0 bytes). Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116523/#review52743
---
kioslave/ftp/ftp.cpp
https://git.reviewboard.kde.org/r/116523/#comment37197
A bit hard to read compared to
if (m_size != 0) { while(true) { ... } }
because m_size doesn't change, so it's weird to read while the size is not
0 - it either was, or it will never be.
kioslave/ftp/ftp.cpp
https://git.reviewboard.kde.org/r/116523/#comment37198
the alternative to the if+while is to just if(m_size==0) {
mimeType(zerosize); return; } before the while.
- David Faure
On March 12, 2014, 12:47 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116523/
---
(Updated March 12, 2014, 12:47 p.m.)
Review request for kdelibs and David Faure.
Bugs: 323491
http://bugs.kde.org/show_bug.cgi?id=323491
Repository: kdelibs
Description
---
The attached patch fixes a bug where copying empty files (size == 0) from an
ftp server to any other remote server (sftp, ftp) results in an error message
that states the file could not be opened.
Diffs
-
kioslave/ftp/ftp.cpp ddc6eaf
Diff: https://git.reviewboard.kde.org/r/116523/diff/
Testing
---
Attempt to copy an empty file from any ftp server to a remote destination,
e.g. sftp server.
Thanks,
Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 1:26 p.m.) Review request for kdelibs and David Faure. Changes --- updated patch. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review52761 --- Ship it! Thanks. I hope we don't get into the case of Unknown Size when the size is actually 0, though. That case would still be buggy, and I'm not sure how we could fix it. - David Faure On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 1:26 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 12, 2014, 2:39 p.m., David Faure wrote: Thanks. I hope we don't get into the case of Unknown Size when the size is actually 0, though. That case would still be buggy, and I'm not sure how we could fix it. Well I could think of one scenario where that could potentially happen. The FTP server does not respond correctly to the SIZE command and the size of the file we are trying to retrieve is actually 0, but I am sure that is a rare server like the one reported in the very old bug report #168011. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review52761 --- On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 1:26 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 13, 2014, 5:05 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review52862 --- This review has been submitted with commit 2dbcc83c25d9b2a8a20b517973f332cf67f57518 by Dawit Alemayehu to branch KDE/4.12. - Commit Hook On March 12, 2014, 1:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 12, 2014, 1:26 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? Dawit Alemayehu wrote: It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. dfaure: any further questions on this? - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
On March 4, 2014, 7:42 p.m., David Faure wrote: I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? It fails and ends up sending an error message. See the if statement in while(true) loop in ftpSendMimeType. Even if that check does not fail and the while(true) loop completes successfully, the next statement if (!buffer.isEmpty()) will be false for empty files. Hence it is completely useless to call ftpSendMimeType from ftpGet for empty files. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
Re: Review Request 116523: Do not call ftpSendMimeType for empty files in Ftp::ftpGet
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/#review51922 --- I don't get it. What's the problem with sending a mimetype for empty files? I would think this is actually expected - for all files, including empty ones. Why does this fix the bug? - David Faure On March 2, 2014, 2:20 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116523/ --- (Updated March 2, 2014, 2:20 p.m.) Review request for kdelibs and David Faure. Bugs: 323491 http://bugs.kde.org/show_bug.cgi?id=323491 Repository: kdelibs Description --- The attached patch fixes a bug where copying empty files (size == 0) from an ftp server to any other remote server (sftp, ftp) results in an error message that states the file could not be opened. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116523/diff/ Testing --- Attempt to copy an empty file from any ftp server to a remote destination, e.g. sftp server. Thanks, Dawit Alemayehu
