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

Correctly parse prompts with brackets #112

Merged
merged 1 commit into from May 31, 2018

Conversation

Projects
None yet
3 participants
@elyscape
Contributor

elyscape commented Apr 9, 2018

Description of the Change

This pull request fixes the pattern that is supposed to parse the shell prompt when it is surrounded by brackets. Some distros, notably those based on Fedora or RHEL, default to prompts of the form [user@host cwd]$ . Unfortunately, the pattern to match these currently assumes that there are one or more characters after the closing bracket and before the prompt separator. To illustrate:

[root@04baf0b50a63 /]# # Fedora default
[root@04baf0b50a63 /]# echo "$PS1"
[\u@\h \W]\$
[root@04baf0b50a63 /]# ls
bin   dev  home  lib64       media  opt   root  sbin  sys  usr
boot  etc  lib   lost+found  mnt    proc  run   srv   tmp  var
[root@04baf0b50a63 /]# # compatible with current regex
[root@04baf0b50a63 /]# PS1='[\u@\h \W] \$ '
[root@04baf0b50a63 /] # ls
bin   dev  home  lib64       media  opt   root  sbin  sys  usr
boot  etc  lib   lost+found  mnt    proc  run   srv   tmp  var

This removes the requirement of additional characters after the closing bracket, effectively changing it to a requirement that either whitespace or nothing is present between closing bracket and the prompt separator.

Alternate Designs

The regular expression could be further modified to optionally capture additional characters between the closing bracket and the separator. However, I could not figure out a way to do this that would not also match if the command contained a closing bracket and a valid prompt separator (e.g. if the line looked like [user@host cwd]$ echo 'hi] $').

Benefits

Improved syntax highlighting on GitHub.

Possible Drawbacks

There shouldn't be any.

@elyscape

This comment has been minimized.

Contributor

elyscape commented May 7, 2018

Pinging @maxbrunsfeld and @50Wliu to make sure this doesn't fall through the cracks.

@lee-dohm

This comment has been minimized.

Member

lee-dohm commented May 15, 2018

@50Wliu Can you take a look at this?

@50Wliu

This comment has been minimized.

Member

50Wliu commented May 16, 2018

effectively changing it to a requirement that either whitespace or nothing is present between closing bracket and the prompt separator.

Is there a possibility that prompts may have extra characters between the closing bracket and prompt separator? Is that what you're referring to in the Alternate Designs section? If so, could you please share some attempts at solving that problem? Thanks 🙇‍♂️

@elyscape

This comment has been minimized.

Contributor

elyscape commented May 16, 2018

Is there a possibility that prompts may have extra characters between the closing bracket and prompt separator?

Basically, with the patch at present, the following will be treated as prompts:

[root@25f3d5fc32fb /]# echo "'$PS1'"
'[\u@\h \W]\$ '
[root@25f3d5fc32fb /]# PS1='[\u@\h \W] \$ '
[root@25f3d5fc32fb /] # echo "'$PS1'"
'[\u@\h \W] \$ '

By contrast, if there is non-whitespace text between the closing bracket and the prompt character, this patch will result in them no longer being treated as prompts. Some examples of things that would be affected:

[root@25f3d5fc32fb /]text# echo "'$PS1'"
'[\u@\h \W]text\$ '
[root@25f3d5fc32fb /]text# PS1='[\u@\h \W] text\$ '
[root@25f3d5fc32fb /] text# echo "'$PS1'"
'[\u@\h \W] text\$ '
[root@25f3d5fc32fb /] text# PS1='[\u@\h \W] text \$ '
[root@25f3d5fc32fb /] text # echo "'$PS1'"
'[\u@\h \W] text \$ '
[root@25f3d5fc32fb /] text # PS1='[\u@\h \W]text \$ '
[root@25f3d5fc32fb /]text # echo "'$PS1'"
'[\u@\h \W]text \$ '

As I mentioned in Alternate Designs, I couldn't figure out a good way to allow arbitrary text between the closing bracket and the prompt character without it potentially matching the command itself. You can see this happening already in the examples above where I'm changing PS1; notice how those lines are highlighted purple though the backslash escaping the $ towards the end of the line. I tried various options, but the only way I could find to avoid that was to modify the behavior as described, such that only optional whitespace is allowed between the closing bracket and the prompt character.

All this being said, as far as I'm aware, putting text between the closing bracket and the prompt character isn't a thing that is regularly done. It's more common to put text at the beginning of PS1, like what virtualenv does:

[root@b975d6f40c5f ~]# virtualenv venv
New python executable in /root/venv/bin/python2
Also creating executable in /root/venv/bin/python
Installing setuptools, pip, wheel...done.
[root@b975d6f40c5f ~]# source venv/bin/activate
(venv) [root@b975d6f40c5f ~]# echo "'$PS1'"
'(venv) [\u@\h \W]\$ '
(venv) [root@b975d6f40c5f ~]# 

This is currently handled but incompletely; the pattern currently assumes that there will be no whitespace between any leading parenthetical and the prompt proper. My proposed change does not alter this. It could be fixed fairly trivially, though, by adding \\s+ to the end of the group on line 12. Should I do that?

@50Wliu

This comment has been minimized.

Member

50Wliu commented May 17, 2018

@elyscape Thanks for the explanation! Using that, I came up with the following regex that seems to work in all cases: \\[\\S+?[@:][^\\n]+?\\].*?. It uses a lot more lazy quantifiers to try to end the match as soon as possible, which bypasses the issue you had where it would greedily look for the last bracket in the line.

Lightshow

Example with more contrast:
shell-bracket-prefixes

@50Wliu

This comment has been minimized.

Member

50Wliu commented May 17, 2018

It could be fixed fairly trivially, though, by adding \s+ to the end of the group on line 12. Should I do that?

If you could fix this in a separate PR, that would be .

@elyscape

This comment has been minimized.

Contributor

elyscape commented May 17, 2018

Sounds good! Change pushed. I'll open another PR with the virtualenv fix.

@50Wliu 50Wliu merged commit 7d6e691 into atom:master May 31, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@elyscape elyscape deleted the elyscape:fix-bracket-prompt branch May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment