__bp_precmd_invoke_cmd saves the previous command's exit status into
__bp_last_ret_value at the top and restores it via __bp_set_ret_value
on the normal exit path. Two early-return paths bypass that restore:
1. The `(( __bp_inside_precmd > 0 )); then return` reentrancy guard
leaves $? equal to the exit status of the `(( ... ))` test, which
is always 0 when the condition is true.
2. The `return 0` inside the nested-call-frame branch (the "another
framework saved our PROMPT_COMMAND" case) explicitly clobbers $?
to 0.
Both paths run before reaching the trailing __bp_set_ret_value, so
the next command in PROMPT_COMMAND (and any direct caller of
__bp_precmd_invoke_cmd) sees $? = 0 instead of the user's actual
last exit status. Add an explicit __bp_set_ret_value call (and
replace `return 0` with a bare `return`) on both early-return paths
so $? and $_ flow through cleanly.
Includes bats coverage for both paths. The path-2 test clears
BATS_VERSION inside the nested call because the path's own guard
short-circuits when BATS_VERSION is set (so bats's own scaffolding
doesn't trip it).
If there's no PROMPT_COMMAND, we will set it to be $prolog\n$epilog.
The glob $prolog\n*\n$epilog does not match that PROMPT_COMMAND since
there's only a single newline.
Co-authored-by: Koichi Murase <myoga.murase@gmail.com>
If PROMPT_COMMAND contains unrelated commands before installing
bash-preexec, then these unrelated commands may trigger
`__bp_preexec_invoke_exec`, which causes interactive mode marker to be
unset. This causes preexec to be missed for the first interactive
command, since bash-preexec assumes that we are still not interactive.
Example of what can trigger such behavior: on Arch Linux in a login
shell /etc/profile.d/80-systemd-osc-context.sh causes
`__systemd_osc_context_precmdline` to be added to PROMPT_COMMAND, and
`__systemd_osc_context_precmdline` trips
`__bp_preexec_interactive_mode`.
This patch avoids the issue by appending our initialization hook as
the last element of PROMPT_COMMAND in Bash >= 5.1, instead of
appending the hook to the first element of PROMPT_COMMAND.
The remaining whitespaces are harmless and do not cause behavioral
differences in actual application. However, this causes differences
in the expected results of test cases in test/bash-preexec.bats. We
want to remove the whitespaces to normalize the expected results. In
particular, we treat PROMPT_COMMAND as an array based on the Bash
version, which causes the version depedenence of the expected results
with the current implementation.
* To make the expected results of test cases consistent, this patch
changes __bp_sanitize_string to remove the whitespaces.
* We also use [:blank:] instead of the variable sp=$' \t'.
We try to remove the existing hooks before the re-adjustment of
PROMPT_COMMAND, but it might not work when another framework saves the
original value of `PROMPT_COMMAND` in another variable and tries to
call it from inside their `PROMPT_COMMAND` hook. For example,
Starship does that [1]. In such a case, our hooks would be called
several times unexpectedly. To avoid this situation, we process our
hooks only when the hooks are called at the top level.
[1] 3d47468414/src/init/starship.bash (L97)
These are oversights of commit 21a26bf.
* Suppress SC2128 for array PROMPT_COMMAND
* Suppress SC2016 for the command string containing expansions like
'__bp_install "$_"'
The Copilot agent suggested to check readonly variables in a finer way
so that PS0 is not checked when PS0 is not used according to the
current version of Bash. However, this duplicates the complicated
test for the version requirement (( BASH_VERSINFO[0] > 5 ||
(BASH_VERSINFO[0] == 5 && BASH_VERSINFO[1] >= 3) )) for selecting the
preexec strategy. However, it is more manageable to store the
currently selected preexec strategy in a variable, and switch the
behavior based on the saved strategy value.
For this purpose, this patch adds a new global variable
"$__bp_hook_preexec_proc" that contains the function name to install
the hook. Then, we switch the check for readonly variables based on
the value of "$__bp_hook_preexec_proc". Now, the conditional checks
in Bats tests also use this variable to switch the tests.
* Do not prefix local varnames with underscores
* Make "__bp_invoke_pre{cmd,exec}_functions" return the last non-zero exit status
* Test "__bp_invoke_pre{cmd,exec}_functions"
* Remove dependency on sed(1) for history processing
We post-process `history 1`'s output to extract the current command.
This processing needs to strip the leading history number, an optional
`*` character indicating whether the entry was modified (or a space),
and then a space separating character.
We were previously using sed(1) for this, but we can implement an
equivalent transformation using bash's native parameter expansion
syntax.
* Improved the substitution pattern
The history number is printed as "%5d" so we need to handle all possible
columns.
declare -a lines=(
"10000 history 1"
" 1000 history 1"
" 1 history 1"
" 1000* history 1"
" 1000 history 1"
)
for line in "${lines[@]}"; do
echo "${line#*[[:digit:]][* ] }"
done
We use `trap` to bootstrap our installation function (__bp_install). We
remove our code upon first execution but need to restore any preexisting
trap calls. We previously used `sed` to process the trap string, but
that had two downsides:
1. `sed` is an external command dependency. It needs to exist on the
system, and we need to invoke it in a subshell (which has some
runtime cost).
2. The regular expression pattern was imperfect and didn't handle
trickier cases like `'` characters in the trap string:
$ (trap "echo 'hello'" DEBUG; trap -p DEBUG)
hello
trap -- 'echo '\''hello'\''' DEBUG
This change removes the dependency on `sed` by locally evaluating the
trap string and extracting any prior trap. This works reliably because
we control the format our trap string, which looks like this (with
newlines expanded):
__bp_trap_string="$(trap -p DEBUG)"
trap - DEBUG
__bp_install
* Exclude the `bind -x` commands from interactive commands
* Check COMP_POINT instead of possibly empty COMP_LINE
The check for COMP_LINE can fail because it can be empty in the
completion function for the empty command line "complete -F _func -E".
* Fix the test for the Bash version for shellcheck
* Add parenthesis and whitespace for clarity
---------
Co-authored-by: Michael Diamond <dimo414@gmail.com>
Some libraries do things like `PROMPT_COMMAND="$PROMPT_COMMAND ; _new_addition"`, that's a space-semicolon-space. It would be counter-productive to try to account for all possibilities, so just swap the install string for a no-op.
Clean up no-op, it it's all that's left
Co-authored-by: Ryan Caloras <rcaloras@gmail.com>
* Disable remaining shellcheck false warning
* Avoid passing empty argument to `return`
* Avoid word splitting of the exit status
* Follow the shellcheck suggestion
* add shellcheck # shellcheck shell=bash directive to allow linting
* BP_PIPESTATUS may be unused in function __bp_precmd_invoke_cmd, ignore
* line 174 - SC2086 (info): Double quote to prevent globbing and word splitting.
* line 249 false alarm 'SC1007 (warning): Remove space after = ', added ignore.
* line 294 SC2155 (warning): Declare and assign separately to avoid masking return values.
* line 295 SC2001 (style): See if you can use ${variable//search/replace}. Added ignore
* line 323 SC1087 (error): Use braces when expanding arrays. Disable warning
* fix indentation
* line 249 false alarm 'SC1007 (warning): empty space to '' removed shellcheck directive
Co-authored-by: Adam Price <git@blinvisible.com>
* Return original value from `__bp_precmd_invoke_cmd`
WIthout this change, `__bp_precmd_invoke_cmd` may not restore `$?` to its original value after being run.
Since `__bp_precmd_invoke_cmd` get installed as the first thing in `PROMPT_COMMAND`, this will cause problems if there is an existing `PROMPT_COMMAND` (which is perhaps not aware of `bash-preexec`) that wants to do something with the exit status of the command that was just run.
* Updating tests to allow preserving original exist status for other commands in PROMPT_COMMAND
Co-authored-by: Ryan Caloras <rcaloras@alumni.cmu.edu>
* Remove the underscore prefix of "__bp_imported"
This formally exposes it as part of the public API.
* Fix Bats test after renaming the variable
* Rename the variable upon reviewer request
* Fix inconsistency after renaming variables
Variables were reanmed in 7fbc07fb4e, but
it didn’t cover the test or ReadMe. This commit fixes that.
Since the precmd_functions and preexec_functions arrays won't be actually defined until __bp_install, which is called only during the first $PROMPT_COMMAND, declare empty arrays for these parameters.
This ensures that the shell is aware these things exist, which helps avoud unbound parameters failures in other scripts which depend upon this package.
- Return with an exit code of 1 when run in any shell other than Bash.
Should prevent any functions or variables from being defined. This check
was previously run later during initialization.
- Updated test case for this
- Should address #113
This change allows us to avoid unnecessary command substitutions which are (relatively speaking) quite expensive. Although __bp_sanitize_string is only used during installation __bp_trim_whitespace is invoked in a loop in the critial path as part of __bp_preexec_invoke_exec. Eliminating these subshells will reduce bash-preexec's overhead.
See https://github.com/dimo414/prompt.gem/commit/9764cdb3 for another example of a similar change.
Note: the printf -v syntax was added to Bash in v3.1: https://mywiki.wooledge.org/BashFAQ/061
- Need to update unit tests for echos on both cases
- Small edge case where preexec is not invoked the first time if
PROMPT_COMMAND is appended to after bash-preexec is sourced.
- Handle cases where we append with semicolon and \n
- Relaxes requirement that bash-prexec.sh needs to be included as last
thing in bash_profile.sh
- Preserves existing prompt command in PROMPT_COMMAND variable. May make
sense to still move this to a function in precmd.
- Should address #97 and #39
- Keeps existing traps preserved as preexec functions
- Removes trailing \n on PROMPT_COMMAND added for #58.
In a [`--posix` shell](https://www.gnu.org/software/bash/manual/html_node/Bash-POSIX-Mode.html) (e.g. a shell invoked as `sh` instead of `bash`) the ARG parameter is not optional:
> 41. The trap builtin doesn’t check the first argument for a possible signal specification and revert the signal handling to the original disposition if it is, unless that argument consists solely of digits and is a valid signal number. If users want to reset the handler for a given signal to the original disposition, they should use ‘-’ as the first argument.
It may not be the intent of bash-preexec to actively _support_ POSIX mode, but the failure behavior here is pretty bad:
```
$ bash --noprofile --norc --posix || echo BASH CRASHED
bash-5.0$ source bash_preexec.sh
trap: usage: trap [-lp] [[arg] signal_spec ...]
BASH CRASHED
```
So this small tweak seems worthwhile, even if POSIX mode ends up being problematic for other reasons.
