-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 broken reload/restart and preexec #2066
fix broken reload/restart and preexec #2066
Conversation
Hi @gaelicWizard, seems like we merged some bogus stuff in our recent merges, wanna take a look? |
@NoahGorny, it was deliberate to remove the weird Mac-specific handling of the startup files. I expressly said so in the commit message! Bash on Mac loads startup files exactly the way it does on any other *nix. It sticks out like a sore thumb to me. If we're reloading the shell startup file, then we should reload the file that the shell started up with. By switching on
I vote for #1. I'll do a patch shortly. |
function __check_precmd_conflict() { | ||
local f | ||
__bp_trim_whitespace f "${1?}" | ||
! _bash-it-array-contains-element "${f}" "${precmd_functions[@]}" | ||
_bash-it-array-contains-element "${f}" "${precmd_functions[@]}" | ||
} | ||
|
||
function __check_preexec_conflict() { | ||
local f | ||
__bp_trim_whitespace f "${1?}" | ||
! _bash-it-array-contains-element "${f}" "${preexec_functions[@]}" | ||
_bash-it-array-contains-element "${f}" "${preexec_functions[@]}" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's entirely possible I reversed the conditional.
Should it return success if there is a conflict, or if there isn't a conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we check ! __check_preexec_conflict
when we are adding, which makes sense. If the element is in the array, there is conflict, so we should not add it. It was reversed and I fixed it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Now I need to go dig through my main branch to find out if I had fixed this and didn't pick it or if my preexec has been broken for months...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Yep, it's been broken for a couple of weeks since I added the __bp_trim_whitespace
and adopted _bash-it-array-contains-element
... 😢)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NoahGorny, can we get the preexec fix merged asap? ♥
Yap, such variable is the best solution |
…lict It goes the other way around!
cc3edde
to
056c392
Compare
I removed the part about the loading of the bashrc, but we should fix this asap as master is currently broken |
@NoahGorny, I'm curious how you found a problem with the startup file reloading. In the common case, switching on OS or type of shell should usually do the same thing. What actually happened? |
It breaks on WSL, as its a login shell and I only have bashrc installed there |
🤯 A bash login shell, which doesn't load (Many Linux distributions import EDIT: fix in #2067 |
We broke some things in our recent merges!
Description
Motivation and Context
How Has This Been Tested?
Locally
Screenshots (if appropriate):
Types of changes
Checklist:
clean_files.txt
and formatted it usinglint_clean_files.sh
.