git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] specify encoding for sed command
@ 2018-04-05  3:10 Stephon Harris
  2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
  2018-04-08 23:17 ` [PATCH] specify encoding for sed command Junio C Hamano
  0 siblings, 2 replies; 22+ messages in thread
From: Stephon Harris @ 2018-04-05  3:10 UTC (permalink / raw)
  To: git

Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b09c8a23626b4..52a4ab5e2165a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -282,7 +282,7 @@ __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
+unset $(set |LANG=C sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null
 
 # This function is equivalent to
 #

--
https://github.com/git/git/pull/481

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-05  3:10 [PATCH] specify encoding for sed command Stephon Harris
@ 2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
  2018-04-05  9:42   ` Eric Sunshine
                     ` (2 more replies)
  2018-04-08 23:17 ` [PATCH] specify encoding for sed command Junio C Hamano
  1 sibling, 3 replies; 22+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-04-05  6:53 UTC (permalink / raw)
  To: Stephon Harris; +Cc: git, Duy Nguyen, SZEDER Gábor


On Thu, Apr 05 2018, Stephon Harris wrote:

> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index b09c8a23626b4..52a4ab5e2165a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -282,7 +282,7 @@ __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
> +unset $(set |LANG=C sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null

This is getting closer to the issue than your previous patch, but
there's still some open questions:

1) What platform OS / version / sed version is this on?

2) What's the output from "set" that's causing this error? Do we have an
   isolated test case for that?

3) There's other invocations of "sed" in the file, aren't those affected
   as well?

4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we
   already do it for our invocation to "git merge".

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
@ 2018-04-05  9:42   ` Eric Sunshine
  2018-04-05  9:43   ` SZEDER Gábor
  2018-04-10  7:18   ` Matt Coleman
  2 siblings, 0 replies; 22+ messages in thread
From: Eric Sunshine @ 2018-04-05  9:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Stephon Harris, Git List, Duy Nguyen, SZEDER Gábor

On Thu, Apr 5, 2018 at 2:53 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Apr 05 2018, Stephon Harris wrote:
>> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash
>
> This is getting closer to the issue than your previous patch, but
> there's still some open questions:
>
> 1) What platform OS / version / sed version is this on?
>
> 2) What's the output from "set" that's causing this error? Do we have an
>    isolated test case for that?
>
> 3) There's other invocations of "sed" in the file, aren't those affected
>    as well?
>
> 4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we
>    already do it for our invocation to "git merge".

Also, missing Signed-off-by: (see Documentation/SubmittingPatches).

Thanks.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
  2018-04-05  9:42   ` Eric Sunshine
@ 2018-04-05  9:43   ` SZEDER Gábor
  2018-04-10  7:18   ` Matt Coleman
  2 siblings, 0 replies; 22+ messages in thread
From: SZEDER Gábor @ 2018-04-05  9:43 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Stephon Harris, Git mailing list, Duy Nguyen

On Thu, Apr 5, 2018 at 8:53 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Apr 05 2018, Stephon Harris wrote:
>
>> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash

Please:
  - prefix the subject line with "completion:".
  - nit: replace "running" with "sourcing".
  - wrap the body of the commit message around 70 characters.
  - sign off your patch; see Documentation/SubmittingPatches and 'git
    commit -s'.

>> ---
>>  contrib/completion/git-completion.bash | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index b09c8a23626b4..52a4ab5e2165a 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -282,7 +282,7 @@ __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
>> +unset $(set |LANG=C sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null

I was wondering how could you see an error message from 'sed', when the
stderr of the whole thing is redirected to /dev/null.  It turns out that
such a redirection doesn't affect the stderr of the commands executed
inside the command substitution:

  $ echo $(echo foo ; echo >&2 error) 2>/dev/null
  error
  foo

Interesting, I didn't know that.

> This is getting closer to the issue than your previous patch, but
> there's still some open questions:
>
> 1) What platform OS / version / sed version is this on?
>
> 2) What's the output from "set" that's causing this error? Do we have an
>    isolated test case for that?

A quick internet search suggests that this error message tends to appear
on Macs, when its 'sed' encounters an invalid UTF-8 character in its
input.

> 3) There's other invocations of "sed" in the file, aren't those affected
>    as well?

The two 'sed' invocations in _git_stash() are suspect, as they process
the output of 'git stash list', which includes the stashes'
descriptions, which can contain whatever the users wished to store there
(though they would probably get a "Warning: commit message did not
conform to UTF-8" message from 9e83266525 (commit-tree: encourage UTF-8
commit messages., 2006-12-22) when doing so).

The 'sed' invocation in __git_complete_revlist_file() is suspect as
well, as it processes the output of 'git ls-tree'.  Pathnames can
contain anything, and while 'git ls-tree' quotes pathnames with
"unusual" characters, I don't know whether it quotes all characters that
can possibly upset Stephon's 'sed'.

What about 'awk' on Stephon's platform?  We already have one 'awk'
invocation in __git_match_ctag(), which we use for 'git grep <TAB>' and
'git log -L:<TAB>'.  That 'awk' script uses a regexp to match the symbol
name in a ctags file; does any programming language allow such unusual
characters in symbol names?

What about 'sed' and 'awk' scripts that don't use regexps at all?  (I
intend to add such an 'awk' script in a day or two...)

> 4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we
>    already do it for our invocation to "git merge".

That would perhaps be the safest thing to do...
But how can we set it for a whole file, when the file is not executed as
a script but sourced into the user's shell environment?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-05  3:10 [PATCH] specify encoding for sed command Stephon Harris
  2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
@ 2018-04-08 23:17 ` Junio C Hamano
  1 sibling, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2018-04-08 23:17 UTC (permalink / raw)
  To: Stephon Harris; +Cc: git

Stephon Harris <theonestep4@gmail.com> writes:

> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index b09c8a23626b4..52a4ab5e2165a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -282,7 +282,7 @@ __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
> +unset $(set |LANG=C sed -ne 's/^\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\)=.*/\1/p') 2>/dev/null

Shouldn't LC_ALL and LANG both set and exported to C, as

 (1) ancient systems understand only LANG but not LC_*; and

 (2) modern ones that understand both give precedence to LC_ALL over
     LANG?

If we were to set only one, it is probably more sensible to set
LC_ALL, I suspect, but it may be better to set both, which sends a
sign to the readers that we know what we are doing ;-)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
  2018-04-05  9:42   ` Eric Sunshine
  2018-04-05  9:43   ` SZEDER Gábor
@ 2018-04-10  7:18   ` Matt Coleman
  2018-04-11 20:42     ` Matt Coleman
  2 siblings, 1 reply; 22+ messages in thread
From: Matt Coleman @ 2018-04-10  7:18 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Stephon Harris, git, Duy Nguyen, SZEDER Gábor

> 1) What platform OS / version / sed version is this on?
I'm experiencing this on macOS Sierra (10.12.6). The issue occurs with the OS's native sed, which is FreeBSD sed so the version number is kind of ambiguous.

The error goes away if I set LANG=C or LC_ALL=C or change it to use GNU sed (installed via homebrew as gsed).

> 2) What's the output from "set" that's causing this error? Do we have an
>   isolated test case for that?
On my system, it's caused by whatever powerline is doing to Bash's PS1 variable:
$ set | grep ^PS1= | sed 's/foo/bar/'
sed: RE error: illegal byte sequence

I'm not sure exactly which character is bad here:
$ xxd <<<$PS1
00000000: 5c5b 1b5b 303b 3338 3b32 3b32 3535 3b32  \[.[0;38;2;255;2
00000010: 3535 3b32 3535 3b34 383b 323b 303b 3133  55;255;48;2;0;13
00000020: 353b 3137 353b 316d 5c5d c2a0 6d61 7474  5;175;1m\]..matt
00000030: c2a0 5c5b 1b5b 303b 3338 3b32 3b30 3b31  ..\[.[0;38;2;0;1
00000040: 3335 3b31 3735 3b34 383b 323b 3838 3b38  35;175;48;2;88;8
00000050: 383b 3838 3b32 326d 5c5d ee82 b0c2 a05c  8;88;22m\].....\
00000060: 5b1b 5b30 3b33 383b 323b 3230 383b 3230  [.[0;38;2;208;20
00000070: 383b 3230 383b 3438 3b32 3b38 383b 3838  8;208;48;2;88;88
00000080: 3b38 383b 316d 5c5d 7ec2 a05c 5b1b 5b30  ;88;1m\]~..\[.[0
00000090: 3b33 383b 323b 3133 383b 3133 383b 3133  ;38;2;138;138;13
000000a0: 383b 3438 3b32 3b38 383b 3838 3b38 383b  8;48;2;88;88;88;
000000b0: 3232 6d5c 5dee 82b1 c2a0 5c5b 1b5b 303b  22m\].....\[.[0;
000000c0: 3338 3b32 3b32 3038 3b32 3038 3b32 3038  38;2;208;208;208
000000d0: 3b34 383b 323b 3838 3b38 383b 3838 3b31  ;48;2;88;88;88;1
000000e0: 6d5c 5dc2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2  m\].............
000000f0: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2  ................
00000100: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2  ................
00000110: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2  ................
00000120: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2  ................
00000130: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a05c  ...............\
00000140: 5b1b 5b30 3b33 383b 323b 3438 3b34 383b  [.[0;38;2;48;48;
00000150: 3438 3b34 383b 323b 3838 3b38 383b 3838  48;48;2;88;88;88
00000160: 3b32 326d 5c5d c2a0 ee82 b25c 5b1b 5b30  ;22m\].....\[.[0
00000170: 3b33 383b 323b 3135 383b 3135 383b 3135  ;38;2;158;158;15
00000180: 383b 3438 3b32 3b34 383b 3438 3b34 386d  8;48;2;48;48;48m
00000190: 5c5d c2a0 3230 3138 2d30 342d 3130 5c5b  \]..2018-04-10\[
000001a0: 1b5b 303b 3338 3b32 3b39 383b 3938 3b39  .[0;38;2;98;98;9
000001b0: 383b 3438 3b32 3b34 383b 3438 3b34 383b  8;48;2;48;48;48;
000001c0: 3232 6d5c 5dc2 a0ee 82b3 5c5b 1b5b 303b  22m\].....\[.[0;
000001d0: 3338 3b32 3b32 3038 3b32 3038 3b32 3038  38;2;208;208;208
000001e0: 3b34 383b 323b 3438 3b34 383b 3438 3b31  ;48;2;48;48;48;1
000001f0: 6d5c 5dc2 a030 333a 3136 c2a0 5c5b 1b5b  m\]..03:16..\[.[
00000200: 306d 5c5d 205c 5b1b 5b30 3b33 383b 323b  0m\] \[.[0;38;2;
00000210: 3135 383b 3135 383b 3135 383b 3438 3b32  158;158;158;48;2
00000220: 3b34 383b 3438 3b34 386d 5c5d c2a0 c2a0  ;48;48;48m\]....
00000230: 5c5b 1b5b 303b 3338 3b32 3b34 383b 3438  \[.[0;38;2;48;48
00000240: 3b34 383b 3439 3b32 326d 5c5d ee82 b0c2  ;48;49;22m\]....
00000250: a05c 5b1b 5b30 6d5c 5d0a                 .\[.[0m\].

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-10  7:18   ` Matt Coleman
@ 2018-04-11 20:42     ` Matt Coleman
  2018-04-12 22:12       ` Matthew Coleman
  0 siblings, 1 reply; 22+ messages in thread
From: Matt Coleman @ 2018-04-11 20:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Stephon Harris, git, Duy Nguyen, SZEDER Gábor

I found another (possibly better) way to fix this:

> On Apr 10, 2018, at 3:18 AM, Matt Coleman <cat.moleman@gmail.com> wrote:
> 
>> 1) What platform OS / version / sed version is this on?
> I'm experiencing this on macOS Sierra (10.12.6). The issue occurs with the OS's native sed, which is FreeBSD sed so the version number is kind of ambiguous.
> 
> The error goes away if I set LANG=C or LC_ALL=C or change it to use GNU sed (installed via homebrew as gsed).

If I change it to use awk instead of sed, it works with mawk, gawk, and macOS awk:
unset $(set | awk -F '=' '/^__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*=/ {print $1}') 2>/dev/null

I compared sed vs. awk on Linux and Mac and they all take about the same amount of time to run (within 0.002ms).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-11 20:42     ` Matt Coleman
@ 2018-04-12 22:12       ` Matthew Coleman
  2018-04-13  0:01         ` SZEDER Gábor
  0 siblings, 1 reply; 22+ messages in thread
From: Matthew Coleman @ 2018-04-12 22:12 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Stephon Harris, git, Duy Nguyen, SZEDER Gábor

I did a little more digging into this issue today.

> On Apr 11, 2018, at 4:42 PM, Matt Coleman <cat.moleman@gmail.com> wrote:
> 
> I found another (possibly better) way to fix this:
> 
>> On Apr 10, 2018, at 3:18 AM, Matt Coleman <cat.moleman@gmail.com> wrote:
>> 
>>> 1) What platform OS / version / sed version is this on?
>> I'm experiencing this on macOS Sierra (10.12.6). The issue occurs with the OS's native sed, which is FreeBSD sed so the version number is kind of ambiguous.
>> 
>> The error goes away if I set LANG=C or LC_ALL=C or change it to use GNU sed (installed via homebrew as gsed).
> 
> If I change it to use awk instead of sed, it works with mawk, gawk, and macOS awk:
> unset $(set | awk -F '=' '/^__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*=/ {print $1}') 2>/dev/null
> 
> I compared sed vs. awk on Linux and Mac and they all take about the same amount of time to run (within 0.002ms).

The issue isn't actually caused by `sed`: it's caused by the way that the `set` builtin outputs characters in the Unicode Private Use Area (PUA) in the build of Bash 3.2.57 that macOS Sierra ships with.

Powerline uses several PUA code points to make some of its pretty text UI elements:

Code point (hex value): description
U+E0A0 (0xEE82A0): Version control branch
U+E0A1 (0xEE82A1): LN (line) symbol
U+E0A2 (0xEE82A2): Closed padlock
U+E0B0 (0xEE82B0): Rightwards black arrowhead
U+E0B1 (0xEE82B1): Rightwards arrowhead
U+E0B2 (0xEE82B2): Leftwards black arrowhead
U+E0B3 (0xEE82B3): Leftwards arrowhead

macOS Bash 3.2.57's `set` builtin has garbled output where Powerline's special symbols should be in the PS1 variable, but Bash 4.4.19 (installed on macOS via homebrew) and Bash 4.3.38 (Ubuntu 16.04) both display it correctly in the output of `set`. `echo $PS1` does display the symbols correctly on these versions of Bash.

So...

> 3) There's other invocations of "sed" in the file, aren't those affected as well?
The short answer: no. Slightly longer: not by the same thing that's affecting the line in the patch, at least.

Long: As described above, the problem isn't actually `sed`: it's the `set` builtin in macOS's build of Bash. The other invocations of `sed` should be safe, because `sed` properly handles the PUA code points on its own:

$ echo $'\xee\x82\xb0' | sed 's/./@/'
@

The way that `set` is displaying the PS1 variable seems to be sending them to `sed` individually or somehow split up:

$ for character in $'\xee' $'\x82' $'\xb0' $'\xee\x82' $'\x82\xb0'; do echo $character | sed 's/./@/'; done
sed: RE error: illegal byte sequence
sed: RE error: illegal byte sequence
sed: RE error: illegal byte sequence
sed: RE error: illegal byte sequence
sed: RE error: illegal byte sequence

Interestingly, Bash 3.2.25's `set` builtin on CentOS 5 correctly displays the octal values for the symbols (prompt edited to keep this email ASCII):

$ PS1=$'\xee\x82\xb0 '
>  set | grep PS1
PS1=$'\356\202\260 '

I haven't started digging through Bash's codebase, but it could either be a bug that was introduced between 3.2.25 and 3.2.57 or Apple used some silly flags when compiling Bash. Ideally, this should be fixed in Bash, but since Apple's using such an old version of Bash for license reasons, I think it's unlikely that they'll fix the issue.

I think the best way to move forward with this is a new patch that uses `awk` instead of `sed`: I tested several `awk` variants and the command was portable without requiring any changes to LANG or LC_ALL.

Does that sound like a good plan?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-12 22:12       ` Matthew Coleman
@ 2018-04-13  0:01         ` SZEDER Gábor
  2018-04-13  3:00           ` Matthew Coleman
  0 siblings, 1 reply; 22+ messages in thread
From: SZEDER Gábor @ 2018-04-13  0:01 UTC (permalink / raw)
  To: Matthew Coleman; +Cc: SZEDER Gábor, Stephon Harris, git, Duy Nguyen


> I did a little more digging into this issue today.

> The issue isn't actually caused by `sed`: it's caused by the way that
> the `set` builtin outputs characters in the Unicode Private Use Area
> (PUA) in the build of Bash 3.2.57 that macOS Sierra ships with.
> 
> Powerline uses several PUA code points to make some of its pretty text
> UI elements:
> 
> Code point (hex value): description
> U+E0A0 (0xEE82A0): Version control branch
> U+E0A1 (0xEE82A1): LN (line) symbol
> U+E0A2 (0xEE82A2): Closed padlock
> U+E0B0 (0xEE82B0): Rightwards black arrowhead
> U+E0B1 (0xEE82B1): Rightwards arrowhead
> U+E0B2 (0xEE82B2): Leftwards black arrowhead
> U+E0B3 (0xEE82B3): Leftwards arrowhead
> 
> macOS Bash 3.2.57's `set` builtin has garbled output where Powerline's
> special symbols should be in the PS1 variable, but Bash 4.4.19
> (installed on macOS via homebrew) and Bash 4.3.38 (Ubuntu 16.04) both
> display it correctly in the output of `set`. `echo $PS1` does display
> the symbols correctly on these versions of Bash.

Thanks for digging.

> I think the best way to move forward with this is a new patch that uses
> `awk` instead of `sed`: I tested several `awk` variants and the command
> was portable without requiring any changes to LANG or LC_ALL.
> 
> Does that sound like a good plan?

No ;)
Could you please give the patch below a try?

I intended it as a fix for a minor performance regression introduced in
the same commit that triggered this issue, but if it can work around
this issue, too, all the better.

I'm just not sure whether we should burden this otherwise nice and short
commit message with the details of this Powerline-macOS-Bash-sed issue...


  -- >8 --

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

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.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>

TODO: as an unexpected bonus, this might work around the issue with
'sed' and invalid UTF-8 characters in the environment as well, at least
in Bash.

  https://public-inbox.org/git/0102016293c8dca7-6626fcde-548d-476e-b61f-c83ecdeedfe1-000000@eu-west-1.amazonses.com/
---
 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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] specify encoding for sed command
  2018-04-13  0:01         ` SZEDER Gábor
@ 2018-04-13  3:00           ` Matthew Coleman
  2018-04-13 10:30             ` [PATCH] completion: reduce overhead of clearing cached --options SZEDER Gábor
  0 siblings, 1 reply; 22+ messages in thread
From: Matthew Coleman @ 2018-04-13  3:00 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Stephon Harris, git, Duy Nguyen

>> I think the best way to move forward with this is a new patch that uses
>> `awk` instead of `sed`: I tested several `awk` variants and the command
>> was portable without requiring any changes to LANG or LC_ALL.
>> 
>> Does that sound like a good plan?
> 
> No ;)
> Could you please give the patch below a try?
Your patch works great and is clearly a way better approach than parsing the output of `set`.

> I'm just not sure whether we should burden this otherwise nice and short
> commit message with the details of this Powerline-macOS-Bash-sed issue...
It might be worth mentioning it briefly, in order to reduce the chances of a regression in the future. Maybe something like...
"In Bash we can do better: run the 'compgen -v __gitcomp_builtin_' builtin command, which lists the same variables, but without the risk of errors caused by poor Unicode handling in some shells' 'set' builtin command or the overhead of executing 'sed'."

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-13  3:00           ` Matthew Coleman
@ 2018-04-13 10:30             ` SZEDER Gábor
  2018-04-13 21:44               ` Jakub Narebski
  0 siblings, 1 reply; 22+ messages in thread
From: SZEDER Gábor @ 2018-04-13 10:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matthew Coleman, Stephon Harris, Duy Nguyen, git, 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 <theonestep4@gmail.com>
Issue-on-macOS-explained-by: Matthew Coleman <matt@1eanda.com>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-13 10:30             ` [PATCH] completion: reduce overhead of clearing cached --options SZEDER Gábor
@ 2018-04-13 21:44               ` Jakub Narebski
  2018-04-13 22:23                 ` SZEDER Gábor
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2018-04-13 21:44 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Matthew Coleman, Stephon Harris, Duy Nguyen, git

SZEDER Gábor <szeder.dev@gmail.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-13 21:44               ` Jakub Narebski
@ 2018-04-13 22:23                 ` SZEDER Gábor
  2018-04-14 13:27                   ` Jakub Narebski
  2018-04-16  5:10                   ` Junio C Hamano
  0 siblings, 2 replies; 22+ messages in thread
From: SZEDER Gábor @ 2018-04-13 22:23 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Junio C Hamano, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> 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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-13 22:23                 ` SZEDER Gábor
@ 2018-04-14 13:27                   ` Jakub Narebski
  2018-04-16 18:23                     ` Jacob Keller
  2018-04-16  5:10                   ` Junio C Hamano
  1 sibling, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2018-04-14 13:27 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

SZEDER Gábor <szeder.dev@gmail.com> writes:
> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-13 22:23                 ` SZEDER Gábor
  2018-04-14 13:27                   ` Jakub Narebski
@ 2018-04-16  5:10                   ` Junio C Hamano
  2018-04-16 13:15                     ` SZEDER Gábor
  1 sibling, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2018-04-16  5:10 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Jakub Narebski, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> 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?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-16  5:10                   ` Junio C Hamano
@ 2018-04-16 13:15                     ` SZEDER Gábor
  2018-04-16 13:29                       ` Jakub Narębski
  0 siblings, 1 reply; 22+ messages in thread
From: SZEDER Gábor @ 2018-04-16 13:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jakub Narebski, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

On Mon, Apr 16, 2018 at 7:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> SZEDER Gábor <szeder.dev@gmail.com> 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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-16 13:15                     ` SZEDER Gábor
@ 2018-04-16 13:29                       ` Jakub Narębski
  2018-04-16 22:43                         ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Narębski @ 2018-04-16 13:29 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

On 16 April 2018 at 15:15, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Mon, Apr 16, 2018 at 7:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > SZEDER Gábor <szeder.dev@gmail.com> writes:
> >> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> >>> SZEDER Gábor <szeder.dev@gmail.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-14 13:27                   ` Jakub Narebski
@ 2018-04-16 18:23                     ` Jacob Keller
  2018-04-16 20:35                       ` Matthew Coleman
  0 siblings, 1 reply; 22+ messages in thread
From: Jacob Keller @ 2018-04-16 18:23 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: SZEDER Gábor, Junio C Hamano, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

On Sat, Apr 14, 2018 at 6:27 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> SZEDER Gábor <szeder.dev@gmail.com> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-16 18:23                     ` Jacob Keller
@ 2018-04-16 20:35                       ` Matthew Coleman
  0 siblings, 0 replies; 22+ messages in thread
From: Matthew Coleman @ 2018-04-16 20:35 UTC (permalink / raw)
  To: Jacob Keller; +Cc: Jakub Narebski, SZEDER Gábor, Junio C Hamano, Stephon Harris, Duy Nguyen, Git mailing list

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 <jacob.keller@gmail.com> wrote:
> 
> On Sat, Apr 14, 2018 at 6:27 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>>> SZEDER Gábor <szeder.dev@gmail.com> 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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] completion: reduce overhead of clearing cached --options
  2018-04-16 13:29                       ` Jakub Narębski
@ 2018-04-16 22:43                         ` Junio C Hamano
  2018-04-17 22:02                           ` [PATCH v2] " SZEDER Gábor
  0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2018-04-16 22:43 UTC (permalink / raw)
  To: Jakub Narębski; +Cc: SZEDER Gábor, Matthew Coleman, Stephon Harris, Duy Nguyen, Git mailing list

Jakub Narębski <jnareb@gmail.com> writes:

> On 16 April 2018 at 15:15, SZEDER Gábor <szeder.dev@gmail.com> 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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2] completion: reduce overhead of clearing cached --options
  2018-04-16 22:43                         ` Junio C Hamano
@ 2018-04-17 22:02                           ` " SZEDER Gábor
  2018-04-17 23:45                             ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: SZEDER Gábor @ 2018-04-17 22:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matthew Coleman, Stephon Harris, Duy Nguyen, Jakub Narębski, git, 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 necessary 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.
ZSH will still continue to run that pipeline.

This change also happens to work around an issue in the default Bash
version shipped in macOS (3.2.57), reported by users of the Powerline
shell prompt, 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 of said Bash version on macOS 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 (i.e. a
newer version on the same platform) and 3.2.25 on CentOS (i.e. a
slightly earlier version, though on a different platform) are not
affected.  ZSH in macOS (the versions shipped by default or installed
via homebrew) or on other platforms isn't affected either.

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 <theonestep4@gmail.com>
Issue-on-macOS-explained-by: Matthew Coleman <matt@1eanda.com>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

Updated the commit message to explicitly mention that ZSH is
unaffected.  The patch is the same.

 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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] completion: reduce overhead of clearing cached --options
  2018-04-17 22:02                           ` [PATCH v2] " SZEDER Gábor
@ 2018-04-17 23:45                             ` Junio C Hamano
  0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2018-04-17 23:45 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Matthew Coleman, Stephon Harris, Duy Nguyen, Jakub Narębski, git

SZEDER Gábor <szeder.dev@gmail.com> 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 necessary 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.
> ZSH will still continue to run that pipeline.
> 
> This change also happens to work around an issue in the default Bash
> ...
> Updated the commit message to explicitly mention that ZSH is
> unaffected.  The patch is the same.

Thanks.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-05  3:10 [PATCH] specify encoding for sed command Stephon Harris
2018-04-05  6:53 ` Ævar Arnfjörð Bjarmason
2018-04-05  9:42   ` Eric Sunshine
2018-04-05  9:43   ` SZEDER Gábor
2018-04-10  7:18   ` Matt Coleman
2018-04-11 20:42     ` Matt Coleman
2018-04-12 22:12       ` Matthew Coleman
2018-04-13  0:01         ` SZEDER Gábor
2018-04-13  3:00           ` Matthew Coleman
2018-04-13 10:30             ` [PATCH] completion: reduce overhead of clearing cached --options SZEDER Gábor
2018-04-13 21:44               ` Jakub Narebski
2018-04-13 22:23                 ` SZEDER Gábor
2018-04-14 13:27                   ` Jakub Narebski
2018-04-16 18:23                     ` Jacob Keller
2018-04-16 20:35                       ` Matthew Coleman
2018-04-16  5:10                   ` Junio C Hamano
2018-04-16 13:15                     ` SZEDER Gábor
2018-04-16 13:29                       ` Jakub Narębski
2018-04-16 22:43                         ` Junio C Hamano
2018-04-17 22:02                           ` [PATCH v2] " SZEDER Gábor
2018-04-17 23:45                             ` Junio C Hamano
2018-04-08 23:17 ` [PATCH] specify encoding for sed command Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox