Re: [PATCH] Showing stash state in bash prompt

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2009, 19:14 UTC
Message-ID: <7v4ovo4xyt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090513112535.GD2106@bug.science-computing.de>

Daniel Trstenjak <Daniel.Trstenjak@science-computing.de> writes:

> Showing stash state in bash prompt.

That's what you already said in the "Subject:" ;-)

Here in the proposed commit message, you describe what the code after applying your patch does in a bit more detail, defend why such a new behaviour is desirable, and defend why your implementation is superior than possible alternative solutions to achieve that goal.

Going back to the "Subject: ", we typically put the name of the affected area and colon to the message, and use imperative mood, as if you are giving an order to the code to "do this (instead of what you currently do)".

E.g.

	Subject: [PATCH] completion: show presense of stashed changes

	Users often forget that there are stashed changes that want to be
	unstashed.  Add a '$' in the prompt string to remind them.

	Signed-off-by: ...

Note that I do not necessarily agree with the above justification; I am just illustrating the established style around here.

Show 12 quoted lines

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1683e6d..86e39a5 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -117,6 +117,7 @@ __git_ps1 ()
>  
>  		local w
>  		local i
> +		local s
>  		local c
>  
>  		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then

Ok.

Show 5 quoted lines

> @@ -136,15 +137,19 @@ __git_ps1 ()
>  					else
>  						i="#"
>  					fi
> +					stash_list=`git stash list`

Don't you want to localize this variable to avoid contaminating the namespace?

> +					if [ -n "$stash_list" ]; then
> +					        s="$"

Is the presense the only thing that matters? Notice that I reworded your "stash state" to "presense of" to clarify that you are giving only one bit of information in the counter-proposed "Subject:" above.

I personally prefer your "only one bit" patch over a possible alternative to count the stash entries with "git stash list | wc -l" that would be way more costly, and if you thought about such an alternative and discarded it, it would save other people's time if you say so in your proposed commit message. So...

	Subject: [PATCH] completion: show presense of stashed changes

	Users often forget that there are stashed changes that want to be
	unstashed.  Add a '$' in the prompt string to remind them when
	there is a stashed state.

	We could count and show the number of stash entries instead, but
        that would be more costly at runtime than it is worth.

	Signed-off-by: ...

and with localization of stash_list variable I think the patch becomes better.

Thanks.