Prompt improvements, primarily for zsh users #240

floatingatoll · 2021-06-15T21:45:40Z

I've been having some issues with the prompts set by maws on my macOS install, and so today I worked out what they are and improved prompt handling in the script. I've done limited testing of this on zsh and will be collecting additional reviews and testing now that I've reached a stable point. The changelog additions for this pull request are:

Fixed

Enable zsh dynamic prompts for users who don't already have them enabled.
Fix issues with double-whitespace when injecting into space-separated prompts

Changed

Convert conditionals to bash-native to simplify maintenance and readability
Add internal logic to allow more complex prefix/suffix whitespace logic
Remove blank lines and code comments when printing bash/zsh script

Added

Allow users to disable prompt modification with MAWS_PROMPT_DISABLE=1. (This also lets users implement their own custom prompts without forking, as long as they define function maws_profile () { } in bashrc/zshrc.)

floatingatoll · 2021-07-12T16:22:06Z

I've had these changes live on my workstation for the past four weeks with no issues, on macOS zsh with ktx and maws and correct whitespace:

source "$HOME"/ktx/ktx
function maws_profile () {} # overwritten on first run of maws

# disable maws prompt rewriting, and set the prefix/suffix to match my prompt
export MAWS_PROMPT_DISABLE=1
export MAWS_PROMPT_PREFIX=' '
export MAWS_PROMPT_SUFFIX=''

# DIY my own prompt for ktx + maws
export PS1='%1~/$(maws_profile) $( [[ -n $KUBECONFIG ]] && echo -n "[${KUBECONFIG##*/}] " )%# '
setopt prompt_subst

gene1wood · 2021-07-12T16:44:18Z

mozilla_aws_cli/role_picker.py

-            echo " (maws keys expired)"
+PROMPT_BASH_CODE = r'''
+function maws_profile {
+    if [[ -n $MAWS_PROMPT ]]; then


I'd used [ instead of [[ to be most broadly compatible. [ is a POSIX standard whereas [[ is a bash extension that comes from KSH.

What would you think about retaining the POSIX standard [ across this code?

Also it's always good to quote shell variables to avoid unexpected behavior when the value in the variable contains spaces or other oddities.

I'd used [ instead of [[ to be most broadly compatible. [ is a POSIX standard whereas [[ is a bash extension that comes from KSH.

What would you think about retaining the POSIX standard [ across this code?

The original code for PROMPT_BASH_CODE already took for granted that the shell was bash or bash-compatible (the use of $(date +%s), specifically), and would not only fail with syntax errors but also fail to modify the user's prompt (the assumption that PS1 is the prompt), each time they eval the output of maws; in a brief chat with someone else, they just don't eval maws output and grab the environment variables instead. I don't expect this patch makes that scenario any worse for them, and I'm not taking "support non-bash-compatible prompts" into the scope of the pull request.

Also it's always good to quote shell variables to avoid unexpected behavior when the value in the variable contains spaces or other oddities.

When working with bash-specific conditionals inside [[ ... ]], overquoting not only is unnecessary but can break functionality of regular expressions and string-prefix/suffix comparisons. The specified bash code passes all shellcheck reviews and the only warning is an SC2016 false positive, as we're specifically avoiding string interpolation in a case where virtually everyone else is expecting to interpolate (because we're deferring interpolation until prompt eval time).

I would be more wary if this were not PROMPT_BASH_CODE and were instead meant for other shells, but as above, this has always taken for granted bash and expanding or repairing that constraint is out of scope here.

gene1wood · 2021-07-12T16:47:15Z

mozilla_aws_cli/role_picker.py

+
+# if the user hasn't disabled prompt injection,
+# and we aren't already injecting maws_profile:
+if [[ -z $MAWS_PROMPT_DISABLE && $PS1 != *'$(maws_profile)' ]]; then


Best to quote this variable in the conditional test

Quoting the variable on the left is unnecessary (see above); quoting the variable on the right will break the string-prefix comparison, as the wildcard needs to remain unquoted to activate that functionality inside [[ ... ]].

gene1wood · 2021-07-12T17:04:52Z

mozilla_aws_cli/role_picker.py

+    if [[ $PS1 == *'\$ ' ]]; then
+        # prompt ends with dynamic '\$ '
+        # if the original prompt surrounds the final '\$' with whitespace,
+        # we surround the substitution with whitespace to maintain that.
+        [[ $PS1 == *' \$ ' ]] && MAWS_PROMPT_PREFIX="" MAWS_PROMPT_SUFFIX=" "
+        # inject our substitution before the original '$ '
+        PS1="${PS1%\\$ }\$(maws_profile)\\$ "
+    elif [[ $PS1 == *'$ ' ]]; then
+        # prompt ends with hard-coded '$ '
+        # if the original prompt surrounds the final '$' with whitespace,
+        # we surround the substitution with whitespace to maintain that.
+        [[ $PS1 == *' $ ' ]] && MAWS_PROMPT_PREFIX="" MAWS_PROMPT_SUFFIX=" "
+        # inject our substitution before the original '$ '
        PS1="${PS1%\$ }\$(maws_profile)\$ "
-    elif [ "${PS1% }" != "${PS1}" ]; then
-        PS1="${PS1% }\$(maws_profile) "
+    elif [[ $PS1 == *'%# ' ]]; then
+        # prompt ends with dynamic '%# '
+        # if the original prompt surrounds the final '$' with whitespace,
+        # we only suffix bot not prefix with whitespace to maintain that.
+        [[ $PS1 == *' %# ' ]] && MAWS_PROMPT_PREFIX="" MAWS_PROMPT_SUFFIX=" "
+        # inject our substitution before the original '%# '
+        PS1="${PS1%\%# }\$(maws_profile)%# "
    else
+        # we're the last entry in the prompt, so we don't need extra whitespace.
+        # if the original prompt ends with whitespace,
+        # we don't need to prefix whitespace ourselves.
+        [[ $PS1 == *' ' ]] && MAWS_PROMPT_PREFIX=""
+        # inject our substitution before the original '%# '


What is it about how git does this for bash and ZSH that differs from how we're doing it such that they don't encounter this whitespace challenge?

Git's default instructions in 3a) only support the default bash prompt case, which does not have whitespace between the CWD and the $/# character terminator; however, the macOS default zsh prompt has the extra whitespace by default, and so in that scenario the git 3a) instructions will encounter the same problem; the instructions provided in 3b) would require significant rewriting beyond just offering whitespace tuneable variables, and given their pre/post model, I'm not certain it would solve the problem, as they're using the parameters in a string-join manner rather than a "do you wish to prepend/append a space". So I went for the low-hanging fruit of "make it work the same for everyone who never cares about this, while making it possible to fix this if you do", without taking the full 3b) path.

floatingatoll added 6 commits June 15, 2021 11:06

prompt: add extra newlines to simplify code management

8aaac0e

prompt: switch to bash-native conditionals

42abad5

prompt: enable modifying prefix/suffix whitespace

c7e4ce5

prompt: enable dynamic prompt evaluation for zsh

908bffb

prompt: add MAWS_PROMPT_DISABLE to skip injection

be6dad0

prompt: fix double-whitespace characters in certain prompts

c5db7db

floatingatoll force-pushed the prompt branch from fc89da2 to ce94102 Compare June 15, 2021 22:34

prompt: remove default-duplicating conditions

236b5e0

floatingatoll force-pushed the prompt branch from ce94102 to 236b5e0 Compare June 15, 2021 22:36

floatingatoll marked this pull request as ready for review July 12, 2021 16:18

floatingatoll requested a review from gene1wood July 12, 2021 16:18

gene1wood reviewed Jul 12, 2021

View reviewed changes

floatingatoll marked this pull request as draft July 13, 2021 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prompt improvements, primarily for zsh users #240

Prompt improvements, primarily for zsh users #240

floatingatoll commented Jun 15, 2021

floatingatoll commented Jul 12, 2021

gene1wood Jul 12, 2021

gene1wood Jul 12, 2021

floatingatoll Jul 12, 2021 •

edited

Loading

floatingatoll Jul 12, 2021 •

edited

Loading

gene1wood Jul 12, 2021

floatingatoll Jul 12, 2021 •

edited

Loading

gene1wood Jul 12, 2021

floatingatoll Jul 12, 2021 •

edited

Loading

Prompt improvements, primarily for zsh users #240

Are you sure you want to change the base?

Prompt improvements, primarily for zsh users #240

Conversation

floatingatoll commented Jun 15, 2021

Fixed

Changed

Added

floatingatoll commented Jul 12, 2021

gene1wood Jul 12, 2021

Choose a reason for hiding this comment

gene1wood Jul 12, 2021

Choose a reason for hiding this comment

floatingatoll Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

floatingatoll Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

gene1wood Jul 12, 2021

Choose a reason for hiding this comment

floatingatoll Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

gene1wood Jul 12, 2021

Choose a reason for hiding this comment

floatingatoll Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

floatingatoll Jul 12, 2021 •

edited

Loading

floatingatoll Jul 12, 2021 •

edited

Loading

floatingatoll Jul 12, 2021 •

edited

Loading

floatingatoll Jul 12, 2021 •

edited

Loading