During install create the profile dir if it does not already exist #449

tolgabalci · 2017-03-02T00:24:31Z

While installing posh-git using install.ps1 on Windows 7 Enterprise SP1 laptops for users running Powershell 4.0 or 5.0 had some users receive this error:

PS C:\tools\posh-git> .\install.ps1
Add-Content : Could not find a part of the path
'C:\Users\*username*\Documents\WindowsPowerShell\Microsoft.PowerShell_profile.ps1'.
At C:\tools\posh-git\src\Utils.ps1:180 char:9
+         Add-Content -LiteralPath $profilePath -Value $profileContent -Encoding U ...

Nothing had previously been added to the user's Profile scripts AND the folder "WindowsPowerShell" did not exists under the user's documents. This caused Add-Content to fail.

The pull request is to create the Powershell profile directory for the current user if it does not already exists.

Tested both with and without -WhatIf and it works good.

rkeithhill

Just a few minor issues to fix but otherwise looks good.

rkeithhill · 2017-03-02T02:33:20Z

src/Utils.ps1

@@ -176,6 +176,14 @@ function Add-PoshGitToProfile {
        $profileContent = "`nImport-Module '$ModuleBasePath\posh-git.psd1'"
    }

+    # Make sure the profile path exists
+    $profileDir = Split-Path -Parent $profilePath
+    if (-not (Test-Path -LiteralPath $profileDir)) { 


In the code base we tend not to use -not in favor of !. We're not 100% there yet but I don't want to introduce more script that needs to be changed.

rkeithhill · 2017-03-02T02:40:04Z

src/Utils.ps1

+    $profileDir = Split-Path -Parent $profilePath
+    if (-not (Test-Path -LiteralPath $profileDir)) { 
+        if ($PSCmdlet.ShouldProcess($profileDir, "Create current user Powershell profile directory")) {
+            New-Item $profileDir -Type Directory 


Two things. Better to use the actual parameter name ItemType than the alias Type. Also, this command will output the created directory. Better to just redirect the output to $null. Then add support for -verbose e.g.:

New-Item $profileDir -ItemType Directory -Verbose:$VerbosePreference > $null

rkeithhill · 2017-03-02T02:40:51Z

src/Utils.ps1

+    # Make sure the profile path exists
+    $profileDir = Split-Path -Parent $profilePath
+    if (-not (Test-Path -LiteralPath $profileDir)) { 
+        if ($PSCmdlet.ShouldProcess($profileDir, "Create current user Powershell profile directory")) {


PowerShell is properly spelled with a capital S - PowerShell.

tolgabalci · 2017-03-02T03:59:33Z

Great callouts! Thanks.

Amended the suggested changes and retested. (Both with $VerbosePreference = "SilentlyContinue" and "Continue"). Tests looked good.

rkeithhill · 2017-03-02T04:07:56Z

src/Utils.ps1

@@ -176,6 +176,14 @@ function Add-PoshGitToProfile {
        $profileContent = "`nImport-Module '$ModuleBasePath\posh-git.psd1'"
    }

+    # Make sure the profile path exists
+    $profileDir = Split-Path -Parent $profilePath


Sorry missed one other minor issue (which I myself forget from time-to-time). Can you switch the order on params here and use -LiteralPath e.g. $profileDir = Split-Path -LiteralPath $profilePath -Parent. Thanks.

No problem, I moved the -Parent parameter to the end.

I also looked at using -LiteralPath with Split-Path, but it does not have an explicit -LiteralPath parameter, however based on the below test it looks like it already implicitly treats the path literally.

C:\test[1]> Get-Item -LiteralPath c:\test[1]\File[1].txt Directory: C:\test[1] Mode LastWriteTime Length Name ---- ------------- ------ ---- -a---- 3/2/2017 3:18 AM 0 File[1].txt C:\test[1]> Get-Item -Path c:\test[1]\File[1].txt C:\test[1]> Split-Path -Path c:\test[1]\File[1].txt -Parent c:\test[1]

I created a directory and file with names that both need to be interpreted literally.

Get-Item with -LiteralPath works fine

Get-Item with -Path fails to retrieve the file

Split-Path with -Path however still correctly parses the path to retrieve the parent

So, I think Split-Path is good with just -Path. Looking through the other instances of Split-Path in the code, I figured I would keep it consistent and not explicitly state -Path, but let me know if you want me to change it. It's no problem.

rkeithhill · 2017-03-02T04:08:43Z

You should also be able to call Add-PoshGitToProfile -Verbose and see the verbose output from New-Item. BTW thanks for the PR! See the one other issue I found - it's a quick tweak.

dahlbyk · 2017-03-02T15:02:57Z

LGTM, but care to add a test for this scenario?

rkeithhill · 2017-03-02T17:06:50Z

@tolgabalci OK for the ParentSet parameter set, there is no -LiteralPath parameter. Looks good. I'm going to merge this and I'll add a test later tonight. If you're really inspired to add the test, you can always open a separate PR.

dahlbyk · 2017-03-02T19:24:22Z

It just dawned on me that there's a much simpler fix for this: New-Item -Force will automatically create directories as necessary. This is what install.ps1 used to do.

rkeithhill · 2017-03-02T19:27:40Z

Do you think it is worth changing this? I'm not sure that it is.

dahlbyk · 2017-03-02T19:41:20Z

Just a little less path-splitting logic to maintain. And thinking through edge cases, it's technically possible (though unlikely with defaults) that more than $PROFILE's immediate parent could be missing. Of course, you could work around this with -Force when creating the parent. ¯\_(ツ)_/¯

tolgabalci · 2017-03-03T11:24:47Z

@rkeithhill Thank you for adding the test and showing me how to do it!

@dahlbyk New-Item will actually create the full parent directory tree without -Force when used with -ItemType Directory. I do not think -Force is really intended for use when creating directories. I was trying it out, it does have the interesting side effect of suppressing errors when one of the directories in the directory tree being created is a file and it fails to create the path.

@rkeithhill @dahlbyk, thank you so much for being so responsive!

dahlbyk · 2017-03-03T14:19:22Z

New-Item will actually create the full parent directory tree without -Force when used with -ItemType Directory. I do not think -Force is really intended for use when creating directories. I was trying it out, it does have the interesting side effect of suppressing errors when one of the directories in the directory tree being created is a file and it fails to create the path.

Interesting… I suppose the nested directory variations are all things we could test for, if you are interested in exploring publicly. 😁

rkeithhill self-assigned this Mar 2, 2017

rkeithhill added the Installation label Mar 2, 2017

rkeithhill added this to the v0.7.1 milestone Mar 2, 2017

rkeithhill removed their assignment Mar 2, 2017

rkeithhill requested changes Mar 2, 2017

View reviewed changes

rkeithhill reviewed Mar 2, 2017

View reviewed changes

During install create the profile dir if it does not already exist

fd38100

dahlbyk approved these changes Mar 2, 2017

View reviewed changes

rkeithhill approved these changes Mar 2, 2017

View reviewed changes

rkeithhill merged commit a3db658 into dahlbyk:master Mar 2, 2017

rkeithhill mentioned this pull request Mar 3, 2017

Add test for update to Add-PoshGitToProfile #452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

During install create the profile dir if it does not already exist #449

During install create the profile dir if it does not already exist #449

tolgabalci commented Mar 2, 2017 •

edited

Loading

rkeithhill left a comment

rkeithhill Mar 2, 2017

rkeithhill Mar 2, 2017

rkeithhill Mar 2, 2017

tolgabalci commented Mar 2, 2017

rkeithhill Mar 2, 2017

tolgabalci Mar 2, 2017

rkeithhill commented Mar 2, 2017

dahlbyk commented Mar 2, 2017

rkeithhill commented Mar 2, 2017

dahlbyk commented Mar 2, 2017 •

edited

Loading

rkeithhill commented Mar 2, 2017

dahlbyk commented Mar 2, 2017 •

edited

Loading

tolgabalci commented Mar 3, 2017 •

edited

Loading

dahlbyk commented Mar 3, 2017

During install create the profile dir if it does not already exist #449

During install create the profile dir if it does not already exist #449

Conversation

tolgabalci commented Mar 2, 2017 • edited Loading

rkeithhill left a comment

Choose a reason for hiding this comment

rkeithhill Mar 2, 2017

Choose a reason for hiding this comment

rkeithhill Mar 2, 2017

Choose a reason for hiding this comment

rkeithhill Mar 2, 2017

Choose a reason for hiding this comment

tolgabalci commented Mar 2, 2017

rkeithhill Mar 2, 2017

Choose a reason for hiding this comment

tolgabalci Mar 2, 2017

Choose a reason for hiding this comment

rkeithhill commented Mar 2, 2017

dahlbyk commented Mar 2, 2017

rkeithhill commented Mar 2, 2017

dahlbyk commented Mar 2, 2017 • edited Loading

rkeithhill commented Mar 2, 2017

dahlbyk commented Mar 2, 2017 • edited Loading

tolgabalci commented Mar 3, 2017 • edited Loading

dahlbyk commented Mar 3, 2017

tolgabalci commented Mar 2, 2017 •

edited

Loading

dahlbyk commented Mar 2, 2017 •

edited

Loading

dahlbyk commented Mar 2, 2017 •

edited

Loading

tolgabalci commented Mar 3, 2017 •

edited

Loading