fileapi: Fix dangling reply reference on case-insensitive filesystems

Reply file names embed the configuration name verbatim.  Reconfiguring
a build tree with a build type that differs from the previous one only
in case (e.g. Debug -> debug) makes CMake write a reply whose name also
differs only in case from the existing file.  On a case-insensitive
filesystem the write is skipped because the name already exists, but
RemoveOldReplyFiles compared on-disk names to the just-written names
textually and deleted the surviving file, leaving the reply index
citing a target or directory reply that no longer exists on disk.

Prune reply files by file identity via cmSystemTools::GetFileId instead
of by name, so an on-disk entry that aliases a reply we just wrote is
kept.  An entry whose identity cannot be obtained is retained rather
than deleted.

Fixes: #28022

Source/cmFileAPI.cxx

@@ -6,6 +6,7 @@
 #include <cassert>
 #include <chrono>
 #include <ctime>
+#include <functional>
 #include <iomanip>
 #include <iterator>
 #include <sstream>
@@ -155,12 +156,20 @@ std::vector<std::string> cmFileAPI::LoadDir(std::string const& dir)
 void cmFileAPI::RemoveOldReplyFiles()
 {
   std::string const reply_dir = this->APIv1 + "/reply";
-  std::vector<std::string> files = this->LoadDir(reply_dir);
-  for (std::string const& f : files) {
-    if (this->ReplyFiles.find(f) == this->ReplyFiles.end()) {
-      std::string file = cmStrCat(reply_dir, '/', f);
-      cmSystemTools::RemoveFile(file);
-    }
+  std::vector<std::string> const files = this->LoadDir(reply_dir);
+
+  // Reply names embed the configuration verbatim, so on a case-insensitive
+  // filesystem a "debug" reply can alias a just-written "Debug" one; deleting
+  // by name would strip a file the index still cites.  Decide by identity.
+  std::vector<std::string> const toRemove =
+    cmFileAPI::FilesToRemove<cmSystemTools::FileId>(
+      files, this->ReplyFiles,
+      [&reply_dir](std::string const& name,
+                   cmSystemTools::FileId& id) -> bool {
+        return cmSystemTools::GetFileId(cmStrCat(reply_dir, '/', name), id);
+      });
+  for (std::string const& f : toRemove) {
+    cmSystemTools::RemoveFile(cmStrCat(reply_dir, '/', f));
   }
 }
 

Source/cmFileAPI.h

@@ -4,6 +4,7 @@
 
 #include "cmConfigure.h" // IWYU pragma: keep
 
+#include <functional>
 #include <map>
 #include <memory>
 #include <string>
@@ -66,6 +67,49 @@ public:
   /** Build a JSON object with major and minor fields.  */
   static Json::Value BuildVersion(unsigned int major, unsigned int minor);
 
+  /** Return the subset of 'entries' to delete after a configure: those not
+      named in 'replyNames' whose identity ('getId') also matches no reply.
+      Deciding by identity rather than name keeps an entry that aliases a
+      just-written reply on a case-insensitive filesystem; an entry whose
+      identity cannot be obtained is retained.  Static/templated for tests. */
+  template <typename FileIdT>
+  static std::vector<std::string> FilesToRemove(
+    std::vector<std::string> const& entries,
+    std::unordered_set<std::string> const& replyNames,
+    std::function<bool(std::string const&, FileIdT&)> const& getId)
+  {
+    std::vector<FileIdT> keptIds;
+    for (std::string const& name : replyNames) {
+      FileIdT id;
+      if (getId(name, id)) {
+        keptIds.push_back(id);
+      }
+    }
+
+    std::vector<std::string> toRemove;
+    for (std::string const& entry : entries) {
+      if (replyNames.find(entry) != replyNames.end()) {
+        continue;
+      }
+      FileIdT id;
+      if (!getId(entry, id)) {
+        continue;
+      }
+      bool aliasesKept = false;
+      for (FileIdT const& keptId : keptIds) {
+        if (id == keptId) {
+          aliasesKept = true;
+          break;
+        }
+      }
+      if (aliasesKept) {
+        continue;
+      }
+      toRemove.push_back(entry);
+    }
+    return toRemove;
+  }
+
 private:
   cmake* CMakeInstance;
 

Tests/CMakeLib/CMakeLists.txt

@@ -12,6 +12,7 @@ set(CMakeLib_TESTS
   testDateTime.cxx
   testDebug.cxx
   testDocumentationFormatter.cxx
+  testCMFileAPI.cxx
   testGccDepfileReader.cxx
   testGeneratedFileStream.cxx
   testGenExBoundOperand.cxx

Tests/CMakeLib/testCMFileAPI.cxx

@@ -0,0 +1,102 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file LICENSE.rst or https://cmake.org/licensing for details.  */
+#include <functional>
+#include <iostream>
+#include <map>
+#include <set>
+#include <string>
+#include <unordered_set>
+#include <utility>
+#include <vector>
+
+#include "cmFileAPI.h"
+
+namespace {
+
+// Identity oracle over a name->id map.  A name absent from the map models a
+// file whose identity cannot be obtained (GetFileId failure).
+std::function<bool(std::string const&, int&)> makeOracle(
+  std::map<std::string, int> const& ids)
+{
+  return [ids](std::string const& name, int& id) -> bool {
+    auto const it = ids.find(name);
+    if (it == ids.end()) {
+      return false;
+    }
+    id = it->second;
+    return true;
+  };
+}
+
+bool checkCase(char const* label, std::vector<std::string> const& entries,
+               std::unordered_set<std::string> const& replyNames,
+               std::map<std::string, int> const& ids,
+               std::set<std::string> const& expected)
+{
+  std::vector<std::string> const removed =
+    cmFileAPI::FilesToRemove<int>(entries, replyNames, makeOracle(ids));
+  std::set<std::string> const actual(removed.begin(), removed.end());
+  if (actual != expected) {
+    std::cout << "FAILED: " << label << "\n  expected removals:";
+    for (std::string const& e : expected) {
+      std::cout << ' ' << e;
+    }
+    std::cout << "\n  actual removals:  ";
+    for (std::string const& a : actual) {
+      std::cout << ' ' << a;
+    }
+    std::cout << '\n';
+    return false;
+  }
+  return true;
+}
+
+} // namespace
+
+int testCMFileAPI(int /*unused*/, char* /*unused*/[])
+{
+  bool ok = true;
+
+  // An on-disk entry that resolves to the same identity as a just-written
+  // reply (a case-only-different name aliasing one inode) is kept.
+  ok &= checkCase(
+    "case-variant alias kept", { "target-foo-Debug-H.json" },
+    { "target-foo-debug-H.json" },
+    { { "target-foo-debug-H.json", 1 }, { "target-foo-Debug-H.json", 1 } },
+    {});
+
+  // A genuinely stale entry with a distinct identity is removed.
+  ok &= checkCase("distinct stale removed", { "target-old-H.json" },
+                  { "target-new-H.json" },
+                  { { "target-new-H.json", 1 }, { "target-old-H.json", 2 } },
+                  { "target-old-H.json" });
+
+  // An entry whose exact name was just written is kept without consulting the
+  // identity oracle (its name is intentionally absent from the id map).
+  ok &= checkCase("exact-name match kept", { "index-x.json" },
+                  { "index-x.json" }, {}, {});
+
+  // Fail-safe: an entry whose identity cannot be obtained is retained.
+  ok &= checkCase("candidate id failure retained", { "unreadable.json" },
+                  { "target-new-H.json" }, { { "target-new-H.json", 1 } }, {});
+
+  // No kept-side fail-safe is needed: an alias shares its inode, so when a
+  // kept reply's identity is unknown the candidate's is too and the
+  // candidate-side fail-safe retains it.  Both names are absent to model it.
+  ok &= checkCase("unknown kept id: correlated alias retained",
+                  { "alias-of-unidentified.json" },
+                  { "target-new-H.json", "unidentified-kept.json" },
+                  { { "target-new-H.json", 1 } }, {});
+
+  // Mixed: an aliasing entry is kept while an unrelated stale entry is
+  // removed.
+  ok &= checkCase("mixed alias and stale", { "a-alias.json", "stale.json" },
+                  { "a.json", "b.json" },
+                  { { "a.json", 1 },
+                    { "b.json", 2 },
+                    { "a-alias.json", 1 },
+                    { "stale.json", 3 } },
+                  { "stale.json" });
+
+  return ok ? 0 : 1;
+}

Tests/RunCMake/FileAPI/ConfigCaseReconfigure-prep.cmake

@@ -0,0 +1,2 @@
+file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/query")
+file(WRITE "${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/query/codemodel-v2" "")

Tests/RunCMake/FileAPI/ConfigCaseReconfigure-recon-check.cmake

@@ -0,0 +1,21 @@
+set(reply_dir "${RunCMake_TEST_BINARY_DIR}/.cmake/api/v1/reply")
+
+# Every reply file referenced from the index must still exist on disk after
+# the reconfigure.  A dangling "jsonFile" reference is the bug this guards; it
+# manifests only on a case-insensitive filesystem (Windows, default macOS).
+file(GLOB reply_files "${reply_dir}/*.json")
+set(dangling "")
+foreach(reply_file IN LISTS reply_files)
+  file(READ "${reply_file}" content)
+  string(REGEX MATCHALL "\"jsonFile\"[ \t]*:[ \t]*\"[^\"]+\"" refs "${content}")
+  foreach(ref IN LISTS refs)
+    string(REGEX REPLACE "\"jsonFile\"[ \t]*:[ \t]*\"([^\"]+)\"" "\\1" name "${ref}")
+    if(NOT EXISTS "${reply_dir}/${name}")
+      get_filename_component(from "${reply_file}" NAME)
+      string(APPEND dangling "\n  '${name}' referenced by ${from} is missing")
+    endif()
+  endforeach()
+endforeach()
+if(dangling)
+  set(RunCMake_TEST_FAILED "Dangling File API reply references:${dangling}")
+endif()

Tests/RunCMake/FileAPI/ConfigCaseReconfigure.cmake

@@ -0,0 +1,2 @@
+enable_language(C)
+add_library(foo STATIC empty.c)

Tests/RunCMake/FileAPI/RunCMakeTest.cmake

@@ -125,6 +125,19 @@ run_cmake(ProjectQueryGood)
 run_cmake(ProjectQueryBad)
 run_cmake(FailConfigure)
 
+# Reconfiguring with a case-only build-type change (Debug -> debug) must not
+# leave the reply index citing a target file that cleanup deleted on a
+# case-insensitive filesystem (Windows, default macOS).
+function(run_config_case)
+  if(NOT RunCMake_GENERATOR_IS_MULTI_CONFIG)
+    set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/ConfigCaseReconfigure-build)
+    run_cmake_with_options(ConfigCaseReconfigure -DCMAKE_BUILD_TYPE=Debug)
+    set(RunCMake_TEST_NO_CLEAN 1)
+    run_cmake_command(ConfigCaseReconfigure-recon ${CMAKE_COMMAND} . -DCMAKE_BUILD_TYPE=debug)
+  endif()
+endfunction()
+run_config_case()
+
 function(run_object object)
   set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${object}-build)
   list(APPEND RunCMake_TEST_OPTIONS ${ARGN} -DCMAKE_POLICY_DEFAULT_CMP0118=NEW)