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

Possible regresion of issue 771 #842

Closed
edubxb opened this issue Jan 17, 2020 · 12 comments
Closed

Possible regresion of issue 771 #842

edubxb opened this issue Jan 17, 2020 · 12 comments
Labels
🐛 bug Something isn't working as expected.

Comments

@edubxb
Copy link

edubxb commented Jan 17, 2020

Bug Report

Current Behavior

Same as described in issue #771

Expected Behavior

Same as described in issue #771

Additional context/Screenshots

No error with version 0.32.2, with other newests versions, the issue occurs.

Environment

  • Starship version: 0.33.1
  • Shell type: bash
  • Shell version: GNU bash, version 5.0.11(1)-release (x86_64-pc-linux-gnu)
  • Shell plugin manager: bash-it
  • Terminal emulator: Tilix
  • Operating system: Debian Testing

Relevant Shell Configuration

https://github.com/edubxb/dotfiles/blob/master/.bashrc

Starship Configuration

https://github.com/edubxb/dotfiles/blob/master/.config/starship.toml

@edubxb edubxb added the 🐛 bug Something isn't working as expected. label Jan 17, 2020
@edubxb edubxb changed the title Possible regresion of isssue 771 Possible regresion of issue 771 Jan 17, 2020
@chipbuster
Copy link
Contributor

Sorry about being slow on the uptake here.

I'm eyeing #799 as a possible culprit here. I'd suspect that the cuplrit is a command which appends to PROMPT_COMMAND without checking to see if there's already a semicolon on the end.

Could you share the output of echo $PROMPT_COMMAND? That'll at least get us started in figuring out what's going on.

@edubxb
Copy link
Author

edubxb commented Jan 22, 2020

Hi, no worries.

My current PROMPT_COMMAND value:

_direnv_hook;__vte_prompt_command;starship_precmd;preexec_invoke_cmd

The components are:

  • _direnv_hook: added by direnv
  • __vte_prompt_command: added by the vte script from libvte Debian package
  • starship_precmd: I think you already know this 😉
  • preexec_invoke_cmd: added by Bash-it

@chipbuster
Copy link
Contributor

Huh, so I guess I'm wrong.

Can you post the exact error message you get? Does it also occur on line 8 with ``(_z --add "$(command pwd -P 2>/dev/null)" 2>/dev/null &)`, or is there a different message that gets displayed?

@edubxb
Copy link
Author

edubxb commented Jan 23, 2020

The error:

bash: PROMPT_COMMAND: line 0: syntax error near unexpected token `;;'
bash: PROMPT_COMMAND: line 0: `_direnv_hook;__vte_prompt_command;starship_precmd;;preexec_invoke_cmd'

The PROMPT_COMMAND value:

_direnv_hook;__vte_prompt_command;starship_precmd;;preexec_invoke_cmd

@chipbuster
Copy link
Contributor

Damned if we do, damned if we don't. (Thanks for telling me where the commands come from by the way--that ended up being very helpful).

This is caused by #799, which looks like it was intended to avoid this kind of problem in the first place. (See the discussion on #784 for details).

It seems that there's no solid convention for whether you throw a semicolon on the end or not.

@InNoobWeTrust Should we consider this a bug on our end or should we file something against bash-it? If it's a bug, it probably in the safe_append_prompt_command function in bash-it.

@InNoobWeTrust
Copy link
Contributor

InNoobWeTrust commented Jan 24, 2020

@InNoobWeTrust Should we consider this a bug on our end or should we file something against bash-it? If it's a bug, it probably in the safe_append_prompt_command function in bash-it.

My personal opinion is that one who mess with PROMPT_COMMAND should do proper check, so bash-it should be aware this kind of problem, like direnv and pyenv did (just my hypothesis without actually looking at the code, but they never break my system)...

BUT, there is still hope, we may fix this ourselves with a little trick!
I'm thinking about this for quite a day and right before going to bed (after Lunar New Year eve here 😛), an idea came out. It's weird but seems like it can help us at this:
(DISCLAIMER: not for the faint hearted)

❯ echo hello; nop(){ $@; unset -f nop; } && nop
hello

~
❯ echo hello; nop(){ $@; unset -f nop; } && nop echo bug
hello
bug

~
❯ nop
The program nop is not installed. Install it by executing:
 pkg install graphviz

~
❯

Practically, instead of just terminating ours, we can pull the trigger and release our alien nop command into theirs so whatever they throw at us (oh, I miss the lemons 😋) we can still be safe (and enjoy lemon juice, of course).

This can create debate and I haven't checked if it complies to POSIX but it works for bash, at least on Termux (on Android).

I can't guarantee that this is bug-free (our alien command isolate their command in a subshell), so may issues on their side arise if they rely heavily on current shell env (eg. they add 2 separate commands that are tightly coupled, but it's insane of them to do so).

What is your opinion, @chipbuster? Do we have enough courage to try this or we prefer being safe and file a bug report to bash-it?

@chipbuster
Copy link
Contributor

@InNoobWeTrust My general feel is to prefer straightforward methods over clever hacks. We don't even have to formally file a bug report (yet), we can just ask what their preferred method for handling this is. If it turns out that they don't have a workaround and refuse to fix this, we can definitely look into

like direnv and pyenv did (just my hypothesis without actually looking at the code, but they never break my system)

We haven't seen breakages, but this isn't a 100% guarantee that they won't, since those tend to be loaded earlier in the PROMPT_COMMAND order. It seems this problem pops up if there's a rogue command being loaded before something else, so commands that show up early in PROMPT_COMMAND aren't as likely to be affected.

Also, Happy New Year! 🎆

@InNoobWeTrust
Copy link
Contributor

Forgot to say, we can suggest them to try our current method in their code. Since the situation is the same with us.
safe_append_prompt_command#L526

@edubxb
Copy link
Author

edubxb commented Jan 30, 2020

Until this issue be properly fixed, I applied this simple workaround:

Adding this at the end of my .bashrc

PROMPT_COMMAND=${PROMPT_COMMAND/;;/;}

@InNoobWeTrust
Copy link
Contributor

Continue digging google for similar issues today, I found out that we can instead use \n as the separator instead of ; and everything will still work like normal.

Ref: Bug 704960 - /etc/profile.d/vte.sh overwrites PROMPT_COMMAND

For bash/zsh, $'\n' is needed for newline to be correctly interpreted

PROMPT_COMMAND=${PROMPT_COMMAND:+$PROMPT_COMMAND$'\n'}'starship_precmd'$'\n'

Midnight commander also uses this approach in their repo
src/subshell/common.c#L828

Test on Debian

echo -e "$PROMPT_COMMAND"
starship_precmd;_pyenv_virtualenv_hook;

❯ PROMPT_COMMAND=${PROMPT_COMMAND:+${PROMPT_COMMAND}$'\n'}'date'$'\n'
Thu 30 Jan 2020 03:53:22 PM +07

❯ echo -e "$PROMPT_COMMAND"
starship_precmd;_pyenv_virtualenv_hook;
date

Thu 30 Jan 2020 03:53:25 PM +07

❯ PROMPT_COMMAND=${PROMPT_COMMAND:+${PROMPT_COMMAND}$'\n'}'echo hello'$'\n'
Thu 30 Jan 2020 03:53:31 PM +07
hello

❯ echo -e "$PROMPT_COMMAND"
starship_precmd;_pyenv_virtualenv_hook;
date

echo hello

Thu 30 Jan 2020 03:53:33 PM +07
hello

How do you guys think about this? If this is good enough, I'm going to make a pull request. 😉

@edubxb
Copy link
Author

edubxb commented Jan 30, 2020

LGTM

@chipbuster
Copy link
Contributor

Problem discussion continued (and eventually resolved) on #890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working as expected.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants