Refactor base::GetUniquePath

This removes base::GetUniquePathNumber, as there really isn't a way that method can be used correctly other than to generate the resulting path. One call-site that tried to use GetUniquePathNumber to truncate the generated path after getting the uniquifing number (which defeats the whole point of calling GetUniquePathNumber) has been rewritten to call GetUniquePath at the end instead. This also adds a new GetUniquePathWithSuffixFormat overload, which lets callers pass in a format different from the standard " (%d)" format. This is used by for example linux shell integration code to generate unique file names for .desktop files (which can't have spaces in their path). Bug: 336633543 Change-Id: I7dd4a7757835ffdcef2a693a21bc6e8c84056878 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5502054 Reviewed-by: Daniel Cheng <[email protected]> Reviewed-by: Carlos Knippschild <[email protected]> Reviewed-by: Greg Thompson <[email protected]> Commit-Queue: Marijn Kruisselbrink <[email protected]> Cr-Commit-Position: refs/heads/main@{#1295137}
zsjyrlw · May 1, 2024 · 11da37b · 11da37b
1 parent 3c8f4d8
commit 11da37b
Show file tree

Hide file tree

Showing 6 changed files with 108 additions and 111 deletions.
diff --git a/base/files/file_util.cc b/base/files/file_util.cc
@@ -25,12 +25,14 @@
 
 #include "base/bit_cast.h"
 #include "base/check_op.h"
+#include "base/containers/contains.h"
 #include "base/containers/span.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/functional/function_ref.h"
 #include "base/notreached.h"
 #include "base/posix/eintr_wrapper.h"
+#include "base/ranges/algorithm.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -474,28 +476,29 @@ bool WriteFile(const FilePath& filename, StringPiece data) {
   return WriteFile(filename, data.data(), size) == size;
 }
 
-int GetUniquePathNumber(const FilePath& path) {
+FilePath GetUniquePath(const FilePath& path) {
+  return GetUniquePathWithSuffixFormat(path, " (%d)");
+}
+
+FilePath GetUniquePathWithSuffixFormat(const FilePath& path,
+                                       cstring_view suffix_format) {
   DCHECK(!path.empty());
-  if (!PathExists(path))
-    return 0;
+  DCHECK_EQ(base::ranges::count(suffix_format, '%'), 1);
+  DCHECK(base::Contains(suffix_format, "%d"));
 
+  if (!PathExists(path)) {
+    return path;
+  }
   std::string number;
   for (int count = 1; count <= kMaxUniqueFiles; ++count) {
-    StringAppendF(&number, " (%d)", count);
-    if (!PathExists(path.InsertBeforeExtensionASCII(number)))
-      return count;
+    StringAppendF(&number, suffix_format.c_str(), count);
+    FilePath candidate_path = path.InsertBeforeExtensionASCII(number);
+    if (!PathExists(candidate_path)) {
+      return candidate_path;
+    }
     number.clear();
   }
-
-  return -1;
-}
-
-FilePath GetUniquePath(const FilePath& path) {
-  DCHECK(!path.empty());
-  const int uniquifier = GetUniquePathNumber(path);
-  if (uniquifier > 0)
-    return path.InsertBeforeExtensionASCII(StringPrintf(" (%d)", uniquifier));
-  return uniquifier == 0 ? path : FilePath();
+  return FilePath();
 }
 
 }  // namespace base
diff --git a/base/files/file_util.h b/base/files/file_util.h
@@ -23,6 +23,7 @@
 #include "base/files/file_path.h"
 #include "base/files/scoped_file.h"
 #include "base/functional/callback.h"
+#include "base/strings/cstring_view.h"
 #include "base/types/pass_key.h"
 #include "build/build_config.h"
 
@@ -619,21 +620,22 @@ BASE_EXPORT bool GetCurrentDirectory(FilePath* path);
 // Sets the current working directory for the process.
 BASE_EXPORT bool SetCurrentDirectory(const FilePath& path);
 
-// The largest value attempted by GetUniquePath{Number,}.
+// The largest value attempted by GetUniquePath.
 enum { kMaxUniqueFiles = 100 };
 
-// Returns the number N that makes |path| unique when formatted as " (N)" in a
-// suffix to its basename before any file extension, where N is a number between
-// 1 and 100 (inclusive). Returns 0 if |path| does not exist (meaning that it is
-// unique as-is), or -1 if no such number can be found.
-BASE_EXPORT int GetUniquePathNumber(const FilePath& path);
-
 // Returns |path| if it does not exist. Otherwise, returns |path| with the
 // suffix " (N)" appended to its basename before any file extension, where N is
 // a number between 1 and 100 (inclusive). Returns an empty path if no such
 // number can be found.
 BASE_EXPORT FilePath GetUniquePath(const FilePath& path);
 
+// Same as `GetUniquePath()`, except this method allows specifying a custom
+// suffix printf format string in cases where the default format doesn't work
+// (for example because you need a filename without spaces in it). Passing
+// " (%d)" as `suffix_format` makes this behave identical to `GetUniquePath()`.
+BASE_EXPORT FilePath GetUniquePathWithSuffixFormat(const FilePath& path,
+                                                   cstring_view suffix_format);
+
 // Sets the given |fd| to non-blocking mode.
 // Returns true if it was able to set it in the non-blocking mode, otherwise
 // false.

diff --git a/base/files/file_util_unittest.cc b/base/files/file_util_unittest.cc
@@ -2845,13 +2845,9 @@ TEST_F(FileUtilTest, CreateAndOpenTemporaryStreamTest) {
   }
 }
 
-TEST_F(FileUtilTest, GetUniquePathTest) {
-  // Create a unique temp directory and use it to generate a unique file path.
-  base::ScopedTempDir temp_dir;
-  EXPECT_TRUE(temp_dir.CreateUniqueTempDir());
-  EXPECT_TRUE(temp_dir.IsValid());
-  FilePath base_name(FILE_PATH_LITERAL("Unique_Base_Name.txt"));
-  FilePath base_path = temp_dir.GetPath().Append(base_name);
+TEST_F(FileUtilTest, GetUniquePath) {
+  FilePath base_name(FPL("Unique_Base_Name.txt"));
+  FilePath base_path = temp_dir_.GetPath().Append(base_name);
   EXPECT_FALSE(PathExists(base_path));
 
   // GetUniquePath() should return unchanged path if file does not exist.
@@ -2865,14 +2861,14 @@ TEST_F(FileUtilTest, GetUniquePathTest) {
   }
 
   static const FilePath::CharType* const kExpectedNames[] = {
-      FILE_PATH_LITERAL("Unique_Base_Name (1).txt"),
-      FILE_PATH_LITERAL("Unique_Base_Name (2).txt"),
-      FILE_PATH_LITERAL("Unique_Base_Name (3).txt"),
+      FPL("Unique_Base_Name (1).txt"),
+      FPL("Unique_Base_Name (2).txt"),
+      FPL("Unique_Base_Name (3).txt"),
   };
 
   // Call GetUniquePath() three times against this existing file name.
   for (const FilePath::CharType* expected_name : kExpectedNames) {
-    FilePath expected_path = temp_dir.GetPath().Append(expected_name);
+    FilePath expected_path = temp_dir_.GetPath().Append(expected_name);
     FilePath path = GetUniquePath(base_path);
     EXPECT_EQ(expected_path, path);
 
@@ -2887,6 +2883,68 @@ TEST_F(FileUtilTest, GetUniquePathTest) {
   }
 }
 
+TEST_F(FileUtilTest, GetUniquePathTooManyFiles) {
+  // Create a file with the desired path.
+  const FilePath some_file = temp_dir_.GetPath().Append(FPL("SomeFile.txt"));
+  ASSERT_TRUE(File(some_file, File::FLAG_CREATE | File::FLAG_WRITE).IsValid());
+
+  // Now create 100 collisions.
+  for (int i = 1; i <= kMaxUniqueFiles; ++i) {
+    FilePath path =
+        temp_dir_.GetPath().AppendASCII(StringPrintf("SomeFile (%d).txt", i));
+    ASSERT_EQ(GetUniquePath(some_file), path);
+    ASSERT_TRUE(File(path, File::FLAG_CREATE | File::FLAG_WRITE).IsValid());
+  }
+
+  // Verify that the limit has been reached.
+  EXPECT_EQ(GetUniquePath(some_file), base::FilePath());
+}
+
+TEST_F(FileUtilTest, GetUniquePathWithSuffixFormat) {
+  const char kSuffix[] = "_%d";
+  FilePath base_name(FPL("Unique_Base_Name.txt"));
+  FilePath base_path = temp_dir_.GetPath().Append(base_name);
+  EXPECT_FALSE(PathExists(base_path));
+
+  // GetUniquePathWithSuffixFormat() should return unchanged path if file does
+  // not exist.
+  EXPECT_EQ(base_path, GetUniquePathWithSuffixFormat(base_path, kSuffix));
+
+  // Create the file.
+  {
+    File file(base_path,
+              File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE);
+    EXPECT_TRUE(PathExists(base_path));
+  }
+
+  static const FilePath::CharType* const kExpectedNames[] = {
+      FPL("Unique_Base_Name_1.txt"),
+      FPL("Unique_Base_Name_2.txt"),
+      FPL("Unique_Base_Name_3.txt"),
+  };
+
+  // Call GetUniquePathWithSuffixFormat() three times against this existing file
+  // name.
+  for (const FilePath::CharType* expected_name : kExpectedNames) {
+    FilePath expected_path = temp_dir_.GetPath().Append(expected_name);
+    FilePath path = GetUniquePathWithSuffixFormat(base_path, kSuffix);
+    EXPECT_EQ(expected_path, path);
+
+    // Verify that a file with this path indeed does not exist on the file
+    // system.
+    EXPECT_FALSE(PathExists(path));
+
+    // Create the file so it exists for the next call to
+    // GetUniquePathWithSuffixFormat() in the loop.
+    File file(path, File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE);
+    EXPECT_TRUE(PathExists(path));
+  }
+
+  // Verify that a different suffix still ends up with number 1.
+  EXPECT_EQ(temp_dir_.GetPath().Append(FPL("Unique_Base_Name (1).txt")),
+            GetUniquePathWithSuffixFormat(base_path, " (%d)"));
+}
+
 TEST_F(FileUtilTest, FileToFILE) {
   File file;
   FILE* stream = FileToFILE(std::move(file), "w");
@@ -4345,54 +4403,6 @@ TEST_F(FileUtilTest, NonExistentContentUriTest) {
 }
 #endif
 
-TEST_F(FileUtilTest, GetUniquePathNumberNoFile) {
-  // This file does not exist.
-  const FilePath some_file = temp_dir_.GetPath().Append(FPL("SomeFile.txt"));
-
-  // The path is unique as-is.
-  EXPECT_EQ(GetUniquePathNumber(some_file), 0);
-}
-
-TEST_F(FileUtilTest, GetUniquePathNumberFileExists) {
-  // Create a file with the desired path.
-  const FilePath some_file = temp_dir_.GetPath().Append(FPL("SomeFile.txt"));
-  ASSERT_TRUE(File(some_file, File::FLAG_CREATE | File::FLAG_WRITE).IsValid());
-
-  // The file exists, so the number 1 is needed to make it unique.
-  EXPECT_EQ(GetUniquePathNumber(some_file), 1);
-}
-
-TEST_F(FileUtilTest, GetUniquePathNumberFilesExist) {
-  // Create a file with the desired path and with it suffixed with " (1)"
-  const FilePath some_file = temp_dir_.GetPath().Append(FPL("SomeFile.txt"));
-  ASSERT_TRUE(File(some_file, File::FLAG_CREATE | File::FLAG_WRITE).IsValid());
-  const FilePath some_file_one =
-      temp_dir_.GetPath().Append(FPL("SomeFile (1).txt"));
-  ASSERT_TRUE(
-      File(some_file_one, File::FLAG_CREATE | File::FLAG_WRITE).IsValid());
-
-  // This time the number 2 is needed to make it unique.
-  EXPECT_EQ(GetUniquePathNumber(some_file), 2);
-}
-
-TEST_F(FileUtilTest, GetUniquePathNumberTooManyFiles) {
-  // Create a file with the desired path.
-  const FilePath some_file = temp_dir_.GetPath().Append(FPL("SomeFile.txt"));
-  ASSERT_TRUE(File(some_file, File::FLAG_CREATE | File::FLAG_WRITE).IsValid());
-
-  // Now create 100 collisions.
-  for (int i = 1; i <= kMaxUniqueFiles; ++i) {
-    ASSERT_EQ(GetUniquePathNumber(some_file), i);
-    ASSERT_TRUE(File(temp_dir_.GetPath().AppendASCII(
-                         StringPrintf("SomeFile (%d).txt", i)),
-                     File::FLAG_CREATE | File::FLAG_WRITE)
-                    .IsValid());
-  }
-
-  // Verify that the limit has been reached.
-  EXPECT_EQ(GetUniquePathNumber(some_file), -1);
-}
-
 #if BUILDFLAG(IS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING) && \
     defined(ARCH_CPU_32_BITS)
 // TODO(crbug.com/327582285): Re-enable these tests. They may be failing due to

diff --git a/chrome/browser/safe_browsing/test_safe_browsing_database_helper.cc b/chrome/browser/safe_browsing/test_safe_browsing_database_helper.cc
@@ -63,11 +63,11 @@ class InsertingDatabaseFactory : public safe_browsing::TestV4DatabaseFactory {
     const base::FilePath base_store_path(FILE_PATH_LITERAL("UrlDb.store"));
     for (const auto& id : lists_to_insert_) {
       if (!base::Contains(*store_map, id)) {
-        const base::FilePath store_path =
-            base_store_path.InsertBeforeExtensionASCII(base::StringPrintf(
-                " (%d)", base::GetUniquePathNumber(base_store_path)));
+        const base::FilePath store_path = base::GetUniquePath(base_store_path);
         store_map->insert(
-            {id, store_factory_->CreateV4Store(db_task_runner, store_path)});
+            {id, store_factory_->CreateV4Store(
+                     db_task_runner,
+                     store_path.empty() ? base_store_path : store_path)});
       }
     }
 

diff --git a/chrome/browser/shell_integration_linux.cc b/chrome/browser/shell_integration_linux.cc
@@ -531,20 +531,10 @@ std::optional<base::SafeBaseName> GetUniqueWebShortcutFilename(
   // guaranteed by `ReplaceIllegalCharactersInPath`.
   DCHECK(base::i18n::IsFilenameLegal(base_name_no_extension.AsUTF16Unsafe()));
 
-  // TODO(crbug.com/336633543): Evaluate unique file selection code after other
-  // OS implementations of create shortcut are complete.
   base::FilePath filepath =
       desktop_path.Append(base_name_no_extension.AddExtension(".desktop"));
-  for (size_t i = 1; i < 100; ++i) {
-    if (base::PathExists(filepath)) {
-      filepath = desktop_path.Append(
-          base::StrCat({base_name_no_extension.value(), "_",
-                        base::NumberToString(i), ".desktop"}));
-    } else {
-      return base::SafeBaseName::Create(filepath);
-    }
-  }
-  return std::nullopt;
+  return base::SafeBaseName::Create(
+      base::GetUniquePathWithSuffixFormat(filepath, "_%d"));
 }
 
 std::vector<base::FilePath> GetExistingProfileShortcutFilenames(

diff --git a/components/offline_pages/core/model/offline_page_model_utils.cc b/components/offline_pages/core/model/offline_page_model_utils.cc
@@ -69,29 +69,21 @@ base::FilePath GenerateUniqueFilenameForOfflinePage(
       target_dir.Append(filename_generation::GenerateFilename(
           title, url, false /* can_save_as_complete */, kMHTMLMimeType));
 
-  // Find a unique name based on |suggested_path|.
-  int uniquifier = base::GetUniquePathNumber(suggested_path);
-  std::string suffix;
-  if (uniquifier > 0) {
-    suffix = base::StringPrintf(" (%d)", uniquifier);
-  }
-
-  // Truncation.
+  // Truncation based on the maximum length the suffix may have " (99)".
+  const int kMaxSuffixLength = 5;
   int max_path_component_length =
       base::GetMaximumPathComponentLength(target_dir);
   if (max_path_component_length != -1) {
     int limit = max_path_component_length -
-                suggested_path.Extension().length() - suffix.length();
+                suggested_path.Extension().length() - kMaxSuffixLength;
     if (limit <= 0 ||
         !filename_generation::TruncateFilename(&suggested_path, limit)) {
       return base::FilePath();
     }
   }
 
-  // Adding uniquifier suffix if needed.
-  if (!suffix.empty()) {
-    suggested_path = suggested_path.InsertBeforeExtensionASCII(suffix);
-  }
+  // Find a unique name based on |suggested_path|.
+  suggested_path = base::GetUniquePath(suggested_path);
 
   return suggested_path;
 }