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