Skip to content

Commit 6677be6

Browse files
author
MarcoFalke
committed
Merge bitcoin#17473: refactor: Settings code cleanups
e9fd366 refactor: Remove null setting check in GetSetting() (Russell Yanofsky) cba2710 scripted-diff: Remove unused ArgsManager type flags in tests (Russell Yanofsky) 425bb30 refactor: Add util_CheckValue test (Russell Yanofsky) 0fa5435 refactor: Add ArgsManager::GetSettingsList method (Russell Yanofsky) 3e18552 refactor: Get rid of ArgsManagerHelper class (Russell Yanofsky) dc0f148 refactor: Replace FlagsOfKnownArg with GetArgFlags (Russell Yanofsky) 57e8b7a refactor: Clean up includeconf comments (Russell Yanofsky) 3f7dc9b refactor: Clean up long lines in settings code (Russell Yanofsky) Pull request description: This PR doesn't change behavior. It just implements some suggestions from bitcoin#15934 and bitcoin#16545 and few other small cleanups. ACKs for top commit: jnewbery: Code review ACK e9fd366 MarcoFalke: ACK e9fd366 🚟 Tree-SHA512: 6e100d92c72f72bc39567187ab97a3547b3c06e5fcf1a1b74023358b8bca552124ca6a53c0ab53179b7f1329c03d9a73faaef6d73d2cd1a2321568a0286525e2
2 parents 3e94938 + e9fd366 commit 6677be6

File tree

7 files changed

+235
-81
lines changed

7 files changed

+235
-81
lines changed

‎src/test/getarg_tests.cpp

+7-7
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ static void SetupArgs(const std::vector<std::pair<std::string, unsigned int>>& a
4343

4444
BOOST_AUTO_TEST_CASE(boolarg)
4545
{
46-
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL);
46+
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
4747
SetupArgs({foo});
4848
ResetArgs("-foo");
4949
BOOST_CHECK(gArgs.GetBoolArg("-foo", false));
@@ -97,8 +97,8 @@ BOOST_AUTO_TEST_CASE(boolarg)
9797

9898
BOOST_AUTO_TEST_CASE(stringarg)
9999
{
100-
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_STRING);
101-
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_STRING);
100+
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
101+
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY);
102102
SetupArgs({foo, bar});
103103
ResetArgs("");
104104
BOOST_CHECK_EQUAL(gArgs.GetArg("-foo", ""), "");
@@ -124,8 +124,8 @@ BOOST_AUTO_TEST_CASE(stringarg)
124124

125125
BOOST_AUTO_TEST_CASE(intarg)
126126
{
127-
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_INT);
128-
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_INT);
127+
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
128+
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY);
129129
SetupArgs({foo, bar});
130130
ResetArgs("");
131131
BOOST_CHECK_EQUAL(gArgs.GetArg("-foo", 11), 11);
@@ -159,8 +159,8 @@ BOOST_AUTO_TEST_CASE(doubledash)
159159

160160
BOOST_AUTO_TEST_CASE(boolargno)
161161
{
162-
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL);
163-
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_BOOL);
162+
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
163+
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY);
164164
SetupArgs({foo, bar});
165165
ResetArgs("-nofoo");
166166
BOOST_CHECK(!gArgs.GetBoolArg("-foo", true));

‎src/test/settings_tests.cpp

+13
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,19 @@ BOOST_AUTO_TEST_CASE(Simple)
4646
CheckValues(settings2, R"("val2")", R"(["val2","val3"])");
4747
}
4848

49+
// Confirm that a high priority setting overrides a lower priority setting even
50+
// if the high priority setting is null. This behavior is useful for a high
51+
// priority setting source to be able to effectively reset any setting back to
52+
// its default value.
53+
BOOST_AUTO_TEST_CASE(NullOverride)
54+
{
55+
util::Settings settings;
56+
settings.command_line_options["name"].push_back("value");
57+
BOOST_CHECK_EQUAL(R"("value")", GetSetting(settings, "section", "name", false, false).write().c_str());
58+
settings.forced_settings["name"] = {};
59+
BOOST_CHECK_EQUAL(R"(null)", GetSetting(settings, "section", "name", false, false).write().c_str());
60+
}
61+
4962
// Test different ways settings can be merged, and verify results. This test can
5063
// be used to confirm that updates to settings code don't change behavior
5164
// unintentionally.

‎src/test/util_tests.cpp

+127-19
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include <util/system.h>
66

77
#include <clientversion.h>
8+
#include <optional.h>
89
#include <sync.h>
910
#include <test/util/setup_common.h>
1011
#include <test/util/str.h>
@@ -189,12 +190,119 @@ struct TestArgsManager : public ArgsManager
189190
AddArg(arg.first, "", arg.second, OptionsCategory::OPTIONS);
190191
}
191192
}
193+
using ArgsManager::GetSetting;
194+
using ArgsManager::GetSettingsList;
192195
using ArgsManager::ReadConfigStream;
193196
using ArgsManager::cs_args;
194197
using ArgsManager::m_network;
195198
using ArgsManager::m_settings;
196199
};
197200

201+
//! Test GetSetting and GetArg type coercion, negation, and default value handling.
202+
class CheckValueTest : public TestChain100Setup
203+
{
204+
public:
205+
struct Expect {
206+
util::SettingsValue setting;
207+
bool default_string = false;
208+
bool default_int = false;
209+
bool default_bool = false;
210+
const char* string_value = nullptr;
211+
Optional<int64_t> int_value;
212+
Optional<bool> bool_value;
213+
Optional<std::vector<std::string>> list_value;
214+
const char* error = nullptr;
215+
216+
Expect(util::SettingsValue s) : setting(std::move(s)) {}
217+
Expect& DefaultString() { default_string = true; return *this; }
218+
Expect& DefaultInt() { default_int = true; return *this; }
219+
Expect& DefaultBool() { default_bool = true; return *this; }
220+
Expect& String(const char* s) { string_value = s; return *this; }
221+
Expect& Int(int64_t i) { int_value = i; return *this; }
222+
Expect& Bool(bool b) { bool_value = b; return *this; }
223+
Expect& List(std::vector<std::string> m) { list_value = std::move(m); return *this; }
224+
Expect& Error(const char* e) { error = e; return *this; }
225+
};
226+
227+
void CheckValue(unsigned int flags, const char* arg, const Expect& expect)
228+
{
229+
TestArgsManager test;
230+
test.SetupArgs({{"-value", flags}});
231+
const char* argv[] = {"ignored", arg};
232+
std::string error;
233+
bool success = test.ParseParameters(arg ? 2 : 1, (char**)argv, error);
234+
235+
BOOST_CHECK_EQUAL(test.GetSetting("-value").write(), expect.setting.write());
236+
auto settings_list = test.GetSettingsList("-value");
237+
if (expect.setting.isNull() || expect.setting.isFalse()) {
238+
BOOST_CHECK_EQUAL(settings_list.size(), 0);
239+
} else {
240+
BOOST_CHECK_EQUAL(settings_list.size(), 1);
241+
BOOST_CHECK_EQUAL(settings_list[0].write(), expect.setting.write());
242+
}
243+
244+
if (expect.error) {
245+
BOOST_CHECK(!success);
246+
BOOST_CHECK_NE(error.find(expect.error), std::string::npos);
247+
} else {
248+
BOOST_CHECK(success);
249+
BOOST_CHECK_EQUAL(error, "");
250+
}
251+
252+
if (expect.default_string) {
253+
BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), "zzzzz");
254+
} else if (expect.string_value) {
255+
BOOST_CHECK_EQUAL(test.GetArg("-value", "zzzzz"), expect.string_value);
256+
} else {
257+
BOOST_CHECK(!success);
258+
}
259+
260+
if (expect.default_int) {
261+
BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), 99999);
262+
} else if (expect.int_value) {
263+
BOOST_CHECK_EQUAL(test.GetArg("-value", 99999), *expect.int_value);
264+
} else {
265+
BOOST_CHECK(!success);
266+
}
267+
268+
if (expect.default_bool) {
269+
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), false);
270+
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), true);
271+
} else if (expect.bool_value) {
272+
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", false), *expect.bool_value);
273+
BOOST_CHECK_EQUAL(test.GetBoolArg("-value", true), *expect.bool_value);
274+
} else {
275+
BOOST_CHECK(!success);
276+
}
277+
278+
if (expect.list_value) {
279+
auto l = test.GetArgs("-value");
280+
BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), expect.list_value->begin(), expect.list_value->end());
281+
} else {
282+
BOOST_CHECK(!success);
283+
}
284+
}
285+
};
286+
287+
BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
288+
{
289+
using M = ArgsManager;
290+
291+
CheckValue(M::ALLOW_ANY, nullptr, Expect{{}}.DefaultString().DefaultInt().DefaultBool().List({}));
292+
CheckValue(M::ALLOW_ANY, "-novalue", Expect{false}.String("0").Int(0).Bool(false).List({}));
293+
CheckValue(M::ALLOW_ANY, "-novalue=", Expect{false}.String("0").Int(0).Bool(false).List({}));
294+
CheckValue(M::ALLOW_ANY, "-novalue=0", Expect{true}.String("1").Int(1).Bool(true).List({"1"}));
295+
CheckValue(M::ALLOW_ANY, "-novalue=1", Expect{false}.String("0").Int(0).Bool(false).List({}));
296+
CheckValue(M::ALLOW_ANY, "-novalue=2", Expect{false}.String("0").Int(0).Bool(false).List({}));
297+
CheckValue(M::ALLOW_ANY, "-novalue=abc", Expect{true}.String("1").Int(1).Bool(true).List({"1"}));
298+
CheckValue(M::ALLOW_ANY, "-value", Expect{""}.String("").Int(0).Bool(true).List({""}));
299+
CheckValue(M::ALLOW_ANY, "-value=", Expect{""}.String("").Int(0).Bool(true).List({""}));
300+
CheckValue(M::ALLOW_ANY, "-value=0", Expect{"0"}.String("0").Int(0).Bool(false).List({"0"}));
301+
CheckValue(M::ALLOW_ANY, "-value=1", Expect{"1"}.String("1").Int(1).Bool(true).List({"1"}));
302+
CheckValue(M::ALLOW_ANY, "-value=2", Expect{"2"}.String("2").Int(2).Bool(true).List({"2"}));
303+
CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"}));
304+
}
305+
198306
BOOST_AUTO_TEST_CASE(util_ParseParameters)
199307
{
200308
TestArgsManager testArgs;
@@ -289,12 +397,12 @@ BOOST_AUTO_TEST_CASE(util_ArgParsing)
289397
BOOST_AUTO_TEST_CASE(util_GetBoolArg)
290398
{
291399
TestArgsManager testArgs;
292-
const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL);
293-
const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL);
294-
const auto c = std::make_pair("-c", ArgsManager::ALLOW_BOOL);
295-
const auto d = std::make_pair("-d", ArgsManager::ALLOW_BOOL);
296-
const auto e = std::make_pair("-e", ArgsManager::ALLOW_BOOL);
297-
const auto f = std::make_pair("-f", ArgsManager::ALLOW_BOOL);
400+
const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY);
401+
const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY);
402+
const auto c = std::make_pair("-c", ArgsManager::ALLOW_ANY);
403+
const auto d = std::make_pair("-d", ArgsManager::ALLOW_ANY);
404+
const auto e = std::make_pair("-e", ArgsManager::ALLOW_ANY);
405+
const auto f = std::make_pair("-f", ArgsManager::ALLOW_ANY);
298406

299407
const char *argv_test[] = {
300408
"ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"};
@@ -333,8 +441,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
333441
TestArgsManager testArgs;
334442

335443
// Params test
336-
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_BOOL);
337-
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_BOOL);
444+
const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
445+
const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY);
338446
const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"};
339447
testArgs.SetupArgs({foo, bar});
340448
std::string error;
@@ -406,16 +514,16 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
406514

407515
TestArgsManager test_args;
408516
LOCK(test_args.cs_args);
409-
const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL);
410-
const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL);
411-
const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_STRING);
412-
const auto d = std::make_pair("-d", ArgsManager::ALLOW_STRING);
517+
const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY);
518+
const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY);
519+
const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_ANY);
520+
const auto d = std::make_pair("-d", ArgsManager::ALLOW_ANY);
413521
const auto e = std::make_pair("-e", ArgsManager::ALLOW_ANY);
414-
const auto fff = std::make_pair("-fff", ArgsManager::ALLOW_BOOL);
415-
const auto ggg = std::make_pair("-ggg", ArgsManager::ALLOW_BOOL);
416-
const auto h = std::make_pair("-h", ArgsManager::ALLOW_BOOL);
417-
const auto i = std::make_pair("-i", ArgsManager::ALLOW_BOOL);
418-
const auto iii = std::make_pair("-iii", ArgsManager::ALLOW_INT);
522+
const auto fff = std::make_pair("-fff", ArgsManager::ALLOW_ANY);
523+
const auto ggg = std::make_pair("-ggg", ArgsManager::ALLOW_ANY);
524+
const auto h = std::make_pair("-h", ArgsManager::ALLOW_ANY);
525+
const auto i = std::make_pair("-i", ArgsManager::ALLOW_ANY);
526+
const auto iii = std::make_pair("-iii", ArgsManager::ALLOW_ANY);
419527
test_args.SetupArgs({a, b, ccc, d, e, fff, ggg, h, i, iii});
420528

421529
test_args.ReadConfigString(str_config);
@@ -618,8 +726,8 @@ BOOST_AUTO_TEST_CASE(util_GetArg)
618726
BOOST_AUTO_TEST_CASE(util_GetChainName)
619727
{
620728
TestArgsManager test_args;
621-
const auto testnet = std::make_pair("-testnet", ArgsManager::ALLOW_BOOL);
622-
const auto regtest = std::make_pair("-regtest", ArgsManager::ALLOW_BOOL);
729+
const auto testnet = std::make_pair("-testnet", ArgsManager::ALLOW_ANY);
730+
const auto regtest = std::make_pair("-regtest", ArgsManager::ALLOW_ANY);
623731
test_args.SetupArgs({testnet, regtest});
624732

625733
const char* argv_testnet[] = {"cmd", "-testnet"};

‎src/util/settings.cpp

+19-10
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ SettingsValue GetSetting(const Settings& settings,
5656
bool get_chain_name)
5757
{
5858
SettingsValue result;
59+
bool done = false; // Done merging any more settings sources.
5960
MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
6061
// Weird behavior preserved for backwards compatibility: Apply negated
6162
// setting even if non-negated setting would be ignored. A negated
@@ -68,7 +69,9 @@ SettingsValue GetSetting(const Settings& settings,
6869
// precedence over early settings, but for backwards compatibility in
6970
// the config file the precedence is reversed for all settings except
7071
// chain name settings.
71-
const bool reverse_precedence = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !get_chain_name;
72+
const bool reverse_precedence =
73+
(source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) &&
74+
!get_chain_name;
7275

7376
// Weird behavior preserved for backwards compatibility: Negated
7477
// -regtest and -testnet arguments which you would expect to override
@@ -77,19 +80,23 @@ SettingsValue GetSetting(const Settings& settings,
7780
// negated values, or at least warn they are ignored.
7881
const bool skip_negated_command_line = get_chain_name;
7982

83+
if (done) return;
84+
8085
// Ignore settings in default config section if requested.
81-
if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION && !never_ignore_negated_setting) return;
86+
if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION &&
87+
!never_ignore_negated_setting) {
88+
return;
89+
}
8290

8391
// Skip negated command line settings.
8492
if (skip_negated_command_line && span.last_negated()) return;
8593

86-
// Stick with highest priority value, keeping result if already set.
87-
if (!result.isNull()) return;
88-
8994
if (!span.empty()) {
9095
result = reverse_precedence ? span.begin()[0] : span.end()[-1];
96+
done = true;
9197
} else if (span.last_negated()) {
9298
result = false;
99+
done = true;
93100
}
94101
});
95102
return result;
@@ -101,7 +108,7 @@ std::vector<SettingsValue> GetSettingsList(const Settings& settings,
101108
bool ignore_default_section_config)
102109
{
103110
std::vector<SettingsValue> result;
104-
bool result_complete = false;
111+
bool done = false; // Done merging any more settings sources.
105112
bool prev_negated_empty = false;
106113
MergeSettings(settings, section, name, [&](SettingsSpan span, Source source) {
107114
// Weird behavior preserved for backwards compatibility: Apply config
@@ -111,14 +118,16 @@ std::vector<SettingsValue> GetSettingsList(const Settings& settings,
111118
// value is followed by non-negated value, in which case config file
112119
// settings will be brought back from the dead (but earlier command
113120
// line settings will still be ignored).
114-
const bool add_zombie_config_values = (source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) && !prev_negated_empty;
121+
const bool add_zombie_config_values =
122+
(source == Source::CONFIG_FILE_NETWORK_SECTION || source == Source::CONFIG_FILE_DEFAULT_SECTION) &&
123+
!prev_negated_empty;
115124

116125
// Ignore settings in default config section if requested.
117126
if (ignore_default_section_config && source == Source::CONFIG_FILE_DEFAULT_SECTION) return;
118127

119128
// Add new settings to the result if isn't already complete, or if the
120129
// values are zombies.
121-
if (!result_complete || add_zombie_config_values) {
130+
if (!done || add_zombie_config_values) {
122131
for (const auto& value : span) {
123132
if (value.isArray()) {
124133
result.insert(result.end(), value.getValues().begin(), value.getValues().end());
@@ -129,8 +138,8 @@ std::vector<SettingsValue> GetSettingsList(const Settings& settings,
129138
}
130139

131140
// If a setting was negated, or if a setting was forced, set
132-
// result_complete to true to ignore any later lower priority settings.
133-
result_complete |= span.negated() > 0 || source == Source::FORCED;
141+
// done to true to ignore any later lower priority settings.
142+
done |= span.negated() > 0 || source == Source::FORCED;
134143

135144
// Update the negated and empty state used for the zombie values check.
136145
prev_negated_empty |= span.last_negated() && result.empty();

‎src/util/settings.h

+9-2
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,18 @@ struct Settings {
4343
//! [section] keywords)
4444
//! @param get_chain_name - enable special backwards compatible behavior
4545
//! for GetChainName
46-
SettingsValue GetSetting(const Settings& settings, const std::string& section, const std::string& name, bool ignore_default_section_config, bool get_chain_name);
46+
SettingsValue GetSetting(const Settings& settings,
47+
const std::string& section,
48+
const std::string& name,
49+
bool ignore_default_section_config,
50+
bool get_chain_name);
4751

4852
//! Get combined setting value similar to GetSetting(), except if setting was
4953
//! specified multiple times, return a list of all the values specified.
50-
std::vector<SettingsValue> GetSettingsList(const Settings& settings, const std::string& section, const std::string& name, bool ignore_default_section_config);
54+
std::vector<SettingsValue> GetSettingsList(const Settings& settings,
55+
const std::string& section,
56+
const std::string& name,
57+
bool ignore_default_section_config);
5158

5259
//! Return true if a setting is set in the default config file section, and not
5360
//! overridden by a higher priority command-line or network section value.

0 commit comments

Comments
 (0)