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 files/folders created at pwd by New-Item when given path is drive root (#5228) #6584

Closed
wants to merge 0 commits into from

Conversation

mcbobke
Copy link
Contributor

@mcbobke mcbobke commented Apr 6, 2018

PR Summary

If the Path argument to New-Item is the root of a drive such as C:\, the LocationGlobber attempts to generate a drive root relative path from the path C:. GetDriveRootRelativePathFromPSPath determines that this is an absolute path, and strips the drive name and colon from the path; originally, this would result in GenerateRelativePath determining that the current working directory should be given as the relative path. Now, a check is made on whether stripping the drive name and colon results in an empty string; if it does, the root of the current PSDrive context.Drive.Root is returned as the relative path.

This fixes #5228.

PR Checklist

@mcbobke mcbobke changed the title Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) WIP: Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) Apr 6, 2018
@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 6, 2018

Running the tests in question that are failing seem to be failing both on the current preview build (6.1.0-preview.1) and my build. I can't determine if this is a breaking change,

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 6, 2018

For now, I will leave this as ready to merge and non-breaking until review.

@mcbobke mcbobke changed the title WIP: Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) Apr 6, 2018
@mcbobke mcbobke changed the title Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) WIP: Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) Apr 6, 2018
@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 6, 2018

Confirmed that this broke Get-ChildItem when Path is not specified (by default, then attempts to use $pwd).

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 7, 2018

@mcbobke Feel free to ask if you need help.

@mcbobke mcbobke reopened this Apr 8, 2018
@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 8, 2018

I've been working on a different solution and am currently testing it before I commit. Thanks for the encouragement, I will definitely be asking some questions once it's up!

@mcbobke mcbobke changed the title WIP: Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) Fix for files/folders created at pwd by New-Item when given path is drive root (#5228) Apr 8, 2018
@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 8, 2018

Alright, I think I'm on the right track this time. Because the argument for Path may not be given if that parameter is not specified when using the cmdlet, returning an empty string in the spot that I was doing so before would break the expected functionality of creating an item in the current working directory.

Testing for an empty path after GetDriveRootRelativePathFromPSPath strips the drive name and colon ensures that the case in which a drive root is specified as the path is handled. Rather than attempt to generate a drive relative path from an empty path (which would generate the relative path to the current working directory), the current context's PSDrive's root is returned.

Looking forward to review.

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 8, 2018

@mcbobke The PR contains alien commits. I suggest you to rebase and squash before we start review.

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 8, 2018

@iSazonov I did my best to clean up the commit history; is there anything I can do to clean it up further?

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 9, 2018

@mcbobke I still see an alien commit https://github.com/PowerShell/PowerShell/pull/6584/commits

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 9, 2018

Whoops! Apologies. Looks like it's fixed now.

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 9, 2018

@mcbobke No, your commit contains unrelated code.

I see you make the PR from forked master branch. You should create new branch for every new work.

  1. fork the repo
  2. create new branch
  3. add commits to the new branch "work"
  4. make PR based on branch "work"
  5. after merge the PR remove the branch "work"
  6. rebase your local master branch from the original repo
  7. goto 2

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 9, 2018

Understood @iSazonov . How should I branch this work off now that it's in master? Reset to previous commit, create new branch, push changes to new branch?

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 9, 2018

If you haven't experience with git you could simply backup your code (copy files) and re-create your fork, create new branch and copy your files (or changed code blocks) back.

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 9, 2018

I have some experience with Git, but I have learned some things while working on this change. :P Does that mean I need to recreate the PR as well? I can revert the commit and branch it off, if I can also edit the PR. Otherwise, I'd create a new one.

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 9, 2018

You are free to choose a way.
When I started with the project I was doing a full reset a couple of times.

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 9, 2018

Okay, what I'll do then is branch off and create a new PR and reference this one. Thanks for all of your help, this has been a learning experience!

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 9, 2018

@mcbobke If our docs is not clear for the repo newbies please feel free to commit additions with your experience.

@mcbobke
Copy link
Contributor Author

mcbobke commented Apr 9, 2018

That's a really good idea. I will look through and see where I can add some clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New-Item ignore's Path param if Name param is specified
2 participants