Fixed copying read-only directories

base/files/file_util_unittest.cc

Index: base/files/file_util_unittest.cc
diff --git a/base/files/file_util_unittest.cc b/base/files/file_util_unittest.cc
index bd4839aad50771e4784594ea526900d857b2ab48..f6279915038a7631877ee85de9bc32b17ba36f40 100644
--- a/base/files/file_util_unittest.cc
+++ b/base/files/file_util_unittest.cc
@@ -1378,6 +1378,29 @@ TEST_F(FileUtilTest, CopyDirectoryWithTrailingSeparators) {
   EXPECT_TRUE(PathExists(file_name_to));
 }
 
+// Sets the source file to full access.
+void SetFullAccess(const FilePath& path) {
+  #if defined(OS_WIN)
+    // On Windows, it involves clearing a bit.
+    DWORD attrs = GetFileAttributes(path.value().c_str());
+    ASSERT_NE(INVALID_FILE_ATTRIBUTES, attrs);
+    ASSERT_TRUE(SetFileAttributes(
+        path.value().c_str(), attrs & ~FILE_ATTRIBUTE_READONLY));
+    // Files in the temporary directory should not be indexed ever. If this

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

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?

[ripp, 2014/10/23 07:04:51]

TODO added to SetReadOnly function

+    // assumption change, fix this unit test accordingly.
+    // FILE_ATTRIBUTE_NOT_CONTENT_INDEXED doesn't exist on XP.
+    DWORD expected =
+        (attrs & (FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_DIRECTORY));
+    if (win::GetVersion() >= win::VERSION_VISTA)
+      expected |= FILE_ATTRIBUTE_NOT_CONTENT_INDEXED;
+    attrs = GetFileAttributes(path.value().c_str());
+    ASSERT_EQ(expected, attrs);
+  #else
+    // On all other platforms, it involves setting of all bits.
+    EXPECT_TRUE(SetPosixFilePermissions(path, S_IRUSR | S_IXUSR | S_IWUSR));
+  #endif
+}
+
 // Sets the source file to read-only.
 void SetReadOnly(const FilePath& path) {
 #if defined(OS_WIN)
@@ -1386,17 +1409,20 @@ void SetReadOnly(const FilePath& path) {
   ASSERT_NE(INVALID_FILE_ATTRIBUTES, attrs);
   ASSERT_TRUE(SetFileAttributes(
       path.value().c_str(), attrs | FILE_ATTRIBUTE_READONLY));
-  attrs = GetFileAttributes(path.value().c_str());
   // Files in the temporary directory should not be indexed ever. If this
   // assumption change, fix this unit test accordingly.
   // FILE_ATTRIBUTE_NOT_CONTENT_INDEXED doesn't exist on XP.
-  DWORD expected = FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_READONLY;
+  DWORD expected =
+      (attrs & (FILE_ATTRIBUTE_ARCHIVE | FILE_ATTRIBUTE_DIRECTORY)) |
+      FILE_ATTRIBUTE_READONLY;
   if (win::GetVersion() >= win::VERSION_VISTA)
     expected |= FILE_ATTRIBUTE_NOT_CONTENT_INDEXED;
+  attrs = GetFileAttributes(path.value().c_str());
   ASSERT_EQ(expected, attrs);
 #else
   // On all other platforms, it involves removing the write bit.
-  EXPECT_TRUE(SetPosixFilePermissions(path, S_IRUSR));
+  EXPECT_TRUE(SetPosixFilePermissions(
+      path, DirectoryExists(path) ? S_IRUSR | S_IXUSR : S_IRUSR));
 #endif
 }
 
@@ -1413,9 +1439,10 @@ bool IsReadOnly(const FilePath& path) {
 }
 
 TEST_F(FileUtilTest, CopyDirectoryACL) {
-  // Create a directory.
+  // Create source directories.
   FilePath src = temp_dir_.path().Append(FILE_PATH_LITERAL("src"));
-  CreateDirectory(src);
+  FilePath src_subdir = src.Append(FILE_PATH_LITERAL("subdir"));
+  CreateDirectory(src_subdir);
   ASSERT_TRUE(PathExists(src));

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

src_subdir ?

[ripp, 2014/10/23 07:04:51]

Done.

 
   // Create a file under the directory.
@@ -1424,12 +1451,20 @@ TEST_F(FileUtilTest, CopyDirectoryACL) {
   SetReadOnly(src_file);
   ASSERT_TRUE(IsReadOnly(src_file));
 
+  // Make data directory read-only

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

nit: remove "data" and end with period.

[ripp, 2014/10/23 07:04:51]

Done.

+  SetReadOnly(src_subdir);
+

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

ASERT_TRUE(IsReadOnly())

[ripp, 2014/10/23 07:04:51]

Done.

   // Copy the directory recursively.
   FilePath dst = temp_dir_.path().Append(FILE_PATH_LITERAL("dst"));
   FilePath dst_file = dst.Append(FILE_PATH_LITERAL("src.txt"));
   EXPECT_TRUE(CopyDirectory(src, dst, true));
 
+  FilePath dst_subdir = dst.Append(FILE_PATH_LITERAL("subdir"));
+  ASSERT_FALSE(IsReadOnly(dst_subdir));
   ASSERT_FALSE(IsReadOnly(dst_file));
+
+  // Give write permissions to allow deletion

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

nit: end with period.

[ripp, 2014/10/23 07:04:51]

Done.

+  SetFullAccess(src_subdir);

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

Why not SetReadOnly(path, false) ? There is a lot of duplication in the new
function.

[ripp, 2014/10/23 07:04:51]

Ok, moved to SetReadOnly(path, false)

 }
 
 TEST_F(FileUtilTest, CopyFile) {