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

["SZEDER Gábor" <szeder@ira.uka.de>]

Hi,


On Mon, Jan 30, 2012 at 11:50:04AM -0600, Jonathan Nieder wrote:
> 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

Yeah, that's unnecessary.  I'm not sure why I did that, perhaps just
blindly followed suit of gitcomp_1(), without realizing that I don't
do any word-splitting in __gitcomp_nl() except when invoking compgen.

> before setting it to "\n"
> by mistake.

But that is deliberate, that's why it's called __gitcomp_nl(), see
a31e6262 (completion: optimize refs completion, 2011-10-15), third
paragraph.

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

I think loosing the name of $suffix would be OK, because the comment
above the function explains what the fourth parameter is about.

However, that comment also says that "If [the 4. argument is]
specified but empty, nothing is appended.", but this patch changes
this behavior, because "${4:- }" is substituted by a SP when $4 is an
empty string.  You have to drop the colon and use "${4- }" there:

$ foo=""
$ echo ,${foo:- },
, ,
$ echo ,${foo- },
,,


Best,
Gábor