Skip to content

Commit ed58449

Browse files
[minor] Add environment variables to CommandOptions
ProcessStartInfo.Environment was never touched, so a child always inherited the host's environment verbatim with no way to add, override or remove a variable for a single call. That blocks a class of CLI behaviour with no flag equivalent: GIT_TERMINAL_PROMPT=0, without which a fetch needing credentials blocks forever on a prompt no terminal will answer; GIT_ASKPASS and SSH_ASKPASS, the supported way to supply credentials without exposing them in a process listing; and LC_ALL=C, without which output parsing silently depends on the host locale. The entries are an overlay rather than a replacement, so a name that is not listed keeps whatever the caller had, and a null value removes a variable, matching ProcessStartInfo.Environment semantics. Leaving the property null preserves today's behaviour exactly. Elevation requires UseShellExecute, which has nowhere to put an environment, so combining the two throws rather than silently dropping variables a caller may be relying on for credentials. Fixes #40
1 parent c99e717 commit ed58449

4 files changed

Lines changed: 181 additions & 0 deletions

File tree

README.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,11 @@ class Program
163163
options: new()
164164
{
165165
WorkingDirectory = AbsoluteDirectoryPath.Create(@"C:\repos\my project"),
166+
EnvironmentVariables = new Dictionary<string, string?>
167+
{
168+
["GIT_TERMINAL_PROMPT"] = "0",
169+
["LC_ALL"] = "C",
170+
},
166171
});
167172

168173
Console.WriteLine($"Process exited with code: {exitCode}");
@@ -172,6 +177,19 @@ class Program
172177

173178
Without a `WorkingDirectory` the process inherits the current directory of the calling process, which is what commands did before this option existed.
174179

180+
`EnvironmentVariables` is an overlay on the inherited environment, not a replacement: a name you do not list keeps whatever the calling process had. A `null` value removes a variable, which is how you unset something the parent had set:
181+
182+
```csharp
183+
EnvironmentVariables = new Dictionary<string, string?>
184+
{
185+
["GIT_DIR"] = null,
186+
}
187+
```
188+
189+
Environment variables are the only control surface some tools expose, so this covers behaviour with no command-line equivalent — `GIT_TERMINAL_PROMPT=0` to make an authenticating `git fetch` fail rather than block forever on a prompt no terminal will answer, `GIT_ASKPASS`/`SSH_ASKPASS` to supply credentials without putting them on a command line where any process listing can read them, and `LC_ALL=C` to force stable, machine-parseable output rather than whatever the host locale produces.
190+
191+
> **_NOTE:_** _`EnvironmentVariables` cannot be combined with `Elevation.Elevated` on Windows. Elevation requires `UseShellExecute`, which offers nowhere to pass an environment, so the call throws `ArgumentException` rather than silently dropping the variables._
192+
175193
The type is `AbsoluteDirectoryPath` rather than a string on purpose. A relative directory would have to be resolved against the caller's current directorythe process-global state this option exists to avoid depending on, since it is shared by every thread and races with concurrent calls.
176194

177195
`CommandOptions.Elevation` carries the privilege level, so a single options object replaces the separate `Elevation` argument.
@@ -219,6 +237,7 @@ class Program
219237
### CommandOptions Record
220238

221239
- `WorkingDirectory`: An `AbsoluteDirectoryPath` naming the directory the process starts in, or `null` to inherit the caller's current directory.
240+
- `EnvironmentVariables`: An `IReadOnlyDictionary<string, string?>` applied over the inherited environment, or `null` to inherit it unchanged. A `null` value removes a variable.
222241
- `Elevation`: The privilege level under which to run the command. Defaults to `Elevation.Default`.
223242

224243
### Elevation Enum

RunCommand.Test/RunCommandTests.cs

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,35 @@ private static (string FileName, string[] Arguments) GetPrintWorkingDirectoryCom
323323
private static string CreateDirectoryForTest([CallerMemberName] string caller = "") =>
324324
Directory.CreateDirectory(Path.Join(Path.GetTempPath(), $"{nameof(RunCommandTests)} {caller}")).FullName;
325325

326+
/// <summary>
327+
/// Returns a command that prints the value of an environment variable, wrapped in brackets so
328+
/// that an empty value is still distinguishable from no output at all.
329+
/// </summary>
330+
private static (string FileName, string[] Arguments) GetPrintEnvironmentVariableCommand(string name) =>
331+
RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
332+
? ("cmd", ["/c", $"echo [%{name}%]"])
333+
: ("sh", ["-c", $"echo \"[${name}]\""]);
334+
335+
// Tests run in parallel and the host environment is process-wide, so each test needs its own
336+
// variable name to avoid stepping on another test's value.
337+
private static string EnvironmentVariableNameFor([CallerMemberName] string caller = "") =>
338+
$"RUNCOMMAND_TEST_{caller.ToUpperInvariant()}";
339+
340+
private static async Task<string> ReadEnvironmentVariableFromChildAsync(string name, CommandOptions options)
341+
{
342+
(string fileName, string[] arguments) = GetPrintEnvironmentVariableCommand(name);
343+
List<string> output = [];
344+
345+
int exitCode = await RunCommand.ExecuteAsync(
346+
fileName,
347+
arguments,
348+
new LineOutputHandler(onStandardOutput: output.Add),
349+
options).ConfigureAwait(false);
350+
351+
Assert.AreEqual(0, exitCode, "Expected the command to run successfully.");
352+
return string.Concat(output).Trim();
353+
}
354+
326355
/// <summary>
327356
/// Returns a command that runs for long enough to be cancelled mid-flight.
328357
/// </summary>
@@ -477,4 +506,98 @@ public async Task ExecuteAsyncShouldThrowArgumentNullExceptionWhenOptionsAreNull
477506
await Assert.ThrowsAsync<ArgumentNullException>(
478507
() => RunCommand.ExecuteAsync("dotnet", ["--version"], new OutputHandler(), null!)).ConfigureAwait(false);
479508
}
509+
[TestMethod]
510+
public async Task ExecuteAsyncShouldSetAnEnvironmentVariableForTheChildProcess()
511+
{
512+
string name = EnvironmentVariableNameFor();
513+
514+
string reported = await ReadEnvironmentVariableFromChildAsync(
515+
name,
516+
new CommandOptions { EnvironmentVariables = new Dictionary<string, string?> { [name] = "expected" } }).ConfigureAwait(false);
517+
518+
Assert.AreEqual("[expected]", reported, "Expected the child to see the variable that was set for it.");
519+
}
520+
521+
[TestMethod]
522+
public async Task ExecuteAsyncShouldOverrideAnInheritedEnvironmentVariable()
523+
{
524+
string name = EnvironmentVariableNameFor();
525+
Environment.SetEnvironmentVariable(name, "inherited");
526+
527+
try
528+
{
529+
string reported = await ReadEnvironmentVariableFromChildAsync(
530+
name,
531+
new CommandOptions { EnvironmentVariables = new Dictionary<string, string?> { [name] = "override" } }).ConfigureAwait(false);
532+
533+
Assert.AreEqual("[override]", reported, "Expected the overlay to win over the inherited value.");
534+
}
535+
finally
536+
{
537+
Environment.SetEnvironmentVariable(name, null);
538+
}
539+
}
540+
541+
[TestMethod]
542+
public async Task ExecuteAsyncShouldRemoveAnInheritedEnvironmentVariableWhenTheValueIsNull()
543+
{
544+
string name = EnvironmentVariableNameFor();
545+
Environment.SetEnvironmentVariable(name, "inherited");
546+
547+
try
548+
{
549+
string reported = await ReadEnvironmentVariableFromChildAsync(
550+
name,
551+
new CommandOptions { EnvironmentVariables = new Dictionary<string, string?> { [name] = null } }).ConfigureAwait(false);
552+
553+
// An unset variable prints differently per shell -- cmd echoes the name back verbatim,
554+
// sh prints nothing -- so this pins the part that matters on both: the inherited value
555+
// did not reach the child.
556+
Assert.DoesNotContain("inherited", reported, "Expected a null value to remove the inherited variable.");
557+
}
558+
finally
559+
{
560+
Environment.SetEnvironmentVariable(name, null);
561+
}
562+
}
563+
564+
[TestMethod]
565+
public async Task ExecuteAsyncShouldInheritTheEnvironmentWhenNoVariablesAreGiven()
566+
{
567+
string name = EnvironmentVariableNameFor();
568+
Environment.SetEnvironmentVariable(name, "inherited");
569+
570+
try
571+
{
572+
string reported = await ReadEnvironmentVariableFromChildAsync(name, new CommandOptions()).ConfigureAwait(false);
573+
574+
Assert.AreEqual("[inherited]", reported, "Expected an unset overlay to leave the previous behaviour untouched.");
575+
}
576+
finally
577+
{
578+
Environment.SetEnvironmentVariable(name, null);
579+
}
580+
}
581+
582+
[TestMethod]
583+
public async Task ExecuteAsyncShouldRejectEnvironmentVariablesCombinedWithElevation()
584+
{
585+
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
586+
{
587+
Assert.Inconclusive("Elevation only changes how the process is started on Windows.");
588+
}
589+
590+
// Elevation forces UseShellExecute, which cannot carry an environment. Failing loudly beats
591+
// silently dropping variables the caller may be relying on.
592+
await Assert.ThrowsAsync<ArgumentException>(
593+
() => RunCommand.ExecuteAsync(
594+
"cmd",
595+
["/c", "exit 0"],
596+
new OutputHandler(),
597+
new CommandOptions
598+
{
599+
Elevation = Elevation.Elevated,
600+
EnvironmentVariables = new Dictionary<string, string?> { ["ANY"] = "value" },
601+
})).ConfigureAwait(false);
602+
}
480603
}

RunCommand/CommandOptions.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace ktsu.RunCommand;
44

5+
using System.Collections.Generic;
56
using ktsu.Semantics.Paths;
67

78
/// <summary>
@@ -24,6 +25,18 @@ public sealed record CommandOptions
2425
/// </remarks>
2526
public AbsoluteDirectoryPath? WorkingDirectory { get; init; }
2627

28+
/// <summary>
29+
/// Gets the environment variables to apply over the inherited environment, or
30+
/// <see langword="null"/> to inherit the calling process's environment unchanged.
31+
/// </summary>
32+
/// <remarks>
33+
/// The entries are an overlay rather than a replacement: a name not listed here keeps whatever
34+
/// the calling process had. A <see langword="null"/> value removes a variable, matching the
35+
/// semantics of <see cref="System.Diagnostics.ProcessStartInfo.Environment"/>, which is how a
36+
/// caller unsets something the parent had set.
37+
/// </remarks>
38+
public IReadOnlyDictionary<string, string?>? EnvironmentVariables { get; init; }
39+
2740
/// <summary>
2841
/// Gets the privilege level under which to run the command.
2942
/// </summary>

RunCommand/RunCommand.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,17 @@ private static ProcessStartInfo CreateStartInfo(string fileName, OutputHandler o
298298
bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
299299
useElevation = options.Elevation == Elevation.Elevated && isWindows;
300300

301+
if (useElevation && options.EnvironmentVariables is not null)
302+
{
303+
// Elevation needs UseShellExecute, which starts the process through the shell and offers
304+
// nowhere to put an environment. Saying so here beats letting Process.Start fail with a
305+
// message that does not mention either setting, and beats silently dropping variables a
306+
// caller may be relying on for credentials or machine-parseable output.
307+
throw new ArgumentException(
308+
"Environment variables cannot be set for an elevated command, because elevation requires UseShellExecute.",
309+
nameof(options));
310+
}
311+
301312
ProcessStartInfo startInfo = new()
302313
{
303314
FileName = fileName,
@@ -322,6 +333,21 @@ private static ProcessStartInfo CreateStartInfo(string fileName, OutputHandler o
322333
startInfo.StandardErrorEncoding = outputHandler.Encoding;
323334
startInfo.UseShellExecute = false;
324335

336+
if (options.EnvironmentVariables is not null)
337+
{
338+
foreach (KeyValuePair<string, string?> variable in options.EnvironmentVariables)
339+
{
340+
if (variable.Value is null)
341+
{
342+
_ = startInfo.Environment.Remove(variable.Key);
343+
}
344+
else
345+
{
346+
startInfo.Environment[variable.Key] = variable.Value;
347+
}
348+
}
349+
}
350+
325351
if (isWindows)
326352
{
327353
startInfo.LoadUserProfile = true;

0 commit comments

Comments
 (0)