Correctly talk to VMS servers (translate UNIX paths to VMS and vice versa).

Correctly talk to VMS servers (translate UNIX paths to VMS and vice versa).

This way we get a native VMS listing and not a UNIX emulation mode, which is
slightly different than native UNIX and confuses the parser.

TEST=Covered by net_unittests.
BUG=22193
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27129

[eroman, 2009-09-23 21:02:25]

lgtm

http://codereview.chromium.org/215058/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/215058/diff/1/2#newcode660
Line 660: line.find("unix") != std::string::npos ||
Does it matter that this will additionally recognize things like "notunix"
(since we are just scanning for a substring).

I wander if you can't use the StringTokenizer class (on LWS) instead

http://codereview.chromium.org/215058/diff/1/2#newcode674
Line 674: }
style nit: maybe de-indent this by 2?

http://codereview.chromium.org/215058/diff/1/2#newcode708
Line 708: if (line.find('"') == std::string::npos)
Is it possible for the string to contain escaped doublequotes?

http://codereview.chromium.org/215058/diff/1/2#newcode714
Line 714: if (line[line.length() - 1] != '/')
<sidecomment>
just thinking out loud: we may eventually want to break out these string path
manipulations into utility functions. I remember doing this trailing "/"
horridness in some other places too.
<sidecomment>

http://codereview.chromium.org/215058/diff/1/2#newcode998
Line 998: std::string command(system_type_ == SYSTEM_TYPE_VMS ? "LIST *.*;0" :
"LIST");
weird!

http://codereview.chromium.org/215058/diff/1/3
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/215058/diff/1/3#newcode88
Line 88: enum SystemType {
Could you add a short comment to this type?

http://codereview.chromium.org/215058/diff/1/5
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/215058/diff/1/5#newcode79
Line 79: ReplaceSubstringsAfterOffset(&result, 0, ":[000000]", "/");
Ok, I really don't know anything about VMS paths.
I will try to look at this more closely after educating myself.

http://codereview.chromium.org/215058/diff/1/6
File net/ftp/ftp_util.h (right):

http://codereview.chromium.org/215058/diff/1/6#newcode10
Line 10: namespace net {
I suggest organizing this similarly to HttpUtil for consistency.

Rather than putting the symbols directly in net::*, you can put the symbols in
net::FtpUtil::*

http://codereview.chromium.org/215058/diff/1/6#newcode14
Line 14: std::string UnixDirectoryPathToVMS(const std::string& unix_path);
I suggest adding some simple comments.

[Paweł Hajdan Jr., 2009-09-23 21:24:13]

Patch updated.

I can actually explain the LIST *.*;0 thing. It orders the server to list all
files (first *) of all types (second *) in their current version (;0). The
actual output of that is even more weird and is currently not handled correctly.

[wtc, 2009-09-23 23:45:47]

LGTM.  Some suggested changes below.

I didn't review the unit tests and the Unix/VMS path
conversion functions carefully.

http://codereview.chromium.org/215058/diff/4001/4002
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/215058/diff/4001/4002#newcode330
Line 330: std::string path(current_remote_directory_);
A comment summarizing what you're doing here would be nice.
I can't figure it out the purpose of the second "if" statement,
or rather, I don't understand why we need the current
remote directory at the beginning.

http://codereview.chromium.org/215058/diff/4001/4002#newcode331
Line 331: if (!path.empty() && path[path.length() - 1] == '/')
Since ProcessResponsePWD ensures that current_remote_directory_
ends in '/', we don't need the path[path.length() - 1] == '/'
test here.  Up to you...

http://codereview.chromium.org/215058/diff/4001/4002#newcode333
Line 333: if (request_->url.has_path() && request_->url.path() != "/")
If non-empty, request_->url.path() must start with '/',
right?

http://codereview.chromium.org/215058/diff/4001/4002#newcode657
Line 657: if (IsStringASCII(line)) {
Why do we skip this if 'line' is not ASCII?  A comment
would be nice ("Leave system_type_ as SYSTEM_TYPE_UNKNOWN"
if |line| is not ASCII), or better yet, do that with code:

  if (IsStringASCII(line)) {
    ...
  } else {
    SYSTEM_TYPE_UNKNOWN;
  }

http://codereview.chromium.org/215058/diff/4001/4002#newcode659
Line 659: if (line.find("l8") != std::string::npos ||
A comment explaining why "l8" means Unix would be nice.
It's not obvious at all.

http://codereview.chromium.org/215058/diff/4001/4002#newcode706
Line 706: if (line.find('"') != std::string::npos) {
Nit: save line.find('"') in a local variable so we
don't need to call it again on the next line.

http://codereview.chromium.org/215058/diff/4001/4002#newcode708
Line 708: if (line.find('"') == std::string::npos)
Same here: save line.find('"') in a variable so we don't
need to call it again on line 710.

http://codereview.chromium.org/215058/diff/4001/4003
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/215058/diff/4001/4003#newcode114
Line 114: // will be used in directory context, |is_directory| should be true.
Can we change
  If the path will be used in directory context
to
  If the path is a directory
or
  If the path refers to a directory
?

I don't quite understand what "directory context" means.

http://codereview.chromium.org/215058/diff/4001/4005
File net/ftp/ftp_util.cc (right):

http://codereview.chromium.org/215058/diff/4001/4005#newcode14
Line 14: 
Please add comments to describe what VMS paths look like,
and why we need separate conversion functions for Unix file
and directory paths.

http://codereview.chromium.org/215058/diff/4001/4005#newcode25
Line 25: if (unix_path[0] == '/') {
Nit: please reformat this function following our
convention of "no else after a return statement".

http://codereview.chromium.org/215058/diff/4001/4006
File net/ftp/ftp_util.h (right):

http://codereview.chromium.org/215058/diff/4001/4006#newcode12
Line 12: class FtpUtil {
Just declare these as global functions in the net
namespace.  See net/base/net_util.h, base/file_util.h,
and base/string_util.h.  Let's be consistent.

You can put these in the net::ftp_util namespace.

[Paweł Hajdan Jr., 2009-09-24 01:08:35]

On 2009/09/23 23:45:47, wtc wrote:
> http://codereview.chromium.org/215058/diff/4001/4002#newcode331
> Line 331: if (!path.empty() && path[path.length() - 1] == '/')
> Since ProcessResponsePWD ensures that current_remote_directory_
> ends in '/', we don't need the path[path.length() - 1] == '/'
> test here.  Up to you...

It is necessary, and unit tests prove that. Welcome to VMS.

> http://codereview.chromium.org/215058/diff/4001/4002#newcode333
> Line 333: if (request_->url.has_path() && request_->url.path() != "/")
> If non-empty, request_->url.path() must start with '/',
> right?

VMS disagrees. ;-)

> http://codereview.chromium.org/215058/diff/4001/4002#newcode657
> Line 657: if (IsStringASCII(line)) {
> Why do we skip this if 'line' is not ASCII?  A comment
> would be nice ("Leave system_type_ as SYSTEM_TYPE_UNKNOWN"
> if |line| is not ASCII), or better yet, do that with code:
> 
>   if (IsStringASCII(line)) {
>     ...
>   } else {
>     SYSTEM_TYPE_UNKNOWN;
>   }

Done.

> http://codereview.chromium.org/215058/diff/4001/4002#newcode659
> Line 659: if (line.find("l8") != std::string::npos ||
> A comment explaining why "l8" means Unix would be nice.
> It's not obvious at all.

I don't know why, just added a generic comment. Mozilla does the same.

> http://codereview.chromium.org/215058/diff/4001/4002#newcode706
> Line 706: if (line.find('"') != std::string::npos) {
> Nit: save line.find('"') in a local variable so we
> don't need to call it again on the next line.

Done.

> http://codereview.chromium.org/215058/diff/4001/4002#newcode708
> Line 708: if (line.find('"') == std::string::npos)
> Same here: save line.find('"') in a variable so we don't
> need to call it again on line 710.

Done.

> http://codereview.chromium.org/215058/diff/4001/4003
> File net/ftp/ftp_network_transaction.h (right):
> 
> http://codereview.chromium.org/215058/diff/4001/4003#newcode114
> Line 114: // will be used in directory context, |is_directory| should be true.
> Can we change
>   If the path will be used in directory context
> to
>   If the path is a directory
> or
>   If the path refers to a directory
> ?
> 
> I don't quite understand what "directory context" means.

Done.

> http://codereview.chromium.org/215058/diff/4001/4005
> File net/ftp/ftp_util.cc (right):
> 
> http://codereview.chromium.org/215058/diff/4001/4005#newcode14
> Line 14: 
> Please add comments to describe what VMS paths look like,
> and why we need separate conversion functions for Unix file
> and directory paths.

Done.

> http://codereview.chromium.org/215058/diff/4001/4005#newcode25
> Line 25: if (unix_path[0] == '/') {
> Nit: please reformat this function following our
> convention of "no else after a return statement".

Done.

> http://codereview.chromium.org/215058/diff/4001/4006
> File net/ftp/ftp_util.h (right):
> 
> http://codereview.chromium.org/215058/diff/4001/4006#newcode12
> Line 12: class FtpUtil {
> Just declare these as global functions in the net
> namespace.  See net/base/net_util.h, base/file_util.h,
> and base/string_util.h.  Let's be consistent.
> 
> You can put these in the net::ftp_util namespace.

eroman asked me to be consistent with http_util.h, which seems to be a closer
match.

[wtc, 2009-09-24 18:21:55]

LGTM.

http://codereview.chromium.org/215058/diff/4001/4002
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/215058/diff/4001/4002#newcode331
Line 331: if (!path.empty() && path[path.length() - 1] == '/')
I don't understand your reply.

current_remote_directory_ is initialized to an empty string
by the constructor, and is only modified by the
ProcessResponsePWD method.

Lines 714-716 in ProcessResponsePWD makes sure that
current_remote_directory_ ends in '/'.

This means, at line 331 here, if path (initialized
to current_remote_directory_) is not empty, then
path should end in '/'.

What's the flaw in my reasoning?

I don't mind having a redundant path[path.length() - 1] == '/' test.  I just
wanted to make sure I understand the
code correctly.

http://codereview.chromium.org/215058/diff/4001/4002#newcode333
Line 333: if (request_->url.has_path() && request_->url.path() != "/")
If request_->url is:
  ftp://ftp.example.com/path

is request_->url.path() equal to "/path" or just "path"?

I think it's equal to "/path", which is why I think
request_->url.path() must start with '/'.  Did I misunderstand
something?

http://codereview.chromium.org/215058/diff/4001/4006
File net/ftp/ftp_util.h (right):

http://codereview.chromium.org/215058/diff/4001/4006#newcode12
Line 12: class FtpUtil {
I agree with being consistent with net/http/http_util.h
instead.  As long as we don't introduce a new convention,
it's fine.

http://codereview.chromium.org/215058/diff/6003/7003
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/215058/diff/6003/7003#newcode667
Line 667: if (line.find("l8") != std::string::npos ||
I did a web search for "l8 ftp" and found some hits.
It seems that there is an "L8" ftp server (and client)
for Unix.

[Paweł Hajdan Jr., 2009-09-24 19:24:07]

Patch updated. Should be more straightforward now. If the code is not clear,
then something is wrong with the code, so I fixed that.

[wtc, 2009-09-24 20:37:41]

LGTM.

http://codereview.chromium.org/215058/diff/2005/2006
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/215058/diff/2005/2006#newcode336
Line 336: path = path.substr(0, path.length() - 1);
Try
    path.erase(path.length() - 1);
This erases all characters starting at index path.length() - 1.
I think
    path.resize(path.length() - 1);
will also do the trick.

http://codereview.chromium.org/215058/diff/2005/2006#newcode722
Line 722: line = line.substr(0, line.length() - 1);
Same here: try
  line.erase(line.length() - 1);
or
  line.resize(line.length() - 1);

http://codereview.chromium.org/215058/diff/2005/2007
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/215058/diff/2005/2007#newcode210
Line 210: // Current directory on the remote server, as returned by last PWD
command.
Please add
  with any trailing slash removed

[Paweł Hajdan Jr., 2009-09-24 21:01:04]

Done. Will commit when the tree re-opens.