util: Use ArgsManager::GetPathArg more widely #24675

1. 
   hebasto commented at 9:11 pm on March 25, 2022: member

   This PR is a continuation of bitcoin/bitcoin#24265 and bitcoin/bitcoin#24306.

   Now the following command-line arguments / configure options been read with the GetPathArg method:

   • -conf, also includeconf values been normalized
   • -rpccookiefile
2. DrahtBot added the label GUI on Mar 25, 2022
3. DrahtBot added the label RPC/REST/ZMQ on Mar 25, 2022
4. DrahtBot added the label Utils/log/libs on Mar 25, 2022
5. 
   hebasto commented at 11:28 am on April 4, 2022: member

   Friendly ping @ryanofsky @prusnak
6. 
   laanwj commented at 12:50 pm on April 6, 2022: member

   Concept ACK
7. 
   jarolrod commented at 11:01 pm on April 8, 2022: member

   concept ack
8. 
   DrahtBot commented at 3:54 am on April 10, 2022: contributor

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

   Conflicts

   No conflicts as of last run.

9. in src/util/system.cpp:952 in 88f48f39f8 outdated

   948@@ -949,7 +949,7 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
   949             const size_t default_includes = add_includes({});
   950 
   951             for (const std::string& conf_file_name : conf_file_names) {
   952-                std::ifstream conf_file_stream{GetConfigFile(conf_file_name)};
   953+                std::ifstream conf_file_stream{GetConfigFile(fs::NormalizedPathFromString(conf_file_name))};
   

   ---

   ---

   

   ryanofsky commented at 6:15 pm on April 20, 2022:

   In commit “util, refactor: Use GetPathArg to read “-conf” value” (88f48f39f84ae52f85819321dddf6e6bd9381be2)

   I think it would be better to use fs::PathFromString not fs::NormalizedPathFromString here

   • So this is a pure refactoring change that doesn’t affect behavior
   • To avoid changing user provided paths unnecessarily in a way that could make issues harder to debug
   • To avoid interpreting paths unnecessarily, and let the OS handle it

   ---

   

   hebasto commented at 11:17 am on April 21, 2022:

   Updated.
10. in src/fs.h:163 in bc9ea0d350 outdated

    154@@ -155,6 +155,18 @@ static inline path PathFromString(const std::string& string)
    155 #endif
    156 }
    157 
    158+/**
    159+ * Convert byte string to path object which in turn has been
    160+ * normalized, with redundant "." and ".." path components
    161+ * and trailing separators removed.
    162+ */
    163+static inline path NormalizedPathFromString(std::string string)
    

    ---

    ---

    

    ryanofsky commented at 6:27 pm on April 20, 2022:

    In commit “util, refactor: Add fs::NormalizedPathFromString function” (bc9ea0d3506610c0613bc27ee0c5c9d56e91092a)

    I think it would probably be better not to add this function because:

    • We already have other functions for converting strings to paths (PathFromString and u8path), and introducing a third function will probably add confusion. Especially when there is no explanation about when this function is supposed to be favored or avoided compared to the other ones.
    • I don’t think as a general rule that it is a good thing to normalize paths. I think debugging and understanding log messages if easier if paths are passed around verbatim and manipulated as little as possible, and the operating system is allowed to interpret them.
    • If it’s useful to have a normalization function that removes trailing slashes, I don’t think there’s a reason for it to do string decoding at the same. This function would be simpler and more accessible as:

    0static inline path NormalizedPath(std::path path)
    1{
    2    fs::path result = path.lexically_normal();
    3    // Remove trailing slash, if present.
    4    return result.has_filename() ? result : result.parent_path();
    5}
    

    ---

    

    hebasto commented at 11:16 am on April 21, 2022:

    The commit has been dropped.
11. ryanofsky approved
12. 
    ryanofsky commented at 6:35 pm on April 20, 2022: contributor

    Code review ACK 92aacdabf9b66bb7e030d80e842870dfcd1f9709, but it would be good to consider suggestions related to the NormalizedPathFromString function.
13. util, refactor: Use GetPathArg to read "-conf" value

    Also "includeconf" values been normalized.

    1276090705
14. util, refactor: Use GetPathArg to read "-rpccookiefile" value 138c668e2b
15. util, refactor: Drop explicit conversion to fs::path

    Removes unhelpful noise/verbosity.
    See: https://github.com/bitcoin/bitcoin/pull/24306#discussion_r809363741

    b01f336708
16. hebasto force-pushed on Apr 21, 2022
17. 
    hebasto commented at 11:15 am on April 21, 2022: member

    @ryanofsky

    Thank you for your review! Agree with all of points you mentioned above.

    Updated.

18. 
    jarolrod commented at 5:50 am on August 2, 2022: member

    Code Review ACK b01f336708019f8c8274ea701d3446e4123e7af2

    This is a nice cleanup and it is correct to expand the usage of GetPathArg to conf settings that are being passed a path, obviously :)

19. MarcoFalke removed the label GUI on Aug 2, 2022
20. MarcoFalke removed the label RPC/REST/ZMQ on Aug 2, 2022
21. MarcoFalke removed the label Utils/log/libs on Aug 2, 2022
22. DrahtBot added the label Utils/log/libs on Aug 2, 2022
23. ryanofsky approved
24. 
    ryanofsky commented at 3:50 pm on August 4, 2022: contributor

    Code review ACK b01f336708019f8c8274ea701d3446e4123e7af. Changes since last review: just dropping first commit (NormalizedPathFromString) as suggested
25. fanquake merged this on Aug 4, 2022
26. fanquake closed this on Aug 4, 2022

    ---

27. hebasto deleted the branch on Aug 4, 2022
28. sidhujag referenced this in commit bef507a126 on Aug 4, 2022
29. bitcoin locked this on Aug 4, 2023

Contributors
hebasto laanwj jarolrod DrahtBot ryanofsky

Labels
Utils/log/libs