refactor: Remove null setting check in GetSetting()

Also rename the "result_complete" variable in GetSettingsList() to "done" to be
more consistent with GetSetting().

This change doesn't affect current behavior but could be useful in the future
to support dynamically changing settings at runtime and adding new settings
sources, because it lets high priority sources reset settings back to default
(see test).

By removing a special case for null, this change also helps merge code treat
settings values more like black boxes, and interfere less with settings parsing
and retrieval.

Author: ryanofsky

src/test/settings_tests.cpp

@@ -45,6 +45,19 @@ BOOST_AUTO_TEST_CASE(Simple)
     CheckValues(settings2, R"("val2")", R"(["val2","val3"])");
 }
 
+// Confirm that a high priority setting overrides a lower priority setting even
+// if the high priority setting is null. This behavior is useful for a high
+// priority setting source to be able to effectively reset any setting back to
+// its default value.
+BOOST_AUTO_TEST_CASE(NullOverride)
+{
+    util::Settings settings;
+    settings.command_line_options["name"].push_back("value");
+    BOOST_CHECK_EQUAL(R"("value")", GetSetting(settings, "section", "name", false, false).write().c_str());
+    settings.forced_settings["name"] = {};
+    BOOST_CHECK_EQUAL(R"(null)", GetSetting(settings, "section", "name", false, false).write().c_str());
+}
+
 // Test different ways settings can be merged, and verify results. This test can
 // be used to confirm that updates to settings code don't change behavior
 // unintentionally.

src/util/settings.cpp

@@ -56,6 +56,7 @@ SettingsValue GetSetting(const Settings& settings,
     bool get_chain_name)
 {
     SettingsValue result;
+    bool done = false; // Done merging any more settings sources.
     MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
         // Weird behavior preserved for backwards compatibility: Apply negated
         // setting even if non-negated setting would be ignored. A negated
@@ -79,6 +80,8 @@ SettingsValue GetSetting(const Settings& settings,
         // negated values, or at least warn they are ignored.
         const bool skip_negated_command_line = get_chain_name;
 
+        if (done) return;
+
         // Ignore settings in default config section if requested.
         if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION &&
             !never_ignore_negated_setting) {
@@ -88,13 +91,12 @@ SettingsValue GetSetting(const Settings& settings,
         // Skip negated command line settings.
         if (skip_negated_command_line && span.last_negated()) return;
 
-        // Stick with highest priority value, keeping result if already set.
-        if (!result.isNull()) return;
-
         if (!span.empty()) {
             result = reverse_precedence ? span.begin()[0] : span.end()[-1];
+            done = true;
         } else if (span.last_negated()) {
             result = false;
+            done = true;
         }
     });
     return result;
@@ -106,7 +108,7 @@ std::vector<SettingsValue> GetSettingsList(const Settings& settings,
     bool ignore_default_section_config)
 {
     std::vector<SettingsValue> result;
-    bool result_complete = false;
+    bool done = false; // Done merging any more settings sources.
     bool prev_negated_empty = false;
     MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
         // Weird behavior preserved for backwards compatibility: Apply config
@@ -125,7 +127,7 @@ std::vector<SettingsValue> GetSettingsList(const Settings& settings,
 
         // Add new settings to the result if isn't already complete, or if the
         // values are zombies.
-        if (!result_complete || add_zombie_config_values) {
+        if (!done || add_zombie_config_values) {
             for (const auto& value : span) {
                 if (value.isArray()) {
                     result.insert(result.end(), value.getValues().begin(), value.getValues().end());
@@ -136,8 +138,8 @@ std::vector<SettingsValue> GetSettingsList(const Settings& settings,
         }
 
         // If a setting was negated, or if a setting was forced, set
-        // result_complete to true to ignore any later lower priority settings.
-        result_complete |= span.negated() > 0 || source == Source::FORCED;
+        // done to true to ignore any later lower priority settings.
+        done |= span.negated() > 0 || source == Source::FORCED;
 
         // Update the negated and empty state used for the zombie values check.
         prev_negated_empty |= span.last_negated() && result.empty();