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

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 <redacted>
---
 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.

[...]

[...]

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