util: Make ArgsManager::GetPathArg more widely usable

ryanofsky commented at 11:07 pm on February 9, 2022: member

Improve ArgsManager::GetPathArg method added in recent PR #24265, so it is usable more places. This PR starts to use it for the -settings option. This can also be helpful for #24274 which is parsing more path options.

Add GetPathArg default argument so it is less awkward to use to parse options that have default values.
Fix GetPathArg negated argument handling. Return path{} not path{“0”} when path argument is negated.
Add unit tests for default and negated cases
Move GetPathArg method declaration next to GetArg declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable.

DrahtBot added the label Utils/log/libs on Feb 9, 2022

ryanofsky force-pushed on Feb 9, 2022

ryanofsky commented at 11:21 pm on February 9, 2022: member

Updated 615da97a072e1f9ba1923173d12dab198155cf4c -> 5effc38445d4ef48bf5128def405d6da5a614dca (pr/patharg.1 -> pr/patharg.2, compare) to fix typo and window build error https://cirrus-ci.com/task/6421739189043200?logs=ci#L3944

kiminuo commented at 12:28 pm on February 10, 2022: contributor

I wonder what you would think (conceptually) about taking it a step further (not necessarily in this PR) and remove LocalTestingSetup which complicates understanding of the test code as one needs to take into account more code: https://github.com/kiminuo/bitcoin/commit/dceafb6fc5d057a7936227a41c0bfe3fc5ef56a9 -> https://github.com/kiminuo/bitcoin/commit/a01d80c3f67bafb9c79e67f39603784748b51cd4

in src/util/system.h:332 in 5effc38445 outdated

325@@ -336,6 +326,16 @@ class ArgsManager
326      */
327     std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
328 
329+    /**
330+     * Return path argument or default value
331+     *
332+     * It is guaranteed that the returned path has no trailing slashes.

MarcoFalke commented at 3:40 pm on February 10, 2022:

unless the default value has them

kiminuo commented at 9:15 pm on February 10, 2022:

Passes on Ubuntu:

0ResetArgs("");
1BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default/"), fs::path{"default/"});

ryanofsky commented at 5:45 pm on February 11, 2022:

re: #24306 (review)

unless the default value has them

Thanks, rewrote this

in src/util/system.h:338 in 5effc38445 outdated

325@@ -336,6 +326,16 @@ class ArgsManager
326      */
327     std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
328 
329+    /**
330+     * Return path argument or default value
331+     *
332+     * It is guaranteed that the returned path has no trailing slashes.
333+     *
334+     * @param arg Argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")

kiminuo commented at 9:31 pm on February 10, 2022:

Maybe we should document default_value too. Also, is it valid to pass the empty path to default_value parameter?

Do I understand correctly that returning an empty path means “not success”? If so, any opinion on returning std::optional<fs::path> instead?

ryanofsky commented at 5:46 pm on February 11, 2022:

re: #24306 (review)

Maybe we should document default_value too.

Thanks, added comment.

Also, is it valid to pass the empty path to default_value parameter?

Yes, there are already existing calls that use this default.

Do I understand correctly that returning an empty path means “not success”? If so, any opinion on returning std::optional<fs::path> instead?

No, an empty path is just an empty path, and it’s up to the caller to decide whether to treat the empty path like an error. GetArgPath() is supposed to be a convenience function, and it would not convenient or useful for callers to have to distinguish between nullopt result and an empty result. There also no cases that ever require this, even in theory, given the other APIs that are available GetSetting() GetArg() IsArgSet() IsArgNegated() as substitutes or complements to retrieve path arguments.

prusnak commented at 10:30 pm on February 10, 2022: contributor

I closed #24274 in favor of this PR. @ryanofsky Maybe incorporate https://github.com/bitcoin/bitcoin/pull/24274/commits/7db1e1fa731fa357f7cb1ee2aef441ff248044d4 into this PR?

in src/util/system.h:335 in 5effc38445 outdated

325@@ -336,6 +326,16 @@ class ArgsManager
326      */
327     std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
328 
329+    /**

prusnak commented at 10:31 pm on February 10, 2022:

Do we really need to change the order of methods in the header file? This makes the review unnecessarily harder than it has to be.

ryanofsky commented at 5:45 pm on February 11, 2022:

re: #24306 (review)

Do we really need to change the order of methods in the header file? This makes the review unnecessarily harder than it has to be.

It is just moving a comment, and the comment is entirely rewritten at this point, but if you still think this is a problem, just let me know what would be a better way to fix this.

in src/test/getarg_tests.cpp:269 in 5effc38445 outdated

254+    ResetArgs("-dir=override");
255+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"override"});
256+    ResetArgs("");
257+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"default"});
258+    ResetArgs("-nodir");
259+    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{""});

prusnak commented at 10:34 pm on February 10, 2022:

Let’s add the same checks but without the second parameter (default value) given:

0    ResetArgs("-dir=override");
1    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{"override"});
2    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"override"});
3    ResetArgs("");
4    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{});
5    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"default"});
6    ResetArgs("-nodir");
7    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{});
8    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{});

ryanofsky commented at 5:45 pm on February 11, 2022:

re: #24306 (review)

Let’s add the same checks but without the second parameter (default value) given:

0    ResetArgs("-dir=override");
1    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{"override"});
2    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"override"});
3    ResetArgs("");
4    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{});
5    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{"default"});
6    ResetArgs("-nodir");
7    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{});
8    BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir", "default"), fs::path{});

Those cases are already covered above, and I think this style of writing the test makes it less readable and useful as a clear description of GetPathArg behavior. GetPathArg is supposed to be a convenience function (it’s a high-level wrapper around lower-level GetArg and GetSetting methods), so if you want to ensure that GetPathArg has convenient and legible behavior, it helps to be to easily see how one GetPathArg call responds to different command lines. Having to think about how different GetPathArg calls that would never appear together in code would respond to the same command line separates the test from reality and makes it harder to understand.

in src/test/getarg_tests.cpp:262 in 5effc38445 outdated

248@@ -249,6 +249,14 @@ BOOST_AUTO_TEST_CASE(patharg)
249 
250     ResetArgs("-dir=user/.bitcoin/.//");
251     BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path);
252+
253+    // Check negated and default arguments
254+    ResetArgs("-dir=override");

hebasto commented at 12:50 pm on February 11, 2022:

What should we expect for -dir and -dir=?

ryanofsky commented at 5:45 pm on February 11, 2022:

re: #24306 (review)

What should we expect for -dir and -dir=?

Good point. Added tests for these.

in src/util/system.h:337 in 5effc38445 outdated

332+     * It is guaranteed that the returned path has no trailing slashes.
333+     *
334+     * @param arg Argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")
335+     * @return Lexically normalized path without trailing separators
336+     */
337+    fs::path GetPathArg(const std::string& arg, const fs::path& default_value={}) const;

hebasto commented at 12:59 pm on February 11, 2022:

In #24265 I was thinking about choosing const std::string& arg vs std::string arg.

Considering that (a) the longest arg in our codebase is 12 characters long, and (b) modern compilers optimization methods (small string optimization, copy elision etc), I’ve chosen to pass arg by value.

Has my consideration a flaw?

style nit: clang-format-diff.py insist on:

0    fs::path GetPathArg(const std::string& arg, const fs::path& default_value = {}) const;

ryanofsky commented at 5:46 pm on February 11, 2022:

re: #24306 (review)

In #24265 I was thinking about choosing const std::string& arg vs std::string arg.

Interesting! I restored your version. I had just changed this because I thought the difference was not intended. I don’t have any special insight, but your reasoning makes sense to me.

style nit: clang-format-diff.py insist on:

Updated, thanks!

hebasto commented at 12:59 pm on February 11, 2022: member

Concept ACK.

Fix GetPathArg negated argument handling. Return path{} not path{“0”} when path argument is negated.

:+1:

ryanofsky force-pushed on Feb 11, 2022

ryanofsky commented at 8:22 pm on February 11, 2022: member

Updated 5effc38445d4ef48bf5128def405d6da5a614dca -> 693cdca5f552f74de3665a874ce6acf6c48cb555 (pr/patharg.2 -> pr/patharg.3, compare) adding @prusnak’s commit from #24274 and tweaking comments and tests following some suggestions.

re: #24306 (comment)

I wonder what you would think (conceptually) about taking it a step further (not necessarily in this PR) and remove LocalTestingSetup which complicates understanding of the test code as one needs to take into account more code: kiminuo@dceafb6

I think the LocalTestingSetup in get_args is actually a pretty good and appropriate use of a test fixture. But I understand that test fixtures are less widely used in our codebase than some other codebases, so people may want to get rid of them. Normally test fixtures are used to write small isolated tests, but for some reason we tend to write large multipart tests. In any case, I wouldn’t object to getting rid of this test fixture if you also renane m_local_args to local_args to reflect the fact that it is no longer a member variable.

re: #24306 (comment)

@ryanofsky Maybe incorporate https://github.com/bitcoin/bitcoin/pull/24274/commits/7db1e1fa731fa357f7cb1ee2aef441ff248044d4 into this PR?

Thanks! Added as third commit

kiminuo commented at 9:33 pm on February 11, 2022: contributor

I wonder what you would think (conceptually) about taking it a step further (not necessarily in this PR) and remove LocalTestingSetup which complicates understanding of the test code as one needs to take into account more code: kiminuo@dceafb6

I think the LocalTestingSetup in get_args is actually a pretty good and appropriate use of a test fixture. But I understand that test fixtures are less widely used in our codebase than some other codebases, so people may want to get rid of them. Normally test fixtures are used to write small isolated tests, but for some reason we tend to write large multipart tests.

For me it’s simply a matter of “how much code I need to read to understand the test fully” (not about test fixtures in general). With LocalTestingSetup it’s more code (not trivial imo) and I fear I might miss something. For this reason alone, I think it’s worth to apply the suggestion because more people will be able to read the test with high confidence that test is correct and they understand all the prerequisites. Obviously, I’m not a great C++ programmer but then my argument is general: Adding the test fixture for these tests do not seem to add substantial value and without the test fixture we can clearly see what is actually needed to be done for the tests to pass (I find it very educational). If I fail to see some test fixture advantages, please let me know. Anyway, it’s just a suggestion. Feel free to apply it or ignore it as you find it the best.

In any case, I wouldn’t object to getting rid of this test fixture if you also renane m_local_args to local_args to reflect the fact that it is no longer a member variable.

Sure. 👍 Here: https://github.com/kiminuo/bitcoin/commit/a01d80c3f67bafb9c79e67f39603784748b51cd4

in src/util/system.h:334 in 693cdca5f5 outdated

325@@ -336,6 +326,18 @@ class ArgsManager
326      */
327     std::string GetArg(const std::string& strArg, const std::string& strDefault) const;
328 
329+    /**
330+     * Return path argument or default value
331+     *
332+     * @param arg Argument to get a path from (e.g., "-datadir", "-blocksdir" or "-walletdir")
333+     * @param default_value Optional default value to return instead of the empty path.
334+     * @return lightly normalized path if argument is set, with redundant "."

hebasto commented at 5:58 pm on February 12, 2022:

What does “lightly” mean in this context?

ryanofsky commented at 6:43 pm on February 17, 2022:

re: #24306 (review)

What does “lightly” mean in this context?

I’m not sure what’s motivating the question, because I think you basically wrote this function and understand what it does. Do you want me to change something about this comment? What would your suggested change be?

“Lightly” just means the function removes extra dots and slashes like the sentence says. But the point of the comment, and next sentence is to avoid geting into the weeds about file paths and and instead just point users to the unit test and implementation which I think are clearer than any comment here could be.

hebasto commented at 8:18 am on February 19, 2022:

I’m not sure what’s motivating the question, because I think you basically wrote this function and understand what it does.

I’m sorry about confusion. It was not my intention. My motivation is the following. The std::filesystem::path::lexically_normal function returns a path “converted to normal form in its generic format”. As normal form is well defined (at least within the std::filesystem library scope), I see no reasons for a term “lightly normalized path”. It just raises a useless question about difference between a “lightly normalized path” and a “normalized path”.

What would your suggested change be?

0     * [@return](/bitcoin-bitcoin/contributor/return/) normalized path if argument is set, with redundant "."

Do you want me to change something about this comment?

It is really a minor, so feel free to ignore it.

ryanofsky commented at 4:41 pm on February 25, 2022:

What would your suggested change be?

Thanks! Applied suggestion

in src/init.cpp:138 in 693cdca5f5 outdated

134@@ -135,7 +135,7 @@ static const char* BITCOIN_PID_FILENAME = "bitcoind.pid";
135 
136 static fs::path GetPidFile(const ArgsManager& args)
137 {
138-    return AbsPathForConfigVal(fs::PathFromString(args.GetArg("-pid", BITCOIN_PID_FILENAME)));
139+    return AbsPathForConfigVal(args.GetPathArg("-pid", BITCOIN_PID_FILENAME));

hebasto commented at 6:10 pm on February 12, 2022:

693cdca5f552f74de3665a874ce6acf6c48cb555

0    return AbsPathForConfigVal(args.GetPathArg("-pid", fs::path{BITCOIN_PID_FILENAME}));

ryanofsky commented at 6:48 pm on February 17, 2022:

re: #24306 (review)

Suggestion seems worse to me, since it is hardcoding assumptions about the type of BITCOIN_PID_FILENAME argument and the GetPathArg parameter in a place where they should not be relevant. If either of those type changes, or if more efficient overloads are added, this code could break or be deoptimized unnecessarily. This also adds unhelpful noise/verbosity to the code. I think the advantage of using statically typed langauges like C++ is that you declare types up front, and after that rely on the compiler to detect errors and optimize things appropriately, and don’t need to add manual type casts or type checks in places like this.

hebasto commented at 6:58 pm on March 5, 2022:

Is similar reasoning applicable to 5b946edd73640c6ecdfb4cbac1d4351e634678dc “util, refactor: Use GetPathArg to read “-settings” value”: https://github.com/bitcoin/bitcoin/blob/5b946edd73640c6ecdfb4cbac1d4351e634678dc/src/util/system.cpp#L522?

ryanofsky commented at 2:37 am on March 16, 2022:

Is similar reasoning applicable to 5b946ed “util, refactor: Use GetPathArg to read “-settings” value”:

Yes thanks for pointing that out. Since this PR was merged already, I wouldn’t open a separate PR just to simplify that line, but there should be other chances to clean it up in the future

hebasto commented at 7:21 am on March 26, 2022:

Addressed as a part of #24675.

in src/init.cpp:1252 in 693cdca5f5 outdated

1245@@ -1246,7 +1246,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1246         // Read asmap file if configured
1247         std::vector<bool> asmap;
1248         if (args.IsArgSet("-asmap")) {
1249-            fs::path asmap_path = fs::PathFromString(args.GetArg("-asmap", ""));
1250+            fs::path asmap_path = args.GetPathArg("-asmap");
1251             if (asmap_path.empty()) {
1252                 asmap_path = fs::PathFromString(DEFAULT_ASMAP_FILENAME);
1253             }

hebasto commented at 6:12 pm on February 12, 2022:

693cdca5f552f74de3665a874ce6acf6c48cb555

 0--- a/src/init.cpp
 1+++ b/src/init.cpp
 2@@ -1246,10 +1246,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 3         // Read asmap file if configured
 4         std::vector<bool> asmap;
 5         if (args.IsArgSet("-asmap")) {
 6-            fs::path asmap_path = args.GetPathArg("-asmap");
 7-            if (asmap_path.empty()) {
 8-                asmap_path = fs::PathFromString(DEFAULT_ASMAP_FILENAME);
 9-            }
10+            fs::path asmap_path = args.GetPathArg("-asmap", fs::path{DEFAULT_ASMAP_FILENAME});
11             if (!asmap_path.is_absolute()) {
12                 asmap_path = gArgs.GetDataDirNet() / asmap_path;
13             }

ryanofsky commented at 6:41 pm on February 17, 2022:

re: #24306 (review)

Added, thanks!

in src/init/common.cpp:84 in 693cdca5f5 outdated

80@@ -81,7 +81,7 @@ void AddLoggingArgs(ArgsManager& argsman)
81 void SetLoggingOptions(const ArgsManager& args)
82 {
83     LogInstance().m_print_to_file = !args.IsArgNegated("-debuglogfile");
84-    LogInstance().m_file_path = AbsPathForConfigVal(fs::PathFromString(args.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE)));
85+    LogInstance().m_file_path = AbsPathForConfigVal(args.GetPathArg("-debuglogfile", DEFAULT_DEBUGLOGFILE));

hebasto commented at 6:12 pm on February 12, 2022:

693cdca5f552f74de3665a874ce6acf6c48cb555

0    LogInstance().m_file_path = AbsPathForConfigVal(args.GetPathArg("-debuglogfile", fs::path{DEFAULT_DEBUGLOGFILE}));

ryanofsky commented at 6:42 pm on February 17, 2022:

re: #24306 (review)

Again this suggestion seems a little worse to me (see previous comment)

hebasto approved

hebasto commented at 6:13 pm on February 12, 2022: member

ACK 693cdca5f552f74de3665a874ce6acf6c48cb555

693cdca5f552f74de3665a874ce6acf6c48cb555 commit message “Use GetFileArg where possible” looks outdated, no?

uvhw referenced this in commit 47d44ccc3e on Feb 14, 2022

ryanofsky force-pushed on Feb 18, 2022

ryanofsky commented at 0:56 am on February 18, 2022: member

Thanks for reviews.

Updated 693cdca5f552f74de3665a874ce6acf6c48cb555 -> d7005c6392f451d29c39d3af07545450acd4967d (pr/patharg.3 -> pr/patharg.4, compare) just taking suggestions to fix a commit message and extend PR to cover -asmap

re: #24306#pullrequestreview-880877479

693cdca commit message “Use GetFileArg where possible” looks outdated, no?

Thanks, updated the commit title now

re: #24306 (comment)

Feel free to apply it or ignore it as you find it the best.

In any case, I wouldn’t object to getting rid of this test fixture if you also renane m_local_args to local_args to reflect the fact that it is no longer a member variable.

Sure. +1 Here: kiminuo@a01d80c

Thanks, like I said I’m perfectly happy with this change. I do think overlap with this PR is pretty minor (and it greatly increases the number of lines changed) so I posted it separately as #24375

DrahtBot commented at 5:02 am on February 19, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24455 (refactor: Split ArgsManager out of util/system by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto approved

hebasto commented at 8:18 am on February 19, 2022: member

re-ACK d7005c6392f451d29c39d3af07545450acd4967d

ryanofsky force-pushed on Feb 25, 2022

ryanofsky commented at 4:42 pm on February 25, 2022: member

Updated d7005c6392f451d29c39d3af07545450acd4967d -> b1443f72b5646f7ae98ab394e6f1fdf79daf2b67 (pr/patharg.4 -> pr/patharg.5, compare), just tweaking comment as suggested

in src/init.cpp:1232 in b1443f72b5 outdated

1245@@ -1246,10 +1246,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1246         // Read asmap file if configured
1247         std::vector<bool> asmap;
1248         if (args.IsArgSet("-asmap")) {
1249-            fs::path asmap_path = fs::PathFromString(args.GetArg("-asmap", ""));
1250-            if (asmap_path.empty()) {
1251-                asmap_path = fs::PathFromString(DEFAULT_ASMAP_FILENAME);
1252-            }
1253+            fs::path asmap_path = args.GetPathArg("-asmap", DEFAULT_ASMAP_FILENAME);

Empact commented at 4:53 pm on March 1, 2022:

I’m concerned this may not be equivalent on WIN32: PathFromString is not called on the default value in GetPathArg, so DEFAULT_ASMAP_FILENAME, a char *, is interpreted / converted by GetPathArg into a fs::path. I see path(const char* c) : std::filesystem::path(c) {} in fs.h. PathFromString is different on WIN32: u8path(string).

ryanofsky commented at 6:33 pm on March 1, 2022:

I’m concerned this may not be equivalent on WIN32: PathFromString is not called on the default value in GetPathArg, so DEFAULT_ASMAP_FILENAME, a char *, is interpreted / converted by GetPathArg into a fs::path. I see path(const char* c) : std::filesystem::path(c) {} in fs.h. PathFromString is different on WIN32: u8path(string).

That’s correct. The behavior is inherently different on different platforms because windows path objects represent paths with wchar_t while POSIX paths are represented with char. On windows, the fs::path const char* constructor will call the std::filesystem::path constructor and do a locale conversion from char to wchar_t using the the current code page. Normally we want to avoid conversions based on the code page, and the reason we define an fs::path class instead of using std::filesystem::path is to prevent these conversions from happening accidentally. But in this case, DEFAULT_ASMAP_FILENAME is just ASCII text, so the conversion should be harmless and always result in the same path, regardless of code page. That’s why the const char* constructor is allowed, and the reason for the code comment “Allow literal string arguments, which are safe as long as the literals are ASCII.”

MarcoFalke commented at 7:39 am on March 2, 2022:

We simply assume that char pointers are ASCII, but this is not true. In fact the unit tests get this wrong by passing char pointer to a string containing non ASCII chars (emojis).

ryanofsky commented at 2:16 am on March 3, 2022:

re: #24306 (review)

We simply assume that char pointers are ASCII, but this is not true. In fact the unit tests get this wrong by passing char pointer to a string containing non ASCII chars (emojis).

I don’t think I understand. I’m not aware of any places where unit tests or other code are doing something wrong. It is true that the fs::path interface does not guarantee there are no invalid conversions. It’s just trying to add some safety and prevent unintended conversions. In any case, I’m happy to make changes if there are any suggestions.

MarcoFalke commented at 11:30 am on March 3, 2022:

For example in the unit test:

0    fs::path tmpfile1 = tmpfolder / "fs_tests_₿_🏃";

I think the second argument is a char pointer, but it is not ascii

ryanofsky commented at 7:37 pm on March 3, 2022:

re: #24306 (review)

I think the second argument is a char pointer, but it is not ascii

You’re right, that’s a real bug. Created #24469 to fix and maybe implement a followup

Empact commented at 4:55 pm on March 1, 2022: member

Concept ACK

util: Add GetPathArg default path argument

Let GetPathArg method be used more places for path arguments that have
default values, like "-settings" and BITCOIN_SETTINGS_FILENAME in the
next commit.

Also:

- Fix negated argument handling. Return path{} not path{"0"} when path
  argument is negated.

- Add new tests for default and negated cases

- Move GetPathArg() method declaration next to GetArg() declarations.
  The two methods are close substitutes for each other, so this should
  help keep them consistent and make them more discoverable.

687e655ae2

util, refactor: Use GetPathArg to read "-settings" value

Take advantage of GetPathArg to simplify code slightly.

5b946edd73

Use GetPathArg where possible

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

60aa179d8f

MarcoFalke referenced this in commit 08bcfa2767 on Mar 2, 2022

DrahtBot added the label Needs rebase on Mar 2, 2022

sidhujag referenced this in commit 6dabdd11c8 on Mar 2, 2022

ryanofsky force-pushed on Mar 3, 2022

ryanofsky commented at 2:19 am on March 3, 2022: member

Rebased b1443f72b5646f7ae98ab394e6f1fdf79daf2b67 -> 60aa179d8f9a675efa2d78eaadc09e3ba450f50f (pr/patharg.5 -> pr/patharg.6, compare) due to conflict with #24375

DrahtBot removed the label Needs rebase on Mar 3, 2022

ryanofsky referenced this in commit 2f5fd3cf92 on Mar 3, 2022

w0xlt approved

w0xlt commented at 0:01 am on March 4, 2022: contributor

Tested ACK 60aa179 on Ubuntu 21.10

hebasto approved

hebasto commented at 7:01 pm on March 5, 2022: member

re-ACK 60aa179d8f9a675efa2d78eaadc09e3ba450f50f

MarcoFalke merged this on Mar 7, 2022

MarcoFalke closed this on Mar 7, 2022

sidhujag referenced this in commit 245c19aba9 on Mar 7, 2022

MarcoFalke referenced this in commit 76d44e832f on Mar 10, 2022

sidhujag referenced this in commit 941d3a743e on Mar 11, 2022

hebasto referenced this in commit 3ca8da860b on Mar 25, 2022

hebasto referenced this in commit f6e14d6f8d on Mar 25, 2022

hebasto referenced this in commit 92aacdabf9 on Mar 25, 2022

hebasto referenced this in commit b01f336708 on Apr 21, 2022

janus referenced this in commit 4616fc3fe1 on Jul 24, 2022

fanquake referenced this in commit e09ad284c7 on Aug 4, 2022

Rspigler referenced this in commit 5ba30ec515 on Aug 21, 2022

DrahtBot locked this on Mar 26, 2023

util: Make ArgsManager::GetPathArg more widely usable #24306

Conflicts