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.

Hi,


On Mon, Jan 30, 2012 at 11:50:04AM -0600, Jonathan Nieder wrote:

Felipe Contreras wrote:

quoted

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.

quoted

 * 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?

quoted

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

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.

[...]

quoted

--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash

[...]

quoted

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

SZEDER Gábor wrote:

On Mon, Jan 30, 2012 at 11:50:04AM -0600, Jonathan Nieder wrote:

quoted

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.

quoted

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.

Yep, sorry for the ambiguity.  I meant that setting IFS to " \t\n"
(before setting it to "\n") was not done for any serious reason.
The explanation is definitely clearer with "by mistake" dropped.

Thanks,
Jonathan