Re: [PATCH v2 3/4] completion: cleanup __gitcomp*

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 30, 2012, 17:50 UTC
Message-ID: <20120130175004.GG10618@burratino>
In-Reply-To: <1327944197-6379-4-git-send-email-felipec@infradead.org>

Felipe Contreras wrote:

> I don't know why there's so much code; these functions don't seem to be
> doing much:

Unless you mean "This patch has had inadequate review and I don't understand the code I'm patching, so do not trust it", please drop this commentary or place it after the three dashes.

Show 9 quoted lines

>  * no need to check $#, ${3:-$cur} is much easier
>  * __gitcomp_nl doesn't seem to using the initial IFS
>
> This makes the code much simpler.
>
> Eventually it would be nice to wrap everything that touches compgen and
> COMPREPLY in one function for the zsh wrapper.
>
> Comments by Jonathan Nieder.

I don't want this acknowledgement. Who should care that I commented on something?

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |   20 +++-----------------
>  1 files changed, 3 insertions(+), 17 deletions(-)

This diffstat tells me more of what I wanted to know about the patch than the description did.

I imagine it would have been enough to say something along the lines of "The __gitcomp and __gitcomp_nl functions are unnecessarily verbose. __gitcomp_nl sets IFS to " \t\n" unnecessarily before setting it to "\n" by mistake. Both functions use 'if' statements to read parameters with defaults, where the ${parameter:-default} idiom would be just as clear. By fixing these, we can make each function almost a one-liner."

By the way, the subject ("clean up __gitcomp*") tells me almost as little as something like "fix __gitcomp*". A person reading the shortlog would like to know _how_ you are fixing it, or what the impact of the change will be --- e.g., something like "simplify __gitcomp and __gitcomp_nl" would be clearer.

[...]

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash

[...]

Show 18 quoted lines

> @@ -524,18 +520,8 @@ __gitcomp ()
>  #    appended.
>  __gitcomp_nl ()
>  {
> -	local s=$'\n' IFS=' '$'\t'$'\n'
> -	local cur_="$cur" suffix=" "
> -
> -	if [ $# -gt 2 ]; then
> -		cur_="$3"
> -		if [ $# -gt 3 ]; then
> -			suffix="$4"
> -		fi
> -	fi
> -
> -	IFS=$s
> -	COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_"))
> +	local IFS=$'\n'
> +	COMPREPLY=($(compgen -P "${2-}" -S "${4:- }" -W "$1" -- "${3:-$cur}"))

This loses the nice name $suffix for the -S argument. Not a problem, just noticing.