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

net/ftp/ftp_directory_listing_parser_ls.cc

Index: net/ftp/ftp_directory_listing_parser_ls.cc
===================================================================
--- net/ftp/ftp_directory_listing_parser_ls.cc	(revision 131350)
+++ net/ftp/ftp_directory_listing_parser_ls.cc	(working copy)
@@ -51,21 +51,6 @@
           LooksLikeUnixPermission(text.substr(4, 3)));
 }
 
-bool LooksLikePermissionDeniedError(const string16& text) {
-  // Try to recognize a three-part colon-separated error message:
-  //
-  //   1. ftpd server name
-  //   2. directory name (often just ".")
-  //   3. message text (usually "Permission denied")
-  std::vector<string16> parts;
-  base::SplitString(CollapseWhitespace(text, false), ':', &parts);
-
-  if (parts.size() != 3)
-    return false;
-
-  return parts[2].find(ASCIIToUTF16("Permission denied")) != string16::npos;
-}
-
 // Returns the column index of the end of the date listing and detected
 // last modification time.
 bool DetectColumnOffsetAndModificationTime(const std::vector<string16>& columns,
@@ -159,10 +144,11 @@
                                                current_time,
                                                &column_offset,
                                                &entry.last_modified)) {
-      // If we can't recognize a normal listing line, maybe it's an error?
-      // In that case, just ignore the error, but still recognize the data
-      // as valid listing.
-      if (LooksLikePermissionDeniedError(lines[i]))
+      // Some servers send a message in on of the first few lines.

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

nit: "in on of the" --> "in one of the".

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

Done.

+      // All those messages have in common is the string ".:",
+      // where "." means the current directory, and ":" separates it
+      // from the rest of the message, which may be empty.
+      if (lines[i].find(ASCIIToUTF16(".:")) != string16::npos)

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

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]

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
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.

         continue;
 
       return false;