-
-
Notifications
You must be signed in to change notification settings - Fork 800
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
Make SSH_AGENT_PID mapping to Windows PID #699
Conversation
Fixes dahlbyk/posh-sshell#21 PID and WINPID mismatch was introduced in the recent cygwin release, see git-for-windows/git#2274
Git for windows doesn't have it by default
Hi, for my personal use I am fine with 0.7 branch, but would you like me to submit this to the 1.0 aka master branch? |
I'd like to close this now that v1 has shipped. Any objections? |
@rkeithhill Hey, since you didn't merge it 1.5 years ago, I think it makes sense to close it now. However, for me this patch still works relatively fine. |
Thanks for the reply! |
I'm so haunted. I finally was able to upgrade at work to the most recent software here (posh-git 1.1.0 and posh-sshell 0.3.1) and this bit me hard. Spent few hours trying to figure out what was going on. Finally realized the PID was not seen in windows. My git bash equivalent continued working like a champ. I realize the original author here from some PRs doesn't use or have time for this any more but some of us don't have modern enough windows even still in 2024 (love corporate!) to not have this and have to rely on git's ssh client. This should be reopened and merged here on 0.7.x line and it should also be raised on posh-sshell and applied there. Its a simple port to the 'Agent.ps1' identically. Thankfully after running back into this my memory came back and I went and looked and I had documented exactly applying this patch back in March of 2021 pointing to this very PR. So for 4 years, few hundred devs have been able to work smoothly in powershell so I think its worth having! And anyone that does have same situation that tries to upgrade is going to run into this and waste hours for something so easily addressed. And shame on me, I had actually documented this in a readme really well as well as the code but 3 years later I forgot about it. However, that all said, this is not solely enough. Two additional lines need patched
Both in 2021 and now in 2024 - using windows 2016, those lines return 'ps' path twice. This fix then and now worked. The fix needs to be on posh-sshell as well.
|
I don't use SSH anymore, but glad to merge/release PRs here and in posh-sshell. |
@dahlbyk Perfect! I'll get a PR up to the posh-sshell this weekend and I'll create a new PR off this one to add the extra two lines that needed fixed. I'll likely drop the ignore item added here as I didn't take that in 2021 and never missed it. |
Port of dahlbyk/posh-git#699. This does drop -ErrorAction Ignore as unnecessary. It further adds [0] to full name parent directory for bin / usr lookup of ssh location as the lookup of 'ps' and and will get multiple (2) results. This ensures it only uses the first in that situation.
Port of dahlbyk#699. This does drop -ErrorAction Ignore as unnecessary. It further adds [0] to full name parent directory for bin / usr lookup of ssh location as the lookup of 'ps' and and will get multiple (2) results. This ensures it only uses the first in that situation.
Note: Both my older usage off 0.7.x and usage from this past week off posh-sshell where based off the released versions. I understand releasing is unlikely and for our usage since we rebundle, we don't have users installing the normal way anyways. So I plan to re-take latest masters off both and work forwards. I can drop back and also do a bit more testing with 0.7.x. Longer plan there will be to start reviewing merit of any open PRs that are not yet applied to see what else might be beneficial given there are a good number of them. |
Fixes dahlbyk/posh-sshell#21
PID and WINPID mismatch was introduced in the recent cygwin release,
see git-for-windows/git#2274