CreateDirectory() should check if an existing path is actually a directory before skipping it. Also update a couple instances and comments to reflect current behaviour (see also http://codereview.chromium.org/1681).

Review URL: http://codereview.chromium.org/1709

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@2060 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 63 insertions, 20 deletions

diff --git a/base/file_util.h b/base/file_util.h
index 304d3cb..45d4a33 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -151,6 +151,9 @@ bool PathExists(const std::wstring& path);
 // Returns true if the given path is writable by the user, false otherwise.
 bool PathIsWritable(const std::wstring& path);
 
+// Returns true if the given path exists and is a directory, false otherwise.
+bool DirectoryExists(const std::wstring& path);
+
 #if defined(OS_WIN)
 // Gets the creation time of the given file (expressed in the local timezone),
 // and returns it via the creation_time parameter.  Returns true if successful,
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index 201a2c0..f6beb57 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -111,6 +111,13 @@ bool PathExists(const std::wstring& path) {
   return (stat64(WideToUTF8(path).c_str(), &file_info) == 0);
 }
 
+bool DirectoryExists(const std::wstring& path) {
+  struct stat64 file_info;
+  if (stat64(WideToUTF8(path).c_str(), &file_info) == 0)
+    return S_ISDIR(file_info.st_mode);
+  return false;
+}
+
 // TODO(erikkay): implement
 #if 0
 bool GetFileCreationLocalTimeFromHandle(int fd,
@@ -179,7 +186,7 @@ bool CreateDirectory(const std::wstring& full_path) {
       path = *i;
     else
       AppendToPath(&path, *i);
-    if (!PathExists(path)) {
+    if (!DirectoryExists(path)) {
       if (mkdir(WideToUTF8(path).c_str(), 0777) != 0)
         return false;
     }
@@ -324,5 +331,3 @@ std::wstring FileEnumerator::Next() {
   
   
 } // namespace file_util
-
-
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index a93eda3..142967e 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -517,7 +517,7 @@ TEST_F(FileUtilTest, CopyFile) {
   std::wstring dest_file(dir_name_from);
   file_util::AppendToPath(&dest_file, L"DestFile.txt");
   ASSERT_TRUE(file_util::CopyFile(file_name_from, dest_file));
-  
+
   // Copy the file to another location using '..' in the path.
   std::wstring dest_file2(dir_name_from);
   file_util::AppendToPath(&dest_file2, L"..");
@@ -697,15 +697,15 @@ TEST_F(FileUtilTest, CreateShortcutTest) {
 TEST_F(FileUtilTest, CreateTemporaryFileNameTest) {
   std::wstring temp_file;
   file_util::CreateTemporaryFileName(&temp_file);
-  EXPECT_EQ(file_util::PathExists(temp_file), true);
-  EXPECT_EQ(file_util::Delete(temp_file, false), true);
+  EXPECT_TRUE(file_util::PathExists(temp_file));
+  EXPECT_TRUE(file_util::Delete(temp_file, false));
 }
 
 TEST_F(FileUtilTest, CreateNewTempDirectoryTest) {
   std::wstring temp_dir;
   file_util::CreateNewTempDirectory(std::wstring(), &temp_dir);
-  EXPECT_EQ(file_util::PathExists(temp_dir), true);
-  EXPECT_EQ(file_util::Delete(temp_dir, false), true);
+  EXPECT_TRUE(file_util::PathExists(temp_dir));
+  EXPECT_TRUE(file_util::Delete(temp_dir, false));
 }
 
 TEST_F(FileUtilTest, CreateDirectoryTest) {
@@ -717,13 +717,44 @@ TEST_F(FileUtilTest, CreateDirectoryTest) {
 #elif defined(OS_POSIX)
   file_util::AppendToPath(&test_path, L"dir/tree/likely/doesnt/exist/");
 #endif
-  
-  EXPECT_EQ(file_util::PathExists(test_path), false);
-  EXPECT_EQ(file_util::CreateDirectory(test_path), true);
-  EXPECT_EQ(file_util::PathExists(test_path), true);
-  EXPECT_EQ(file_util::Delete(test_root, true), true);
-  EXPECT_EQ(file_util::PathExists(test_root), false);
-  EXPECT_EQ(file_util::PathExists(test_path), false);
+
+  EXPECT_FALSE(file_util::PathExists(test_path));
+  EXPECT_TRUE(file_util::CreateDirectory(test_path));
+  EXPECT_TRUE(file_util::PathExists(test_path));
+  // CreateDirectory returns true if the DirectoryExists returns true.
+  EXPECT_TRUE(file_util::CreateDirectory(test_path));
+
+  // Doesn't work to create it on top of a non-dir
+  file_util::AppendToPath(&test_path, L"foobar.txt");
+  EXPECT_FALSE(file_util::PathExists(test_path));
+  CreateTextFile(test_path, L"test file");
+  EXPECT_TRUE(file_util::PathExists(test_path));
+  EXPECT_FALSE(file_util::CreateDirectory(test_path));
+
+  EXPECT_TRUE(file_util::Delete(test_root, true));
+  EXPECT_FALSE(file_util::PathExists(test_root));
+  EXPECT_FALSE(file_util::PathExists(test_path));
+}
+
+TEST_F(FileUtilTest, DetectDirectoryTest) {
+  // Check a directory
+  std::wstring test_root = test_dir_;
+  file_util::AppendToPath(&test_root, L"detect_directory_test");
+  EXPECT_FALSE(file_util::PathExists(test_root));
+  EXPECT_TRUE(file_util::CreateDirectory(test_root));
+  EXPECT_TRUE(file_util::PathExists(test_root));
+  EXPECT_TRUE(file_util::DirectoryExists(test_root));
+
+  // Check a file
+  std::wstring test_path(test_root);
+  file_util::AppendToPath(&test_path, L"foobar.txt");
+  EXPECT_FALSE(file_util::PathExists(test_path));
+  CreateTextFile(test_path, L"test file");
+  EXPECT_TRUE(file_util::PathExists(test_path));
+  EXPECT_FALSE(file_util::DirectoryExists(test_path));
+  EXPECT_TRUE(file_util::Delete(test_path, false));
+
+  EXPECT_TRUE(file_util::Delete(test_root, true));
 }
 
 static const struct goodbad_pair {
diff --git a/base/file_util_win.cc b/base/file_util_win.cc
index f5e39fa..f2822d1 100644
--- a/base/file_util_win.cc
+++ b/base/file_util_win.cc
@@ -176,6 +176,13 @@ bool PathIsWritable(const std::wstring& path) {
   return true;
 }
 
+bool DirectoryExists(const std::wstring& path) {
+  DWORD fileattr = GetFileAttributes(path.c_str());
+  if (fileattr != INVALID_FILE_ATTRIBUTES)
+    return (fileattr & FILE_ATTRIBUTE_DIRECTORY) != 0;
+  return false;
+}
+
 bool GetFileCreationLocalTimeFromHandle(HANDLE file_handle,
                                         LPSYSTEMTIME creation_time) {
   if (!file_handle)
@@ -419,7 +426,7 @@ bool CreateNewTempDirectory(const std::wstring& prefix,
 }
 
 bool CreateDirectory(const std::wstring& full_path) {
-  if (PathExists(full_path))
+  if (DirectoryExists(full_path))
     return true;
   int err = SHCreateDirectoryEx(NULL, full_path.c_str(), NULL);
   return err == ERROR_SUCCESS;
diff --git a/base/path_service.cc b/base/path_service.cc
index 37ac433..078815d 100644
--- a/base/path_service.cc
+++ b/base/path_service.cc
@@ -205,9 +205,7 @@ bool PathService::Override(int key, const std::wstring& path) {
     return false;
 
   // make sure the directory exists:
-  if (!file_util::PathExists(file_path) &&
-      // TODO(darin): what if this path is not that of a directory?
-      !file_util::CreateDirectory(file_path))
+  if (!file_util::CreateDirectory(file_path))
     return false;
 
   file_util::TrimTrailingSeparator(&file_path);
@@ -251,4 +249,3 @@ void PathService::RegisterProvider(ProviderFunc func, int key_start,
 #endif
   path_data->providers = p;
 }
-