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@xxxxxxxxx>
> ---
> 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.
--
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