Re: [PATCH] completion: reduce overhead of clearing cached --options

[Junio C Hamano]

Jakub Narębski  writes:

> On 16 April 2018 at 15:15, SZEDER Gábor  wrote:
>> No.  'sed' would only need need help when its input comes from a buggy
>> 'set' builtin of a particular version of Bash from a particular vendor.
>>
>> As far as I can test this in Travis CI's OSX builds, ZSH doesn't seem to
>> be affected, neither the version Apple ships by default nor the version
>> installed via homebrew.
>
> That's nice - this means that the patch fixes all of the issue.
> The above information should be, in my opinion, included
> in the commit message, though.

Yeah, I tend to agree.  Thanks.

Re: [PATCH] completion: reduce overhead of clearing cached --options

[Matthew Coleman]

Disclaimer: I'm not a zsh user, so please correct anything I might have gotten 
wrong.

I created a .zshrc with the following contents:
autoload -Uz compinit
compinit
source 
/usr/local/lib/python3.6/site-packages/powerline/bindings/zsh/powerline.zsh

zsh doesn't have broken Unicode output in its `set` builtin, so it was not 
affected by the original issue. Applying the patch does not cause any change in 
behavior.

Since the commit only changes a file with "bash" in its name, but the 
conditional references zsh variables, I think it's worth mentioning something 
about it in the commit message.

I think the bash side of things is all set for this commit, but can a similar 
improvement be made (using a builtin instead of parsing set|sed) for zsh? 
Again, I'm not a zsh user, so some input from someone who's written zsh 
completion rules would be very helpful. Can any of the builtins mentioned in 
this zsh documentation be used instead of set|sed?
http://zsh.sourceforge.net/Doc/Release/Zsh-Modules.html#The-zsh_002fcomputil-Module

> On Apr 16, 2018, at 2:23 PM, Jacob Keller  wrote:
> 
> On Sat, Apr 14, 2018 at 6:27 AM, Jakub Narebski  wrote:
>> SZEDER Gábor  writes:
>>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
 SZEDER Gábor  writes:
> 
> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
> builtin command, which lists the same variables, but without a
> pipeline and 'sed' it can do so with lower overhead.
 
 What about ZSH?
>>> 
>>> Nothing, ZSH is unaffected by this patch.
>> 
>> All right, so for ZSH we would need LC_ALL=C trick, or come with some
>> equivalent of 'compgen -v __gitcomp_builtin_' for this shell.
>> 
>> Good patch, though it does not solve whole of the problem.
>> 
>> Best,
>> --
>> Jakub Narębski
> 
> Is ZSH actually affected by the broken set behavior, though?
> 
> Thanks,
> Jake

Re: [PATCH] completion: reduce overhead of clearing cached --options

[Jacob Keller]

On Sat, Apr 14, 2018 at 6:27 AM, Jakub Narebski  wrote:
> SZEDER Gábor  writes:
>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
>>> SZEDER Gábor  writes:

 In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
 builtin command, which lists the same variables, but without a
 pipeline and 'sed' it can do so with lower overhead.
>>>
>>> What about ZSH?
>>
>> Nothing, ZSH is unaffected by this patch.
>
> All right, so for ZSH we would need LC_ALL=C trick, or come with some
> equivalent of 'compgen -v __gitcomp_builtin_' for this shell.
>
> Good patch, though it does not solve whole of the problem.
>
> Best,
> --
> Jakub Narębski

Is ZSH actually affected by the broken set behavior, though?

Thanks,
Jake

Re: [PATCH] completion: reduce overhead of clearing cached --options

[Jakub Narębski]

On 16 April 2018 at 15:15, SZEDER Gábor  wrote:
> On Mon, Apr 16, 2018 at 7:10 AM, Junio C Hamano  wrote:
> > SZEDER Gábor  writes:
> >> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
> >>> SZEDER Gábor  writes:
> 
>  In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
>  builtin command, which lists the same variables, but without a
>  pipeline and 'sed' it can do so with lower overhead.
> >>>
> >>> What about ZSH?
> >>
> >> Nothing, ZSH is unaffected by this patch.
> >
> > OK, do we want to follow it up with [PATCH 2/1] to add the LC_ALL=C
> > thing to the ZSH side to help that "sed", then?
>
> No.  'sed' would only need need help when its input comes from a buggy
> 'set' builtin of a particular version of Bash from a particular vendor.
>
> As far as I can test this in Travis CI's OSX builds, ZSH doesn't seem to
> be affected, neither the version Apple ships by default nor the version
> installed via homebrew.

That's nice - this means that the patch fixes all of the issue.
The above information should be, in my opinion, included
in the commit message, though.

Best,
-- 
Jakub Narębski

Re: [PATCH] completion: reduce overhead of clearing cached --options

[SZEDER Gábor]

On Mon, Apr 16, 2018 at 7:10 AM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
>>> SZEDER Gábor  writes:
 In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
 builtin command, which lists the same variables, but without a
 pipeline and 'sed' it can do so with lower overhead.
>>>
>>> What about ZSH?
>>
>> Nothing, ZSH is unaffected by this patch.
>
> OK, do we want to follow it up with [PATCH 2/1] to add the LC_ALL=C
> thing to the ZSH side to help that "sed", then?

No.  'sed' would only need need help when its input comes from a buggy
'set' builtin of a particular version of Bash from a particular vendor.

As far as I can test this in Travis CI's OSX builds, ZSH doesn't seem to
be affected, neither the version Apple ships by default nor the version
installed via homebrew.

Re: [PATCH] completion: reduce overhead of clearing cached --options

[Junio C Hamano]

SZEDER Gábor  writes:

> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
>> SZEDER Gábor  writes:
>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
>>> builtin command, which lists the same variables, but without a
>>> pipeline and 'sed' it can do so with lower overhead.
>>
>> What about ZSH?
>
> Nothing, ZSH is unaffected by this patch.

OK, do we want to follow it up with [PATCH 2/1] to add the LC_ALL=C
thing to the ZSH side to help that "sed", then?

Re: [PATCH] completion: reduce overhead of clearing cached --options

[Jakub Narebski]

SZEDER Gábor  writes:
> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
>> SZEDER Gábor  writes:
>>>
>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
>>> builtin command, which lists the same variables, but without a
>>> pipeline and 'sed' it can do so with lower overhead.
>>
>> What about ZSH?
>
> Nothing, ZSH is unaffected by this patch.

All right, so for ZSH we would need LC_ALL=C trick, or come with some
equivalent of 'compgen -v __gitcomp_builtin_' for this shell.

Good patch, though it does not solve whole of the problem.

Best,
-- 
Jakub Narębski

Re: [PATCH] completion: reduce overhead of clearing cached --options

[SZEDER Gábor]

On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski  wrote:
> SZEDER Gábor  writes:
>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
>> builtin command, which lists the same variables, but without a
>> pipeline and 'sed' it can do so with lower overhead.
>
> What about ZSH?

Nothing, ZSH is unaffected by this patch.

Re: [PATCH] completion: reduce overhead of clearing cached --options

[Jakub Narebski]

SZEDER Gábor  writes:

> To get the names of all '$__git_builtin_*' variables caching --options
> of builtin commands in order to unset them, 8b0eaa41f2 (completion:
> clear cached --options when sourcing the completion script,
> 2018-03-22) runs a 'set |sed s///' pipeline.  This works both in Bash
> and in ZSH, but has a higher than necessasry overhead with the extra
> processes.

Typo: necessasry -> necessary

>
> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
> builtin command, which lists the same variables, but without a
> pipeline and 'sed' it can do so with lower overhead.

What about ZSH?

-- 
Jakub Narębski

[PATCH] completion: reduce overhead of clearing cached --options

[SZEDER Gábor]

To get the names of all '$__git_builtin_*' variables caching --options
of builtin commands in order to unset them, 8b0eaa41f2 (completion:
clear cached --options when sourcing the completion script,
2018-03-22) runs a 'set |sed s///' pipeline.  This works both in Bash
and in ZSH, but has a higher than necessasry overhead with the extra
processes.

In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'
builtin command, which lists the same variables, but without a
pipeline and 'sed' it can do so with lower overhead.

This change also happens to work around an issue reported by users of
the Powerline shell prompt on macOS, which was triggered by the same
commit 8b0eaa41f2 as well.  Powerline uses several Unicode Private Use
Area code points to represent some of its pretty text UI elements
(arrows and what not), and these are stored in the $PS1 variable.
Apparently the 'set' builtin command of the default Bash version shipped
in macOS (3.2.57) has issues with these code points, and produces
garbled output where Powerline's special symbols should be in the $PS1
variable.  This, in turn, triggers the following error message in the
downstream 'sed' process:

  sed: RE error: illegal byte sequence

Other Bash versions, notably 4.4.19 on macOS (via homebrew) and 3.2.25
on CentOS don't seem to be affected.

With this patch neither the 'set' builtin is invoked to print garbage,
nor 'sed' to choke on it.

Issue-on-macOS-reported-by: Stephon Harris 
Issue-on-macOS-explained-by: Matthew Coleman 
Signed-off-by: SZEDER Gábor 
---
 contrib/completion/git-completion.bash | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index b09c8a2362..4ef59a51be 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -282,7 +282,11 @@ __gitcomp ()
 
 # Clear the variables caching builtins' options when (re-)sourcing
 # the completion script.
-unset $(set |sed -ne 
's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
+if [[ -n ${ZSH_VERSION-} ]]; then
+   unset $(set |sed -ne 
's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
+else
+   unset $(compgen -v __gitcomp_builtin_)
+fi
 
 # This function is equivalent to
 #
-- 
2.17.0.366.gbe216a3084