posix copydirectory implementation

Implement CopyDirectory() for the POSIX branch of file_util.
This lets us pass some more base unit tests and at least one net test.

[Dean McNamee, 2008-09-16 11:04:37]

http://codereview.chromium.org/3077/diff/1/2
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/1/2#newcode113
Line 113: base::strlcpy(top_dir, from_path.c_str(), sizeof(top_dir));
You should use arraysize(), since strlcpy works in characters, not bytes, it's
just more correct (even though it's the same).  You should also probably fail if
it was truncated?

[Mark Mentovai, 2008-09-16 13:59:37]

I think I like this better than the other one anyway, it looks OK but this isn't
a full review yet.  Let me know if you want to use this instead of the other and
I'll give it a more careful look.

http://codereview.chromium.org/3077/diff/1/2
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/1/2#newcode118
Line 118: LOG(ERROR) << "fts_open failed: " << strerror(errno);
Doesn't look like we have enough #includes to guarantee access to strerror
<string.h> and errno <errno.h>.

http://codereview.chromium.org/3077/diff/1/2#newcode149
Line 149: // TODO(port): use a native file path rather than all these
Yup, working on this.

[Erik does not do reviews, 2008-09-16 17:41:49]

The same big comment I gave the other review applies here: we need to make the
windows and posix APIs consistent wrt wildcards.  The Windows version takes them
by accident, and at least one unit test depends on them.  We should add error
checking to enforce no wildcards in the from filename for both windows and posix
versions.

Although it's unfortunate that this work was done twice, I think this version is
likely to be better.  While the FileEnumeration API will mostly work, it doesn't
provide support for easily handling edge cases (symlinks, cycles, etc.) that I
think we should handle for the copy case.

http://codereview.chromium.org/3077/diff/1/2
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/1/2#newcode161
Line 161: error = EINVAL;
If it's a directory causing a cycle, can't we just prune it here rather than
returning an error?

[Mark Mentovai, 2008-09-16 19:22:38]

LG otherwise, and agree with Erik about nailing down the expected behavior a bit
better.  I think that can be done as a separate change.

http://codereview.chromium.org/3077/diff/1/2
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/1/2#newcode128
Line 128: to_path + (ent->fts_path + from_path.size());
Each + here means something else.  Either turn the first one into .append, or
turn the second one into [subscripting], or both, or something.

http://codereview.chromium.org/3077/diff/1/2#newcode140
Line 140: if (mkdir(target_path.c_str(), 0777) != 0) {
I wonder if we should use CreateDirectory to get mkdir -p semantics.  The
Windows version seems to.  The conversion sucks, but the conversions will all go
away soon.

http://codereview.chromium.org/3077/diff/1/2#newcode143
Line 143: break;
You've gotten pretty break-happy in here.  This isn't in its own breakable
context, so why can't we just fall to the break a couple lines down?

http://codereview.chromium.org/3077/diff/1/2#newcode153
Line 153: break;
Excess break again?

http://codereview.chromium.org/3077/diff/1/2#newcode173
Line 173: case FTS_DEFAULT:  // Some other sort of file.
I don't see FTS_W handled (whiteout, should be ignored).  We should probably
have a "skip it" case for that, and also maybe a default: label.

[Mark Mentovai, 2008-09-16 19:33:30]

http://codereview.chromium.org/3077/diff/1/2
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/1/2#newcode124
Line 124: while (!error && (ent = fts_read(fts)) != NULL) {
You should account (below this loop) for fts_read returning NULL but setting
errno.  According to the man page:

     If all the members of the hierarchy have been returned, fts_read()
     returns NULL and sets the external variable errno to 0.  If an error
     unrelated to a file in the hierarchy occurs, fts_read() returns NULL and
     sets errno appropriately.

[Evan Martin, 2008-09-16 22:25:04]

Updated.

http://codereview.chromium.org/3077/diff/1/2
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/1/2#newcode140
Line 140: if (mkdir(target_path.c_str(), 0777) != 0) {
On 2008/09/16 19:22:38, Mark Mentovai wrote:
> I wonder if we should use CreateDirectory to get mkdir -p semantics.  The
> Windows version seems to.  The conversion sucks, but the conversions will all
go
> away soon.

Since we're copying directories from the root forward, we create all
intermediate directories.  Unless the path containing the toplevel to_path
doesn't exist, but I think it's acceptable for that to be an error.

http://codereview.chromium.org/3077/diff/1/2#newcode173
Line 173: case FTS_DEFAULT:  // Some other sort of file.
On 2008/09/16 19:22:38, Mark Mentovai wrote:
> I don't see FTS_W handled (whiteout, should be ignored).  We should probably
> have a "skip it" case for that, and also maybe a default: label.

I don't have an FTS_W in my manpage.  I'd prefer for the code to explicitly
handle all cases, rather than a default: branch.  Or do you mean we should have
a default branch that flags an error?

[Evan Martin, 2008-09-16 22:57:54]

Ok, another update.

[Mark Mentovai, 2008-09-17 02:20:07]

Very nice, I like.

http://codereview.chromium.org/3077/diff/210/24
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/210/24#newcode70
Line 70: arraysize(top_dir)) < utf8_path_string.size()) {
You want >= arraysize(top_dir).  strlcpy returns the length of utf8_path without
the terminating NUL regardless of how many characters were copied.

http://codereview.chromium.org/3077/diff/210/24#newcode122
Line 122: DCHECK(to_path.find('*') == std::string::npos);
Cool.

http://codereview.chromium.org/3077/diff/210/24#newcode127
Line 127: arraysize(top_dir)) < from_path.size()) {
As above.

http://codereview.chromium.org/3077/diff/210/24#newcode151
Line 151: continue;
Yeah, you're right, that does make more sense than break.

http://codereview.chromium.org/3077/diff/210/24#newcode165
Line 165: error = EINVAL;  // We don't know the error here.
I bet you can rely on errno.  If you're paranoid, you can set errno = 0 before
CopyFile and use EINVAL instead if it's still 0 after CopyFile fails.

[Evan Martin, 2008-09-17 18:12:33]

I am sad I got strlcpy wrong.  Please look again.

[Evan Martin, 2008-09-18 17:56:28]

ping.

[Mark Mentovai, 2008-09-18 18:55:44]

LGTM

[Erik does not do reviews, 2008-09-18 19:25:18]

otherwise LGTM

http://codereview.chromium.org/3077/diff/38/218
File base/file_util_posix.cc (right):

http://codereview.chromium.org/3077/diff/38/218#newcode121
Line 121: // TODO(evanm): remove this once we're sure it's ok.
I'm not sure why we'd remove this.  I think the TODO should be to add this to
the windows version of the code once we're sure it's OK.