Tweak the syntax for clearing the DEBUG trap #106
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Thanks for your contributions @dimo414 👍 🍔 🍟 |
akinomyoga
added a commit
to akinomyoga/bash-preexec
that referenced
this pull request
Feb 3, 2024
This is an oversight of chaning `trap DEBUG` to `trap - DEBUG` in rcaloras#106 The oversight did not cause a test error because the test always succeeds. The problem is that even if __bp_install is broken and fails to remove `trap - DEBUG`, the current test case failed to detect the failure and produce a false positive. This patch fixes it.
akinomyoga
added a commit
to akinomyoga/bash-preexec
that referenced
this pull request
Feb 3, 2024
This is an oversight of chaning `trap DEBUG` to `trap - DEBUG` in rcaloras#106 The oversight did not cause a test error because the test always succeeds. The problem is that even if __bp_install is broken and fails to remove `trap - DEBUG`, the current test case failed to detect the failure and produce a false negative. This patch fixes it.
akinomyoga
added a commit
to akinomyoga/bash-preexec
that referenced
this pull request
Feb 3, 2024
This is an oversight of changing `trap DEBUG` to `trap - DEBUG` in rcaloras#106 The oversight did not cause a test error because the test always succeeds. The problem is that even if __bp_install is broken and fails to remove `trap - DEBUG`, the current test case failed to detect the failure and produce a false negative. This patch fixes it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In a
--posixshell (e.g. a shell invoked asshinstead ofbash) the ARG parameter is not optional:It may not be the intent of bash-preexec to actively support POSIX mode, but the failure behavior here is pretty bad:
So this small tweak seems worthwhile, even if POSIX mode ends up being incompatible for other reasons.
CC @lguelorget