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

Avoid overwriting existing PROMPT_COMMAND #46

Merged
merged 2 commits into from
Jul 12, 2022

Conversation

akinomyoga
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

akinomyoga commented Jul 11, 2022

@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
Copy link
Owner

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
Copy link
Contributor Author

akinomyoga commented Jul 11, 2022

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 #46 (comment) 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
Copy link
Owner

Awesome! Thanks!

@petobens petobens merged commit c57fe23 into petobens:master Jul 12, 2022
@akinomyoga akinomyoga deleted the prompt_command branch July 12, 2022 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not compatible with VTE
2 participants