Add location history for Set-Location to enable 'cd -' scenario (issue #2188) (#5051)

* Implement location history feature for Set-Location.
This makes it possible to go back to the last directory using 'cd -',
which is exactly the same syntax as on Linux shells.

* Use an existing stack implementation instead of relying on an environment variable and move logic for 'cd -' into the internal class of SessionState.Path.SetLocation
Keep the (additional) setting of an environment for Unix system in case of mixed usage of cd and Set-Location to provide consistency.

* Use a bounded stack with a limit of 1000 and adapt tests.
Remove setting environment variable on Linux as discussed in PR.

* Fix test: use get-location explicitly to avoid subtle difference in the path string (e.g. an additional slash at the end)
Make syntax to .net call of environment consistent with surrounding code

* Move initialization of _workingLocationHistoryStack into constructor of SessionStateInternal and set the limit to 20 as discussed in PR 5051.

* Make new BoundedStack class in engine utils internal

src/System.Management.Automation/engine/SessionState.cs

@@ -7,6 +7,7 @@ using System.Collections;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.Diagnostics;
+using System.Management.Automation.Internal;
 using System.Management.Automation.Runspaces;
 using Dbg = System.Management.Automation;
 using System.Diagnostics.CodeAnalysis;
@@ -66,6 +67,10 @@ namespace System.Management.Automation
 
             _workingLocationStack = new Dictionary<String, Stack<PathInfo>>(StringComparer.OrdinalIgnoreCase);
 
+            // Conservative choice to limit the Set-Location history in order to limit memory impact in case of a regression.
+            const int locationHistoryLimit = 20;
+            _SetLocationHistory = new BoundedStack<PathInfo>(locationHistoryLimit);
+
             GlobalScope = new SessionStateScope(null);
             ModuleScope = GlobalScope;
             _currentScope = GlobalScope;

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

@@ -6,6 +6,7 @@
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.IO;
+using System.Management.Automation.Internal;
 using System.Management.Automation.Provider;
 using Dbg = System.Management.Automation;
 
@@ -230,10 +231,28 @@ namespace System.Management.Automation
             ProviderInfo provider = null;
             string providerId = null;
 
+            // Replace path with last working directory when '-' was passed.
+            bool pushNextLocation = true;
+            if (originalPath.Equals("-", StringComparison.OrdinalIgnoreCase))
+            {
+                if (_SetLocationHistory.Count <= 0)
+                {
+                    throw new InvalidOperationException(SessionStateStrings.SetContentToLastLocationWhenHistoryIsEmpty);
+                }
+                var previousLocation =  _SetLocationHistory.Pop();
+                path = previousLocation.Path;
+                pushNextLocation = false;
+            }
+
+            if (pushNextLocation)
+            {
+                var newPushPathInfo = GetNewPushPathInfo();
+                _SetLocationHistory.Push(newPushPathInfo);
+            }
+
             PSDriveInfo previousWorkingDrive = CurrentDrive;
 
             // First check to see if the path is a home path
-
             if (LocationGlobber.IsHomePath(path))
             {
                 path = Globber.GetHomeRelativePath(path);
@@ -783,6 +802,11 @@ namespace System.Management.Automation
 
         #region push-Pop current working directory
 
+        /// <summary>
+        /// A bounded stack for the location history of Set-Location
+        /// </summary>
+        private BoundedStack<PathInfo> _SetLocationHistory;
+
         /// <summary>
         /// A stack of the most recently pushed locations
         /// </summary>
@@ -811,8 +835,23 @@ namespace System.Management.Automation
                 stackName = _defaultStackName;
             }
 
-            // Create a new instance of the directory/drive pair
+            // Get the location stack from the hashtable
+            Stack<PathInfo> locationStack = null;
 
+            if (!_workingLocationStack.TryGetValue(stackName, out locationStack))
+            {
+                locationStack = new Stack<PathInfo>();
+                _workingLocationStack[stackName] = locationStack;
+            }
+
+            // Push the directory/drive pair onto the stack
+            var newPushPathInfo = GetNewPushPathInfo();
+            locationStack.Push(newPushPathInfo);
+        }
+
+        private PathInfo GetNewPushPathInfo()
+        {
+             // Create a new instance of the directory/drive pair
             ProviderInfo provider = CurrentDrive.Provider;
             string mshQualifiedPath =
                 LocationGlobber.GetMshQualifiedPath(CurrentDrive.CurrentLocation, CurrentDrive);
@@ -829,19 +868,7 @@ namespace System.Management.Automation
                 CurrentDrive.Name,
                 mshQualifiedPath);
 
-            // Get the location stack from the hashtable
-
-            Stack<PathInfo> locationStack = null;
-
-            if (!_workingLocationStack.TryGetValue(stackName, out locationStack))
-            {
-                locationStack = new Stack<PathInfo>();
-                _workingLocationStack[stackName] = locationStack;
-            }
-
-            // Push the directory/drive pair onto the stack
-
-            locationStack.Push(newPushLocation);
+            return newPushLocation;
         }
 
         /// <summary>

src/System.Management.Automation/engine/Utils.cs

@@ -1459,4 +1459,53 @@ namespace System.Management.Automation.Internal
             }
         }
     }
+
+    /// <summary>
+    /// An bounded stack based on a linked list.
+    /// </summary>
+    internal class BoundedStack<T> : LinkedList<T>
+    {
+        private readonly int _capacity;
+
+        /// <summary>
+        /// Lazy initialisation, i.e. it sets only its limit but does not allocate the memory for the given capacity.
+        /// </summary>
+        /// <param name="capacity"></param>
+        internal BoundedStack(int capacity)
+        {
+            _capacity = capacity;
+        }
+
+        /// <summary>
+        /// Push item.
+        /// </summary>
+        /// <param name="item"></param>
+        internal void Push(T item)
+        {
+            this.AddFirst(item);
+
+            if(this.Count > _capacity)
+            {
+                this.RemoveLast();
+            }
+        }
+
+        /// <summary>
+        /// Pop item.
+        /// </summary>
+        /// <returns></returns>
+        internal T Pop()
+        {
+            var item = this.First.Value;
+            try
+            {
+                this.RemoveFirst();
+            }
+            catch (InvalidOperationException)
+            {
+                throw new InvalidOperationException(SessionStateStrings.BoundedStackIsEmpty);
+            }
+            return item;
+        }
+    }
 }

src/System.Management.Automation/resources/SessionStateStrings.resx

@@ -279,6 +279,12 @@
   <data name="GetContentWriterDynamicParametersProviderException" xml:space="preserve">
     <value>The dynamic parameters for the GetContentWriter operation cannot be retrieved for the '{0}' provider for path '{1}'. {2}</value>
   </data>
+  <data name="SetContentToLastLocationWhenHistoryIsEmpty" xml:space="preserve">
+    <value>There is no location history left to navigate backwards.</value>
+  </data>
+  <data name="BoundedStackIsEmpty" xml:space="preserve">
+    <value>The BoundedStack is empty.</value>
+  </data>
   <data name="ClearContentProviderException" xml:space="preserve">
     <value>Attempting to perform the ClearContent operation on the '{0}' provider failed for path '{1}'. {2}</value>
   </data>

test/powershell/Modules/Microsoft.PowerShell.Management/Set-Location.Tests.ps1

@@ -93,4 +93,38 @@ Describe "Set-Location" -Tags "CI" {
             Remove-PSDrive -Name 'Z'
         }
     }
+
+    Context 'Set-Location with last location history' {
+
+        It 'Should go to last location when specifying minus as a path' {
+            $initialLocation = Get-Location
+            Set-Location ([System.IO.Path]::GetTempPath())
+            Set-Location -
+            (Get-Location).Path | Should -Be ($initialLocation).Path
+        }
+
+        It 'Should go back to previous locations when specifying minus twice' {
+            $initialLocation = (Get-Location).Path
+            Set-Location ([System.IO.Path]::GetTempPath())
+            $firstLocationChange = (Get-Location).Path
+            Set-Location ([System.Environment]::GetFolderPath("user"))
+            Set-Location -
+            (Get-Location).Path | Should -Be $firstLocationChange
+            Set-Location -
+            (Get-Location).Path | Should -Be $initialLocation
+        }
+
+        It 'Location History is limited' {
+            $initialLocation = (Get-Location).Path
+            $maximumLocationHistory = 20
+            foreach ($i in 1..$maximumLocationHistory) {
+                Set-Location ([System.IO.Path]::GetTempPath())
+            }
+            foreach ($i in 1..$maximumLocationHistory) {
+                Set-Location -
+            }
+            (Get-Location).Path | Should Be $initialLocation
+            { Set-Location - } | Should -Throw -ErrorId 'System.InvalidOperationException,Microsoft.PowerShell.Commands.SetLocationCommand'
+        }
+    }
 }