Fix #80 Get-BuildVariable breaks if git is aliased #81

RandomNoun7 · 2018-11-13T07:00:41Z

hub is a tool put out by git that adds
commandline tools to help developers work with github from the
commandline in a more natural way. It is designed to be used by aliasing
git to hub.exe. Hub then handles the additional commands it knows how to
implement, and transparently passes through any native git commands to
git.exe.

The Get-BuildVariable cmdlet does not account for the possibility that
git might be aliased, and assumes that the output of Get-Command 'git'
can authoritatively be treated as either returning a Path to git or
determining that git is not installed on a system. Unfortunately if git
is aliased to hub.exe then the result of Get-Command is not an object of
type System.Management.Automation.ApplicationInfo that has a Path
property but an object of type System.Management.Automation.AliasInfo
which does not contain a path either to git or to the executable it's
been aliased to.

One possible strategy would be to find that git has been aliased and
find the path to executable that it has been aliased to, but this seems
less than ideal since the third party binary is not what this module
really wants to execute, and this module will only ever need the
capabilities of the native git.exe, not the extended capabilities of hub.

This change switches the method of determining the path to git over to
looking at each path in the $env:PATH variable, and returning the path
to the first instance of git.exe that it finds. This should be the same
method used by Get-Command, and isn't fooled by the presence of aliases.

This change should be a fix for #80

RandomNoun7 · 2018-11-13T07:45:38Z

This attempt at a fix does not pass testing on my local machine. More work is needed and I need to add tests.

RandomNoun7 · 2018-11-13T08:28:05Z

In testing I discovered that Invoke-Git was also vulnerable to this issue. I will leave the issue and PR title unchanged. The existing tests now pass on my machine.

RandomNoun7 · 2018-11-13T08:30:29Z

BuildHelpers/Public/Get-BuildVariables.ps1

@@ -75,7 +75,7 @@ function Get-BuildVariables {
    $Path = ( Resolve-Path $Path ).Path
    $Environment = Get-Item ENV:
    if(!$PSboundParameters.ContainsKey('GitPath')) {
-        $GitPath = (Get-Command $GitPath -ErrorAction SilentlyContinue)[0].Path
+        $GitPath = (Get-ChildItem ($env:PATH -split ';') -filter git.exe -ErrorAction SilentlyContinue | Select-Object -First 1).Fullname


I need to fix the fact that git.exe is hard coded here. I will either need to change the -filter to something like -filter "*$GitPath*" or change the param default to git.exe

I think I've changed my mind on this. If the user provides a git path they should be providing a full path to the git executable. If they provide anything at all this variable is given directly to the Process Start info Filename Parameter anyway, so if they do something like provide an input that it can't use, it will already fail.

If the user does not provide input to the $GitPath parameter, we already know conclusively that we want to search for git.exe. We aren't searching for git.com or git.bat, or anything like that.

This says to me that it is safe to simply leave the $GitPath parameter optional, and there is no need to provide a default value.

Hiyo! @gaelcolas has an old PR in one of my repos for a similar thing, looking to resolve just git (this should still pick up git.exe, but buys us compatibility with systems that don't use .exe). Might be worth looking into something like this here? (This is a pre-skimming-your-whole-commit-and-my-old-code comment : P)

I forgot about this PR for a while, and suddenly I get a notice that there was a comment. If @gaelcolas wants to link me the PR that would be great. Otherwise I'll go searching around for it on Monday.

RandomNoun7 · 2018-11-13T08:32:23Z

BuildHelpers/Public/Invoke-Git.ps1

@@ -84,7 +84,7 @@
    # http://stackoverflow.com/questions/8761888/powershell-capturing-standard-out-and-error-with-start-process
    $pinfo = New-Object System.Diagnostics.ProcessStartInfo
    if(!$PSBoundParameters.ContainsKey('GitPath')) {
-        $GitPath = (Get-Command $GitPath -ErrorAction Stop)[0].Path
+        $GitPath = (Get-ChildItem ($env:PATH -split ';') -filter git.exe -ErrorAction SilentlyContinue | Select-Object -First 1).Fullname


Same comment as above.

[hub](https://github.com/github/hub) is a tool put out by git that adds commandline tools to help developers work with github from the commandline in a more natural way. It is designed to be used by aliasing git to hub.exe. Hub then handles the additional commands it knows how to implement, and transparently passes through any native git commands to git.exe. The Get-BuildVariable cmdlet does not account for the possibility that git might be aliased, and assumes that the output of Get-Command 'git' can authoritatively be treated as either returning a Path to git or determining that git is not installed on a system. Unfortunately if git is aliased to hub.exe then the result of Get-Command is not an object of type `System.Management.Automation.ApplicationInfo` that has a `Path` property but an object of type `System.Management.Automation.AliasInfo` which does not contain a path either to git or to the executable it's been aliased to. One possible strategy would be to find that git has been alaised and find the path to executable that it has been aliased to, but this seems less than ideal since the third party binary is not what this module really wants to execute, and this module will only ever need the capabilities of the native git.exe, not the extended capabilities of hub. This change switches the method of determining the path to git over to looking at each path in the $env:PATH variable, and returning the path to the first instance of git.exe that it finds. This should be the same method used by Get-Command, and isn't fooled by the presence of aliases. This change should be a fix for RamblingCookieMonster#80

RandomNoun7 · 2018-11-13T09:32:49Z

BuildHelpers/Public/Get-BuildVariables.ps1

@@ -69,13 +69,13 @@ function Get-BuildVariables {
            }
            $true
        })]
-        $GitPath = 'git'
+        $GitPath


The hard coded git.exe we are searching for makes this default value unnecessary.

RandomNoun7 · 2018-11-13T09:33:01Z

BuildHelpers/Public/Invoke-Git.ps1

@@ -77,14 +77,14 @@
            }
            $true
        })]
-        [string]$GitPath = 'git'
+        [string]$GitPath


The hard coded git.exe we are searching for makes this default value unnecessary.

RandomNoun7 · 2018-11-13T09:36:17Z

I have not been able to find a good way to add another set of tests for the git commands with an invalid alias like New-Alias -Name git -Value notgit inside the Pester tests and have it work as expected.

The closest I have been able to do is manually test via the following procedure.

Run tests in a normal session. All Tests pass.
Add the alias described Above.
Re-Run tests. The two git command tests now fail.
Checkout my feature branch
Re-Run test and observe that all tests pass.
Start a new fresh PowerShell session that does not have an alias.
With my feature branch still checked out and no alias, Re-Run tests and observe that all pass.

RandomNoun7 force-pushed the issues/80/fix-get-buildvariable-if-git-is-aliased branch from f259da7 to 5d8069d Compare November 13, 2018 07:08

RandomNoun7 force-pushed the issues/80/fix-get-buildvariable-if-git-is-aliased branch from 5d8069d to 4ff88c2 Compare November 13, 2018 08:26

RandomNoun7 commented Nov 13, 2018

View reviewed changes

RandomNoun7 force-pushed the issues/80/fix-get-buildvariable-if-git-is-aliased branch from 4ff88c2 to 24d3518 Compare November 13, 2018 09:31

RandomNoun7 commented Nov 13, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #80 Get-BuildVariable breaks if git is aliased #81

Fix #80 Get-BuildVariable breaks if git is aliased #81

RandomNoun7 commented Nov 13, 2018 •

edited

Loading

RandomNoun7 commented Nov 13, 2018

RandomNoun7 commented Nov 13, 2018

RandomNoun7 Nov 13, 2018 •

edited

Loading

RandomNoun7 Nov 13, 2018

RamblingCookieMonster Nov 22, 2018

RandomNoun7 Dec 15, 2018

RandomNoun7 Nov 13, 2018

RandomNoun7 Nov 13, 2018

RandomNoun7 Nov 13, 2018

RandomNoun7 commented Nov 13, 2018 •

edited

Loading

Fix #80 Get-BuildVariable breaks if git is aliased #81

Are you sure you want to change the base?

Fix #80 Get-BuildVariable breaks if git is aliased #81

Conversation

RandomNoun7 commented Nov 13, 2018 • edited Loading

RandomNoun7 commented Nov 13, 2018

RandomNoun7 commented Nov 13, 2018

RandomNoun7 Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

RandomNoun7 Nov 13, 2018

Choose a reason for hiding this comment

RamblingCookieMonster Nov 22, 2018

Choose a reason for hiding this comment

RandomNoun7 Dec 15, 2018

Choose a reason for hiding this comment

RandomNoun7 Nov 13, 2018

Choose a reason for hiding this comment

RandomNoun7 Nov 13, 2018

Choose a reason for hiding this comment

RandomNoun7 Nov 13, 2018

Choose a reason for hiding this comment

RandomNoun7 commented Nov 13, 2018 • edited Loading

RandomNoun7 commented Nov 13, 2018 •

edited

Loading

RandomNoun7 Nov 13, 2018 •

edited

Loading

RandomNoun7 commented Nov 13, 2018 •

edited

Loading