Skip to content

Avoid use of regex in bash hook output#1043

Merged
zimbatm merged 1 commit intodirenv:masterfrom
scop:refactor/bash-hook-no-regex
Mar 1, 2024
Merged

Avoid use of regex in bash hook output#1043
zimbatm merged 1 commit intodirenv:masterfrom
scop:refactor/bash-hook-no-regex

Conversation

@scop
Copy link
Copy Markdown
Contributor

@scop scop commented Jan 2, 2023

Not that this would be performance critical, but in principle.

Also makes the match test more exact, rather than hitting anything with a _direnv_hook substring.

Comment thread internal/cmd/shell_bash.go Outdated
return $previous_exit_status;
};
if ! [[ "${PROMPT_COMMAND:-}" =~ _direnv_hook ]]; then
if [[ ";${PROMPT_COMMAND-};" != *";_direnv_hook;"* ]]; then
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if [[ ";${PROMPT_COMMAND-};" != *";_direnv_hook;"* ]]; then
if [[ ";${PROMPT_COMMAND-};" != *"_direnv_hook"* ]]; then

The item might be alone in the list or at the start of the list.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The added semicolons we have around the strings both on the left and right sides of the != cover those cases, too.

If it's alone there, the test becomes [[ ";_direnv_hook;" != *";_direnv_hook;"* ]] which is false as wanted; if it's the first followed by something else it becomes [[ ";_direnv_hook;something_else;" != *";_direnv_hook;"* ]] which is also false as wanted.

Removing the semicolons per the suggestion would cause undesired results e.g. if $PROMPT_COMMAND would be this_is_not_the_direnv_hook_we_are_looking_for. Theoretical, but we could just do the right thing for that, too :)

@r-vdp
Copy link
Copy Markdown

r-vdp commented Jan 3, 2023

I just wanted to make a PR for this because the current implementation also fails when you have something like this in your .bashrc: alias '!=true &&'.

This is a contrived example of course, but if we'd move the exclamation mark inside the [[ ... ]], then such a confusion can no longer happen since aliases are not resolved in that case.

If this PR gets merged, the issue also goes away, since there is no negation anymore then.

@zimbatm
Copy link
Copy Markdown
Member

zimbatm commented Feb 28, 2024

Rebased. Is it still worth merging?

@scop
Copy link
Copy Markdown
Contributor Author

scop commented Feb 29, 2024

Sure, thanks.

@zimbatm zimbatm merged commit 9fdb8c6 into direnv:master Mar 1, 2024
@zimbatm
Copy link
Copy Markdown
Member

zimbatm commented Mar 1, 2024

only took a year to get merge :-p

@scop scop deleted the refactor/bash-hook-no-regex branch March 2, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants