Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utils and libraries: Allow values quoting in config files #14370

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Oct 2, 2018

The using double quotes to quote command line option values with spaces
(e.g. paths) is allowed on all platforms. Options in config files are
handled in the same way now.

The config file values can be surrounded by quotation marks. This allows
for explicit using of whitespaces (e.g. in paths).

@ken2812221
Copy link
Contributor

ken2812221 commented Oct 2, 2018

Doesn't the path with spaces in it work without quotes?
How to specify a path that has quotes at the end?

@hebasto
Copy link
Member Author

hebasto commented Oct 2, 2018

@ken2812221

Doesn't the path with spaces in it work without quotes?

Yes, it works.
But it is inconsistent. Compare:
bitcoin-qt -blocksdir="/some/path/with space"
with a line in a config file
blocksdir=/some/path/with space

@hebasto hebasto changed the title Allow values quoting in config files utils and libraries: Allow values quoting in config files Oct 2, 2018
@hebasto
Copy link
Member Author

hebasto commented Oct 2, 2018

@ken2812221

How to specify a path that has quotes at the end?

I didn't catch it. Can you provide a use case?

@practicalswift
Copy link
Contributor

practicalswift commented Oct 2, 2018

Concept ACK

Seems like a nice usability improvement

@maflcko
Copy link
Member

maflcko commented Oct 3, 2018

This is a breaking change with previous versions? In any case, this needs extensive release notes documentation or update the documentation of our config file parser. (Not sure if we have that user documentation already)

@ken2812221
Copy link
Contributor

ken2812221 commented Oct 6, 2018

If someone is using a weird path datadir=/path/contains/quote", It would strip out the ". It would also strip it out if it only appear once. Also, in command line you can specify -datadir=/path/"contain whitespace", it is still inconsistent with conf file.

@hebasto
Copy link
Member Author

hebasto commented Oct 7, 2018

@MarcoFalke
Thank you for your review.

This is a breaking change with previous versions? In any case, this needs extensive release notes documentation or update the documentation of our config file parser. (Not sure if we have that user documentation already)

#14427 addressed to your concerns about user documentation. Would you mind to review it?

@hebasto
Copy link
Member Author

hebasto commented Oct 7, 2018

@ken2812221
Thank you for your review.

Also, in command line you can specify -datadir=/path/"contain whitespace", it is still inconsistent with conf file.

My bad. I'm going to fix it.

@hebasto hebasto changed the title utils and libraries: Allow values quoting in config files utils and libraries: [WIP] Allow values quoting in config files Oct 7, 2018
@hebasto hebasto changed the title utils and libraries: [WIP] Allow values quoting in config files utils and libraries: Allow values quoting in config files Oct 7, 2018
@hebasto
Copy link
Member Author

hebasto commented Oct 7, 2018

@ken2812221
Fixed. Please re-review.

@ryanofsky
Copy link
Contributor

I think it's better for config file parser to either:

  1. not interpret quotes at all
  2. interpret quotes in a very simple way like https://en.wikipedia.org/wiki/INI_file#Quoted_values
  3. interpret quotes in a documented way like https://github.com/toml-lang/toml#user-content-string

than to do what this PR does and try to make config file quoting vaguely resemble posix shell quoting and windows msvcrt command line quoting without actually implementing either scheme.

The config file values can be surrounded by quotation marks. This allows
for explicit using of whitespaces (e.g. in paths).
@hebasto
Copy link
Member Author

hebasto commented Oct 8, 2018

@ryanofsky
Thank you for your review.

interpret quotes in a very simple way like https://en.wikipedia.org/wiki/INI_file#Quoted_values

Agree. Done. Please re-review.

@ken2812221
Copy link
Contributor

Concept ACK. This is much better. Also the use can specify any characters that it wants.

@maflcko
Copy link
Member

maflcko commented Oct 8, 2018

This needs tests before merge.

@hebasto
Copy link
Member Author

hebasto commented Oct 13, 2018

@MarcoFalke

This needs tests before merge.

I've modified the src/test/util_tests.cpp unit tests. Should this patch be placed in this PR?

@laanwj
Copy link
Member

laanwj commented Oct 16, 2018

i'm not sure this is worth it

  • you've always been able to use spaces in .conf arguments, this just needs to be documented better
  • as far as I know, explicit quoting is not part of .ini file format as used in other software, and that's what our conf format is based on (but let me know otherwise if this is wrong!)
  • as @MarcoFalke mentioned by this is a breaking change, what if you want to have " around a value? you'll now end up with two sets?

@meshcollider
Copy link
Contributor

as @MarcoFalke mentioned by this is a breaking change, what if you want to have " around a value? you'll now end up with two sets?

Especially in, for example, an rpcpassword or something

@laanwj
Copy link
Member

laanwj commented Oct 16, 2018

yes, there is plenty of tooling/scripting that assumes you can just dump a value after =, this will all have to be rewritten to take this into account

(edit: although arguably, #13143 already complicates this, and provides a use-case for quoting maybe?!)

@laanwj
Copy link
Member

laanwj commented Oct 18, 2018

Closing in favor of #14497

@laanwj laanwj closed this Oct 18, 2018
@hebasto hebasto deleted the 20181002-config-quotes branch October 18, 2018 17:55
laanwj added a commit that referenced this pull request Oct 20, 2018
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - #14370
  - #14427
  - #14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 3, 2021
1fb3c16 Add `doc/bitcoin-conf.md` (Hennadii Stepanov)

Pull request description:

  From the IRC:
  > 2018-10-16T05:35:03  \<wumpus\> if something can be solved by better documentation, please work on documentation!
  > 2018-10-16T05:35:12  \<wumpus\> don't change the code instead

  Refs:

  - bitcoin#14370
  - bitcoin#14427
  - bitcoin#14494

  Based on the BITCOIN.CONF(5) manual page written by Micah Anderson \<micah@debian.org\> for the Debian system.

Tree-SHA512: 16393c9073c027fa1c46f8b59651e60b9a3159b3aeb9b3102040c292d2787f32b1ead5977957ac3ac0759a4bf626650a2325b68ad84320964ac089ffc2d3b4f4
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants