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

Fix for Set-Location failing to restore current working directory (#6749) #6774

Merged
merged 9 commits into from May 17, 2018

Conversation

@mcbobke
Copy link
Contributor

mcbobke commented Apr 30, 2018

PR Summary

#5228 was originally fixed by #6600, however this caused a regression as noted in #6749. Using Set-Location to change to a different drive such as Set-Location D: will fail to restore the current working directory of that drive as a result of the changes in #6600. This PR contains the changes from #6600 as well as a new check in SessionStateLocationAPIs.cs that determines if the path given to Set-Location is simply a drive name terminated by a colon. If so, it changes the desired path to be the new drive's current working directory.

This fixes #6749 and fixes #5228.

PR Checklist

mcbobke and others added 3 commits Apr 9, 2018
[Feature] Checks to see if the root of a PSDrive was given as the Path argument to a parameter that supports it. If so, returns the current context's Drive's Root.
If given path is a colon-terminated drive
@mcbobke

This comment has been minimized.

Copy link
Contributor Author

mcbobke commented Apr 30, 2018

Tests still need to be written, creating PR for tracking.

@mcbobke mcbobke force-pushed the mcbobke:workon5228and6749 branch 2 times, most recently Apr 30, 2018
@mcbobke mcbobke force-pushed the mcbobke:workon5228and6749 branch 2 times, most recently Apr 30, 2018
@daxian-dbw daxian-dbw requested a review from rjmholt Apr 30, 2018
@mcbobke mcbobke changed the title WIP: Fix for Set-Location failing to restore current working directory (#6749) Fix for Set-Location failing to restore current working directory (#6749) May 1, 2018
@mcbobke mcbobke force-pushed the mcbobke:workon5228and6749 branch to 594eacf May 1, 2018
It "Should set location to new drive's current working directory when path is the colon-terminated name of a different drive" {
try
{
Set-Location 'TestDrive:\'

This comment has been minimized.

Copy link
@iSazonov

iSazonov May 3, 2018

Collaborator

I believe we should save current location before set new one and restore in finally.

This comment has been minimized.

Copy link
@mcbobke

mcbobke May 3, 2018

Author Contributor

Should I add that before the try block, or inside it?

This comment has been minimized.

Copy link
@iSazonov

iSazonov May 3, 2018

Collaborator

I meant:

try {
    $oldLocation - Get-Location
    Set-Location 'TestDrive:\'
    ...
} finally {
    Set-Location $oldLocation
}

This comment has been minimized.

Copy link
@mcbobke

mcbobke May 3, 2018

Author Contributor

Thanks, will add that now.

@rjmholt

This comment has been minimized.

Copy link
Member

rjmholt commented May 11, 2018

I just cloned and built this branch, and I think I'm still getting a repro of #5228.

Scratch that, I did something wrong. Rebuilt and it works!

Copy link
Member

rjmholt left a comment

Looks good to me 😄

// If the path is simply a colon-terminated drive,
// not a slash-terminated path to the root of a drive,
// set the path to the current working directory of that drive.

This comment has been minimized.

Copy link
@rjmholt

rjmholt May 11, 2018

Member

A small style point -- I'd remove the newline here so the comment doesn't float.

Having looked at some of the older parts of this codebase, my experience is that comments that aren't glued to code have a tendency to float off into limbo (so that a couple of years down the track, an old comment will sit above a wildly different block of code with nobody certain about what it's referring to).

This comment has been minimized.

Copy link
@mcbobke

mcbobke May 12, 2018

Author Contributor

Thanks for the update! I'll adjust this later tonight.

This comment has been minimized.

Copy link
@mcbobke

mcbobke May 12, 2018

Author Contributor

Newlines have been removed from the floating comments.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented May 12, 2018

@SteveL-MSFT The PR is a good place to understand whether we will address CodeFactor issues and how we will do it.
The factor does not evaluate changes but only the entire files. Current policy don't allow PR unrelated style changes.

string colonTerminatedVolume = CurrentDrive.Name + ':';
if (CurrentDrive.VolumeSeparatedByColon && (path.Length == colonTerminatedVolume.Length))
{
path = Path.Combine((colonTerminatedVolume + "\\"), CurrentDrive.CurrentLocation);

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw May 14, 2018

Member

colonTerminatedVolume + "\"

Please use Path.DirectorySeparatorChar instead of "\\".

This comment has been minimized.

Copy link
@mcbobke

mcbobke May 16, 2018

Author Contributor

This has been changed.

@daxian-dbw

This comment has been minimized.

Copy link
Member

daxian-dbw commented May 14, 2018

I think a PR doesn't need to address CodeFactor issues that are not introduced by the PR itself.
We need to review the CodeFactor rules, not all of them make sense to keep, then we can see if it's possible to bulk update the code base.

Copy link
Contributor Author

mcbobke left a comment

Path.DirectorySeparatorChar is now being used.

string colonTerminatedVolume = CurrentDrive.Name + ':';
if (CurrentDrive.VolumeSeparatedByColon && (path.Length == colonTerminatedVolume.Length))
{
path = Path.Combine((colonTerminatedVolume + "\\"), CurrentDrive.CurrentLocation);

This comment has been minimized.

Copy link
@mcbobke

mcbobke May 16, 2018

Author Contributor

This has been changed.

@daxian-dbw daxian-dbw merged commit 4e3db1b into PowerShell:master May 17, 2018
4 of 5 checks passed
4 of 5 checks passed
CodeFactor 6 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

iSazonov commented May 17, 2018

@mcbobke Thanks for your contribution!

@mcbobke

This comment has been minimized.

Copy link
Contributor Author

mcbobke commented May 17, 2018

Thank you all!

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