Skip to content
This repository was archived by the owner on Sep 18, 2023. It is now read-only.

Prompt improvements, primarily for zsh users #240

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Prompt improvements, primarily for zsh users #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8aaac0e
prompt: add extra newlines to simplify code management
floatingatoll Jun 15, 2021
42abad5
prompt: switch to bash-native conditionals
floatingatoll Jun 15, 2021
c7e4ce5
prompt: enable modifying prefix/suffix whitespace
floatingatoll Jun 15, 2021
908bffb
prompt: enable dynamic prompt evaluation for zsh
floatingatoll Jun 15, 2021
be6dad0
prompt: add MAWS_PROMPT_DISABLE to skip injection
floatingatoll Jun 15, 2021
c5db7db
prompt: fix double-whitespace characters in certain prompts
floatingatoll Jun 15, 2021
236b5e0
prompt: remove default-duplicating conditions
floatingatoll Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### 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

### 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.)

## [1.2.3] - 2021-04-27

### Fixed
Expand Down
58 changes: 48 additions & 10 deletions mozilla_aws_cli/role_picker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,64 @@

logger = logging.getLogger(__name__)

PROMPT_BASH_CODE = r'''function maws_profile {
if [ -n "${MAWS_PROMPT}" ]; then
if [ -n "${AWS_SESSION_EXPIRATION}" ] && [ $(date +%s) -gt ${AWS_SESSION_EXPIRATION} ]; then
echo " (maws keys expired)"
PROMPT_BASH_CODE = r'''
function maws_profile {
if [[ -n $MAWS_PROMPT ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@floatingatoll floatingatoll Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@floatingatoll floatingatoll Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

# either a whitespace character or blank, depending on what
# was selected by the prompt injection routine below.
echo -n "$MAWS_PROMPT_PREFIX"
if [[ -n $AWS_SESSION_EXPIRATION && "$(date +%s)" -gt $AWS_SESSION_EXPIRATION ]]; then
echo -n "(maws keys expired)"
else
echo " (${MAWS_PROMPT})"
echo -n "(${MAWS_PROMPT})"
fi
# either a whitespace character or blank, depending on what
# followed the maws substitution point in the original prompt
echo -n "$MAWS_PROMPT_SUFFIX"
fi
}
if test "${PS1#*\$\(maws_profile\)}" = "$PS1"; then
# zsh requires this in order to evaluate the prompt dynamically like bash
[[ -n "$ZSH_VERSION" ]] && setopt prompt_subst
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to quote this variable in the conditional test

Copy link
Contributor Author

@floatingatoll floatingatoll Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 [[ ... ]].

# by default, we prefix but not suffix; good for example '\w\$',
# but has to be overridden below for various whitespace cases.
MAWS_PROMPT_PREFIX=" " MAWS_PROMPT_SUFFIX=""
# maws_profile is missing from PS1
if [ "${PS1%\$ }" != "${PS1}" ]; then
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 '%# '
Comment on lines +41 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@floatingatoll floatingatoll Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

PS1="${PS1}\$(maws_profile) "
fi
fi'''
fi
'''


def output_set_env_vars(var_map, message=None):
Expand Down