In #14708, It was introduced that to warn when unrecognized section names are exist in the config file.
But m_config_sections.clear() in ArgsManager::ReadConfigStream() is called every time when reading each configuration file, so it can warn about only last reading file if includeconf exists.
This PR fix lack of warning by collecting all section names by moving m_config_sections.clear() to ArgsManager::ReadConfigFiles() .
Also add a test code to confirm this situation.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
utACK f7b0e60eac70fde74684ac406967c38ba8308679
test-nit: Wouldn't it be cleaner to also clear them in ReadConfigString?
@MarcoFalke Thank you for your review.
I'm sorry, but I don't understand. Is ReadConfigString related to this Python test?
I think removing includeconf=~ from including file rather than making both 2 included file empty is another idea.
Concept ACK.
I wonder if it should warn unrecognized sections per file (section + file + line)? It also avoid duplicate warns but maybe it shouldn't.
@AkioNak The unit tests don't read from files, but from strings. Since you moved the clear() to the method that reads files, it would no longer be called in the unit tests.
@MarcoFalke Thanks! I understand. I will check the unit test code and add to call the clear() .
The tests are passing without the clear(), but I guess the state might be dirty (haven't actually checked) and future tests might break unexpectedly.
@promag @MarcoFalke Addressed.
Both CI checks have failed:
Appveyou -> Many linker error occurred. they are almost unresolved external symbols. but now I don't know what's wrong.
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/22165173
Travis CI -> The Compile time was over 25min for each failed jobs. I think it is not related to this change. https://travis-ci.org/bitcoin/bitcoin/builds/489501276?utm_source=github_status&utm_medium=notification
edited: All checks have passed. Thanks for someone who restart them.
815 | @@ -816,7 +816,7 @@ void InitParameterInteraction() 816 | 817 | // Warn if unrecognized section name are present in the config file. 818 | for (const auto& section : gArgs.GetUnrecognizedSections()) { 819 | - InitWarning(strprintf(_("Section [%s] is not recognized."), section)); 820 | + InitWarning(strprintf(_("%s:%i:Section [%s] is not recognized."), section.file, section.line, section.name));
Would it make sense to move the %s:%i out of the translation message to keep it the same? The translators aren't going to understand that part anyway.
156 | @@ -150,9 +157,9 @@ class ArgsManager 157 | std::string m_network GUARDED_BY(cs_args); 158 | std::set<std::string> m_network_only_args GUARDED_BY(cs_args); 159 | std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args); 160 | - std::set<std::string> m_config_sections GUARDED_BY(cs_args); 161 | + std::list<SectionInfo> m_config_sections GUARDED_BY(cs_args); 162 | 163 | - NODISCARD bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false); 164 | + NODISCARD bool ReadConfigStream(std::istream& stream, const std::string& label, std::string& error, bool ignore_invalid_keys = false);
Why is the argument called label when in the SectionInfo it's file? I'd name it filepath.
I thought it might not only be a file stream. (unit test uses std::istringstream).
But now I agree that filepath is better than label. Fixed.
129 | @@ -130,6 +130,13 @@ enum class OptionsCategory { 130 | HIDDEN // Always the last option to avoid printing these in the help 131 | }; 132 | 133 | +struct SectionInfo 134 | +{ 135 | + std::string name;
nit, I think these should have m_ prefix? Developer notes mentions:
- Class member variables have a `m_` prefix.
Thanks. Fixed.
798 | @@ -803,7 +799,7 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect 799 | if (!str.empty()) { 800 | if (*str.begin() == '[' && *str.rbegin() == ']') { 801 | const std::string section = str.substr(1, str.size() - 2); 802 | - sections.insert(section); 803 | + sections.push_back(SectionInfo{section, label, linenr});
Could use emplace_back?
Thanks. Addressed.
369 | - std::set_difference( 370 | - m_config_sections.begin(), m_config_sections.end(), 371 | - available_sections.begin(), available_sections.end(), 372 | - std::inserter(diff, diff.end())); 373 | - return diff; 374 | + auto u = m_config_sections;
Use better variable name. Maybe also drop auto.
I mean, could have explicit type.
Thanks. I understand. Addressed.
370 | - m_config_sections.begin(), m_config_sections.end(), 371 | - available_sections.begin(), available_sections.end(), 372 | - std::inserter(diff, diff.end())); 373 | - return diff; 374 | + auto u = m_config_sections; 375 | + u.remove_if([](const SectionInfo& s){ return available_sections.find(s.name) != available_sections.end(); });
Use better variable name.
Fixed : parameter for lambda function.
811 | @@ -816,8 +812,8 @@ static bool GetConfigOptions(std::istream& stream, std::string& error, std::vect 812 | return false; 813 | } 814 | options.emplace_back(name, value); 815 | - if ((pos = name.rfind('.')) != std::string::npos) { 816 | - sections.insert(name.substr(0, pos)); 817 | + if ((pos = name.rfind('.')) != std::string::npos && prefix.length() <= pos) { 818 | + sections.push_back(SectionInfo{name.substr(0, pos), label, linenr});
Also emplace_back.
Thanks. Addressed.
utACK 53b7a25. Needs squash.
re-utACK 6fffaec0e72c21
utACK 6fffaec0e72c214d538ef50c337950b4c92ccd57, will test.
1. Fix lack of warning by collecting all section names by moving
m_config_sections.clear() to ArgsManager::ReadConfigFiles().
2. Add info(file name, line number) to warning message.
3. Add a test code to confirm this situation.
3. Do clear() in ReadConfigString().
re-utACK 1a7ba84e11
Tested ACK 1a7ba84.
Milestone
0.19.0