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

bergmeister · 2017-10-07T12:01:30Z

PR Summary

Closes #2188
This makes it possible to go back to the last directory using 'cd -', which is exactly the same syntax as on Linux shells.
In contrast to the original proposal, it does not set the environment variable OLDPWD but instead uses a bounded stack to store the history in a similar way how Push/Pop-Location do.
A localised error message is given if there is no location history.

PR Checklist

This makes it possible to go back to the last directory using 'cd -', which is exactly the same syntax as on Linux shells.

PetSerAl · 2017-10-08T19:58:58Z

Should it be some per-Runspace storage rather than per-Process environment variable? For example: PowerShell ISE allow to open multiple PowerShell tabs, which all share same process. Should it be one shared last location for all tabs or each tab should have its own last location?

bergmeister · 2017-10-08T20:32:15Z

@PetSerAl Interesting thought and I did not know that the ISE tabs share the same process. +1 for sharing your knowledge. :)
The reason why it is an environment variable and why its name is OLDPWD is because I want to mimic the exact same behaviour as the native cd - function does on Unix systems. As far as I understand, PowerShell uses the native cd command on Linux/Mac but on Windows uses an alias for Set-Location. I am not sure if it is worth the effort of having a Windows specific implementation given the odd/rare use case that someone is cd-ing in different ISE tabs and wants to use cd - in a tab other than the current. Also note that the ISE is going to be replaced by VisualStudio Code at some point as far as I have heard. Is there some way of defining the environment variable only on a per RunSpace basis What do other people think?

PetSerAl · 2017-10-08T21:33:38Z

As far as I understand, cd can not be native application even for Unix-like shells. That means if you start new process cd it change its own working directory and exit, then working directory of parent process (shell) will be unaffected.

Also PowerShell ISE is just an example of PowerShell host, which use more than one Runspace per Process. PowerShell Remote host can be configured to use shared process as well. Enter-PSHostProcess create addition Runspace in target process. There are modules, which achieve multithreading by using Runspaces.

Simply: there are no rule "only one Runspace per Process", thus any use of shared resource, such as environment variable, should be carefully considered.

bergmeister · 2017-10-08T22:02:32Z

OK. I see what you mean. My assumption was that cd - is only being used interactively and in the same shell. If people were to start using cd - in multi-threaded modules then one could argue that this is bad practice and explicit paths should be specified instead.
Would it be a possible workaround to use both environment and 'normal' (automatic) variables when setting the value and when getting the value checking only the 'normal' variable? The idea behind still setting the environment variable is consistency on Unix with mixed usage of cd and Set-Location.
Let's see what the PowerShell team thinks about this issue.

PetSerAl · 2017-10-08T22:08:16Z

My assumption was that cd - is only being used interactively and in the same shell.

Even when cd - will be used in only one Runspace, other Runspaces can do normal cd something, which affect subsequent cd - in that only one Runspace.

rkeithhill · 2017-10-08T22:36:47Z

I agree with @PetSerAl. See PoshRSJobs for an example of how users can run multiple runspaces in the same process. Rather than make this an environment variable just make it a session (runspace) variable e.g. PSOldLocation or something like that.

iSazonov · 2017-10-09T09:18:31Z

I agree with @PetSerAl too - we should take into account many runspaces.

Main question - Do we really need this in scripts?

Set-Location -
cd -

looks very poorly read. Using Push-Location Pop-Location pair looks more readable and understandable.
I think this question is similar to the question of using aliases in an interactive session and banning its in scripts. I'd rather disable cd - in scripts immediately.

Next question - Should we keep a history and implement some step back by "cd-; cd-; cd -"?

bergmeister · 2017-10-09T12:35:52Z

Ok. If you say runspace variable, do you mean we should declare it as an automatic variable or is there a subtle difference? How would one ban its usage in scripts? About the history: the idea is to keep it simple at the moment and another PR could improve it to use a history stack (that is the point about agile development: make something minimal and viable with value that works and then iteratively enhance it)

rkeithhill · 2017-10-09T16:08:17Z

Should we keep a history and implement some step back by "cd-; cd-; cd -"?

As an example of this, see the PSCX package. It implements a cd stack that you can backwards navigate with cd - and forwards navigate with cd + e.g.:

[master ≡ +0 ~1 -0!] ~\GitHub\rkeithhill\PowerShellEditorServices
12:63ms> cd

     # Directory Stack:
   --- ----------------
     0 C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices
     1 C:\Users\Keith\GitHub\dahlbyk\posh-git
     2 C:\Users\Keith\GitHub\rkeithhill\PowerShell
->   3 C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices

[master ≡ +0 ~1 -0!] ~\GitHub\rkeithhill\PowerShellEditorServices
13:58ms> cd -
[master ↑8] ~\GitHub\rkeithhill\PowerShell
14:73ms> cd -0
[master ≡ +0 ~1 -0!] ~\GitHub\rkeithhill\PowerShellEditorServices
15:74ms> cd +
[rkeithhill/settings-prototype ≡] ~\GitHub\dahlbyk\posh-git

It also handles two cases that I particularly like:

# cd to the directory containing the file
16:47ms> cd $profile
~\Documents\WindowsPowerShell

and

# cd to dir with spaces in path without requiring quoting - something CMD supports
17:5ms> cd c:\program files
C:\program files

How would one ban its usage in scripts?

You don't. That isn't something I'd worry about. However there are other CD type packages out there. Some allow you to fuzzy match when CD'ing to directories.

iSazonov · 2017-10-09T18:16:41Z

I think we should easily implement the history based on "Stack" from the class.
IsInteractive can help to block the feature in scripts. We discussed IsInteractive in another context (shebang and profile loading). We definitely need it.
@lzybkr Could you please comment?

lzybkr · 2017-10-09T19:42:23Z

I'd prefer a stack as well, though it needs to be bounded. You could start with this code from PSReadline.

I don't think it's necessary to prevent the use of cd - in a script - I don't expect it would be used much, and it might occasionally be useful.

lzybkr · 2017-10-09T20:23:56Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs


+        private string ReplacePathWithLastLocationIfApplicable(string path)
+        {
+            Environment.SetEnvironmentVariable("DEBUG_PATH", Path);


I don't think we want this.

True. Well spotted. This was a development debugging leftover, sorry.

lzybkr · 2017-10-09T20:44:51Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

+                        Path = ReplacePathWithLastLocationIfApplicable(Path);
+                        var initialPath = SessionState.Path.CurrentLocation.Path;
                        result = SessionState.Path.SetLocation(Path, CmdletProviderContext);
+                        Environment.SetEnvironmentVariable(environmentVariableNameForPreviousLocation, initialPath);


We shouldn't use environment variables, instead we should expose a new variable (PSOldPWD?) that is an instance of a bounded stack.

We could consider a user configurable stack size, but we can leave that for later (not this PR).

I also wonder if this logic should be in SessionState.Path.SetLocation - that could be useful for alternate implementations of Set-Location that call the api.

lzybkr · 2017-10-09T20:45:29Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

+                        StackName = ReplacePathWithLastLocationIfApplicable(StackName);
+                        var initialPath = SessionState.Path.CurrentLocation.Path;
                        result = SessionState.Path.SetDefaultLocationStack(StackName);
+                        Environment.SetEnvironmentVariable(environmentVariableNameForPreviousLocation, initialPath);


The previous comments apply here, including moving the logic to SessionState.Path.SetDefaultLocationStack.

lzybkr · 2017-10-09T20:46:18Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

-                    {
+                    {   
+                        relationshipPath = ReplacePathWithLastLocationIfApplicable(relationshipPath);
+                        var initialPath = SessionState.Path.CurrentLocation.Path;


You can revert the changes under RELATIONSHIP_SUPPORTED - the code is dead, and I've submitted a PR to delete it.

Ok, will do.

lzybkr · 2017-10-09T20:47:18Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

-                        result = SessionState.Path.SetLocation (relationshipPath, CmdletProviderContext);
+                        relationshipPath = ReplacePathWithLastLocationIfApplicable(relationshipPath);
+                        var initialPath = SessionState.Path.CurrentLocation;
+                        result = SessionState.Path.SetLocation(relationshipPath, CmdletProviderContext);


The previous comments apply here, including moving the logic to SessionState.Path.SetLocation. In fact, I think if you did that, the change here wouldn't be necessary.

lzybkr · 2017-10-09T20:49:39Z

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

@@ -0,0 +1,123 @@
+<?xml version="1.0" encoding="utf-8"?>


I think it's reasonable to put the new string in SessionStateStrings instead of this new file.

…ment 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.

bergmeister · 2017-10-10T20:20:19Z

@lzybkr I have implemented your suggestions and used the existing stack implementation that pusdh/popd use under the hood to avoid code duplication.

iSazonov

I don't like side effects - performance decreases, memory consumption increases.

iSazonov · 2017-10-11T15:25:19Z

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

+            It 'Should go to last location when specifying minus as a path using alias' {
+                $initialLocation = Get-Location
+                Set-Location ([System.IO.Path]::GetTempPath())
+                cd -


Please remove the test - we shouldn't tests aliases.

iSazonov · 2017-10-11T15:32:19Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs

                        result = SessionState.Path.SetLocation(Path, CmdletProviderContext);
+#if UNIX
+                        // Be consistent with most Unix shells that set the environment variable 'OLDPWD' when changing location
+                        Environment.SetEnvironmentVariable("OLDPWD", Path);


I don't see the point - we don't use it anywhere.

Agreed - I also don't see the point. Maybe if ps1 files ran in a new process, then it might make sense, but they don't.

And if setting the environment variable was useful, we should explain why it is UNIX only.

Ok, I've removed it. I found out later that on Linux cd is also an alias for Set-Location (my assumption was that native Linux commands are not overridden by pwsh but this seems to be one of the exceptions)

lzybkr · 2017-10-26T23:47:47Z

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

+            {
+                return PopLocation(locationStackHistoryId);
+            }
+            PushCurrentLocation(locationStackHistoryId);


Nice - so simple. I do have a concern about an unbounded stack though. This stack will not be popped often, so I think there should be some limit.

OK, I've implemented now a bounded stack based on a linked list.

lzybkr · 2017-10-26T23:50:46Z

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

+                $tempLocation = Get-Location
+                Set-Location -
+                Set-Location -
+                (Get-Location).Path | Should Be ($tempLocation).Path


This test doesn't make sense to me - there is one push, but two pops. How can this pass?

I have re-written all tests but the reason why it passed was because Set-Location also pushed to the stack (this was a bug that is fixed now)

lzybkr · 2017-10-26T23:51:57Z

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

+                (Get-Location).Path | Should Be ($tempLocation).Path
+            }
+
+            It 'Should fail if there is no last location' {


I'm also surprised about this test - it seems like you need a way to clear the stack before running cd - to make sure it reliably raises the exception.

I rewrote this test since it does not make sense any more (it was based on the first environment variable based implementation)

Remove setting environment variable on Linux as discussed in PR.

…o SimpleLocationHistory

…he path string (e.g. an additional slash at the end) Make syntax to .net call of environment consistent with surrounding code

iSazonov

I wonder why not enhance the existing API?

bergmeister · 2017-10-30T22:29:39Z

@iSazonov If I understand correctly, changing the existing API to use a bounded stack would be a breaking change for pushd/popd. Enhancing the existing API should just be a matter of changing the type from Stack to the new BoundedStack. Or did you mean something else?

iSazonov · 2017-10-31T04:09:14Z

@bergmeister We should enhance the API not change. Actually you will use new constructor to set limits, existing code will keep current behavior without limits. This will allow us enhance our *-Location cmdlets and allow customers to use the new API possibilities.

bergmeister · 2018-05-26T20:35:58Z

@iSazonov There were test failures due it being the full test suite:

But the problem seems to be either sporadic or due to the test suite itself falling over:

…o SimpleLocationHistory

bergmeister · 2018-05-30T21:43:48Z

Those tests seemed to be sporadic and the new build contains 'only' 1 other sporadic failure

iSazonov · 2018-05-31T06:04:40Z

@bergmeister Fix is ready #6943.

bergmeister · 2018-06-07T17:57:08Z

@iSazonov Any updates on this so that it goes into the next preview for getting some testing exposure before 6.1 goes RTM after that?

iSazonov · 2018-06-08T05:38:31Z

@bergmeister I don't understand. Please reword your question.

bergmeister · 2018-06-08T06:33:42Z

@iSazonov. Nevermind, I just saw that the next preview is already in the making.
Are there any interests to merge this before 6.1 gets released as RTM?

iSazonov · 2018-06-08T07:17:04Z

@bergmeister We are waiting MSFT team sign.

rjmholt

This looks good to me. Thanks @bergmeister!

rjmholt · 2018-06-22T18:11:54Z

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

+            bool pushNextLocation = true;
+            if (originalPath.Equals("-", StringComparison.OrdinalIgnoreCase))
+            {
+                if (_workingLocationHistoryStack.Count > 0)


I would structure this if as:

if (_workingLocationHistoryStack.Count <= 0) { throw new InvalidOperationException(...); } // Golden path behaviour

There are some pretty deep if/else trees in the codebase and they get hard to reason about quickly.

I agree, this is neater, done.

rjmholt · 2018-06-22T18:17:25Z

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

+        {
+            this.AddFirst(item);
+
+            if(this.Count > _capacity)


Maybe include braces for the if and drop the this from property references. That seems to be the style convention of the file.

Yep, I agree. Fixed.

iSazonov · 2018-06-22T18:35:57Z

@daxian-dbw @adityapatwardhan Have you any thoughts before we merge?

…to SimpleLocationHistory

BrucePay

In general it looks good. Just a few things I think need to change.

BrucePay · 2018-06-22T19:21:53Z

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

            _workingLocationStack = new Dictionary<String, Stack<PathInfo>>(StringComparer.OrdinalIgnoreCase);
+            // conservative choice to limit the memory impact in case of a regression
+            const int locationHistoryLimit = 20;
+            _workingLocationHistoryStack = new BoundedStack<PathInfo>(locationHistoryLimit);


This variable name is easily confused with _workingLocationStack and _workingLocationHistoryStack. I would suggest some more meaningful like _SetLocationHistory. (I don't think it needs it's type in it's name, and if we do decide to implement cd + it won't be a stack anymore.)

@bergmeister Please replace _SetLocationHistory -> _setLocationHistory - it is our name convention for static variables.

Also please review CodeFactor issues - maybe you would fix some ones (in your code or near).

BrucePay · 2018-06-22T19:26:43Z

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

            // is used for the pushd and popd commands

            _workingLocationStack = new Dictionary<String, Stack<PathInfo>>(StringComparer.OrdinalIgnoreCase);
+            // conservative choice to limit the memory impact in case of a regression


I would put a space before the comment. I'd also add a comment saying that this is for the Set-Location history to distinguish it from the earlier "pushd and popd" comment.

BrucePay · 2018-06-22T19:43:48Z

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

+        internal T Pop()
+        {
+            var item = this.First.Value;
+            this.RemoveFirst();


Should have a specific error message here instead of letting the base class throw it's own message if the linked list is empty. If we change the implementation to be a circular buffer, then the exception change too.

… when attempting to pop empty bounded stack. Error message was chosen to be similar to the one by LinkedList

bergmeister · 2018-06-22T20:23:33Z

@rjmholt @BrucePay I addressed all your comments

iSazonov · 2018-06-23T05:01:00Z

It seems we set a absolute record in PR review duration 😕

iSazonov · 2018-06-26T07:31:17Z

@bergmeister Thanks for your contribution!

…io (issue #2188) (#5051)" This reverts commit bbd48f6.

iSazonov · 2018-06-29T15:03:37Z

@daxian-dbw Please clarify what is the "Revert..." commit?

bergmeister · 2018-06-29T15:19:16Z

I saw this as well and cannot track it back, the PR is still in master (which is good), so I assume this commit either happened on a branch or the revert got reverted afterwards

rjmholt · 2018-06-29T16:57:58Z

I think when you use GitHub's UI to revert a PR, it generates a new commit that reverts all the commits since that PR (which is not what we want -- reverting the single commit in the commandline is the better way). That commit is generated on another branch and not merged in, but still references all the other commits and shows up everywhere, which manages to confuse everyone.

II'm guessing @daxian-dbw initially tried to use the revert button, and has since overridden it in the commandline. So this merge hasn't been reverted, but a revert commit for it was generated and never used.

I know because I clicked the "revert" button once and was answering questions about it for a week.

daxian-dbw · 2018-06-29T18:47:00Z

The confusing revert commit comes from https://github.com/PowerShell/PowerShell/pull/7182/commits.
Github automatically generates some empty commits when I revert #7151

Changes from this PR is intact, so don't worry :)

Implement location history feature for Set-Location.

030c93d

This makes it possible to go back to the last directory using 'cd -', which is exactly the same syntax as on Linux shells.

bergmeister requested review from adityapatwardhan and daxian-dbw as code owners October 7, 2017 12:01

iSazonov assigned lzybkr Oct 9, 2017

lzybkr suggested changes Oct 9, 2017

View reviewed changes

bergmeister requested review from BrucePay and vors as code owners October 10, 2017 19:50

iSazonov reviewed Oct 11, 2017

View reviewed changes

lzybkr reviewed Oct 26, 2017

View reviewed changes

bergmeister added 3 commits October 29, 2017 23:29

Use a bounded stack with a limit of 1000 and adapt tests.

61d0c2f

Remove setting environment variable on Linux as discussed in PR.

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

bdb2b31

…o SimpleLocationHistory

Fix test: use get-location explicitly to avoid subtle difference in t…

552c3ab

…he path string (e.g. an additional slash at the end) Make syntax to .net call of environment consistent with surrounding code

iSazonov reviewed Oct 30, 2017

View reviewed changes

bergmeister added 2 commits May 30, 2018 20:59

Merge branch 'master' of https://github.com/PowerShell/PowerShell int…

7dc6c80

…o SimpleLocationHistory

[feature]

45d75af

rjmholt approved these changes Jun 22, 2018

View reviewed changes

Merge branch 'master' of https://github.com/bergmeister/PowerShell in…

96b6334

…to SimpleLocationHistory

BrucePay suggested changes Jun 22, 2018

View reviewed changes

bergmeister added 3 commits June 22, 2018 20:54

Address PR feedback by @rjmholt and upstream branch

96af36f

Address naming and comment recommendations by @BrucePay

604cd58

Address last comment by @BrucePay to give more specific error message…

8b06d37

… when attempting to pop empty bounded stack. Error message was chosen to be similar to the one by LinkedList

BrucePay approved these changes Jun 22, 2018

View reviewed changes

iSazonov merged commit bbd48f6 into PowerShell:master Jun 26, 2018

daxian-dbw added a commit that referenced this pull request Jun 26, 2018

Revert "Add location history for Set-Location to enable 'cd -' scenar…

f7d3ba7

…io (issue #2188) (#5051)" This reverts commit bbd48f6.

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add location history for Set-Location to enable 'cd -' scenario (issue #2188)' MicrosoftDocs/PowerShell-Docs#3078

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

bergmeister mentioned this pull request Nov 21, 2019

cd - (switch directories) doesn't work as expected. #11145

Closed

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

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

Uh oh!

Conversation

bergmeister commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

PetSerAl commented Oct 8, 2017

Uh oh!

bergmeister commented Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetSerAl commented Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergmeister commented Oct 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetSerAl commented Oct 8, 2017

Uh oh!

rkeithhill commented Oct 8, 2017

Uh oh!

iSazonov commented Oct 9, 2017

Uh oh!

bergmeister commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkeithhill commented Oct 9, 2017

Uh oh!

iSazonov commented Oct 9, 2017

Uh oh!

lzybkr commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Oct 10, 2017

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Oct 7, 2017 •

edited

Loading

bergmeister commented Oct 8, 2017 •

edited

Loading

PetSerAl commented Oct 8, 2017 •

edited

Loading

bergmeister commented Oct 8, 2017 •

edited

Loading

bergmeister commented Oct 9, 2017 •

edited

Loading

iSazonov commented May 31, 2018 •

edited

Loading

bergmeister commented Jun 7, 2018 •

edited

Loading

rjmholt Jun 22, 2018 •

edited

Loading

bergmeister Jun 22, 2018 •

edited

Loading