util: fix argsman dupe key error #27236
Conversation
fixes bitcoin#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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 8fcbdad. 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.
|
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" |
|
Code review ACK 8fcbdad |
8fcbdad util: fix argsman dupe key error (willcl-ark) Pull request description: fixes bitcoin#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). ACKs for top commit: TheCharlatan: Code review ACK 8fcbdad ryanofsky: Code review ACK 8fcbdad. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR: Tree-SHA512: a5fd49b30ede0a24188623192825bccb952e427cc35f96ff9bfdc737361dcc35ac6480589ddf7f0ddeaebd34361bdaee31e7a91f2c0d857e4ff682614bb6bc04
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.jsoncontains duplicate keys. This change has no effect onbitcoindbecause it treats a corruptsettings.jsonfile as a hard error and doesn't attempt to modify it.If we find a duplicate key and error, clear
valuesbefore returning so thatWriteSettings()will write an empty file, therefore clearing it.This aligns with GUI behaviour added in 1ee6d0b.
The test added only checks that
valuesis empty after a duplicate key is detected. This paves the way for theabortoption in the GUI to properly clearsettings.json, if the user selects the option, but the test does not currently check this entire mechanism (e.g. the file contents).