Skip to content
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.

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

Fix condition where pre-exec hooks would not trigger #109

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gdestuynder
Copy link

When running a pre-exec hook/function for ls ; ls for example, pre-exec would only occur on the first ls command because the 2nd iteration is considered to be "interactive while prompt is being displayed". This is an issue if your hook sets a return value in order to stop the command from executing.

Ex of condition that this would fix:

$ preexec() { return 1; } # this should cause all commands to not run/fail
$ ls -l
$  # This failed properly
$ ls -l && ls -l
<directory listing here>
$ # The 2nd ls -l triggered, instead of calling the pre-exec hook

When running a pre-exec hook/function for `ls ; ls` for example, pre-exec would only occur  on the first `ls` command because the 2nd iteration is considered to be "interactive while prompt is being displayed". This is an issue if your hook sets a return value in order to stop the command from executing.

Ex of condition that this would fix:
```
$ preexec() { return 1; } # this should cause all commands to not run/fail
$ ls -l
$  # This failed properly
$ ls -l && ls -l
<directory listing here>
$ # The 2nd ls -l triggered, instead of calling the pre-exec hook
```
@dimo414
Copy link
Collaborator

dimo414 commented May 12, 2020

Please add some test coverage.

@gdestuynder
Copy link
Author

gdestuynder commented May 12, 2020

Note, on my system, bats 0.4.0 has unbound variables and https://github.com/rcaloras/bash-preexec/blob/master/test/bash-preexec.bats#L6 causes all tests to fail running (this makes bat return 0/success for all tests)

With bats-core 1.1.0 some of the older tests fail due to unbound variables

@gdestuynder
Copy link
Author

Added tests - mainly curious if you think that's an acceptable way to do this (but if you do, I'd like this merged in of course). I suspect all options are kinda hackish.

@gaelicWizard
Copy link
Contributor

This partially addresses #52, but perhaps a third array would be the best fix. This would allow test-every-command hooks to return false and prevent a command from running, and alsö keep intended behavior of running preexec hooks once per user-entered-command. Either way, I'd like to see this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants