Skip to content

Do not enable bash-preexec.sh in non-interactive shells #160

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+30 −1
Open

Do not enable bash-preexec.sh in non-interactive shells #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions bash-preexec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ if [[ -z "${BASH_VERSINFO-}" ]] || (( BASH_VERSINFO[0] < 3 || (BASH_VERSINFO[0]
return 1
fi

# We do not enable bash-preexc in non-interactive shells (except in tests).
# `__bp_inside_test` is set in test/*.bats in bash-preexec repository.
if [[ $- != *i* && ! ${__bp_inside_test-} ]]; then
Comment on lines +50 to +52
Copy link
Contributor

@jparise jparise Sep 21, 2025

Choose a reason for hiding this comment

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

Do you think we could use one of the Bats special variables instead (e.g. $BATS_VERSION)?

Copy link
Contributor Author

@akinomyoga akinomyoga Sep 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, that seems clever.

... Hmm, but wouldn't BATS_VESRION exist also for Bats tests of other Bash frameworks/programs? If another Bash framework/program sources ~/.bash_profile or ~/.bashrc, and if the user's ~/.bashrc sources bash-preexec.sh without checking whether the current Bash is interactive, I guess bash-preexec.sh would be loaded in the Bats tests of that framework.

Copy link
Contributor

@jparise jparise Sep 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. Maybe use $BATS_TEST_TAGS and tag first-party tests with a bash-preexec tag? That would also allow others to opt-in.

It wouldn't be that different from what you're doing now, but it maybe a more standard (Bats-specific) approach.

Copy link
Contributor Author

@akinomyoga akinomyoga Sep 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That seems useful. I'll later check it. Thank you!

return 0
fi

# Avoid duplicate inclusion
if [[ -n "${bash_preexec_imported:-}" || -n "${__bp_imported:-}" ]]; then
return 0
Expand Down
13 changes: 12 additions & 1 deletion test/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Testing `bash-preexec`
======================

**Note on test conditions**
### Note on test conditions

When writing test conditions, use `[ ... ]` instead of `[[ ... ]]` since the
former are supported by Bats on Bash versions before 4.1. In particular, macOS
Expand All @@ -20,3 +20,14 @@ References:
* [Differences between `[` and `[[`](http://mywiki.wooledge.org/BashFAQ/031)
* [Problems with `[[` in Bats](https://github.com/sstephenson/bats/issues/49)
* [Using `|| return 1` instead of `|| false`](https://github.com/bats-core/bats-core/commit/e5695a673faad4d4d33446ed5c99d70dbfa6d8be)


### Set variable `__bp_inside_test` to test bash-preexec

By default, bash-preexec is disabled in a non-interactive shell. However, to
test bash-preexec in non-interactive shells, one needs to enable bash-preexec
by setting variable `__bp_inside_test` to a non-empty string.

```bash
__bp_inside_test=yes
```
6 changes: 6 additions & 0 deletions test/bash-preexec.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
#!/usr/bin/env bats

# This variable tells bash-preexec.sh that it is loaded for testing purposes.
# bash-preexec.sh is intended to be used in interactive shell sessions, so it
# is disabled in non-interactive shells by default. However, it still needs to
# be loaded in non-interactive shells for the Bats tests.
__bp_inside_test=yes

setup() {
PROMPT_COMMAND='' # in case the invoking shell has set this
history -s fake command # preexec requires there be some history
Expand Down
6 changes: 6 additions & 0 deletions test/include-test.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
#!/usr/bin/env bats

# This variable tells bash-preexec.sh that it is loaded for testing purposes.
# bash-preexec.sh is intended to be used in interactive shell sessions, so it
# is disabled in non-interactive shells by default. However, it still needs to
# be loaded in non-interactive shells for the Bats tests.
__bp_inside_test=yes

@test "should not import if it's already defined" {
bash_preexec_imported="defined"
source "${BATS_TEST_DIRNAME}/../bash-preexec.sh"
Expand Down