Fix env behavior to return true on empty vars

include/ignition/common/Util.hh

@@ -215,12 +215,25 @@ namespace ignition
     #endif
 
     /// \brief Find the environment variable '_name' and return its value.
+    ///
+    /// \TODO(mjcarroll): Deprecate and remove in tick-tock.
+    /// 
     /// \param[in] _name Name of the environment variable.
     /// \param[out] _value Value if the variable was found.
     /// \return True if the variable was found or false otherwise.
     bool IGNITION_COMMON_VISIBLE env(
         const std::string &_name, std::string &_value);
 
+    /// \brief Find the environment variable '_name' and return its value.
+    /// \param[in] _name Name of the environment variable.
+    /// \param[out] _value Value if the variable was found.
+    /// \param[in] _allowEmpty Allow set-but-empty variables.
+    ///           (Unsupported on Windows)
+    /// \return True if the variable was found or false otherwise.
+    bool IGNITION_COMMON_VISIBLE env(
+        const std::string &_name, std::string &_value,
+        bool _allowEmpty);
+
     /// \brief Get a UUID
     /// \return A UUID string
     std::string IGNITION_COMMON_VISIBLE uuid();

src/Util.cc

@@ -321,21 +321,41 @@ ignition::common::SystemPaths *ignition::common::systemPaths()
 
 /////////////////////////////////////////////////
 bool ignition::common::env(const std::string &_name, std::string &_value)
+{
+  return env(_name, _value, false);
+}
+
+/////////////////////////////////////////////////
+bool ignition::common::env(const std::string &_name, std::string &_value,
+                           bool _allowEmpty)
 {
   std::string v;
+  bool valid = false;
 #ifdef _WIN32
   const DWORD buffSize = 32767;
   static char buffer[buffSize];
   if (GetEnvironmentVariable(_name.c_str(), buffer, buffSize))
   {
     v = buffer;
   }
+
+  if(!v.empty())
+    valid = true;
+
 #else
   const char *cvar = std::getenv(_name.c_str());
-  if (cvar)
+  if (cvar != nullptr)
+  {
     v = cvar;
+    valid = true;
+
+    if (v[0] == '\0' && !_allowEmpty)
+    {
+      valid = false;
+    } 
+  }
 #endif
-  if (!v.empty())
+  if (valid)
   {
     _value = v;
     return true;

src/Util_TEST.cc

@@ -172,6 +172,100 @@ TEST(Util_TEST, emptyENV)
   EXPECT_TRUE(var.empty());
 }
 
+/////////////////////////////////////////////////
+TEST(Util_TEST, envSet)
+{
+  const auto key = "IGN_ENV_SET";
+  ASSERT_EQ(0, setenv(key, "VALUE", true));
+
+  // Check set var 
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value));
+    EXPECT_FALSE(value.empty());
+    EXPECT_EQ("VALUE", value);
+  }
+
+  // Check set var with allowEmpty
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value, true));
+    EXPECT_FALSE(value.empty());
+    EXPECT_EQ("VALUE", value);
+  }
+
+  // Check set var without allowEmpty
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value, false));
+    EXPECT_FALSE(value.empty());
+    EXPECT_EQ("VALUE", value);
+  }
+
+  unsetenv(key);
+}
+
+/////////////////////////////////////////////////
+TEST(Util_TEST, envUnset)
+{
+  const auto key = "IGN_ENV_UNSET";
+  unsetenv(key);
+
+  // Check unset var (default)
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value));
+    EXPECT_TRUE(value.empty());
+  }
+
+  // Check unset var with allowEmpty
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, true));
+    EXPECT_TRUE(value.empty());
+  }
+
+  // Check unset var without allowEmpty
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, false));
+    EXPECT_TRUE(value.empty());
+  }
+  unsetenv(key);
+}
+
+/////////////////////////////////////////////////
+TEST(Util_TEST, envSetEmpty)
+{
+  const auto key = "IGN_ENV_SET_EMPTY";
+
+  ASSERT_EQ(0, setenv(key, "", true));
+
+  // Check set empty var (default)
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value));
+    EXPECT_TRUE(value.empty());
+  }
+
+  // Check set empty var with allowEmpty
+#ifndef _WIN32
+  {
+    std::string value;
+    EXPECT_TRUE(common::env(key, value, true));
+    EXPECT_TRUE(value.empty());
+  }
+#endif
+
+  // Check set empty var without allowEmpty
+  {
+    std::string value;
+    EXPECT_FALSE(common::env(key, value, false));
+    EXPECT_TRUE(value.empty());
+  }
+  unsetenv(key);
+}
+
 /////////////////////////////////////////////////
 TEST(Util_TEST, findFile)
 {