Merge #12713: Track negated options in the option parser

f7683cba7b Track negated arguments in the argument paser. (Evan Klitzke)
4f872b2450 Add additional tests for GetBoolArg() (Evan Klitzke)

Pull request description:

  This change explicitly enable tracking negated options in the option parser. A negated option is one passed with a `-no` prefix. For example, `-nofoo` is the negated form of `-foo`. Negated options were originally added in the 0.6 release.

  The change here allows code to explicitly distinguish between cases like `-nofoo` and `-foo=0`, which was not possible previously. The option parser does not have any changed semantics as a result of this change, and existing code will parse options just as it did before.

  The motivation for this change is to provide a way to disable options that are otherwise not boolean options. For example, the `-debuglogfile` option is normally interpreted as a string, where the value is the log file name. With this change a user can pass in `-nodebuglogfile` and the code can see that it was explicitly negated, and use that to disable the log file.

  This change originally split out from #12689.

Tree-SHA512: cd5a7354eb03d2d402863c7b69e512cad382781d9b8f18c1ab104fc46d45a712530818d665203082da39572c8a42313c5be09306dc2a7227cdedb20ef7314823

src/test/util_tests.cpp

@@ -185,17 +185,11 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Time)
     BOOST_CHECK_EQUAL(FormatISO8601Time(1317425777), "23:36:17Z");
 }
 
-class TestArgsManager : public ArgsManager
+struct TestArgsManager : public ArgsManager
 {
-public:
-    std::map<std::string, std::string>& GetMapArgs()
-    {
-        return mapArgs;
-    };
-    const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs()
-    {
-        return mapMultiArgs;
-    };
+    std::map<std::string, std::string>& GetMapArgs() { return mapArgs; }
+    const std::map<std::string, std::vector<std::string> >& GetMapMultiArgs() { return mapMultiArgs; }
+    const std::unordered_set<std::string>& GetNegatedArgs() { return m_negated_args; }
 };
 
 BOOST_AUTO_TEST_CASE(util_ParseParameters)
@@ -223,6 +217,54 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
     BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
 }
 
+BOOST_AUTO_TEST_CASE(util_GetBoolArg)
+{
+    TestArgsManager testArgs;
+    const char *argv_test[] = {
+        "ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"};
+    testArgs.ParseParameters(7, (char**)argv_test);
+
+    // Each letter should be set.
+    for (char opt : "abcdef")
+        BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt);
+
+    // Nothing else should be in the map
+    BOOST_CHECK(testArgs.GetMapArgs().size() == 6 &&
+                testArgs.GetMapMultiArgs().size() == 6);
+
+    // The -no prefix should get stripped on the way in.
+    BOOST_CHECK(!testArgs.IsArgSet("-nob"));
+
+    // The -b option is flagged as negated, and nothing else is
+    BOOST_CHECK(testArgs.IsArgNegated("-b"));
+    BOOST_CHECK(testArgs.GetNegatedArgs().size() == 1);
+    BOOST_CHECK(!testArgs.IsArgNegated("-a"));
+
+    // Check expected values.
+    BOOST_CHECK(testArgs.GetBoolArg("-a", false) == true);
+    BOOST_CHECK(testArgs.GetBoolArg("-b", true) == false);
+    BOOST_CHECK(testArgs.GetBoolArg("-c", true) == false);
+    BOOST_CHECK(testArgs.GetBoolArg("-d", false) == true);
+    BOOST_CHECK(testArgs.GetBoolArg("-e", true) == false);
+    BOOST_CHECK(testArgs.GetBoolArg("-f", true) == false);
+}
+
+BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
+{
+    // Test some awful edge cases that hopefully no user will ever exercise.
+    TestArgsManager testArgs;
+    const char *argv_test[] = {"ignored", "-nofoo", "-foo", "-nobar=0"};
+    testArgs.ParseParameters(4, (char**)argv_test);
+
+    // This was passed twice, second one overrides the negative setting.
+    BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
+    BOOST_CHECK(testArgs.GetBoolArg("-foo", false) == true);
+
+    // A double negative is a positive.
+    BOOST_CHECK(testArgs.IsArgNegated("-bar"));
+    BOOST_CHECK(testArgs.GetBoolArg("-bar", false) == true);
+}
+
 BOOST_AUTO_TEST_CASE(util_GetArg)
 {
     TestArgsManager testArgs;

src/util.cpp

@@ -70,8 +70,6 @@
 #include <malloc.h>
 #endif
 
-#include <boost/algorithm/string/case_conv.hpp> // for to_lower()
-#include <boost/algorithm/string/predicate.hpp> // for startswith() and endswith()
 #include <boost/interprocess/sync/file_lock.hpp>
 #include <boost/program_options/detail/config_file.hpp>
 #include <boost/thread.hpp>
@@ -432,7 +430,23 @@ bool DirIsWritable(const fs::path& directory)
     return true;
 }
 
-/** Interpret string as boolean, for argument parsing */
+/**
+ * Interpret a string argument as a boolean.
+ *
+ * The definition of atoi() requires that non-numeric string values like "foo",
+ * return 0. This means that if a user unintentionally supplies a non-integer
+ * argument here, the return value is always false. This means that -foo=false
+ * does what the user probably expects, but -foo=true is well defined but does
+ * not do what they probably expected.
+ *
+ * The return value of atoi() is undefined when given input not representable as
+ * an int. On most systems this means string value between "-2147483648" and
+ * "2147483647" are well defined (this method will return true). Setting
+ * -txindex=2147483648 on most systems, however, is probably undefined.
+ *
+ * For a more extensive discussion of this topic (and a wide range of opinions
+ * on the Right Way to change this code), see PR12713.
+ */
 static bool InterpretBool(const std::string& strValue)
 {
     if (strValue.empty())
@@ -440,13 +454,30 @@ static bool InterpretBool(const std::string& strValue)
     return (atoi(strValue) != 0);
 }
 
-/** Turn -noX into -X=0 */
-static void InterpretNegativeSetting(std::string& strKey, std::string& strValue)
-{
-    if (strKey.length()>3 && strKey[0]=='-' && strKey[1]=='n' && strKey[2]=='o')
+/**
+ * Interpret -nofoo as if the user supplied -foo=0.
+ *
+ * This method also tracks when the -no form was supplied, and treats "-foo" as
+ * a negated option when this happens. This can be later checked using the
+ * IsArgNegated() method. One use case for this is to have a way to disable
+ * options that are not normally boolean (e.g. using -nodebuglogfile to request
+ * that debug log output is not sent to any file at all).
+ */
+void ArgsManager::InterpretNegatedOption(std::string& key, std::string& val)
 {
-        strKey = "-" + strKey.substr(3);
-        strValue = InterpretBool(strValue) ? "0" : "1";
+    if (key.substr(0, 3) == "-no") {
+        bool bool_val = InterpretBool(val);
+        if (!bool_val ) {
+            // Double negatives like -nofoo=0 are supported (but discouraged)
+            LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
+        }
+        key.erase(1, 2);
+        m_negated_args.insert(key);
+        val = bool_val ? "0" : "1";
+    } else {
+        // In an invocation like "bitcoind -nofoo -foo" we want to unmark -foo
+        // as negated when we see the second option.
+        m_negated_args.erase(key);
     }
 }
 
@@ -455,34 +486,34 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
     LOCK(cs_args);
     mapArgs.clear();
     mapMultiArgs.clear();
+    m_negated_args.clear();
 
-    for (int i = 1; i < argc; i++)
-    {
-        std::string str(argv[i]);
-        std::string strValue;
-        size_t is_index = str.find('=');
-        if (is_index != std::string::npos)
-        {
-            strValue = str.substr(is_index+1);
-            str = str.substr(0, is_index);
+    for (int i = 1; i < argc; i++) {
+        std::string key(argv[i]);
+        std::string val;
+        size_t is_index = key.find('=');
+        if (is_index != std::string::npos) {
+            val = key.substr(is_index + 1);
+            key.erase(is_index);
         }
 #ifdef WIN32
-        boost::to_lower(str);
-        if (boost::algorithm::starts_with(str, "/"))
-            str = "-" + str.substr(1);
+        std::transform(key.begin(), key.end(), key.begin(), ::tolower);
+        if (key[0] == '/')
+            key[0] = '-';
 #endif
 
-        if (str[0] != '-')
+        if (key[0] != '-')
             break;
 
-        // Interpret --foo as -foo.
-        // If both --foo and -foo are set, the last takes effect.
-        if (str.length() > 1 && str[1] == '-')
-            str = str.substr(1);
-        InterpretNegativeSetting(str, strValue);
+        // Transform --foo to -foo
+        if (key.length() > 1 && key[1] == '-')
+            key.erase(0, 1);
+
+        // Transform -nofoo to -foo=0
+        InterpretNegatedOption(key, val);
 
-        mapArgs[str] = strValue;
-        mapMultiArgs[str].push_back(strValue);
+        mapArgs[key] = val;
+        mapMultiArgs[key].push_back(val);
     }
 }
 
@@ -500,6 +531,12 @@ bool ArgsManager::IsArgSet(const std::string& strArg) const
     return mapArgs.count(strArg);
 }
 
+bool ArgsManager::IsArgNegated(const std::string& strArg) const
+{
+    LOCK(cs_args);
+    return m_negated_args.find(strArg) != m_negated_args.end();
+}
+
 std::string ArgsManager::GetArg(const std::string& strArg, const std::string& strDefault) const
 {
     LOCK(cs_args);
@@ -711,7 +748,7 @@ void ArgsManager::ReadConfigFile(const std::string& confPath)
             // Don't overwrite existing settings so command line settings override bitcoin.conf
             std::string strKey = std::string("-") + it->string_key;
             std::string strValue = it->value[0];
-            InterpretNegativeSetting(strKey, strValue);
+            InterpretNegatedOption(strKey, strValue);
             if (mapArgs.count(strKey) == 0)
                 mapArgs[strKey] = strValue;
             mapMultiArgs[strKey].push_back(strValue);

src/util.h

@@ -25,6 +25,7 @@
 #include <map>
 #include <stdint.h>
 #include <string>
+#include <unordered_set>
 #include <vector>
 
 #include <boost/signals2/signal.hpp>
@@ -224,6 +225,8 @@ protected:
     mutable CCriticalSection cs_args;
     std::map<std::string, std::string> mapArgs;
     std::map<std::string, std::vector<std::string>> mapMultiArgs;
+    std::unordered_set<std::string> m_negated_args;
+
 public:
     void ParseParameters(int argc, const char*const argv[]);
     void ReadConfigFile(const std::string& confPath);
@@ -244,6 +247,15 @@ public:
      */
     bool IsArgSet(const std::string& strArg) const;
 
+    /**
+     * Return true if the argument was originally passed as a negated option,
+     * i.e. -nofoo.
+     *
+     * @param strArg Argument to get (e.g. "-foo")
+     * @return true if the argument was passed negated
+     */
+    bool IsArgNegated(const std::string& strArg) const;
+
     /**
      * Return string argument or default value
      *
@@ -292,6 +304,11 @@ public:
     // Forces an arg setting. Called by SoftSetArg() if the arg hasn't already
     // been set. Also called directly in testing.
     void ForceSetArg(const std::string& strArg, const std::string& strValue);
+
+private:
+
+    // Munge -nofoo into -foo=0 and track the value as negated.
+    void InterpretNegatedOption(std::string &key, std::string &val);
 };
 
 extern ArgsManager gArgs;