git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] Allow __git_ps1 to be used in PROMPT_COMMAND

From
Simon Oosthoek <s.oosthoek@xs4all.nl>
Date
Oct 8, 2012, 19:50 UTC
Message-ID
<50732EA1.7040608@xs4all.nl>
In-Reply-To
<7vr4p8lto5.fsf@alter.siamese.dyndns.org>
Hi Junio
thanks for your feedback!
On 08/10/12 20:12, Junio C Hamano wrote:
Show 10 quoted lines
> Simon Oosthoek <s.oosthoek@xs4all.nl> writes:
> 
>> changes __git_ps1 to not just allow use in setting PS1
>> with __git_ps1 in a command substitution, but also allows
>> __git_ps1 to be used as PROMPT_COMMAND in bash.
>> This has advantages for using color and I think it is more
>> elegant
> 
> Is "and I think" necessary?  I do not think it matters what _you_
> think when judging it is worth including in the future releases ;-)
Hmm, right :-P

How about "This has advantages for using color without running into prompt-wrapping issues. Only by assigning \[ and \] to PS1 directly can bash know these and the color codes in between don't count in the length of the prompt."?

I'll rewrite the patch later on...
Isn't it confusing that the color codes don't figure in this patch at all?
Show 14 quoted lines
>>  
>>  # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
>> +# when called from PS1 using command substitution
>> +# in this mode it returns text to add to bash PS1 prompt (includes branch name) or
>> +# __git_ps1 accepts 0 or 2 arguments when called from PROMPT_COMMAND
>> +# in that case it _sets_ PS1. The arguments are parts of a PS1 string.
>> +# when both arguments are given, the first is prepended and the second appended
>> +# to the state string when assigned to PS1, otherwise default start/end strings
>> +# are used.
> 
> Sorry, but I cannot parse this.  Is this meant to be a two-item list,
> one describing the command substitution mode (zero or 1 arguments) and
> the other describing the prompt command mode?  If so, perhaps replacing
> the " or" at the end of the first item with ".\n#\n" would make it readable.
I agree, that would make it more readable.
Show 20 quoted lines
> 
>>  __git_ps1 ()
>>  {
>> +	local pcmode=yes
>> +	local ps1pc_start='\u@\h:\w '
>> +	local ps1pc_end='\$ '
>> +
>> +	case "$PROMPT_COMMAND" in
>> +		__git_ps1*)
>> +			if [ $# = 2 ]; then
>> +				ps1pc_start="$1"
>> +				ps1pc_end="$2"
>> +			fi
>> +			case "$PS1" in
>> +			*__git_ps1*)
>> +				echo '__git_ps1: overwriting PS1 due to PROMPT_COMMAND'
> 
> Is this supposed to be an error and/or warning message?  Why is it
> worth warning only when you are overwriting __git_ps1 style of PS1
> and not other user customization?

That's what this is doing, I wasn't able to make it the other way around. I thought that I could detect when PS1 contained __git_ps1 that it shouldn't overwrite the PS1, which worked, but when PROMPT_COMMAND was set with __git_ps1 and PS1=$(__git_ps1) as well, it gave double output. As PROMPT_COMMAND is relatively obscure I thought a warning when overwriting the PS1 was good, but not unless there was a real conflict (double call to __git_ps1). The warning is only shown once.

I don't think it would be possible to detect any other kind of customization without including specific code for it. Do you think it's unnecessary to include a warning? (I think it would take people a long time to figure out why their prompt goes whoopsy without getting a hint that PROMPT_COMMAND messes up the PS1)

(OTOH, if you configure PROMPT_COMMAND, you're bound to know a little bit about what you're doing...)

Perhaps it's better to just do it.
Show 9 quoted lines
> 
>> +			;;
>> +			esac
>> +		;;
>> +		*)  pcmode=no ;; #no output
>> +	esac
> 
> Please align outer "case" "its arms)" and "esac" at the same column,
> like you did for the inner "case/esac".
ok
Show 8 quoted lines
> 
> Auto-detetction based on PROMPT_COMMAND is a flaky approach.  In
> practice, nobody will call PROMPT_COMMAND with the __git_ps1 without
> any parameter (100% people want their prompt to end with some sort
> of whitespace so they want the "what comes after what is computed",
> aka $2), and even more importantly, nobody has been relying on use
> of 0 argument form of __git_ps1 in PROMPT_COMMAND.  So why not
> always require 2 args and take that as a cue to go into the pc mode?
That's a good idea, I'll change that.
Show 8 quoted lines
>> +		if [ $pcmode = yes ]; then
>> +			PS1="$ps1pc_start($c${b##refs/heads/}${f:+ $f}$r$p)$ps1pc_end"
>> +		elif [ $pcmode = no ]; then
>> +			printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> +		fi
> 
> Are there $pcmode other than yes or no?  Why not just "else",
> instead of performing the test twice?
I forgot to remove that one, I had a default at some point ;-)
Cheers
Simon.
Previous: Junio C HamanoNext: Junio C Hamano
Message 27 of 49 in “bash completion with colour hints”
  1. Simon OosthoekSep 26, 2012
  2. Ramkumar RamachandraSep 26, 2012
  3. Simon OosthoekSep 26, 2012
  4. Junio C HamanoSep 27, 2012
  5. Michael J GruberSep 27, 2012
  6. Michael J GruberSep 27, 2012
  7. Simon OosthoekSep 27, 2012
  8. Andreas SchwabSep 27, 2012
  9. Simon OosthoekSep 27, 2012
  10. Add __git_ps1_pc to use as PROMPT_COMMANDSimon Oosthoek, Sep 28, 2012
  11. Junio C HamanoSep 28, 2012
  12. Simon OosthoekOct 1, 2012
  13. Junio C HamanoOct 1, 2012
  14. Simon OosthoekOct 1, 2012
  15. Junio C HamanoOct 1, 2012
  16. Simon OosthoekOct 1, 2012
  17. Junio C HamanoOct 1, 2012
  18. Simon OosthoekOct 1, 2012
  19. Junio C HamanoOct 1, 2012
  20. Michael J GruberOct 2, 2012
  21. Simon OosthoekOct 2, 2012
  22. Junio C HamanoOct 2, 2012
  23. Simon OosthoekOct 2, 2012
  24. Junio C HamanoOct 2, 2012
  25. 1/2 Allow __git_ps1 to be used in PROMPT_COMMANDSimon Oosthoek, Oct 5, 2012
  26. Junio C HamanoOct 8, 2012
  27. Simon OosthoekOct 8, 2012
  28. Junio C HamanoOct 8, 2012
  29. 1/2 Allow __git_ps1 to be used in PROMPT_COMMANDSimon Oosthoek, Oct 10, 2012
  30. Junio C HamanoOct 10, 2012
  31. 2/2 show color hints based on state of the git treeSimon Oosthoek, Oct 10, 2012
  32. 1/2 Allow __git_ps1 to be used in PROMPT_COMMANDSimon Oosthoek, Oct 10, 2012
  33. 2/2 show color hints based on state of the git treeSimon Oosthoek, Oct 10, 2012
  34. 2/2 show color hints based on state of the git treeSimon Oosthoek, Oct 5, 2012
  35. Michael J GruberOct 15, 2012
  36. Simon OosthoekOct 15, 2012
  37. Michael J GruberOct 15, 2012
  38. Simon OosthoekOct 15, 2012
  39. Simon OosthoekOct 15, 2012
  40. Michael J GruberOct 15, 2012
  41. Simon OosthoekOct 8, 2012
  42. Simon OosthoekOct 15, 2012
  43. Junio C HamanoOct 15, 2012
  44. 3/3 Change colors to be based on git status -sb in color modeSimon Oosthoek, Oct 16, 2012
  45. Junio C HamanoOct 16, 2012
  46. 3/3 Change colors to be based on git status -sb in color modeSimon Oosthoek, Oct 16, 2012
  47. Junio C HamanoOct 16, 2012
  48. Junio C HamanoOct 16, 2012
  49. Simon OosthoekOct 17, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.