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

Add Get-PromptConnectionInfo as DefaultPromptPrefix #595

Merged
merged 5 commits into from
Jul 11, 2018
Merged

Conversation

dahlbyk
Copy link
Owner

@dahlbyk dahlbyk commented Jul 9, 2018

Procrastination!

Closes #591 by roughly imitating PowerShell/PowerShell#7191 (show [user@host] if user seems to not match, otherwise [host]). I don't use PS remoting, so I'll need some help verifying this works as expected.

Rather than place the logic inline, I figure exporting Get-PromptConnectionInfo affords flexibility to make this configurable (e.g. $GitPromptSettings.DefaultPromptConnectionShowUserName = 'always|never|different' or $GitPromptSettings.DefaultPromptConnectionFormatWithUserName).

@dahlbyk dahlbyk added this to the v1.0 milestone Jul 9, 2018
@dahlbyk dahlbyk requested a review from rkeithhill July 9, 2018 06:14
src/Utils.ps1 Outdated

function Get-PromptConnectionInfo {
if (Test-Path Env:SSH_CONNECTION) {
if ($sshUserName -and $sshUserName -ne [System.Environment]::UserName) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They made a late change in that PowerShell PR due to user names on Linux being case-sensitive. I think you want -cne here. Also, I thought we were using soft-tabs of 4 spaces? :-) I'll test this tomorrow when I get into work and have access to a system with sshd set to use PowerShell.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I thought we were using soft-tabs of 4 spaces? :-)

I have no idea what you're talking about.

I think you want -cne here.

Fixed. Though I'm tempted to punish anyone who actually hits that scenario. 🙄

@rkeithhill
Copy link
Collaborator

Looking good for the same user:
image

But when I ssh into my Windows box as a different user, I do not get the username. It appears that the Runspace's ConnectionInfo object is $null. That leaves no good way to determine the orginal user. :-(

@dahlbyk
Copy link
Owner Author

dahlbyk commented Jul 10, 2018

But when I ssh into my Windows box as a different user, I do not get the username. It appears that the Runspace's ConnectionInfo object is $null. That leaves no good way to determine the orginal user.

What's different here compared with PowerShell/PowerShell#7191?

@rkeithhill
Copy link
Collaborator

rkeithhill commented Jul 10, 2018

I think it is because that method in the PR is getting a RemoteRunspace. When I run the following command in my ssh session Get-Runspace | Get-Member, I get System.Management.Automation.Runspaces.LocalRunspace. Maybe when we detect an SSH session, we just always display the username? That would be pretty consistent with ssh'ing into sh/bash.

@dahlbyk
Copy link
Owner Author

dahlbyk commented Jul 10, 2018

Taking a closer look at EvaluatePrompt, I'm fuzzy on why we need to do anything. It seems like prompt gets called and then GetRemotePrompt wraps the result.

@rkeithhill does $Host.Runspace get you a RemoteRunspace?

@rkeithhill
Copy link
Collaborator

That code gets used from a PS remoting session. It does not get used when you SSH into a Windows system where the default shell is set to PowerShell.

@rkeithhill
Copy link
Collaborator

$Host.Runspace is a LocalRunspace as well.

@dahlbyk
Copy link
Owner Author

dahlbyk commented Jul 11, 2018

That code gets used from a PS remoting session. It does not get used when you SSH into a Windows system where the default shell is set to PowerShell.

I knew I was missing something. That's the ELI5 I needed.


Added code to always include username ([user@host]: ), plus a setting to change the connection format string.

[PoshGitTextSpan]$DefaultPromptPath = '$(Get-PromptPath)'
[PoshGitTextSpan]$DefaultPromptBeforeSuffix = ''
[PoshGitTextSpan]$DefaultPromptDebug = [PoshGitTextSpan]::new(' [DBG]:', [ConsoleColor]::Magenta)
[PoshGitTextSpan]$DefaultPromptSuffix = '$(">" * ($nestedPromptLevel + 1)) '

[bool]$DefaultPromptAbbreviateHomeDirectory = $true
[string]$DefaultPromptConnectionFormat = '[{1}@{0}]: '
Copy link
Collaborator

@rkeithhill rkeithhill Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead of this setting that isn't perhaps obviously connected to Get-PromptConnectionInfo, we should just have that command take a format string as parameter. We could make that parameter optional (i.e. default it to '[{1}@{0}]: ' . I'd use it explicitly for $DefaultPromptPrefix so folks see they can tweak the format e.g.:

[PoshGitTextSpan]$DefaultPromptPrefix       = '$(Get-PromptConnectionInfo -Format "[{1}@{0}]: ")' 

Although at this point maybe we switch the format placeholders to [{0}@{1}]: ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Thoughts on -Format vs ScriptBlock?

[PoshGitTextSpan]$DefaultPromptPrefix       = '$(Get-PromptConnectionInfo {param($Hostname, $Username) "[$Username@$Hostname]: "})' 

Similarly, should we ditch DefaultPromptAbbreviateHomeDirectory?

[PoshGitTextSpan]$DefaultPromptPath         = '$(Get-PromptPath -AbbreviateHome)'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think -Format is better and we should definitely provide a default. The only reason I would specify the -Format … parameter in that setting is purely pedagogical.

Re DefaultPromptAbbreviateHomeDirectory, sounds good to me. We should note that in the CHANGELOG as a potentially breaking change though.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, DefaultPromptAbbreviateHomeDirectory has been established since v0.7.0, doesn't seem worth breaking everyone now.

I'll replace the new setting with -Format.

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dahlbyk dahlbyk merged commit e5bf84c into master Jul 11, 2018
@dahlbyk dahlbyk deleted the ssh-host branch July 11, 2018 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants