Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from Jun 26, 2018

Conversation

@bergmeister
Copy link
Contributor

commented Oct 7, 2017

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

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.
@PetSerAl

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

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.

@@ -1054,6 +1071,29 @@ protected override void ProcessRecord()
}
} // ProcessRecord

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 9, 2017

Member

I don't think we want this.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 9, 2017

Author Contributor

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

result = SessionState.Path.SetLocation(Path, CmdletProviderContext);
Environment.SetEnvironmentVariable(environmentVariableNameForPreviousLocation, initialPath);

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 9, 2017

Member

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.

result = SessionState.Path.SetDefaultLocationStack(StackName);
Environment.SetEnvironmentVariable(environmentVariableNameForPreviousLocation, initialPath);

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 9, 2017

Member

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

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 9, 2017

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 9, 2017

Author Contributor

Ok, will do.

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 9, 2017

Member

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.

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 9, 2017

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 9, 2017

Author Contributor

Ok.

Use an existing stack implementation instead of relying on an environ…
…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 bergmeister requested review from BrucePay and vors as code owners Oct 10, 2017

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

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

@iSazonov
Copy link
Collaborator

left a comment

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

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 11, 2017

Collaborator

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 30, 2017

Author Contributor

ok

@@ -916,6 +916,10 @@ protected override void ProcessRecord()
}

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);

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 11, 2017

Collaborator

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 26, 2017

Member

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.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 30, 2017

Author Contributor

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)

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 26, 2017

Member

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.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 30, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 26, 2017

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 30, 2017

Author Contributor

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)

(Get-Location).Path | Should Be ($tempLocation).Path
}

It 'Should fail if there is no last location' {

This comment has been minimized.

Copy link
@lzybkr

lzybkr Oct 26, 2017

Member

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.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Oct 30, 2017

Author Contributor

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

bergmeister added some commits Oct 29, 2017

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 t…
…he path string (e.g. an additional slash at the end)

Make syntax to .net call of environment consistent with surrounding code
@iSazonov
Copy link
Collaborator

left a comment

I wonder why not enhance the existing API?

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

@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

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2018

@iSazonov There were test failures due it being the full test suite:
image
But the problem seems to be either sporadic or due to the test suite itself falling over:
image

bergmeister added some commits May 30, 2018

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2018

@bergmeister Fix is ready #6943.

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2018

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

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2018

@bergmeister We are waiting MSFT team sign.

@rjmholt
Copy link
Member

left a comment

This looks good to me. Thanks @bergmeister!

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

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jun 22, 2018

Member

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.

This comment has been minimized.

Copy link
@bergmeister

bergmeister Jun 22, 2018

Author Contributor

I agree, this is neater, done.

{
this.AddFirst(item);

if(this.Count > _capacity)

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jun 22, 2018

Member

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

This comment has been minimized.

Copy link
@bergmeister

bergmeister Jun 22, 2018

Author Contributor

Yep, I agree. Fixed.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2018

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

@BrucePay
Copy link
Collaborator

left a comment

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

@@ -65,6 +66,9 @@ internal SessionStateInternal(SessionStateInternal parent, bool linkToGlobal, Ex
// 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
const int locationHistoryLimit = 20;
_workingLocationHistoryStack = new BoundedStack<PathInfo>(locationHistoryLimit);

This comment has been minimized.

Copy link
@BrucePay

BrucePay Jun 22, 2018

Collaborator

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.)

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jun 23, 2018

Collaborator

@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).

@@ -65,6 +66,9 @@ internal SessionStateInternal(SessionStateInternal parent, bool linkToGlobal, Ex
// 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

This comment has been minimized.

Copy link
@BrucePay

BrucePay Jun 22, 2018

Collaborator

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.

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

This comment has been minimized.

Copy link
@BrucePay

BrucePay Jun 22, 2018

Collaborator

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.

bergmeister added some commits Jun 22, 2018

Address last comment by @BrucePay to give more specific error message…
… when attempting to pop empty bounded stack. Error message was chosen to be similar to the one by LinkedList
@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

@rjmholt @BrucePay I addressed all your comments

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jun 23, 2018

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

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

4 of 5 checks passed

CodeFactor 12 issues fixed. 479 issues found.
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2018

@bergmeister Thanks for your contribution!

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2018

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

@bergmeister

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

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

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.