fixup! src: refactor --trace-env to reuse option selection and handling

nodejs · Jan 7, 2025 · c667dad · c667dad
1 parent 3c65551
commit c667dad
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 32 deletions.
diff --git a/src/node_credentials.cc b/src/node_credentials.cc
@@ -99,7 +99,7 @@ bool SafeGetenv(const char* key, std::string* text, Environment* env) {
     *text = value.value();
   }
 
-  TraceEnv(env, "get", key);
+  TraceEnvVar(env, "get", key);
 
   return has_env;
 }

diff --git a/src/node_env_var.cc b/src/node_env_var.cc
@@ -337,17 +337,17 @@ Maybe<void> KVStore::AssignToObject(v8::Isolate* isolate,
   return JustVoid();
 }
 
-struct TraceEnvOptions {
+struct TraceEnvVarOptions {
   bool print_message : 1 = 0;
   bool print_js_stack : 1 = 0;
   bool print_native_stack : 1 = 0;
 };
 
 template <typename... Args>
-inline void TraceEnvImpl(Environment* env,
-                         TraceEnvOptions options,
-                         const char* format,
-                         Args&&... args) {
+inline void TraceEnvVarImpl(Environment* env,
+                            TraceEnvVarOptions options,
+                            const char* format,
+                            Args&&... args) {
   if (options.print_message) {
     fprintf(stderr, format, std::forward<Args>(args)...);
   }
@@ -359,8 +359,8 @@ inline void TraceEnvImpl(Environment* env,
   }
 }
 
-TraceEnvOptions GetTraceEnvOptions(Environment* env) {
-  TraceEnvOptions options;
+TraceEnvVarOptions GetTraceEnvVarOptions(Environment* env) {
+  TraceEnvVarOptions options;
   auto cli_options = env != nullptr
                          ? env->options()
                          : per_process::cli_options->per_isolate->per_env;
@@ -376,27 +376,31 @@ TraceEnvOptions GetTraceEnvOptions(Environment* env) {
   return options;
 }
 
-void TraceEnv(Environment* env, const char* message) {
-  TraceEnvImpl(env, GetTraceEnvOptions(env), "[--trace-env] %s\n", message);
+void TraceEnvVar(Environment* env, const char* message) {
+  TraceEnvVarImpl(
+      env, GetTraceEnvVarOptions(env), "[--trace-env] %s\n", message);
 }
 
-void TraceEnv(Environment* env, const char* message, const char* key) {
-  TraceEnvImpl(
-      env, GetTraceEnvOptions(env), "[--trace-env] %s \"%s\"\n", message, key);
+void TraceEnvVar(Environment* env, const char* message, const char* key) {
+  TraceEnvVarImpl(env,
+                  GetTraceEnvVarOptions(env),
+                  "[--trace-env] %s \"%s\"\n",
+                  message,
+                  key);
 }
 
-void TraceEnv(Environment* env,
-              const char* message,
-              v8::Local<v8::String> key) {
-  TraceEnvOptions options = GetTraceEnvOptions(env);
+void TraceEnvVar(Environment* env,
+                 const char* message,
+                 v8::Local<v8::String> key) {
+  TraceEnvVarOptions options = GetTraceEnvVarOptions(env);
   if (options.print_message) {
     Utf8Value key_utf8(env->isolate(), key);
-    TraceEnvImpl(env,
-                 options,
-                 "[--trace-env] %s \"%.*s\"\n",
-                 message,
-                 static_cast<int>(key_utf8.length()),
-                 key_utf8.out());
+    TraceEnvVarImpl(env,
+                    options,
+                    "[--trace-env] %s \"%.*s\"\n",
+                    message,
+                    static_cast<int>(key_utf8.length()),
+                    key_utf8.out());
   }
 }
 
@@ -413,7 +417,7 @@ static Intercepted EnvGetter(Local<Name> property,
       env->env_vars()->Get(env->isolate(), property.As<String>());
 
   bool has_env = !value_string.IsEmpty();
-  TraceEnv(env, "get", property.As<String>());
+  TraceEnvVar(env, "get", property.As<String>());
 
   if (has_env) {
     info.GetReturnValue().Set(value_string.ToLocalChecked());
@@ -453,7 +457,7 @@ static Intercepted EnvSetter(Local<Name> property,
   }
 
   env->env_vars()->Set(env->isolate(), key, value_string);
-  TraceEnv(env, "set", key);
+  TraceEnvVar(env, "set", key);
 
   return Intercepted::kYes;
 }
@@ -465,7 +469,7 @@ static Intercepted EnvQuery(Local<Name> property,
   if (property->IsString()) {
     int32_t rc = env->env_vars()->Query(env->isolate(), property.As<String>());
     bool has_env = (rc != -1);
-    TraceEnv(env, "query", property.As<String>());
+    TraceEnvVar(env, "query", property.As<String>());
     if (has_env) {
       // Return attributes for the property.
       info.GetReturnValue().Set(v8::None);
@@ -482,7 +486,7 @@ static Intercepted EnvDeleter(Local<Name> property,
   if (property->IsString()) {
     env->env_vars()->Delete(env->isolate(), property.As<String>());
 
-    TraceEnv(env, "delete", property.As<String>());
+    TraceEnvVar(env, "delete", property.As<String>());
   }
 
   // process.env never has non-configurable properties, so always
@@ -495,7 +499,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
   Environment* env = Environment::GetCurrent(info);
   CHECK(env->has_run_bootstrapping_code());
 
-  TraceEnv(env, "enumerate environment variables");
+  TraceEnvVar(env, "enumerate environment variables");
 
   info.GetReturnValue().Set(
       env->env_vars()->Enumerate(env->isolate()));

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -324,9 +324,11 @@ namespace credentials {
 bool SafeGetenv(const char* key, std::string* text, Environment* env = nullptr);
 }  // namespace credentials
 
-void TraceEnv(Environment* env, const char* message);
-void TraceEnv(Environment* env, const char* message, const char* key);
-void TraceEnv(Environment* env, const char* message, v8::Local<v8::String> key);
+void TraceEnvVar(Environment* env, const char* message);
+void TraceEnvVar(Environment* env, const char* message, const char* key);
+void TraceEnvVar(Environment* env,
+                 const char* message,
+                 v8::Local<v8::String> key);
 
 void DefineZlibConstants(v8::Local<v8::Object> target);
 v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,

diff --git a/test/parallel/test-trace-env.js b/test/parallel/test-trace-env.js
@@ -9,7 +9,7 @@ const fixtures = require('../common/fixtures');
 // Check reads from the Node.js internals.
 spawnSyncAndAssert(process.execPath, ['--trace-env', fixtures.path('empty.js')], {
   stderr(output) {
-    // This is a non-exhaustive list of thes checked
+    // This is a non-exhaustive list of the environment variables checked
     // during startup of an empty script at the time this test was written.
     // If the internals remove any one of them, the checks here can be updated
     // accordingly.