bash-git-prompt icon indicating copy to clipboard operation
bash-git-prompt copied to clipboard

Use eval to get gitstatus.sh output

Open andyjack opened this issue 3 years ago • 2 comments

Modify all gitstatus scripts to output on a single line

andyjack avatar Oct 16 '22 02:10 andyjack

I updated all of the gitstatus* scripts to output the various fields on a single line. That way they could be put into the array variable via eval. The output of both gitstatus.sh and gitstatus_pre-1.7.10.sh looked ok to me.

I did update gitstatus.py to be consistent, however in my testing I found its output fields to be a little different from what gitprompt.sh expects - even without this PR - so the final prompt has incorrect values. I'm not sure if we should apply this change to the .py script. Does it get used in themes much, or only by gitprompt.fish ?

andyjack avatar Oct 16 '22 02:10 andyjack

Hm, it looks like | in the state output can mess things up, I'll look at this some more.

andyjack avatar Oct 16 '22 03:10 andyjack

I force-pushed a change that removed the edits to gitstatus.py, and used proper escaping for values output by the two .sh git status scripts.

andyjack avatar Oct 22 '22 16:10 andyjack

Somehow it seems that this change is more fragile as the read version:

function foo() { echo '1'; echo '2'; echo '3 | & ;'; echo '4'; }

# This version prints out the lines
while IFS=$'\n' read -r line; do echo "-${line}-"; done < <(foo)

# This version gets an error
eval "git_status_fields=( $(foo) )"

guenhter avatar Oct 22 '22 17:10 guenhter

@andyjack As mentioned in the other MR (maybe we move the discussion to here), I'm not convinced that we should abandon the read because of a reason we don't understand. First we should clarify why the read within a PROMT_COMMAND makes the TMOUT fail and then decide what to do.

guenhter avatar Oct 22 '22 17:10 guenhter

Re fragility: - this is why the change to gitstatus.sh escapes the output with printf '%q' "arg" - as is recommended here:

$ foo() { echo 1; echo 2; printf '%q\n' '3 | &;' ; printf '%q\n' 4; }
$ eval "test=( $(foo) )"
$ for i in "${test[@]}"; do echo "{$i}"; done
{1}
{2}
{3 | &;}
{4}

andyjack avatar Oct 22 '22 18:10 andyjack

I don't know why read in PROMPT_COMMAND makes the auto-logout behavior of TMOUT no longer work, but I can confirm it is not restricted to bash-git-prompt. I found that both git-prompt.sh from the git repo (if configured to eventually call read - I mistakenly reported that it was fine in the other pr) - as well the following minimal examples also have the problem.

$ echo "foo" > test.txt
$ cat test.sh
testfn() {
        read -r promptsays < <(cat test.txt)
        echo "p: ${promptsays}"
}

test2() {
        local output=$( cat test.txt )
        read -r promptsays <<< "$output"
        echo "p2: ${promptsays}"
}

testnofile() {
        read -r promptsays <<< "foo bar baz"
        echo "nf: ${promptsays}"
}
# then...
. test.sh

# Any of these will disable TMOUT auto-logout
PROMPT_COMMAND=testfn
PROMPT_COMMAND=test2
PROMPT_COMMAND=testnofile

For git-prompt.sh:

testuser@8d499682ebfc:~$ cd git/contrib/completion/
testuser@8d499682ebfc:~/git/contrib/completion$ export GIT_PS1_SHOWUPSTREAM=auto
testuser@8d499682ebfc:~/git/contrib/completion$ . git-prompt.sh
testuser@8d499682ebfc:~/git/contrib/completion$ PROMPT_COMMAND='__git_ps1 "\u@\h:\w" "\\\$ "'
testuser@8d499682ebfc:~/git/contrib/completion (master =)$ date
# TMOUT no longer works

This could be bug in bash. I went to the bash project page and didn't find anything relevant as either a support item or in their mailing list archive. Implementing a workaround would be better than waiting for a fix, imo - even if it was fixed, extant bash versions would still have the issue.

andyjack avatar Oct 22 '22 18:10 andyjack

I'm closing this because the behavior issue is not related to this repository but is a bigger one. @andyjack I'm advising you to maybe reach our to stackoverflow or the like for getting a workaround without the need of changing all the tools which uses read in the PROMPT_COMMAND. Furthermore this should be propagated to the bash community to get it fixed in case it is really a bug.

guenhter avatar Oct 24 '22 04:10 guenhter

@guenhter I asked about this on the bash support pages, and the reply was:

TMOUT, if set, is also the default timeout for the `read' builtin. In bash-5.1, both are implemented using alarm(3).

You can only have one alarm at a time, so the one set by the read builtin in $PROMPT_COMMAND supersedes the one used for the idle timeout.

Bash-5.2 uses a different framework for read builtin timeouts, so this will work on systems that have a working select(2), but it's not going to work in previous versions.

So the issue with TMOUT is not a bug per se, but a limit of the implementation pre-5.2.

The shared system where I was having the issue has bash 5.0 (ubuntu focal LTS), and the test docker has 5.1 (ubuntu jammy LTS). It's encouraging that there won't be a problem on systems with bash 5.2. However, I don't think LTS releases normally get minor version updates for their packages (bash or otherwise), and they will be supported for at least 3-5 years. So this issue with read in the prompt disabling auto-logout will be present for a long time, and I think that implementing a workaround in this project would still have a benefit for users.

andyjack avatar Oct 31 '22 21:10 andyjack

Very interesting.

I'd suggest to remove the need for the read (or any other variant of it) at all by changing the way how gitprompt.sh and gitstatus.sh interact. Currently the output of the gitstatus.sh is inperpreted line by line, what is very ugly and error prone.

What I have in mind is, that

  • the gitstatus.sh script should be executed but sourced
  • the gitstatus.sh sets then already all the env vars -- no array parsing any more
  • the support for gitstatus.py will be ended (I don't see the benefit of having both)

With this done, there is no need for reading a text line by line any more.

What do you think?

guenhter avatar Nov 01 '22 06:11 guenhter

Using source to get the vars from gitstatus.sh sounds like an improvement to me. There are instances of read in that script that would also have to be rewritten - source'ing from another script that read's doesn't prevent the TMOUT issue:

testuser@8d499682ebfc:~/test-prompt$ cat test.txt
foo
testuser@8d499682ebfc:~/test-prompt$ cat test2.sh
read -r promptsays < <(cat test.txt)
testuser@8d499682ebfc:~/test-prompt$ cat test3.sh
testsrc() {
	. test2.sh
	echo "src: ${promptsays}"
}
testuser@8d499682ebfc:~/test-prompt$ . test3.sh
testuser@8d499682ebfc:~/test-prompt$ PROMPT_COMMAND=testsrc
src: foo
# TMOUT auto-logout doesn't happen

andyjack avatar Nov 06 '22 18:11 andyjack

Yeah, while changing the communication I also stumbled across the read in the status script. Therefore I decided against sourcing but changed the output format of the status script so that the gitprompt.sh no longer has to parse it's stdout lines. This again makes the read obsolete in the gitprompt. I have this still on an Pull Request because the fish shell integration is not working any more and I'm tempted to drop support for that shell...

guenhter avatar Nov 07 '22 05:11 guenhter