Re: [PATCH] Add __git_ps1_pc to use as PROMPT_COMMAND

From: Simon Oosthoek <s.oosthoek@xs4all.nl>
Date: Oct 1, 2012, 18:42 UTC
Message-ID: <5069E41E.3040809@xs4all.nl>
In-Reply-To: <7v626udse6.fsf@alter.siamese.dyndns.org>

On 01/10/12 19:16, Junio C Hamano wrote:

Show 41 quoted lines

> Simon Oosthoek <soosthoek@nieuwland.nl> writes:
> 
>> On 09/28/2012 07:58 PM, Junio C Hamano wrote:
>>> Simon Oosthoek <soosthoek@nieuwland.nl> writes:
>>>
>>>> +# __git_ps1_pc accepts 0 arguments (for now)
>>>> +# It is meant to be used as PROMPT_COMMAND, it sets PS1
>>>> +__git_ps1_pc ()
>>>> +{
>>>> +	local g="$(__gitdir)"
>>>> +	if [ -n "$g" ]; then
>>>> +...
>>>> +	fi
>>>> +}
>>>
>>> This looks awfully similar to the existing code in __git_ps1
>>> function.  Without refactoring to share the logic between them, it
>>> won't be maintainable.
>>>
>>
>> I agree that it's ugly. How about the following:
>>
>> I modified __git_ps1 to work both in PROMPT_COMMAND mode and in that
>> mode support color hints.
>>
>> This way there's one function, so no overlap.
> 
> I think the logical progression would be
> 
>  - there are parts of __git_ps1 you want to reuse for your
>    __git_ps1_pc; separate that part out as a helper function,
>    and make __git_ps1 call it, without changing what __git_ps1
>    does (i.e. no colors, etc.)
> 
>  - add __git_ps1_pc that uses the helper function you separated
>    out.
> 
>  - add whatever bells and whistles that are useful for users of
>    either __git_ps1 or __git_ps1_pc to that helper function.
> 
>

Since this is the shell, it could turn out to be ugly this way:

function ps1_common {
	set global variable (branchname, dirty state, untracked files,
divergence from upstream, etc.)
}

function __git_ps1 {
	call ps1_common
	print PS1 string based on format string or default (color in prompt
isn't an option) and include global variables where necessary
}

function __git_ps1_pc {
	call ps1_common
	set PS1=....
	based on hardcoded PS1 definition, expand the PS1 using global
variables (add color if requested)
}

The problem I see is that it would involve a lot of global variables visible in the shell (OTOH, they could be used by other scripts, but only when using this prompt ;-). Or you could print a string, which can be caught in the relevant _ps1 function, but then this string needs to be chopped up and processed (by a bunch of awk oneliners or bash-voodoo)

The latter introduces so much overhead (in processing and maintainability) that I think it should not be considered.

That leaves passing the variables from one function to another using globally scoped variables in the shell.

It can be done, but the current combined implementation has only the global variables set by the user (GIT_PS1_SHOWDIRTYSTATE and so on) and the rest remains local to the function.

The main obstacle to better code here is the need to work around the shell's oddities. :-(

Cheers

Simon