C++-ify subst_env_in_string

diff --git a/src/Config.cpp b/src/Config.cpp
index 02c5c5addb8a61e2883538f73e4ad4f12a925829..f58ecc6400a76e1d14ee861c97956371e3690408 100644 (file)
--- a/src/Config.cpp
+++ b/src/Config.cpp
@@ -196,21 +196,6 @@ format_bool(bool value)
   return value ? "true" : "false";
 }
 
-std::string
-parse_env_string(const std::string& value)
-{
-  char* errmsg = nullptr;
-  char* substituted = subst_env_in_string(value.c_str(), &errmsg);
-  if (!substituted) {
-    std::string error_message = errmsg;
-    free(errmsg);
-    throw Error(error_message);
-  }
-  std::string result = substituted;
-  free(substituted);
-  return result;
-}
-
 double
 parse_double(const std::string& value)
 {
@@ -673,7 +658,7 @@ Config::set_item(const std::string& key,
 
   switch (it->second) {
   case ConfigItem::base_dir:
-    m_base_dir = parse_env_string(value);
+    m_base_dir = Util::expand_environment_variables(value);
     if (!m_base_dir.empty()) { // The empty string means "disable"
       verify_absolute_path(m_base_dir);
       m_base_dir = Util::normalize_absolute_path(m_base_dir);
@@ -681,7 +666,7 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::cache_dir:
-    set_cache_dir(parse_env_string(value));
+    set_cache_dir(Util::expand_environment_variables(value));
     break;
 
   case ConfigItem::cache_dir_levels:
@@ -733,7 +718,7 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::extra_files_to_hash:
-    m_extra_files_to_hash = parse_env_string(value);
+    m_extra_files_to_hash = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::file_clone:
@@ -749,11 +734,11 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::ignore_headers_in_manifest:
-    m_ignore_headers_in_manifest = parse_env_string(value);
+    m_ignore_headers_in_manifest = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::ignore_options:
-    m_ignore_options = parse_env_string(value);
+    m_ignore_options = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::inode_cache:
@@ -769,7 +754,7 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::log_file:
-    m_log_file = parse_env_string(value);
+    m_log_file = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::max_files:
@@ -781,7 +766,7 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::path:
-    m_path = parse_env_string(value);
+    m_path = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::pch_external_checksum:
@@ -789,11 +774,11 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::prefix_command:
-    m_prefix_command = parse_env_string(value);
+    m_prefix_command = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::prefix_command_cpp:
-    m_prefix_command_cpp = parse_env_string(value);
+    m_prefix_command_cpp = Util::expand_environment_variables(value);
     break;
 
   case ConfigItem::read_only:
@@ -821,7 +806,7 @@ Config::set_item(const std::string& key,
     break;
 
   case ConfigItem::temporary_dir:
-    m_temporary_dir = parse_env_string(value);
+    m_temporary_dir = Util::expand_environment_variables(value);
     m_temporary_dir_configured_explicitly = true;
     break;
 

diff --git a/src/Util.cpp b/src/Util.cpp
index 0cda41f1353b02db22341a7afd4db8992c3a9eb3..b8b92f89a2050fbeb9a97029b563e382323c880c 100644 (file)
--- a/src/Util.cpp
+++ b/src/Util.cpp
@@ -251,6 +251,49 @@ ends_with(string_view string, string_view suffix)
   return string.ends_with(suffix);
 }
 
+std::string
+expand_environment_variables(const std::string& str)
+{
+  std::string result;
+  const char* left = str.c_str();
+  for (const char* right = left; *right; ++right) {
+    if (*right == '$') {
+      result.append(left, right - left);
+
+      left = right + 1;
+      bool curly = *left == '{';
+      if (curly) {
+        ++left;
+      }
+      right = left;
+      while (isalnum(*right) || *right == '_') {
+        ++right;
+      }
+      if (curly && *right != '}') {
+        throw Error(
+          fmt::format("syntax error: missing '}}' after \"{}\"", left));
+      }
+      if (right == left) {
+        // Special case: don't consider a single $ the left of a variable.
+        result += '$';
+      } else {
+        std::string name(left, right - left);
+        const char* value = getenv(name.c_str());
+        if (!value) {
+          throw Error(fmt::format("environment variable \"{}\" not set", name));
+        }
+        result += value;
+        if (!curly) {
+          --right;
+        }
+        left = right + 1;
+      }
+    }
+  }
+  result += left;
+  return result;
+}
+
 int
 fallocate(int fd, long new_size)
 {

diff --git a/src/Util.hpp b/src/Util.hpp
index f4844bee67592888b61da5bf398ca8da63157213..b2cd2844d7e5dd299724a8c917b0c882a51c2759 100644 (file)
--- a/src/Util.hpp
+++ b/src/Util.hpp
@@ -107,6 +107,11 @@ nonstd::string_view dir_name(nonstd::string_view path);
 // Return true if suffix is a suffix of string.
 bool ends_with(nonstd::string_view string, nonstd::string_view suffix);
 
+// Expand all instances of $VAR or ${VAR}, where VAR is an environment variable,
+// in `str`. Throws `Error` if one of the environment variables.
+[[gnu::warn_unused_result]] std::string
+expand_environment_variables(const std::string& str);
+
 // Extends file size to at least new_size by calling posix_fallocate() if
 // supported, otherwise by writing zeros last to the file.
 //

diff --git a/src/legacy_util.cpp b/src/legacy_util.cpp
index 2dafad98c24a48cf0e375230095f3b3f56303d50..00c97e302e59a6d9abcf3b227f357776f5ae447a 100644 (file)
--- a/src/legacy_util.cpp
+++ b/src/legacy_util.cpp
@@ -508,80 +508,6 @@ x_rename(const char* oldpath, const char* newpath)
 #endif
 }
 
-static bool
-expand_variable(const char** str, char** result, char** errmsg)
-{
-  assert(**str == '$');
-
-  bool curly;
-  const char* p = *str + 1;
-  if (*p == '{') {
-    curly = true;
-    ++p;
-  } else {
-    curly = false;
-  }
-
-  const char* q = p;
-  while (isalnum(*q) || *q == '_') {
-    ++q;
-  }
-  if (curly) {
-    if (*q != '}') {
-      *errmsg = format("syntax error: missing '}' after \"%s\"", p);
-      return false;
-    }
-  }
-
-  if (q == p) {
-    // Special case: don't consider a single $ the start of a variable.
-    reformat(result, "%s$", *result);
-    return true;
-  }
-
-  char* name = x_strndup(p, q - p);
-  const char* value = getenv(name);
-  if (!value) {
-    *errmsg = format("environment variable \"%s\" not set", name);
-    free(name);
-    return false;
-  }
-  reformat(result, "%s%s", *result, value);
-  if (!curly) {
-    --q;
-  }
-  *str = q;
-  free(name);
-  return true;
-}
-
-// Substitute all instances of $VAR or ${VAR}, where VAR is an environment
-// variable, in a string. Caller frees. If one of the environment variables
-// doesn't exist, NULL will be returned and *errmsg will be an appropriate
-// error message (caller frees).
-char*
-subst_env_in_string(const char* str, char** errmsg)
-{
-  assert(errmsg);
-  *errmsg = nullptr;
-
-  char* result = x_strdup("");
-  const char* p = str; // Interval start.
-  const char* q = str; // Interval end.
-  for (; *q; ++q) {
-    if (*q == '$') {
-      reformat(&result, "%s%.*s", result, (int)(q - p), p);
-      if (!expand_variable(&q, &result, errmsg)) {
-        free(result);
-        return nullptr;
-      }
-      p = q + 1;
-    }
-  }
-  reformat(&result, "%s%.*s", result, (int)(q - p), p);
-  return result;
-}
-
 void
 set_cloexec_flag(int fd)
 {

diff --git a/src/legacy_util.hpp b/src/legacy_util.hpp
index ded137fb9b779d6d90d00c4716a247fa2b840e20..16049292df299d4baaa981b5ff040c739b948044 100644 (file)
--- a/src/legacy_util.hpp
+++ b/src/legacy_util.hpp
@@ -46,6 +46,5 @@ bool is_full_path(const char* path);
 void update_mtime(const char* path);
 void x_exit(int status) ATTR_NORETURN;
 int x_rename(const char* oldpath, const char* newpath);
-char* subst_env_in_string(const char* str, char** errmsg);
 void set_cloexec_flag(int fd);
 double time_seconds();

diff --git a/unittest/test_Util.cpp b/unittest/test_Util.cpp
index 7a864a134ec7a0ae4fb516be560cade000801bc0..844cd6253916841526c1a54c8f29ddec0139c05f 100644 (file)
--- a/unittest/test_Util.cpp
+++ b/unittest/test_Util.cpp
@@ -159,6 +159,27 @@ TEST_CASE("Util::ends_with")
   CHECK_FALSE(Util::ends_with("x", "xy"));
 }
 
+TEST_CASE("Util::expand_environment_variables")
+{
+  x_setenv("FOO", "bar");
+
+  CHECK(Util::expand_environment_variables("") == "");
+  CHECK(Util::expand_environment_variables("$FOO") == "bar");
+  CHECK(Util::expand_environment_variables("$") == "$");
+  CHECK(Util::expand_environment_variables("$FOO $FOO:$FOO") == "bar bar:bar");
+  CHECK(Util::expand_environment_variables("x$FOO") == "xbar");
+  CHECK(Util::expand_environment_variables("${FOO}x") == "barx");
+
+  DOCTEST_GCC_SUPPRESS_WARNING_PUSH
+  DOCTEST_GCC_SUPPRESS_WARNING("-Wunused-result")
+  CHECK_THROWS_WITH(
+    (void)Util::expand_environment_variables("$surelydoesntexist"),
+    "environment variable \"surelydoesntexist\" not set");
+  CHECK_THROWS_WITH((void)Util::expand_environment_variables("${FOO"),
+                    "syntax error: missing '}' after \"FOO\"");
+  DOCTEST_GCC_SUPPRESS_WARNING_POP
+}
+
 TEST_CASE("Util::fallocate")
 {
   TestContext test_context;

diff --git a/unittest/test_legacy_util.cpp b/unittest/test_legacy_util.cpp
index c03d6d8d3ec087d49d3bbddc9c3ab016375baa30..0630cbfafbf4e43b68ab9da0f2293702a37ef293 100644 (file)
--- a/unittest/test_legacy_util.cpp
+++ b/unittest/test_legacy_util.cpp
@@ -28,36 +28,6 @@
 
 TEST_SUITE_BEGIN("legacy_util");
 
-TEST_CASE("subst_env_in_string")
-{
-  char* errmsg;
-
-  x_setenv("FOO", "bar");
-
-  CHECK_STR_EQ_FREE2("bar", subst_env_in_string("$FOO", &errmsg));
-  CHECK(!errmsg);
-
-  CHECK_STR_EQ_FREE2("$", subst_env_in_string("$", &errmsg));
-  CHECK(!errmsg);
-
-  CHECK_STR_EQ_FREE2("bar bar:bar",
-                     subst_env_in_string("$FOO $FOO:$FOO", &errmsg));
-  CHECK(!errmsg);
-
-  CHECK_STR_EQ_FREE2("xbar", subst_env_in_string("x$FOO", &errmsg));
-  CHECK(!errmsg);
-
-  CHECK_STR_EQ_FREE2("barx", subst_env_in_string("${FOO}x", &errmsg));
-  CHECK(!errmsg);
-
-  CHECK(!subst_env_in_string("$surelydoesntexist", &errmsg));
-  CHECK_STR_EQ_FREE2("environment variable \"surelydoesntexist\" not set",
-                     errmsg);
-
-  CHECK(!subst_env_in_string("${FOO", &errmsg));
-  CHECK_STR_EQ_FREE2("syntax error: missing '}' after \"FOO\"", errmsg);
-}
-
 TEST_CASE("parse_size_with_suffix")
 {
   uint64_t size;