petobens / trueline

Fast and extensible bash powerline prompt with true color and fancy icon support
MIT License
385 stars 36 forks source link

Avoid overwriting existing PROMPT_COMMAND #46

Closed akinomyoga closed 2 years ago

akinomyoga commented 2 years ago

Fixes #43

There seems to be an existing report #43 by another user, but I have also faced a problem with trueline for its behavior overwriting PROMPT_COMMAND. This PR fixes the problem.

Problem

The current setup of trueline overwrites the existing value of PROMPT_COMMAND, so the setups by other plugins sourced before trueline are all disabled.

Bash 5.1 introduced the array PROMPT_COMMAND to resolve the conflicting PROMPT_COMMAND, but trueline even completely breaks this new Bash 5.1 feature by unset PROMPT_COMMAND. With Bash 5.1+, the plugins are supposed to add its PROMPT_COMMAND by e.g. PROMPT_COMMAND+=(_plugin_specific_prompt_command) keeping the existing array elements as is. However, the current trueline removes all the other elements by unsetting the entire array by unset PROMPT_COMMAND.

Solution

With Bash 5.1+, we may just use PROMPT_COMMAND+=(_trueline_prompt_command). To protect it from some older plugins that does not support Bash 5.1+, we can optionally prepend PROMPT_COMMAND=${PROMPT_COMMAND-} as suggested by Martijn in the Bash mailing list [1].

With Bash 5.0 and below, we can prepend _trueline_prompt_command in the existing value of PROMPT_COMMAND.

petobens commented 2 years ago

Thanks for your PR. I've checked it out and I'm always seeing a "background job" indicator even if there are no bg jobs running:

image

If you fix that then I'll happily merge.

akinomyoga commented 2 years ago

@petobens Thank you for your review!

I somehow cannot reproduce the behavior. It seems to show the correct number of jobs in my environment. May I ask what is shown with the following commands in your session?

$ _trueline_test_jobs_segment() { jobs; jobs -p; } > jobs.txt
$ TRUELINE_SEGMENTS+=('test_jobs,black,red,bold')
$ cat -v jobs.txt

Also, does it happen with the plain Bash (e.g. started by bash --noprofile --norc) + trueline? If it only happens with a certain combination of configurations, could you provide me the detailed setup? I'd like to reproduce the problem in my environment if possible.

petobens commented 2 years ago

I doesn't happen with plain bash. I've added the following debug statements to the bg_jobs_segment function:

    local pid=$(jobs -p)
    echo "$pid"
    echo "$(ps -p "$pid" -o command)"

And get: image

Dunno what's going on.. I can take a closer look during the week. Any pointers are welcome.

akinomyoga commented 2 years ago

Ah, OK. I could reproduce the problem. I think you can also reproduce it with the plain Bash + trueline + the following setting:

_trueline_test_jobs_segment() { (true); jobs; }
TRUELINE_SEGMENTS+=('test_jobs,black,red,bold')

This is actually a known bug of Bash not clearing up the terminated foreground jobs in PROMPT_COMMAND, trap handlers and in bind -x handlers. The behavior you provided in https://github.com/petobens/trueline/pull/46#issuecomment-1179907113 is the characteristic behavior of this bug where the processes reported by jobs don't actually exist at that timing. I once created patches for Bash [ 91f4cc1 @ r22-fix1 akinomyoga/bash, 92315fb @ r22-fix2 akinomyoga/bash, 1e35061 @ r22-fix3 akinomyoga/bash ], created a draft for the bug report at 3d002f9 @ akinomyoga/bug-report but haven't yet submitted them to Chet. Maybe this is the time to revisit these patches and submit the report.

A workaround for this Bash bug at the script side is just run the command jobs twice and use the result of the second jobs:

_trueline_test_jobs_segment() { (true); jobs &>/dev/null; jobs; }
TRUELINE_SEGMENTS+=('test_jobs,black,red,bold')

I pushed a workaround 23fb853 for this issue. I hope this solves the problem in your environment.

petobens commented 2 years ago

Awesome! Thanks!