[PATCH v4 0/4] completion: couple of cleanups

[PATCH v4 0/4] completion: couple of cleanups

[Felipe Contreras]

And an improvement for zsh.

v4:

Same as v3, but with an improved commit message with input from Thomas Rast and sending directly to Junio.

v3:

Junio: I see you already picked most of them for 'pu', but I've made further changes based on the feedback:

 * completion: be nicer with zsh
       Improved the code-style

 * completion: simplify __gitcomp*
       Fix
       Improved commit message

Felipe Contreras (4):
  completion: work around zsh option propagation bug
  completion: simplify __git_remotes
  completion: remove unused code
  completion: simplify __gitcomp*

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

-- 
1.7.9

Re: [PATCH v4 1/4] completion: work around zsh option propagation bug

[Junio C Hamano]

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

> Right now when listing commands in zsh (git <TAB><TAB>), all of them
> will show up, instead of only porcelain ones.

Jonathan's rewrite goes straight to the root cause instead, which is
another way to describe the problem.

Explaining user-visible symptoms at the beginning like you did is a good
strategy that I would want to see more contributors follow, though.

> Basically, in zsh, this:
>
>  for i in $__git_all_commands
>
> Should be:
>
>  for i in ${=__git_all_commands}
>
> Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is
> set). sh emulation should take care of that, but the subshell is messing
> up with that. So __git_list_porcelain_commands does not do any
> filtering.

Let me step back a bit and see if we are on the same page wrt the root
cause of the problem and for whom we are explaining the change.

The adaptation of the bash completion script to zsh is done by asking zsh
to obey POSIXy word splitting rules to honor $IFS that is in effect when
the words are split.  However zsh does not do a good job at it in some
cases, and your patch works it around by avoiding a construct known to be
troublesome to zsh.

Am I correct so far?  If so, especially if the first sentence of the above
paragraph is correct, then how would it help others to teach "this is the
right way to do a word-split if we were writing in native zsh" when we are
not?

While it probably is a good description to have in a bug report given to
zsh folks, it is useless for people who read the history of Git.  Unless
you are writing zsh-native completion script and this patch is not about
git-completion.bash, that is.

The readers need to read the solution described in order to understand why
the updated construct is written in an unnatural (to people who write to
POSIXy shells) way, or to avoid reintroducing a similar problem elsewhere
in the future.  I find that what Jonathan gave you helps them much better:

	...
        fn () {
                var='one two'
                printf '%s\n' $var
        }
        x=$(fn)
        : ${y=$(fn)}

    printing "$x" results in two lines as expected, but printing "$y" results
    in a single line because $var is expanded as a single word when evaluating
    fn to compute y.

    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
    instead.

So I'll take the first two lines of the message (good bits), and simplify
the "This fixes a bug tht caused..." from the last paragraph (or perhaps
even drop it).

Thanks.

Re: [PATCH v4 1/4] completion: work around zsh option propagation bug

[Felipe Contreras]

On Fri, Feb 3, 2012 at 10:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Right now when listing commands in zsh (git <TAB><TAB>), all of them
>> will show up, instead of only porcelain ones.
>
> Jonathan's rewrite goes straight to the root cause instead, which is
> another way to describe the problem.
>
> Explaining user-visible symptoms at the beginning like you did is a good
> strategy that I would want to see more contributors follow, though.
>
>> Basically, in zsh, this:
>>
>>  for i in $__git_all_commands
>>
>> Should be:
>>
>>  for i in ${=__git_all_commands}
>>
>> Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is
>> set). sh emulation should take care of that, but the subshell is messing
>> up with that. So __git_list_porcelain_commands does not do any
>> filtering.
>
> Let me step back a bit and see if we are on the same page wrt the root
> cause of the problem and for whom we are explaining the change.
>
> The adaptation of the bash completion script to zsh is done by asking zsh
> to obey POSIXy word splitting rules to honor $IFS that is in effect when
> the words are split.  However zsh does not do a good job at it in some
> cases, and your patch works it around by avoiding a construct known to be
> troublesome to zsh.

Troublesome to zsh emulation, yeah.

> Am I correct so far?  If so, especially if the first sentence of the above
> paragraph is correct, then how would it help others to teach "this is the
> right way to do a word-split if we were writing in native zsh" when we are
> not?

Because without that explanation it's quite difficult to know what
part of the code would behave differently in zsh, and how. Most people
are not familiar with shell features, and would have no idea what
"word splitting" means in a practical context.

> While it probably is a good description to have in a bug report given to
> zsh folks, it is useless for people who read the history of Git.

Of course it's not. It tells you that there is indeed an issue in zsh,
and not in the way we are using it, as it has been acknowledged by zsh
developers.

> The readers need to read the solution described in order to understand why
> the updated construct is written in an unnatural (to people who write to
> POSIXy shells) way, or to avoid reintroducing a similar problem elsewhere
> in the future.

Doesn't this explain that?

---
sh emulation should take care of that, but the subshell is messing
up with that.
---

Granted, for people not familiar with shell features "subshell" should
be accompanied with $(foo).

> I find that what Jonathan gave you helps them much better:
>        ...
>        fn () {
>                var='one two'
>                printf '%s\n' $var
>        }
>        x=$(fn)
>        : ${y=$(fn)}
>
>    printing "$x" results in two lines as expected, but printing "$y" results
>    in a single line because $var is expanded as a single word when evaluating
>    fn to compute y.
>
>    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
>    instead.
>
> So I'll take the first two lines of the message (good bits), and simplify
> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
> even drop it).

I'm not sure about it, because this relies on knowledge of how printf
works, and it's not used that often; an example with 'for' would be
much more clear IMO.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v4 1/4] completion: work around zsh option propagation bug

[Junio C Hamano]

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

>> I find that what Jonathan gave you helps them much better:
>>        ...
>>        fn () {
>>                var='one two'
>>                printf '%s\n' $var
>>        }
>>        x=$(fn)
>>        : ${y=$(fn)}
>>
>>    printing "$x" results in two lines as expected, but printing "$y" results
>>    in a single line because $var is expanded as a single word when evaluating
>>    fn to compute y.
>>
>>    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
>>    instead.
>>
>> So I'll take the first two lines of the message (good bits), and simplify
>> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
>> even drop it).
>
> I'm not sure about it, because this relies on knowledge of how printf
> works, and it's not used that often; an example with 'for' would be
> much more clear IMO.

Meaning, replace the fn() definition with something like:

	fn () {
		var='one two'
                for v in $var
                do
                	echo "$v"
		done
	}

I can see that may make the issue easier to see; as you pointed out, it
requires no implicit knowledge that printf "loops" over the arguments
and applies the format string as manu times as needed to eat them.
Let me update the log message before I merge it to 'next'.

My main point was to illustrate the problematic pattern for people who
write for bash, not for zsh, and that does not change with the above
improvement, though ;-).

Re: [PATCH v4 1/4] completion: work around zsh option propagation bug

[Felipe Contreras]

On Tue, Feb 7, 2012 at 1:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I find that what Jonathan gave you helps them much better:
>>>        ...
>>>        fn () {
>>>                var='one two'
>>>                printf '%s\n' $var
>>>        }
>>>        x=$(fn)
>>>        : ${y=$(fn)}
>>>
>>>    printing "$x" results in two lines as expected, but printing "$y" results
>>>    in a single line because $var is expanded as a single word when evaluating
>>>    fn to compute y.
>>>
>>>    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)'
>>>    instead.
>>>
>>> So I'll take the first two lines of the message (good bits), and simplify
>>> the "This fixes a bug tht caused..." from the last paragraph (or perhaps
>>> even drop it).
>>
>> I'm not sure about it, because this relies on knowledge of how printf
>> works, and it's not used that often; an example with 'for' would be
>> much more clear IMO.
>
> Meaning, replace the fn() definition with something like:
>
>        fn () {
>                var='one two'
>                for v in $var
>                do
>                        echo "$v"
>                done
>        }

Yes.

> I can see that may make the issue easier to see; as you pointed out, it
> requires no implicit knowledge that printf "loops" over the arguments
> and applies the format string as manu times as needed to eat them.
> Let me update the log message before I merge it to 'next'.
>
> My main point was to illustrate the problematic pattern for people who
> write for bash, not for zsh, and that does not change with the above
> improvement, though ;-).

True, but I think that's an addendum. Most likely the people working
on this script will be thinking on bash terms, and would not notice if
they introduce code that is difficult for zsh, even if it's carefully
explained in this commit message; it would be zsh people that find the
bug, thus that's why I address the zsh side _first_.

So, IMO it should look like (looks like a fix has been submitted, so
I've updated accordingly):

---
completion: work around zsh option propagation bug

Right now when listing commands in zsh (git <TAB><TAB>), all of them
will show up, instead of only porcelain ones.

This is caused by a bug in zsh[1] that causes subshells to loose the
word splitting option (SH_WORD_SPLIT) since 4.3.0[2]. It will probably
be fixed in the next release (4.3.16).

Basically, in zsh, this:

 for i in $__git_all_commands

Should be:

 for i in ${=__git_all_commands}

Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is
set). sh emulation should take care of that, but the subshell is
messing things up.

The visible result is that __git_list_porcelain_commands don't do any filtering.

Specifically, the issue is with subshells in parmeter expansion
(e.g. '${var=$(func)'):

       fn () {
               var='one two'
               for v in $var; do
                       echo "$v"
               done
       }

       x=$(fn)
       : ${y=$(fn)}

Printing "$x" results in two lines as expected, but printing "$y"
results in a single line because $var is expanded as a single word
(there's no word-splitting expansion).

So avoid the construct, and use an explicit '[ -n "$foo" ] ||
foo=$(bar)' instead.

[1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296
[2] http://article.gmane.org/gmane.comp.shells.zsh.devel/24338
---

Cheers.

-- 
Felipe Contreras

Re: [PATCH v4 4/4] completion: simplify __gitcomp*

[Junio C Hamano]

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

> @@ -495,11 +495,7 @@ fi
>  # 4: A suffix to be appended to each possible completion word (optional).
>  __gitcomp ()
>  {
> -	local cur_="$cur"
> -
> -	if [ $# -gt 2 ]; then
> -		cur_="$3"
> -	fi
> +	local cur_="${3:-$cur}"
>  	case "$cur_" in
>  	--*=)
>  		COMPREPLY=()

I think this rewrite is wrong, even though it may not make a difference to
the current callers (I didn't check).  Drop the colon from ${3:-...}.

> @@ -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}"))

So is this.

Fixing the above two gives me what I've already sent in $gmane/189683,
so...

Re: [PATCH v4 4/4] completion: simplify __gitcomp*

[SZEDER Gábor]

Hi,


On Fri, Feb 03, 2012 at 12:23:12PM -0800, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > @@ -495,11 +495,7 @@ fi
> >  # 4: A suffix to be appended to each possible completion word (optional).
> >  __gitcomp ()
> >  {
> > -	local cur_="$cur"
> > -
> > -	if [ $# -gt 2 ]; then
> > -		cur_="$3"
> > -	fi
> > +	local cur_="${3:-$cur}"
> >  	case "$cur_" in
> >  	--*=)
> >  		COMPREPLY=()
> 
> I think this rewrite is wrong, even though it may not make a difference to
> the current callers (I didn't check).  Drop the colon from ${3:-...}.
> 
> > @@ -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}"))
> 
> So is this.
> 
> Fixing the above two gives me what I've already sent in $gmane/189683,
> so...
> 

Good point, I missed this when pointed out the similar issue with $4
earlier.

And it does make a difference, it breaks the completion of a single
word in multiple steps, e.g. git log --pretty=<TAB> master..<TAB>.  In
such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third
argument to __gitcomp() and __gitcomp_nl(), which can be empty strings
when the user hits TAB right after the '=' and '..'.  Replacing that
empty string with $cur is bad, because none of the possible completion
words (i.e. $1) will match it, and bash will fall back to filename
completion.

Without the colon, i.e. using "${3-$cur}", it works as expected.


Best,
Gábor

Re: [PATCH v4 4/4] completion: simplify __gitcomp*

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> And it does make a difference, it breaks the completion of a single
> word in multiple steps, e.g. git log --pretty=<TAB> master..<TAB>.  In
> such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third
> argument to __gitcomp() and __gitcomp_nl(), which can be empty strings
> when the user hits TAB right after the '=' and '..'.

After saying "this rewrite is wrong", I was actually wondering if I should
have said "this rewrite is not faithful to the original".  Based on your
analysis, the difference does break the callers, so the rewrite is indeed
wrong.

Thanks for following up.

Re: [PATCH v4 4/4] completion: simplify __gitcomp*

[Felipe Contreras]

2012/2/5 Junio C Hamano <gitster@pobox.com>:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> And it does make a difference, it breaks the completion of a single
>> word in multiple steps, e.g. git log --pretty=<TAB> master..<TAB>.  In
>> such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third
>> argument to __gitcomp() and __gitcomp_nl(), which can be empty strings
>> when the user hits TAB right after the '=' and '..'.
>
> After saying "this rewrite is wrong", I was actually wondering if I should
> have said "this rewrite is not faithful to the original".  Based on your
> analysis, the difference does break the callers, so the rewrite is indeed
> wrong.

That's why we need tests for the completion stuff as well. I was
thinking on doing that, but if I have to write a peer-reviewed essay
with an introduction for the people that are not familiar with the
code in each of the patches, I'd rather not.

-- 
Felipe Contreras

Re: [PATCH v4 2/4] completion: simplify __git_remotes

[SZEDER Gábor]

Hi,


On Thu, Feb 02, 2012 at 10:30:23PM +0200, Felipe Contreras wrote:
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index b435b6d..f86b734 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)"

You could also remove the ngoff variable, because with this patch it's
not used anymore.

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

Re: [PATCH v4 2/4] completion: simplify __git_remotes

[Felipe Contreras]

2012/2/6 SZEDER Gábor <szeder@ira.uka.de>:
> On Thu, Feb 02, 2012 at 10:30:23PM +0200, Felipe Contreras wrote:
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index b435b6d..f86b734 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)"
>
> You could also remove the ngoff variable, because with this patch it's
> not used anymore.

Right, I thought I did that... The change must have been lost in one
of the many revisions =/

-- 
Felipe Contreras