Fixed copying read-only directories

Fixed copying read-only directories

While directory copying, the original code creates top-level directory
with the same permissions as source one. So if source directory is
read-only, the newly created directory is also read-only, and neither files
nor subdirectories can't be created in it

R=rvargas@chromium.org

TEST=Unit test was updated to check such a use case

Committed: https://crrev.com/da4b0f2ba723c28734781887f759a969eea9191a
Cr-Commit-Position: refs/heads/master@{#301332}

[rvargas (doing something else), 2014-10-23 01:12:11]

rvargas@chromium.org changed reviewers:
+ palmer@chromium.org

[rvargas (doing something else), 2014-10-23 01:12:12]

Looks fine to me, but adding palmer to double check.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1389: // Files in the temporary directory
should not be indexed ever. If this
This seems out of place here (yes, it's that way on SetReadOnly). There is
nothing verifying that the flags look in any particular way on the POSIX path...
and if we really think it is important to verify that temp files are not indexed
there should be a dedicated test for that (create a file, inspect the
attributes).

Do you mind splitting that out, or adding a TODO for it?

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1446: ASSERT_TRUE(PathExists(src));
src_subdir ?

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1454: // Make data directory read-only
nit: remove "data" and end with period.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1456: 
ASERT_TRUE(IsReadOnly())

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1466: // Give write permissions to allow
deletion
nit: end with period.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1467: SetFullAccess(src_subdir);
Why not SetReadOnly(path, false) ? There is a lot of duplication in the new
function.

[ripp, 2014-10-23 07:04:52]

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1389: // Files in the temporary directory
should not be indexed ever. If this
On 2014/10/23 01:12:11, rvargas wrote:
> This seems out of place here (yes, it's that way on SetReadOnly). There is
> nothing verifying that the flags look in any particular way on the POSIX
path...
> and if we really think it is important to verify that temp files are not
indexed
> there should be a dedicated test for that (create a file, inspect the
> attributes).
> 
> Do you mind splitting that out, or adding a TODO for it?

TODO added to SetReadOnly function

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1446: ASSERT_TRUE(PathExists(src));
On 2014/10/23 01:12:11, rvargas wrote:
> src_subdir ?

Done.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1454: // Make data directory read-only
On 2014/10/23 01:12:11, rvargas wrote:
> nit: remove "data" and end with period.

Done.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1456: 
On 2014/10/23 01:12:11, rvargas wrote:
> ASERT_TRUE(IsReadOnly())

Done.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1466: // Give write permissions to allow
deletion
On 2014/10/23 01:12:11, rvargas wrote:
> nit: end with period.

Done.

https://codereview.chromium.org/659303002/diff/1/base/files/file_util_unittes...
base/files/file_util_unittest.cc:1467: SetFullAccess(src_subdir);
On 2014/10/23 01:12:11, rvargas wrote:
> Why not SetReadOnly(path, false) ? There is a lot of duplication in the new
> function.

Ok, moved to SetReadOnly(path, false)

[rvargas (doing something else), 2014-10-23 19:15:34]

lgtm after addressing the comments, but wait for palmer.

https://codereview.chromium.org/659303002/diff/20001/base/files/file_util_uni...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/659303002/diff/20001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1400: (attrs & (FILE_ATTRIBUTE_ARCHIVE |
FILE_ATTRIBUTE_DIRECTORY));
nit: should not be extra indentation here.

https://codereview.chromium.org/659303002/diff/20001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1413: void SetReadOnly(const FilePath& path) {
I meant we should remove this function.

[ripp, 2014-10-24 05:51:20]

https://codereview.chromium.org/659303002/diff/20001/base/files/file_util_uni...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/659303002/diff/20001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1400: (attrs & (FILE_ATTRIBUTE_ARCHIVE |
FILE_ATTRIBUTE_DIRECTORY));
On 2014/10/23 19:15:34, rvargas wrote:
> nit: should not be extra indentation here.

Done.

https://codereview.chromium.org/659303002/diff/20001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1413: void SetReadOnly(const FilePath& path) {
On 2014/10/23 19:15:34, rvargas wrote:
> I meant we should remove this function.

Done.

[palmer, 2014-10-24 18:26:46]

LGTM with nits

https://codereview.chromium.org/659303002/diff/40001/base/files/file_util_uni...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/659303002/diff/40001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1394: // TODO(ripp@yandex-team.ru): this seems
out of place here. If we really think
Nit: Move this comment to just above line 1401.

https://codereview.chromium.org/659303002/diff/40001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1407: int mode = read_only ? S_IRUSR : (S_IRUSR
| S_IWUSR);
mode_t, not int

[ripp, 2014-10-27 08:57:06]

https://codereview.chromium.org/659303002/diff/40001/base/files/file_util_uni...
File base/files/file_util_unittest.cc (right):

https://codereview.chromium.org/659303002/diff/40001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1394: // TODO(ripp@yandex-team.ru): this seems
out of place here. If we really think
On 2014/10/24 18:26:46, Chromium Palmer wrote:
> Nit: Move this comment to just above line 1401.

Done.

https://codereview.chromium.org/659303002/diff/40001/base/files/file_util_uni...
base/files/file_util_unittest.cc:1407: int mode = read_only ? S_IRUSR : (S_IRUSR
| S_IWUSR);
On 2014/10/24 18:26:46, Chromium Palmer wrote:
> mode_t, not int

Done.

[ripp, 2014-10-27 08:57:42]

The CQ bit was checked by ripp@yandex-team.ru

[commit-bot: I haz the power, 2014-10-27 08:58:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659303002/60001

[commit-bot: I haz the power, 2014-10-27 10:00:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-27 10:00:42]

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