Fix FTP dir listing parser to support servers returning a "total n" line with an incorrect negative…

Fix FTP dir listing parser to allow total size to be negative,
which some buggy servers do.

Patch by tfh@skip.org

R=mmenke@chromium.org
BUG=536811
Committed: https://chromium.googlesource.com/chromium/src/+/dea09dc050e820e7e38ea705ea6a0bb9e5f874a1

[tfh, 2015-11-19 01:06:07]

Description was changed from

==========
Fix FTP dir listing parser to support servers returning a "total n" line with an
incorrect negative total.

BUG=536811
==========

to

==========
Fix FTP dir listing parser to support servers returning a "total n" line with an
incorrect negative total.

BUG=536811
==========

[tfh, 2015-11-19 01:06:07]

tfh@skip.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2015-11-19 17:47:35]

LGTM, thanks for the patch!

And, for the record, I verified you've also signed the CLA agreement.

You can click the commit box at your leisure.

[tfh, 2015-11-19 20:38:48]

The CQ bit was checked by tfh@skip.org

[commit-bot: I haz the power, 2015-11-19 20:40:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1451993007/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1451993007/1

[commit-bot: I haz the power, 2015-11-19 21:09:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 21:09:17]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mmenke, 2015-11-19 21:18:56]

On 2015/11/19 21:09:17, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

Hrm...So that is a failure in the new test case you added, but it doesn't look
to be affecting the other bots that have run that test so far.

It may have something to do with the magnitude of the total, though it would be
surprising if our int64 parsing varies by platform.

[tfh, 2015-11-19 22:08:29]

On 2015/11/19 21:18:56, mmenke wrote:
> On 2015/11/19 21:09:17, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> 
> Hrm...So that is a failure in the new test case you added, but it doesn't look
> to be affecting the other bots that have run that test so far.
> 

> [ RUN      ] FtpDirectoryListingParserTest.Parse/dir_listing_ls_33
> ../../net/ftp/ftp_directory_listing_parser_unittest.cc:79: Failure
> Value of: lines.size()
>   Actual: 1
> Expected: 9 * entries.size()
> Which is: 27
> Google Test trace:
> ../../net/ftp/ftp_directory_listing_parser_unittest.cc:54: Test case:
dir-listing-ls-33

The dir-listing-ls-33.expected I committed has \r\n line endings (like the other
.expected files), as the unit test requires them (it does a 
base::SplitStringUsingSubstr(expected_listing, "\r\n", &lines)). It looks like
those \r\n have been replaced with \n somewhere during/after "git cl upload".
The local commit definitely contains them.

[mmenke, 2015-11-19 22:18:53]

On 2015/11/19 22:08:29, tfh wrote:
> On 2015/11/19 21:18:56, mmenke wrote:
> > On 2015/11/19 21:09:17, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> > 
> > Hrm...So that is a failure in the new test case you added, but it doesn't
look
> > to be affecting the other bots that have run that test so far.
> > 
> 
> > [ RUN      ] FtpDirectoryListingParserTest.Parse/dir_listing_ls_33
> > ../../net/ftp/ftp_directory_listing_parser_unittest.cc:79: Failure
> > Value of: lines.size()
> >   Actual: 1
> > Expected: 9 * entries.size()
> > Which is: 27
> > Google Test trace:
> > ../../net/ftp/ftp_directory_listing_parser_unittest.cc:54: Test case:
> dir-listing-ls-33
> 
> The dir-listing-ls-33.expected I committed has \r\n line endings (like the
other
> .expected files), as the unit test requires them (it does a 
> base::SplitStringUsingSubstr(expected_listing, "\r\n", &lines)). It looks like
> those \r\n have been replaced with \n somewhere during/after "git cl upload".
> The local commit definitely contains them.

Most other files require line endings of LF.  It looks like the upload script
(Or rietveld) is incorrectly replacing the CRLFs with LFs. 
https://code.google.com/p/chromium/issues/detail?id=525042

Looks like I'll have to land this manually for you.  We could modify the line
format used in the test files, but then we'd have to manually replace the line
endings in tests, and lose the ability to check files that use just LF line
endings.

[mmenke, 2015-11-19 22:25:07]

On 2015/11/19 22:18:53, mmenke wrote:
> On 2015/11/19 22:08:29, tfh wrote:
> > On 2015/11/19 21:18:56, mmenke wrote:
> > > On 2015/11/19 21:09:17, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> > > 
> > > Hrm...So that is a failure in the new test case you added, but it doesn't
> look
> > > to be affecting the other bots that have run that test so far.
> > > 
> > 
> > > [ RUN      ] FtpDirectoryListingParserTest.Parse/dir_listing_ls_33
> > > ../../net/ftp/ftp_directory_listing_parser_unittest.cc:79: Failure
> > > Value of: lines.size()
> > >   Actual: 1
> > > Expected: 9 * entries.size()
> > > Which is: 27
> > > Google Test trace:
> > > ../../net/ftp/ftp_directory_listing_parser_unittest.cc:54: Test case:
> > dir-listing-ls-33
> > 
> > The dir-listing-ls-33.expected I committed has \r\n line endings (like the
> other
> > .expected files), as the unit test requires them (it does a 
> > base::SplitStringUsingSubstr(expected_listing, "\r\n", &lines)). It looks
like
> > those \r\n have been replaced with \n somewhere during/after "git cl
upload".
> > The local commit definitely contains them.
> 
> Most other files require line endings of LF.  It looks like the upload script
> (Or rietveld) is incorrectly replacing the CRLFs with LFs. 
> https://code.google.com/p/chromium/issues/detail?id=525042
> 
> Looks like I'll have to land this manually for you.  We could modify the line
> format used in the test files, but then we'd have to manually replace the line
> endings in tests, and lose the ability to check files that use just LF line
> endings.

Actually...It's just the expected file that's using CRLFs here?  I think we can
just switch those over to use LFs.

[mmenke, 2015-11-19 23:24:14]

And...turns out the CRLFs are in fact needed.  I'll land this manually tomorrow,
so if I manage to still mangle line endings, I'll be around to revert it.

[mmenke, 2015-11-20 17:09:56]

Description was changed from

==========
Fix FTP dir listing parser to support servers returning a "total n" line with an
incorrect negative total.

BUG=536811
==========

to

==========
Fix FTP dir listing parser to allow total size to be negative,
which some buggy servers do.

Patch by tfh@skip.org

BUG=536811
==========

[mmenke, 2015-11-20 17:10:07]

Description was changed from

==========
Fix FTP dir listing parser to allow total size to be negative,
which some buggy servers do.

Patch by tfh@skip.org

BUG=536811
==========

to

==========
Fix FTP dir listing parser to allow total size to be negative,
which some buggy servers do.

Patch by tfh@skip.org

R=mmenke@chromium.org
BUG=536811
==========

[commit-bot: I haz the power, 2015-11-20 17:16:55]

Patchset 1 (id:??) landed as
https://crrev.com/dea09dc050e820e7e38ea705ea6a0bb9e5f874a1
Cr-Commit-Position: refs/heads/master@{#360845}

[mmenke, 2015-11-20 17:17:22]

Committed patchset #1 (id:1) manually as
dea09dc050e820e7e38ea705ea6a0bb9e5f874a1 (presubmit successful).