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

use interactive shells and bind to work around issue in bash #506

Merged
merged 3 commits into from
Dec 31, 2024

Conversation

FlorianPommerening
Copy link
Contributor

This is a workaround to #502 where environment variables in file names are not completed. The issue is that calling compgen in a nested bash did not recognize the environment variables. It looks like this is a bug that was fixed in bash 5.3 but it still affects earlier versions (https://savannah.gnu.org/support/index.php?111125) For those earlier version, calling bind before the call to compgen forces the necessary initialization.

This works with a normal shell, but produces a warning line editing not enabled because bind is meant for interactive contexts. Using -i to turn the nested shell into an interactive shell avoids the warning. An alternative would be to pipe the warning to /dev/null instead.

@kislyuk
Copy link
Owner

kislyuk commented Sep 28, 2024

@FlorianPommerening thanks for digging into this. I like this change and we can call bind as advised by upstream. However I'd like to understand better the possible side effects of starting a bash subprocess with the -i (interactive) option. The bash manual (https://www.gnu.org/software/bash/manual/bash.html#Interactive-Shells) lists a bunch of things that seem like extras that we don't necessarily want.

If the only purpose of -i is to avoid the warning, can we just discard stderr from the shell instead of using -i?

@FlorianPommerening
Copy link
Contributor Author

Yes, I only added the -i to suppress the warning. As far as I understand, calling bind doesn't make sense in a non-interactive context, so it warns about it. However, we only use bind to initialize some hook that compgen expects, so we can ignore the warning. At least in my tests, it worked fine without -i except for generating the warning.

@kislyuk
Copy link
Owner

kislyuk commented Oct 27, 2024

@FlorianPommerening do you want to remove -i or should I? The PR otherwise looks ready to merge.

@FlorianPommerening
Copy link
Contributor Author

Whatever you prefer but I won't get around to it this week.

@FlorianPommerening
Copy link
Contributor Author

Sorry this took a while to get around to even if it was a small change but I updated the PR now.

@kislyuk kislyuk merged commit 27e57fb into kislyuk:main Dec 31, 2024
@kislyuk
Copy link
Owner

kislyuk commented Dec 31, 2024

Thanks for your work on this! Release coming up shortly.

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.

2 participants