util: fix argsman dupe key error #27236

pull willcl-ark wants to merge 1 commits into bitcoin:master from willcl-ark:argsman-dupe-key changing 2 files +4 −1

willcl-ark commented at 9:57 AM on March 10, 2023: contributor

fixes #22638

Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when settings.json contains duplicate keys. This change has no effect on bitcoind because it treats a corrupt settings.json file as a hard error and doesn't attempt to modify it.

If we find a duplicate key and error, clear values before returning so that WriteSettings() will write an empty file, therefore clearing it.

This aligns with GUI behaviour added in 1ee6d0b.

The test added only checks that values is empty after a duplicate key is detected. This paves the way for the abort option in the GUI to properly clear settings.json, if the user selects the option, but the test does not currently check this entire mechanism (e.g. the file contents).

util: fix argsman dupe key error

fixes #22638

If we find a duplicate key and error, clear `values` before returning so that
WriteSettings will write an empty file, therefore clearing it.

This aligns with GUI behaviour added in 1ee6d0b.

8fcbdadfad

DrahtBot commented at 9:57 AM on March 10, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ryanofsky, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Utils/log/libs on Mar 10, 2023
fanquake requested review from ryanofsky on Mar 10, 2023
ryanofsky approved
ryanofsky commented at 3:44 PM on March 10, 2023: contributor

Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR:

bugfix: Fix broken GUI "Reset" button when settings.json contains duplicate keys

Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when settings.json contains duplicate keys. This change has no effect on bitcoind because it treats a corrupt settings.json file as a hard error and doesn't attempt to modify it.
willcl-ark commented at 4:35 PM on March 10, 2023: contributor

Thanks @ryanofsky . I updated the PR description with your recommendation. Did you think I should update the commit title to "bugfix: Fix broken GUI "Reset" button when settings.json contains duplicate keys" also? I'm happy to do that if so.
ryanofsky commented at 4:42 PM on March 10, 2023: contributor

Thanks @ryanofsky . I updated the PR description with your recommendation. Did you think I should update the commit title to "bugfix: Fix broken GUI "Reset" button when settings.json contains duplicate keys" also? I'm happy to do that if so.

Either way seems fine, it's just a choice of whether you think it's more useful to describe the goal of the PR or the implemenation of the PR. Another alternative to changing the title could be changing the first line "fixes #22638" to "fixes #22638, broken gui settings reset button when settings.json contains duplicate keys"
TheCharlatan commented at 8:04 PM on March 10, 2023: contributor

Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3
fanquake merged this on Mar 11, 2023
fanquake closed this on Mar 11, 2023
sidhujag referenced this in commit 7ef87fa38c on Mar 12, 2023
bitcoin locked this on Mar 10, 2024

Type	Reviewers
ACK	ryanofsky, TheCharlatan

Contributors

Labels

Linked (view graph)

#22638 Bug: ArgsManager::ReadSettingsFile can return false even when it does load settings

util: fix argsman dupe key error #27236

Reviews