FTP: make the code handling server messages in directory listings more general.

FTP: make the code handling server messages in directory listings more general.

BUG=122953
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132334

[Paweł Hajdan Jr., 2012-04-12 16:40:09]

+Gavin to make you more familiar with FTP code.

[gavinp, 2012-04-12 18:03:14]

http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
net/ftp/ftp_directory_listing_parser_ls.cc:151: if
(lines[i].find(ASCIIToUTF16(".:")) != string16::npos)
LGTM.  Initially I thought this was going to not allow files with :. in their
name, but it does.

My question though is, why do we not want to show a directory listing at all if
there's a single unparseable line in the response, but only if that line doesn't
contain ".:" ?

[Paweł Hajdan Jr., 2012-04-13 16:02:34]

http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
net/ftp/ftp_directory_listing_parser_ls.cc:151: if
(lines[i].find(ASCIIToUTF16(".:")) != string16::npos)
On 2012/04/12 18:03:14, gavinp wrote:
> My question though is, why do we not want to show a directory listing at all
if
> there's a single unparseable line in the response

This is parser of "ls -l" format. We have other formats too. In this case,
returning false is more like "this is not ls -l format, try parsing this as some
other format". See also ftp_directory_listing_parser.cc

> but only if that line doesn't contain ".:" ?

If the line contains ".:", it is valid. To be more precise, there should be at
most one such line in the listing, and it should appear before any "parseable"
and meaningful lines containing actual listing entries. However, the past
experience with the parser shows that such strictness is not really worth it,
and would needlessly increase complexity of the parser. We're not doing
validation of the listing here, there is no spec anyway.

And the tests with whole listings (ftp_directory_listing_parser_unittest.cc)
should help detect cases in which the parser would confuse formats.

[gavinp, 2012-04-13 17:49:58]

On 2012/04/13 16:02:34, Paweł Hajdan Jr. wrote:
>
http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
> File net/ftp/ftp_directory_listing_parser_ls.cc (right):
> 
>
http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
> net/ftp/ftp_directory_listing_parser_ls.cc:151: if
> (lines[i].find(ASCIIToUTF16(".:")) != string16::npos)
> On 2012/04/12 18:03:14, gavinp wrote:
> > My question though is, why do we not want to show a directory listing at all
> if
> > there's a single unparseable line in the response
> 
> This is parser of "ls -l" format. We have other formats too. In this case,
> returning false is more like "this is not ls -l format, try parsing this as
some
> other format". See also ftp_directory_listing_parser.cc
> 
> > but only if that line doesn't contain ".:" ?
> 
> If the line contains ".:", it is valid. To be more precise, there should be at
> most one such line in the listing, and it should appear before any "parseable"
> and meaningful lines containing actual listing entries. However, the past
> experience with the parser shows that such strictness is not really worth it,
> and would needlessly increase complexity of the parser. We're not doing
> validation of the listing here, there is no spec anyway.
> 
> And the tests with whole listings (ftp_directory_listing_parser_unittest.cc)
> should help detect cases in which the parser would confuse formats.

I'm sold.  Still LGTM of course, and thank you very much for the explanation.

[eroman, 2012-04-13 18:37:26]

lgtm

https://chromiumcodereview.appspot.com/10068016/diff/1/net/ftp/ftp_directory_...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

https://chromiumcodereview.appspot.com/10068016/diff/1/net/ftp/ftp_directory_...
net/ftp/ftp_directory_listing_parser_ls.cc:147: // Some servers send a message
in on of the first few lines.
nit: "in on of the" --> "in one of the".

[Paweł Hajdan Jr., 2012-04-14 10:12:14]

http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
File net/ftp/ftp_directory_listing_parser_ls.cc (right):

http://codereview.chromium.org/10068016/diff/1/net/ftp/ftp_directory_listing_...
net/ftp/ftp_directory_listing_parser_ls.cc:147: // Some servers send a message
in on of the first few lines.
On 2012/04/13 18:37:26, eroman wrote:
> nit: "in on of the" --> "in one of the".

Done.

[commit-bot: I haz the power, 2012-04-14 10:12:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/10068016/7002

[commit-bot: I haz the power, 2012-04-14 12:19:09]

Change committed as 132334