env: avoid virtual member call in ctor; all refresh

We had a virtual member call in the constructor of the various
IEnvironment implementations, which is a code smell.

Instead, lazily populate the `_variables` cache dictionary, and add an
explict `Refresh` method to refresh the cache. We call `Refresh` in the
`SetEnvironmentVariable` method for convenience.

For testing we simply pipe through the pre-computed variables in the
`internal` constructor.

Author: mjcheetham

src/shared/Core/EnvironmentBase.cs

@@ -53,18 +53,45 @@ public interface IEnvironment
         /// <param name="target">Target level of environment variable to set (Machine, Process, or User).</param>
         void SetEnvironmentVariable(string variable, string value,
             EnvironmentVariableTarget target = EnvironmentVariableTarget.Process);
+
+        /// <summary>
+        /// Refresh the current process environment variables. See <see cref="Variables"/>.
+        /// </summary>
+        /// <remarks>This is automatically called after <see cref="SetEnvironmentVariable"/>.</remarks>
+        void Refresh();
     }
 
     public abstract class EnvironmentBase : IEnvironment
     {
+        private IReadOnlyDictionary<string, string> _variables;
+
         protected EnvironmentBase(IFileSystem fileSystem)
         {
             EnsureArgument.NotNull(fileSystem, nameof(fileSystem));
-
             FileSystem = fileSystem;
         }
 
-        public IReadOnlyDictionary<string, string> Variables { get; protected set; }
+        internal EnvironmentBase(IFileSystem fileSystem, IReadOnlyDictionary<string, string> variables)
+            : this(fileSystem)
+        {
+            EnsureArgument.NotNull(variables, nameof(variables));
+            _variables = variables;
+        }
+
+        public IReadOnlyDictionary<string, string> Variables
+        {
+            get
+            {
+                // Variables are lazily loaded
+                if (_variables is null)
+                {
+                    Refresh();
+                }
+
+                Debug.Assert(_variables != null);
+                return _variables;
+            }
+        }
 
         protected IFileSystem FileSystem { get; }
 
@@ -126,9 +153,22 @@ internal virtual bool TryLocateExecutable(string program, ICollection<string> pa
         public void SetEnvironmentVariable(string variable, string value,
             EnvironmentVariableTarget target = EnvironmentVariableTarget.Process)
         {
-            if (Variables.Keys.Contains(variable)) return;
+            // Don't bother setting the variable if it already has the same value
+            if (Variables.TryGetValue(variable, out var currentValue) &&
+                StringComparer.Ordinal.Equals(currentValue, value))
+            {
+                return;
+            }
+
             Environment.SetEnvironmentVariable(variable, value, target);
-            Variables = GetCurrentVariables();
+
+            // Immediately refresh the variables so that the new value is available to callers using IEnvironment
+            Refresh();
+        }
+
+        public void Refresh()
+        {
+            _variables = GetCurrentVariables();
         }
 
         protected abstract IReadOnlyDictionary<string, string> GetCurrentVariables();

src/shared/Core/Interop/MacOS/MacOSEnvironment.cs

@@ -15,11 +15,7 @@ public MacOSEnvironment(IFileSystem fileSystem)
             : base(fileSystem) { }
 
         internal MacOSEnvironment(IFileSystem fileSystem, IReadOnlyDictionary<string, string> variables)
-            : base(fileSystem)
-        {
-            EnsureArgument.NotNull(variables, nameof(variables));
-            Variables = variables;
-        }
+            : base(fileSystem, variables) { }
 
         public override bool TryLocateExecutable(string program, out string path)
         {
@@ -35,4 +31,4 @@ public override bool TryLocateExecutable(string program, out string path)
             return TryLocateExecutable(program, _pathsToIgnore, out path);
         }
     }
-}
+}

src/shared/Core/Interop/Posix/PosixEnvironment.cs

@@ -7,13 +7,10 @@ namespace GitCredentialManager.Interop.Posix
     public class PosixEnvironment : EnvironmentBase
     {
         public PosixEnvironment(IFileSystem fileSystem)
-            : this(fileSystem, null) { }
+            : base(fileSystem) { }
 
         internal PosixEnvironment(IFileSystem fileSystem, IReadOnlyDictionary<string, string> variables)
-            : base(fileSystem)
-        {
-            Variables = variables ?? GetCurrentVariables();
-        }
+            : base(fileSystem, variables) { }
 
         #region EnvironmentBase
 

src/shared/Core/Interop/Windows/WindowsEnvironment.cs

@@ -10,13 +10,10 @@ namespace GitCredentialManager.Interop.Windows
     public class WindowsEnvironment : EnvironmentBase
     {
         public WindowsEnvironment(IFileSystem fileSystem)
-            : this(fileSystem, null) { }
+            : base(fileSystem) { }
 
         internal WindowsEnvironment(IFileSystem fileSystem, IReadOnlyDictionary<string, string> variables)
-            : base(fileSystem)
-        {
-            Variables = variables ?? GetCurrentVariables();
-        }
+            : base(fileSystem, variables) { }
 
         #region EnvironmentBase
 
@@ -48,7 +45,7 @@ public override void AddDirectoryToPath(string directoryPath, EnvironmentVariabl
             Environment.SetEnvironmentVariable("PATH", newValue, target);
 
             // Update the cached PATH variable to the latest value (as well as all other variables)
-            Variables = GetCurrentVariables();
+            Refresh();
         }
 
         public override void RemoveDirectoryFromPath(string directoryPath, EnvironmentVariableTarget target)
@@ -66,7 +63,7 @@ public override void RemoveDirectoryFromPath(string directoryPath, EnvironmentVa
                 Environment.SetEnvironmentVariable("PATH", newValue, target);
 
                 // Update the cached PATH variable to the latest value (as well as all other variables)
-                Variables = GetCurrentVariables();
+                Refresh();
             }
         }
 

src/shared/TestInfrastructure/Objects/TestEnvironment.cs

@@ -1,7 +1,6 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
-using System.Diagnostics;
 using System.Linq;
 
 namespace GitCredentialManager.Tests.Objects
@@ -106,6 +105,11 @@ public void SetEnvironmentVariable(string variable, string value,
             Variables.Add(variable, value);
         }
 
+        public void Refresh()
+        {
+            // Nothing to do!
+        }
+
         #endregion
     }
 }