-
-
Notifications
You must be signed in to change notification settings - Fork 761
fix(lib/shopt): Only enable noclobber in interactive shells #732
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(lib/shopt): Only enable noclobber in interactive shells #732
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||
PR Code Suggestions ✨No code suggestions found for the PR. |
The noclobber option is designed for interactive use to prevent accidental file overwrites by the user. Scripts and non-interactive environments do not need this protection and may actually break when noclobber is enabled. For example, JetBrains IDEs fail to load shell environment variables because their environment reader uses `>` redirection to a temp file, which fails when noclobber is set. Fixes #453
d4fdc52 to
d1f8629
Compare
|
Oh My Bash checks if it is currently in an interactive session so that the entire framework is not loaded in a non-interactive session: Lines 8 to 11 in 63ebf65
In this context, I'm not sure whether #453 actually talks about the behavior in non-interactive scripts; it may talk about the issue caused by interactive scripts. |
|
Thanks for the review, @akinomyoga! You're absolutely right to question this. After researching further, I found the root cause in JetBrains' documentation: Shell Environment Loading The key insight: JetBrains IDEs load shell environments by running You're correct that my current fix ( Better approaches would be:
I'll update my PR with a corrected fix. Would you prefer:
I'm leaning toward the first option as it's more universal. What do you think? |
|
To be honest, this should be fixed at JetBrain's side to use the |
|
So, I guess this line is responsible for the redirection. This line should be updated to use |
|
You can submit a pull request (or an issue) to https://github.com/JetBrains/intellij-community. After you submit it, could you leave a reference to it here? Thank you. edit: It seems they accept reports and patches in youtrack.jetbrains.com. |
|
|
Thank you! |
User description
Summary
The
noclobberoption is designed for interactive use to prevent accidental file overwrites by the user. Scripts and non-interactive environments do not need this protection and may actually break whennoclobberis enabled.For example, JetBrains IDEs fail to load shell environment variables because their environment reader uses
>redirection to a temp file, which fails whennoclobberis set.Changes
set -o noclobberin acase $-check to only enable it in interactive shells (*i*)Test Plan
noclobberis still enabled in interactive bash sessionsFixes #453
PR Type
Bug fix
Description
Wrap
noclobberoption in interactive shell checkPrevents script failures in non-interactive environments
Fixes JetBrains IDE environment variable loading issues
Diagram Walkthrough
flowchart LR A["noclobber setting"] --> B{"Interactive shell?"} B -->|Yes| C["Enable noclobber"] B -->|No| D["Skip noclobber"] C --> E["Protect from accidental overwrites"] D --> F["Allow scripts to run normally"]File Walkthrough
shopt.sh
Conditionally enable noclobber for interactive shellslib/shopt.sh
set -o noclobberin acase $-conditional checkinteractive-only
i(interactive mode)readers