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

Stop Get-Process from adding exceptions to $Error when checking if pageant / ssh-agent aren't running #162

Merged
merged 4 commits into from
Nov 13, 2014

Conversation

paulmarsy
Copy link
Contributor

Updating the Get-Process call in GitUtils.ps1 so exceptions aren't added to the global $Error array

This occurs when pageant or ssh-agent aren't running, and therefore there is no process / pid to retrieve. However this is a valid scenario so shouldn't be added to the global $Error array.

Also being more explicit with the check of the returned value from Get-Process

Updating the Get-Process call in GitUtils.ps1 so exceptions aren't added to the global $Error array

This occurs when pageant or ssh-agent aren't running, and therefore there is no process / pid to retrieve. However this is a valid scenario so shouldn't be added to the global $Error array.

Also being more explicit with the check of the returned value from Get-Process
@lzybkr
Copy link
Collaborator

lzybkr commented Nov 9, 2014

posh-git still works with V2, right?
-ErrorAction Ignore is not available in PowerShell V2, it's only in V3 and above.

@paulmarsy
Copy link
Contributor Author

The Ignore flag was introduced in V3 which has been available since mid 2012 for Windows 7, Windows Server 2008 / 2008 R2 and built into Windows 8 / Windows Server 2012.

You are correct currently the version check in posh-git is set for V2 (install.ps1):

if($PSVersionTable.PSVersion.Major -lt 2) {
    Write-Warning "posh-git requires PowerShell 2.0 or better; you have version $($Host.Version)."
    return
}

It depends on if you wish to maintain support for V2 or considering the length of time V3 has been available allowing posh-git to make use of V3 functionality.

If you want to maintain support for V2 would a change like this be appropriate:

Get-Process | ? { $_.Name -eq 'pageant' } 

Then there are no errors generated at all as we filter the list of all processes for the one we are looking for.

Changing the identification of the pageant / ssh-agent process so an
error being thrown and silently ignored is not an expected scenario and
allows real errors from calling Get-Process to be seen
@paulmarsy
Copy link
Contributor Author

@lzybkr Any feedback on this proposed change?

@lzybkr
Copy link
Collaborator

lzybkr commented Nov 13, 2014

You've summed it up nicely. I have no idea how many people who use posh-git also use V2. I imagine that number is > 0, so unless there is a compelling reason to force the use of V3, I'd use your filter approach. I submitted similar changes to posh-git for similar reasons.

@dahlbyk dahlbyk mentioned this pull request Nov 13, 2014
4 tasks
dahlbyk added a commit that referenced this pull request Nov 13, 2014
Stop Get-Process from adding exceptions to $Error when checking if pageant / ssh-agent aren't running
@dahlbyk dahlbyk merged commit 869d4c5 into dahlbyk:master Nov 13, 2014
@dahlbyk
Copy link
Owner

dahlbyk commented Nov 13, 2014

I have no idea how many people who use posh-git also use V2. I imagine that number is > 0, so unless there is a compelling reason to force the use of V3, I'd use your filter approach.

This isn't the first time this has come up... Opened #163 to put V2 behind us.

In the meantime the filter approach works for me - thanks!

paulmarsy added a commit to paulmarsy/posh-git that referenced this pull request Dec 22, 2014
Merge pull request dahlbyk#162 from paulmarsy/master
paulmarsy added a commit to paulmarsy/posh-git that referenced this pull request Dec 22, 2014
paulmarsy added a commit to paulmarsy/posh-git that referenced this pull request Dec 22, 2014
Revert "Merge pull request dahlbyk#162 from paulmarsy/master"
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.

3 participants