From 36453b9fa3b124f02f48edffaaa68cf32fe395ae Mon Sep 17 00:00:00 2001
From: Jake Ehrlich <jake.ehrlich@sifive.com>
Date: Tue, 15 Aug 2023 14:30:18 -0700
Subject: [PATCH] Responding to review

---
 README.md                    | 10 ++++++++++
 src/runtime/config.cpp       | 22 ++++++++++++++++++----
 tests/config/nominal/pass.sh |  2 +-
 tests/config/nominal/stdout  |  2 +-
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/README.md b/README.md
index 535fc6370..47ca2d205 100644
--- a/README.md
+++ b/README.md
@@ -174,6 +174,16 @@ Below is a full example
 }
 ```
 
+While there are many sources that a config option might come from, the following priority is always observed from lowest
+to highest:
+1) .wakeroot
+2) user config
+3) environment variables
+4) command line options
+
+So a command line option overides anything, an environment variable overrides user config and wakeroot, user config
+overrides wakeroot, and wakeroot overrides nothing
+
 # Documentation
 
 Documentation for wake can be found in [share/doc/wake](share/doc/wake).
diff --git a/src/runtime/config.cpp b/src/runtime/config.cpp
index 6f53c397d..2cfeb1297 100644
--- a/src/runtime/config.cpp
+++ b/src/runtime/config.cpp
@@ -335,14 +335,26 @@ bool WakeConfig::init(const std::string& wakeroot_path, const WakeConfigOverride
     }
   }
 
+  // The priority of config sources is the following from lowest priority to highest:
+  // 1) .wakeroot
+  // 2) user config
+  // 3) environment variables
+  // 4) command line options
+  //
+  // When parsing the user config, the user config path can't be in the user config
+  // but it can be anywhere else so before parsing the user config we parse wakeroot
+  // and then the other two sources. Once we parse the user config though we'll have
+  // go overwrite anything from the user config that should be form an env-var or
+  // a command line option.
+
   // Parse values from .wakeroot
   _config->set_all<WakeConfigProvenance::WakeRoot>(wakeroot_json);
 
   // Sometimes we need to the user_config with an env-var so we check env-vars first here
   _config->set_all_env_var();
 
-  // Further more users may choose to override the user config at the command line level so
-  // we run that too only to run it again later
+  // Furthermore users may choose to override the user config at the command line level so
+  // we run that to only to run it again later
   _config->override_all(overrides);
 
   // Parse user config
@@ -378,10 +390,12 @@ bool WakeConfig::init(const std::string& wakeroot_path, const WakeConfigOverride
   // Parse values from the user config
   _config->set_all<WakeConfigProvenance::UserConfig>(user_config_json);
 
-  // Set all env-vars again as they should override user configs
+  // Set all env-vars again as they should override user configs. Note that
+  // this is the second time we set the env-vars.
   _config->set_all_env_var();
 
-  // Finally apply command line overrides as they override everything
+  // Finally apply command line overrides as they override everything. Note that
+  // this is the second time we set the overrides.
   _config->override_all(overrides);
 
   return true;
diff --git a/tests/config/nominal/pass.sh b/tests/config/nominal/pass.sh
index 57c11a550..c90bdbfd4 100755
--- a/tests/config/nominal/pass.sh
+++ b/tests/config/nominal/pass.sh
@@ -6,4 +6,4 @@ if [ $(uname) != Linux ] ; then
 fi
 
 WAKE="${1:+$1/wake}"
-"${WAKE:-wake}" --config
+WAKE_SHARED_CACHE_MAX_SIZE=1024 "${WAKE:-wake}" --config
diff --git a/tests/config/nominal/stdout b/tests/config/nominal/stdout
index 07f7fbfdc..becd05e97 100644
--- a/tests/config/nominal/stdout
+++ b/tests/config/nominal/stdout
@@ -4,7 +4,7 @@ Wake config:
   log_header = 'foobar $source: ' (WakeRoot)
   log_header_source_width = '13' (WakeRoot)
   label_filter = '.*' (Default)
-  max_cache_size = '1024' (WakeRoot)
+  max_cache_size = '1024' (EnvVar)
   low_cache_size = '512' (WakeRoot)
   cache_miss_on_failure = 'true' (WakeRoot)
   log_header_align = 'true' (WakeRoot)