Daniel Trstenjak <Daniel.Trstenjak@xxxxxxxxxxxxxxxxxxxx> 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.
> 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.
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html