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

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20181002-config-quotes changing 1 files +3 −0
  1. hebasto commented at 4:41 PM on October 2, 2018: member

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

  2. ken2812221 commented at 4:48 PM on October 2, 2018: contributor

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

  3. hebasto commented at 5:01 PM on October 2, 2018: member

    @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

  4. hebasto renamed this:
    Allow values quoting in config files
    utils and libraries: Allow values quoting in config files
    on Oct 2, 2018
  5. hebasto commented at 8:32 PM on October 2, 2018: member

    @ken2812221

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

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

  6. practicalswift commented at 8:45 PM on October 2, 2018: contributor

    Concept ACK

    Seems like a nice usability improvement

  7. fanquake added the label Utils/log/libs on Oct 2, 2018
  8. MarcoFalke commented at 4:40 AM on October 3, 2018: member

    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)

  9. MarcoFalke added the label Needs release notes on Oct 3, 2018
  10. ken2812221 commented at 4:52 PM on October 6, 2018: contributor

    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.

  11. hebasto commented at 5:46 PM on October 7, 2018: member

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

  12. hebasto commented at 6:33 PM on October 7, 2018: member

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

  13. hebasto renamed this:
    utils and libraries: Allow values quoting in config files
    utils and libraries: [WIP] Allow values quoting in config files
    on Oct 7, 2018
  14. hebasto renamed this:
    utils and libraries: [WIP] Allow values quoting in config files
    utils and libraries: Allow values quoting in config files
    on Oct 7, 2018
  15. hebasto force-pushed on Oct 7, 2018
  16. hebasto commented at 9:10 PM on October 7, 2018: member

    @ken2812221 Fixed. Please re-review.

  17. ryanofsky commented at 2:06 AM on October 8, 2018: member

    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.

  18. Allow values quoting in config files
    The config file values can be surrounded by quotation marks. This allows
    for explicit using of whitespaces (e.g. in paths).
    7bc6de6af5
  19. hebasto force-pushed on Oct 8, 2018
  20. hebasto commented at 7:30 AM on October 8, 2018: member

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

  21. ken2812221 commented at 7:41 AM on October 8, 2018: contributor

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

  22. MarcoFalke commented at 7:51 AM on October 8, 2018: member

    This needs tests before merge.

  23. hebasto commented at 7:13 PM on October 13, 2018: member

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

  24. laanwj commented at 5:31 AM on October 16, 2018: member

    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?
  25. meshcollider commented at 5:35 AM on October 16, 2018: 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

  26. laanwj commented at 5:37 AM on October 16, 2018: member

    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?!)

  27. laanwj commented at 6:45 AM on October 18, 2018: member

    Closing in favor of #14497

  28. laanwj closed this on Oct 18, 2018

  29. hebasto deleted the branch on Oct 18, 2018
  30. laanwj referenced this in commit 6746a89519 on Oct 20, 2018
  31. fanquake removed the label Needs release note on Mar 20, 2019
  32. PastaPastaPasta referenced this in commit 7644b0fa8e on Jun 27, 2021
  33. PastaPastaPasta referenced this in commit 1454380ea9 on Jun 28, 2021
  34. PastaPastaPasta referenced this in commit 931802b239 on Jun 29, 2021
  35. PastaPastaPasta referenced this in commit 8a9b3ff5f1 on Jul 1, 2021
  36. PastaPastaPasta referenced this in commit 868f071b9f on Jul 1, 2021
  37. PastaPastaPasta referenced this in commit 30cdf9c6f7 on Jul 1, 2021
  38. PastaPastaPasta referenced this in commit 787113a778 on Jul 3, 2021
  39. DrahtBot locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me