-
Notifications
You must be signed in to change notification settings - Fork 764
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
bash: prefer $HOME over tilde notation #5196
Conversation
This reverts part of ghostty-org#5075. While the ~ notation is more concise, I think the consensus is that using $HOME is more explicit, so prefer it. We've also seen reports that something about ghostty-org#5075 breaks the startup sequence in some environments. Reverting this part of the change will help us narrow that down.
I did some testing on my side. I think this is the reason for the regressions that I discovered. Only time will tell I guess after this is merged. |
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.
on line 59, there is a comment which includes a tilde. Might as well use $HOME there too
I think I'll let that one be for line length reasons. |
Great! I'm still unclear why the |
On my side, all I did was remove the tildes from the variables, reinstalled Ghostty, checked that the edited bash integration was in place, and reverted my Ghostty config and bashrc changes. Now, I could have made a mistake somewhere along the way. I guess we will see after this PR merges for more confirmation. |
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.
LGTM.
@@ -47,7 +47,7 @@ if [ -n "$GHOSTTY_BASH_INJECT" ]; then | |||
if builtin shopt -q login_shell; then | |||
if [[ $__ghostty_bash_flags != *"--noprofile"* ]]; then | |||
[ -r /etc/profile ] && builtin source "/etc/profile" | |||
for rcfile in ~/.bash_profile ~/.bash_login ~/.profile; do | |||
for rcfile in "$HOME/.bash_profile" "$HOME/.bash_login" "$HOME/.profile"; do |
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.
This has the advantage that it'll behave correctly if $HOME includes whitespace. Of course it's a niche case, but it's better to be resilient.
@@ -62,7 +62,7 @@ if [ -n "$GHOSTTY_BASH_INJECT" ]; then | |||
for rcfile in /etc/bash.bashrc /etc/bash/bashrc /etc/bashrc; do | |||
[ -r "$rcfile" ] && { builtin source "$rcfile"; break; } | |||
done | |||
if [[ -z "$GHOSTTY_BASH_RCFILE" ]]; then GHOSTTY_BASH_RCFILE=~/.bashrc; fi | |||
if [[ -z "$GHOSTTY_BASH_RCFILE" ]]; then GHOSTTY_BASH_RCFILE="$HOME/.bashrc"; fi |
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.
As I wrote in the original PR, this is iffy:
I have doubts about the expansion of the tilde here. Bash does expand ~ in the middle, but pure sh does not:
[zbyszek@x1c ghostty]$ sh -c 'echo VAR=~'
VAR=~
[zbyszek@x1c ghostty]$ bash -c 'echo VAR=~'
VAR=/home/zbyszek
@jparise I tested these changes locally and it didn't resolve the issue I'm seeing with the bashrc loading. However, after adding an echo, I did verify that your changes are correctly pointing to the correct file location. With that in mind, it seems like something else is going on than just not identifying the correct location. |
@JacobCallahan to work around, you can probably source the shell integration manually in your bashrc. That would bypass all the bash_preexec stuff. |
Closing this in favor of #5249, which includes these changes. |
@tristan957 can you elaborate? |
This reverts part of #5075. While the ~ notation is more concise, I think the consensus is that using $HOME is more explicit, so prefer it.
We've also seen reports (#5169) that something about #5075 breaks the startup sequence in some environments. Reverting this part of the change will help us narrow that down.
Fixes #5206