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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

>  * 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
[...]
> @@ -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.