From: SZEDER Gábor <szeder@ira.uka.de>
Date: Tue, 13 Jan 2015 23:57:26 GMT
Subject: Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt
Message-ID: <20150114005726.Horde.idyLC0Or9SvaghEN_N_pRg1@webmail.informatik.kit.edu>
In-Reply-To: <alpine.LSU.2.00.1412221808110.2546@hermes-1.csi.cam.ac.uk>

Hi,

Quoting Tony Finch <dot@dotat.at>:
> If you have a prompt which displays the command exit status,
> __git_ps1 without this change corrupts it, although it has
> the correct value in the parent shell:
>
> 	~/src/git (master) 0 $ set | grep ^PS1
> 	PS1='\w$(__git_ps1) $? \$ '
> 	~/src/git (master) 0 $ false
> 	~/src/git (master) 0 $ echo $?
> 	1
> 	~/src/git (master) 0 $
>
> There is a slightly ugly workaround:
>
> 	~/src/git (master) 0 $ set | grep ^PS1
> 	PS1='\w$(x=$?; __git_ps1; exit $x) $? \$ '
> 	~/src/git (master) 0 $ false
> 	~/src/git (master) 1 $
>
> This change makes the workaround unnecessary.
>
> Signed-off-by: Tony Finch <dot@dotat.at>
> ---
>    contrib/completion/git-prompt.sh | 4 ++++
>    1 file changed, 4 insertions(+)
>
> I hope that explains it properly :-)
>
> diff --git a/contrib/completion/git-prompt.sh
> b/contrib/completion/git-prompt.sh
> index c5473dc..5fe69d0 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -288,6 +288,7 @@ __git_eread ()
>    # In this mode you can request colored hints using
> GIT_PS1_SHOWCOLORHINTS=true
>    __git_ps1 ()
>    {
> +	local exit=$?
>    	local pcmode=no
>    	local detached=no
>    	local ps1pc_start='\u@\h:\w '
> @@ -511,4 +512,7 @@ __git_ps1 ()
>    	else
>    		printf -- "$printf_format" "$gitstring"
>    	fi
> +
> +	# preserve exit status
> +	return $exit
>    }
> --
> 2.2.1.68.g56d9796

Makes sense, but the patch doesn't cover all cases, because  
__git_ps1() can exit early, off the top of my head without actually  
having a git clone at hand to look at the code, if:

  * pwd is not in a git repo, which is a quite common case to worry about.
  * .git/HEAD becomes unreadable while __git_ps1() is being executed.   
It's an unlikely race condition so I wouldn't worry much about it, but  
for consistency's sake I think it's better to return $? there as well.

Best,
Gábor