util: warn about ignored recursive -includeconf calls#13197
util: warn about ignored recursive -includeconf calls#13197laanwj merged 2 commits intobitcoin:masterfrom
Conversation
I (personally) prefer if PRs are more or less independent. |
|
utACK 6f10309. BTW couldn't |
|
concept ACK (and quick skim utACK). Will review & test after #12755 is merged. |
|
#12755 is merged, please rebase |
Since -includeconf cannot be used recursively, the user would not see feedback that an -includeconf in an -includeconf'd file was silently ignored.
kallewoof
force-pushed
the
includeconf-warn-nonrecursive
branch
from
6f10309 to
2352aa9
Compare
|
Rebased. |
|
@promag Is it better locking the whole time? |
I don't know what the potential races here are, but If everything else is the same, keeping the lock closely around the section where it is needed is preferable. Also: calls such as |
|
@laanwj I mean, |
As far as I know we don't really make any assumptions in that regard right now, but in sane API design I think that makes sense. |
|
I can try moving the lock to the top of the method and remove it elsewhere. I am concerned it may cause deadlocks though. |
|
ba5b9fc |
|
@kallewoof I think it's great to change that but in a different PR. Commit 2352aa9 is enough here. Sorry for asking here! |
kallewoof
force-pushed
the
includeconf-warn-nonrecursive
branch
from
ba5b9fc to
2352aa9
Compare
|
Yeah doesn't seem as simple as I hoped. Dropping ba5b9fc. |
|
utACK 2352aa9 |
2352aa9 test: Ensure that recursive -includeconf produces appropriate warnings (Karl-Johan Alm) c5bcc7d util: warn about recursive -includeconf arguments in configuration files (Karl-Johan Alm) Pull request description: This is a follow-up PR to #10267, and addresses #10267 (comment). ~~I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.~~ Tree-SHA512: 87f0c32436b70424e33616ffb88d7cb699f90d6a583a10237e224b28fc936d6a9df95536c8c52ee8546b3942da92b2a357e61bf87e00d1462bc10d46d3bee352
Summary: 2352aa9 test: Ensure that recursive -includeconf produces appropriate warnings (Karl-Johan Alm) c5bcc7d util: warn about recursive -includeconf arguments in configuration files (Karl-Johan Alm) Pull request description: This is a follow-up PR to #10267, and addresses bitcoin/bitcoin#10267 (comment). ~~I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.~~ Tree-SHA512: 87f0c32436b70424e33616ffb88d7cb699f90d6a583a10237e224b28fc936d6a9df95536c8c52ee8546b3942da92b2a357e61bf87e00d1462bc10d46d3bee352 Backport of Core PR13197 bitcoin/bitcoin#13197 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4118
…calls 2352aa9 test: Ensure that recursive -includeconf produces appropriate warnings (Karl-Johan Alm) c5bcc7d util: warn about recursive -includeconf arguments in configuration files (Karl-Johan Alm) Pull request description: This is a follow-up PR to bitcoin#10267, and addresses bitcoin#10267 (comment). ~~I am adding extra work for @jnewbery in bitcoin#12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.~~ Tree-SHA512: 87f0c32436b70424e33616ffb88d7cb699f90d6a583a10237e224b28fc936d6a9df95536c8c52ee8546b3942da92b2a357e61bf87e00d1462bc10d46d3bee352
5 participants
This is a follow-up PR to #10267, and addresses #10267 (comment).
I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.