Fix lack of warning of unrecognized section names by AkioNak · Pull Request #15335 · bitcoin/bitcoin

AkioNak · 2019-02-04T06:11:43Z

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.

DrahtBot · 2019-02-04T08:21:03Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)

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.

AkioNak · 2019-02-04T08:56:02Z

If #14866 will be merged before this PR, I will drop the bug-fix code (system.cpp) , but leave the test code.

maflcko · 2019-02-04T15:59:05Z

utACK f7b0e60eac70fde74684ac406967c38ba8308679

maflcko · 2019-02-04T16:01:20Z

test-nit: Wouldn't it be cleaner to also clear them in ReadConfigString?

AkioNak · 2019-02-05T07:51:20Z

@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.

promag · 2019-02-05T14:44:20Z

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.

maflcko · 2019-02-05T15:41:03Z

@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.

AkioNak · 2019-02-05T16:24:56Z

@promag Thank you for your review. I did not focus on that behavior. Will address.

AkioNak · 2019-02-05T16:28:32Z

@MarcoFalke Thanks! I understand. I will check the unit test code and add to call the clear() .

maflcko · 2019-02-05T16:49:24Z

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.

AkioNak · 2019-02-06T14:50:36Z

@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.

src/init.cpp

promag

utACK 53b7a25. Needs squash.

promag · 2019-02-15T15:46:07Z

src/util/system.h

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.

promag · 2019-02-15T15:48:08Z

src/util/system.h

nit, I think these should have m_ prefix? Developer notes mentions:

- Class member variables have a `m_` prefix.

Thanks. Fixed.

promag · 2019-02-15T15:51:38Z

src/util/system.cpp

Could use emplace_back?

Thanks. Addressed.

promag · 2019-02-15T16:00:22Z

src/util/system.cpp

Use better variable name. Maybe also drop auto.

@promag I cannot drop auto simply. I am happy if you explain a little more.

I mean, could have explicit type.

Thanks. I understand.
Addressed.

promag · 2019-02-15T16:00:57Z

src/util/system.cpp

Use better variable name.

Fixed : parameter for lambda function.

promag · 2019-02-15T16:01:17Z

src/util/system.cpp

Also emplace_back.

Thanks. Addressed.

maflcko · 2019-02-18T16:06:16Z

re-utACK 6fffaec0e72c21

promag

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().

maflcko · 2019-02-19T19:13:15Z

re-utACK 1a7ba84

promag · 2019-02-23T17:26:16Z

Tested ACK 1a7ba84.

1a7ba84 Fix lack of warning of unrecognized section names (Akio Nakamura) Pull request description: In bitcoin#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. Tree-SHA512: 26aa0cbe3e4ae2e58cbe73d4492ee5cf465fd4c3e5df2c8ca7e282b627df9e637267af1e3816386b1dc6db2398b31936925ce0e432219fec3a9b3398f01e3e65

Summary: 1a7ba84e11 Fix lack of warning of unrecognized section names (Akio Nakamura) Pull request description: 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. Tree-SHA512: 26aa0cbe3e4ae2e58cbe73d4492ee5cf465fd4c3e5df2c8ca7e282b627df9e637267af1e3816386b1dc6db2398b31936925ce0e432219fec3a9b3398f01e3e65 --- Depends on D5757 This is a backport of Core [[bitcoin/bitcoin#15335 | PR15335]] Test Plan: ninja check test_runner.py feature_config_args.py Reviewers: deadalnix, #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5758

Summary: 1a7ba84e11 Fix lack of warning of unrecognized section names (Akio Nakamura) Pull request description: 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. Tree-SHA512: 26aa0cbe3e4ae2e58cbe73d4492ee5cf465fd4c3e5df2c8ca7e282b627df9e637267af1e3816386b1dc6db2398b31936925ce0e432219fec3a9b3398f01e3e65 --- Depends on D5757 This is a backport of Core [[bitcoin/bitcoin#15335 | PR15335]] Test Plan: - ninja check - test_runner.py feature_config_args.py Reviewers: deadalnix, #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5758

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().

1a7ba84 Fix lack of warning of unrecognized section names (Akio Nakamura) Pull request description: In bitcoin#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. Tree-SHA512: 26aa0cbe3e4ae2e58cbe73d4492ee5cf465fd4c3e5df2c8ca7e282b627df9e637267af1e3816386b1dc6db2398b31936925ce0e432219fec3a9b3398f01e3e65

merge bitcoin#15685,bitcoin#15335,bitcoin#15642,bitcoin#15686,bitcoin#15629

1a7ba84 Fix lack of warning of unrecognized section names (Akio Nakamura) Pull request description: In bitcoin#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. Tree-SHA512: 26aa0cbe3e4ae2e58cbe73d4492ee5cf465fd4c3e5df2c8ca7e282b627df9e637267af1e3816386b1dc6db2398b31936925ce0e432219fec3a9b3398f01e3e65

fanquake added the Utils/log/libs label Feb 4, 2019

laanwj reviewed Feb 12, 2019

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

promag reviewed Feb 15, 2019

View reviewed changes

AkioNak force-pushed the conf_include_multi branch from 53b7a25 to 6fffaec Compare February 18, 2019 07:16

promag reviewed Feb 18, 2019

View reviewed changes

AkioNak force-pushed the conf_include_multi branch from 6fffaec to 1a7ba84 Compare February 19, 2019 01:49

maflcko added this to the 0.19.0 milestone Feb 23, 2019

maflcko merged commit 1a7ba84 into bitcoin:master Mar 2, 2019

AkioNak deleted the conf_include_multi branch March 13, 2019 01:29

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 21, 2021

Merge pull request #4484 from vijaydasmp/backport_v19_vijay_batch_1

6e32a46

merge bitcoin#15685,bitcoin#15335,bitcoin#15642,bitcoin#15686,bitcoin#15629

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

AkioNak commented Feb 4, 2019

Uh oh!

DrahtBot commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

AkioNak commented Feb 4, 2019

Uh oh!

maflcko commented Feb 4, 2019

Uh oh!

maflcko commented Feb 4, 2019

Uh oh!

AkioNak commented Feb 5, 2019

Uh oh!

promag commented Feb 5, 2019

Uh oh!

maflcko commented Feb 5, 2019

Uh oh!

AkioNak commented Feb 5, 2019

Uh oh!

AkioNak commented Feb 5, 2019

Uh oh!

maflcko commented Feb 5, 2019

Uh oh!

AkioNak commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 18, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Feb 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Feb 4, 2019 •

edited

Loading

AkioNak commented Feb 6, 2019 •

edited

Loading

maflcko commented Feb 19, 2019 •

edited

Loading