From 23bbc238aa663934fb18dfbb0cd638a66d31d687 Mon Sep 17 00:00:00 2001
From: Joel Rosdahl <joel@rosdahl.net>
Date: Tue, 20 Dec 2022 21:57:33 +0100
Subject: fix: Improve fix for local/remote cache misses in depend mode

---
 src/ccache.cpp                     | 20 ++++++++++++++++++--
 src/storage/Storage.cpp            |  8 --------
 src/storage/local/LocalStorage.cpp | 10 ++--------
 test/suites/remote_file.bash       | 24 ++++++++++++++++++++++--
 4 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/src/ccache.cpp b/src/ccache.cpp
index 8791f788..c8a21ac3 100644
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -2260,8 +2260,24 @@ cache_compilation(int argc, const char* const* argv)
     MTR_END("main", "find_compiler");
 
     const auto result = do_cache_compilation(ctx, argv);
-    const auto& counters = result ? *result : result.error().counters();
-    ctx.storage.local.increment_statistics(counters);
+    ctx.storage.local.increment_statistics(result ? *result
+                                                  : result.error().counters());
+    const auto& counters = ctx.storage.local.get_statistics_updates();
+
+    if (counters.get(Statistic::cache_miss) > 0) {
+      if (!ctx.config.remote_only()) {
+        ctx.storage.local.increment_statistic(Statistic::local_storage_miss);
+      }
+      if (ctx.storage.has_remote_storage()) {
+        ctx.storage.local.increment_statistic(Statistic::remote_storage_miss);
+      }
+    } else if ((counters.get(Statistic::direct_cache_hit) > 0
+                || counters.get(Statistic::preprocessed_cache_hit) > 0)
+               && counters.get(Statistic::remote_storage_hit) > 0
+               && !ctx.config.remote_only()) {
+      ctx.storage.local.increment_statistic(Statistic::local_storage_miss);
+    }
+
     if (!result) {
       if (result.error().exit_code()) {
         return *result.error().exit_code();
diff --git a/src/storage/Storage.cpp b/src/storage/Storage.cpp
index da707ea6..775f4be7 100644
--- a/src/storage/Storage.cpp
+++ b/src/storage/Storage.cpp
@@ -461,14 +461,6 @@ Storage::get_from_remote_storage(const Digest& key,
           backend->url_for_logging,
           ms);
       local.increment_statistic(core::Statistic::remote_storage_read_miss);
-      if (type == core::CacheEntryType::result) {
-        local.increment_statistic(core::Statistic::remote_storage_miss);
-      } else if (m_config.depend_mode()) {
-        // With the depend mode enabled, a missing manifest means that we can't
-        // even try to look up a result, so note the miss already now.
-        ASSERT(type == core::CacheEntryType::manifest);
-        local.increment_statistic(core::Statistic::remote_storage_miss);
-      }
     }
   }
 }
diff --git a/src/storage/local/LocalStorage.cpp b/src/storage/local/LocalStorage.cpp
index f1984caa..c774c738 100644
--- a/src/storage/local/LocalStorage.cpp
+++ b/src/storage/local/LocalStorage.cpp
@@ -220,14 +220,8 @@ LocalStorage::get(const Digest& key, const core::CacheEntryType type)
 
   increment_statistic(return_value ? core::Statistic::local_storage_read_hit
                                    : core::Statistic::local_storage_read_miss);
-  if (type == core::CacheEntryType::result) {
-    increment_statistic(return_value ? core::Statistic::local_storage_hit
-                                     : core::Statistic::local_storage_miss);
-  } else if (m_config.depend_mode() && !return_value) {
-    // With the depend mode enabled, a missing manifest means that we can't even
-    // try to look up a result, so note the miss already now.
-    ASSERT(type == core::CacheEntryType::manifest);
-    increment_statistic(core::Statistic::local_storage_miss);
+  if (return_value && type == core::CacheEntryType::result) {
+    increment_statistic(core::Statistic::local_storage_hit);
   }
 
   return return_value;
diff --git a/test/suites/remote_file.bash b/test/suites/remote_file.bash
index a48ae561..39184884 100644
--- a/test/suites/remote_file.bash
+++ b/test/suites/remote_file.bash
@@ -11,7 +11,9 @@ SUITE_remote_file_SETUP() {
     unset CCACHE_NODIRECT
     export CCACHE_REMOTE_STORAGE="file:$PWD/remote"
 
-    generate_code 1 test.c
+    touch test.h
+    echo '#include "test.h"' >test.c
+    backdate test.h
 }
 
 SUITE_remote_file() {
@@ -239,7 +241,7 @@ SUITE_remote_file() {
     expect_stat direct_cache_hit 2
     expect_stat cache_miss 1
     expect_stat local_storage_hit 1
-    expect_stat local_storage_miss 3
+    expect_stat local_storage_miss 2
     expect_stat local_storage_read_hit 2
     expect_stat local_storage_read_miss 3
     expect_stat local_storage_write 4
@@ -249,6 +251,24 @@ SUITE_remote_file() {
     expect_stat remote_storage_read_miss 1
     expect_stat remote_storage_write 2 # result + manifest
 
+    # Remote cache read hit for the manifest but no manifest entry matches.
+    $CCACHE -C >/dev/null
+    echo 'int x;' >>test.h
+    backdate test.h
+    $CCACHE_COMPILE -MMD -c test.c
+    expect_stat direct_cache_hit 2
+    expect_stat cache_miss 2
+    expect_stat local_storage_hit 1
+    expect_stat local_storage_miss 3
+    expect_stat local_storage_read_hit 2
+    expect_stat local_storage_read_miss 4 # one manifest read miss
+    expect_stat local_storage_write 7 # download+store manifest, update+store manifest, write result
+    expect_stat remote_storage_hit 1
+    expect_stat remote_storage_miss 2
+    expect_stat remote_storage_read_hit 3
+    expect_stat remote_storage_read_miss 1 # original manifest didn't match -> no read
+    expect_stat remote_storage_write 4
+
     # -------------------------------------------------------------------------
     TEST "umask"
 
-- 
cgit v1.2.1