[PATCH 0/4] completion: trivial cleanups

[PATCH 0/4] completion: trivial cleanups

[Felipe Contreras]

From: Felipe Contreras <felipe.contreras@gmail.com>

And an improvement for zsh.

Felipe Contreras (4):
  completion: simplify __git_remotes
  completion: remove unused code
  completion: cleanup __gitcomp*
  completion: be nicer with zsh

 contrib/completion/git-completion.bash |   66 +++++---------------------------
 1 files changed, 10 insertions(+), 56 deletions(-)

-- 
1.7.8

[PATCH v2 2/4] completion: remove unused code

[Felipe Contreras]

From: Felipe Contreras <felipe.contreras@gmail.com>

No need for this rather complicated piece of code anymore :)

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-completion.bash |   30 ------------------------------
 1 files changed, 0 insertions(+), 30 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 086e38d..4f68f0a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2728,33 +2728,3 @@ if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
 complete -o bashdefault -o default -o nospace -F _git git.exe 2>/dev/null \
 	|| complete -o default -o nospace -F _git git.exe
 fi
-
-if [[ -n ${ZSH_VERSION-} ]]; then
-	__git_shopt () {
-		local option
-		if [ $# -ne 2 ]; then
-			echo "USAGE: $0 (-q|-s|-u) <option>" >&2
-			return 1
-		fi
-		case "$2" in
-		nullglob)
-			option="$2"
-			;;
-		*)
-			echo "$0: invalid option: $2" >&2
-			return 1
-		esac
-		case "$1" in
-		-q)	setopt | grep -q "$option" ;;
-		-u)	unsetopt "$option" ;;
-		-s)	setopt "$option" ;;
-		*)
-			echo "$0: invalid flag: $1" >&2
-			return 1
-		esac
-	}
-else
-	__git_shopt () {
-		shopt "$@"
-	}
-fi
-- 
1.7.8

Re: [PATCH v2 1/4] completion: simplify __git_remotes

[Jonathan Nieder]

Felipe Contreras wrote:

> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> There's no need for all that complicated code that requires nullglob,
> and the complexities related to such option.
>
> As an advantage, this would allow us to get rid of __git_shopt, which is
> used only in this fuction to enable 'nullglob' in zsh.

That is all a longwinded way to say "zsh doesn't support the same
interface as bash for setting the nullglob option, so let's avoid
it and use 'ls' which is simpler", right?

There's a potential speed regression involved --- using "ls" involves
an extra fork/exec.  I believe you have thought about this and done a
little to mitigate it; could you explain this in the commit message so
future coders know what features of your code need to be preserved?

Please consider squashing this with patch 2/4.  It will make both
patches way easier to understand on their own.

Cc-ing Gábor, who I imagine is more familiar with this code than I am.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/completion/git-completion.bash |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1496c6d..086e38d 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -644,12 +644,7 @@ __git_refs_remotes ()
>  __git_remotes ()
>  {
>  	local i ngoff IFS=$'\n' d="$(__gitdir)"
> -	__git_shopt -q nullglob || ngoff=1
> -	__git_shopt -s nullglob
> -	for i in "$d/remotes"/*; do
> -		echo ${i#$d/remotes/}
> -	done
> -	[ "$ngoff" ] && __git_shopt -u nullglob
> +	test -d "$d/remotes" && ls -1 "$d/remotes"
>  	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
>  		i="${i#remote.}"
>  		echo "${i/.url*/}"
> -- 
> 1.7.8
>

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

[Jonathan Nieder]

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

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Jonathan Nieder]

Felipe Contreras wrote:

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -657,7 +657,8 @@ __git_merge_strategies=
>  # is needed.
>  __git_compute_merge_strategies ()
>  {
> -	: ${__git_merge_strategies:=$(__git_list_merge_strategies)}
> +	test "$__git_merge_strategies" && return
> +	__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)

Why the new redirect?  If I add debugging output to
__git_list_merge_strategies that writes to stderr, I want to see it.

Why the 'test "$foo"' form instead of [[ -n which is more common in
this completion script?  Why use "return" instead of

	[[ -n $var ]] || var=$(...)

which feels a little simpler?

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Felipe Contreras]

On Mon, Jan 30, 2012 at 7:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -657,7 +657,8 @@ __git_merge_strategies=
>>  # is needed.
>>  __git_compute_merge_strategies ()
>>  {
>> -     : ${__git_merge_strategies:=$(__git_list_merge_strategies)}
>> +     test "$__git_merge_strategies" && return
>> +     __git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)
>
> Why the new redirect?

It's not new, it was in the original code that your change to the ':'
stuff (eaa4e6e) replaced.

And the reason is explained right above, in the comment:

# 'git merge -s help' (and thus detection of the merge strategy
# list) fails, unfortunately, if run outside of any git working
# tree.  __git_merge_strategies is set to the empty string in
# that case, and the detection will be repeated the next time it
# is needed.

The commands might fail, that's why '2> /dev/null' was used before,
and ':' is used right now.

> If I add debugging output to __git_list_merge_strategies that writes to stderr, I want to see it.

Well, you wouldn't see it right now, so that out of scope of this patch.

> Why the 'test "$foo"' form instead of [[ -n which is more common in
> this completion script?  Why use "return" instead of
>
>        [[ -n $var ]] || var=$(...)
>
> which feels a little simpler?

Because this is _huge_:

[[ "$__git_merge_strategies" ]] ||
__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)

And IMO harder to read. But you are correct that most of the code uses
[[]], which I think is a shame. But I guess people want to keep using
that.

So, how about?

[[ "$__git_merge_strategies" ]] && return
__git_merge_strategies=$(__git_list_merge_strategies 2> /dev/null)

-- 
Felipe Contreras

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Jonathan Nieder]

Felipe Contreras wrote:

> The commands might fail, that's why '2> /dev/null' was used before,
> and ':' is used right now.

Wait, what?

: is a no-op command.  It does not redirect stderr automatically or
do any other magical thing.

[...]
> And IMO harder to read. But you are correct that most of the code uses
> [[]], which I think is a shame. But I guess people want to keep using
> that.

[[ has simpler syntax wrt quoting and other details.  But now that I
check, the code uses [ a lot, too (which, like "test", is a plain
built-in command), so I suppose consistency is the only reason to
prefer one over another.  "git log --grep='if \['" tells me the use of
'[' instead of 'test' here is deliberate.

Re: [PATCH v2 1/4] completion: simplify __git_remotes

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Felipe Contreras wrote:
>
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> There's no need for all that complicated code that requires nullglob,
>> and the complexities related to such option.
>>
>> As an advantage, this would allow us to get rid of __git_shopt, which is
>> used only in this fuction to enable 'nullglob' in zsh.
>
> That is all a longwinded way to say "zsh doesn't support the same
> interface as bash for setting the nullglob option, so let's avoid
> it and use 'ls' which is simpler", right?

;-)

>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1496c6d..086e38d 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -644,12 +644,7 @@ __git_refs_remotes ()
>>  __git_remotes ()
>>  {
>>  	local i ngoff IFS=$'\n' d="$(__gitdir)"
>> -	__git_shopt -q nullglob || ngoff=1
>> -	__git_shopt -s nullglob
>> -	for i in "$d/remotes"/*; do
>> -		echo ${i#$d/remotes/}
>> -	done
>> -	[ "$ngoff" ] && __git_shopt -u nullglob
>> +	test -d "$d/remotes" && ls -1 "$d/remotes"

Yeah, very nice reduction of unnecessary code.

The original loop might have been justifiable if it were doing something
more meaningful inside (e.g. making sure the file really describes a
remote), but as far as I can tell, it merely is a poor-man's emulation of
"ls -1".

You updated it to make the code say what it wanted to say in the way it
should have said from day one ;-).

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Felipe Contreras]

On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> The commands might fail, that's why '2> /dev/null' was used before,
>> and ':' is used right now.
>
> Wait, what?
>
> : is a no-op command.  It does not redirect stderr automatically or
> do any other magical thing.

Why don't you go ahead and try it?

bash -c ': echo "err" > /dev/stderr'

I don't see anything here.

But actually, if I use $(echo "err" > /dev/stderr); _then_ I get
something. Smells a lot like a bug to me.

In any case, if you expected ':' to print errors, now I understand why
you removed 2>/dev/null in eaa4e6e.

> [...]
>> And IMO harder to read. But you are correct that most of the code uses
>> [[]], which I think is a shame. But I guess people want to keep using
>> that.
>
> [[ has simpler syntax wrt quoting and other details.  But now that I
> check, the code uses [ a lot, too (which, like "test", is a plain
> built-in command), so I suppose consistency is the only reason to
> prefer one over another.  "git log --grep='if \['" tells me the use of
> '[' instead of 'test' here is deliberate.

Maybe '[' then.

-- 
Felipe Contreras

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Jonathan Nieder]

Felipe Contreras wrote:
> On Mon, Jan 30, 2012 at 8:25 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> : is a no-op command.  It does not redirect stderr automatically or
>> do any other magical thing.
>
> Why don't you go ahead and try it?
>
> bash -c ': echo "err" > /dev/stderr'

: is a no-op command.  If you have any questions after reading about
it in your manual or online help system of choice, I'll be happy to
answer them.

[...]
> Maybe '[' then.

Honestly, I don't care. :)

(If I had to choose a convention for scripts specific to ksh-style
shells, in order of preference, I would rank them:

 1. Always use [[.
 2. Use "test", spelled out, like the portable shell code in git does.
 3. Use [.

If you have arguments for one convention or another that are
compelling enough that the codebase won't be flipping back and forth
and a patch to go along with them, I imagine no one will mind.)

By the way, since I forget to say enough: thanks for taking care about
this code.  Simpler code is definitely a good thing.

Regards,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

I love both of the above two paragraphs.  Thanks.

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

The patch looks good, including the localness that is kept for IFS.

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Felipe Contreras wrote:
>
>> The commands might fail, that's why '2> /dev/null' was used before,
>> and ':' is used right now.
>
> Wait, what?
>
> : is a no-op command.  It does not redirect stderr automatically or
> do any other magical thing.

s/no-op/true/ ;-)

> ..., so I suppose consistency is the only reason to
> prefer one over another.

Yes. And the script may probably use [[ very heavily.

Early return after || i.e.

	A || return
        B

simply looks ugly and misleading, especially when the remainder B is just
a single line.  But I stopped caring about the styles in this particular
script long time ago, so...

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Felipe Contreras]

2012/1/30 Junio C Hamano <gitster@pobox.com>:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Felipe Contreras wrote:
>> ..., so I suppose consistency is the only reason to
>> prefer one over another.
>
> Yes. And the script may probably use [[ very heavily.
>
> Early return after || i.e.
>
>        A || return
>        B
>
> simply looks ugly and misleading, especially when the remainder B is just
> a single line.  But I stopped caring about the styles in this particular
> script long time ago, so...

What would you rather use?

[ "$__git_merge_strategies" ] &&
__git_merge_strategies=$(__git_list_merge_strategies)

That's 90 characters long. Although much better without the 2>/dev/null.

if [ "$__git_merge_strategies" ]; then
  __git_merge_strategies=$(__git_list_merge_strategies)
fi

That's even more lines =/

-- 
Felipe Contreras

Re: [PATCH v2 4/4] completion: be nicer with zsh

[Jonathan Nieder]

Felipe Contreras wrote:

> What would you rather use?
>
> [ "$__git_merge_strategies" ] &&
> __git_merge_strategies=$(__git_list_merge_strategies)
>
> That's 90 characters long. Although much better without the 2>/dev/null.
>
> if [ "$__git_merge_strategies" ]; then
>   __git_merge_strategies=$(__git_list_merge_strategies)
> fi

Neither, since they both have the test inverted.  But I prefer to
explicitly use the "-n" operator.

	[[ -n $__git_merge_strategies ]] ||
	__git_merge_strategies=$(__git_list_merge_strategies)

seems as fine as any other spelling to me.  It means "either this
value has already been computed and cached, or we need to compute it".

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

[Junio C Hamano]

I managed to pick up 1 & 2:

 1. be nicer with zsh (aka avoid default value assignment on : true
    command);

 2. simplify __git_remotes (aka use ls -1 instead of rolling a loop to do
    that ourselves).

But I do not see your patch for 3 & 4 either on gmane archive nor in my
mailbox (via vger, not direct delivery from you to me). The threads for
these two patches both begin with Jonathan's review for me.

Care to resend 3 & 4?

Thanks.

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

[SZEDER Gábor]

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

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

[Jonathan Nieder]

SZEDER Gábor wrote:
> On Mon, Jan 30, 2012 at 11:50:04AM -0600, Jonathan Nieder wrote:

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

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