{"thread":{"id":"48223","subject":"[PATCH] specify encoding for sed command","startedAt":"2018-04-05T03:10:44Z","lastAt":"2018-06-12T09:51:32Z","messageCount":38,"participants":["Stephon Harris","Ævar Arnfjörð Bjarmason","Eric Sunshine","SZEDER Gábor","Junio C Hamano","Matt Coleman","Matthew Coleman","Jakub Narebski","Jakub Narębski","Jacob Keller","Todd Zullinger","Jonathan Nieder","Dave Borowitz","Rick van Hattem"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343847","messageId":"0102016293c8dca7-6626fcde-548d-476e-b61f-c83ecdeedfe1-000000@eu-west-1.amazonses.com","threadId":"48223","inReplyTo":null,"subject":"[PATCH] specify encoding for sed command","fromName":"Stephon Harris","fromEmail":"theonestep4@gmail.com","sentAt":"2018-04-05T03:10:37Z","receivedAt":"2018-04-05T03:10:44Z","isPatch":true,"sender":{"key":"theonestep4@gmail.com","avatar":null},"body":"Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash\n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b09c8a23626b4..52a4ab5e2165a 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -282,7 +282,7 @@ __gitcomp ()\n \n # Clear the variables caching builtins' options when (re-)sourcing\n # the completion script.\n-unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+unset $(set |LANG=C sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n \n # This function is equivalent to\n #\n\n--\nhttps://github.com/git/git/pull/481\n"},{"id":"343848","messageId":"87605616vr.fsf@evledraar.gmail.com","threadId":"48223","inReplyTo":"0102016293c8dca7-6626fcde-548d-476e-b61f-c83ecdeedfe1-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-05T06:53:28Z","receivedAt":"2018-04-05T06:53:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 05 2018, Stephon Harris wrote:\n\n> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash\n> ---\n> contrib/completion/git-completion.bash | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index b09c8a23626b4..52a4ab5e2165a 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -282,7 +282,7 @@ __gitcomp ()\n>\n> # Clear the variables caching builtins' options when (re-)sourcing\n> # the completion script.\n> -unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> +unset $(set |LANG=C sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n\nThis is getting closer to the issue than your previous patch, but\nthere's still some open questions:\n\n1) What platform OS / version / sed version is this on?\n\n2) What's the output from \"set\" that's causing this error? Do we have an\n isolated test case for that?\n\n3) There's other invocations of \"sed\" in the file, aren't those affected\n as well?\n\n4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we\n already do it for our invocation to \"git merge\".\n"},{"id":"343857","messageId":"CAPig+cTYCtbn6bZ0uNR-fYtPnEh3MtSv3+ssn35FmPq_n2uWPg@mail.gmail.com","threadId":"48223","inReplyTo":"87605616vr.fsf@evledraar.gmail.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-05T09:42:03Z","receivedAt":"2018-04-05T09:42:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 5, 2018 at 2:53 AM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, Apr 05 2018, Stephon Harris wrote:\n>> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash\n>\n> This is getting closer to the issue than your previous patch, but\n> there's still some open questions:\n>\n> 1) What platform OS / version / sed version is this on?\n>\n> 2) What's the output from \"set\" that's causing this error? Do we have an\n> isolated test case for that?\n>\n> 3) There's other invocations of \"sed\" in the file, aren't those affected\n> as well?\n>\n> 4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we\n> already do it for our invocation to \"git merge\".\n\nAlso, missing Signed-off-by: (see Documentation/SubmittingPatches).\n\nThanks.\n"},{"id":"343858","messageId":"CAM0VKj=45wL7PyC-9-KE0drE=0UozoPKOmKbf9-caubfC2p_TA@mail.gmail.com","threadId":"48223","inReplyTo":"87605616vr.fsf@evledraar.gmail.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-05T09:43:05Z","receivedAt":"2018-04-05T09:43:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 5, 2018 at 8:53 AM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Thu, Apr 05 2018, Stephon Harris wrote:\n>\n>> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash\n\nPlease:\n - prefix the subject line with \"completion:\".\n - nit: replace \"running\" with \"sourcing\".\n - wrap the body of the commit message around 70 characters.\n - sign off your patch; see Documentation/SubmittingPatches and 'git\n commit -s'.\n\n>> ---\n>> contrib/completion/git-completion.bash | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n>> index b09c8a23626b4..52a4ab5e2165a 100644\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -282,7 +282,7 @@ __gitcomp ()\n>>\n>> # Clear the variables caching builtins' options when (re-)sourcing\n>> # the completion script.\n>> -unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n>> +unset $(set |LANG=C sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n\nI was wondering how could you see an error message from 'sed', when the\nstderr of the whole thing is redirected to /dev/null. It turns out that\nsuch a redirection doesn't affect the stderr of the commands executed\ninside the command substitution:\n\n $ echo $(echo foo ; echo >&2 error) 2>/dev/null\n error\n foo\n\nInteresting, I didn't know that.\n\n> This is getting closer to the issue than your previous patch, but\n> there's still some open questions:\n>\n> 1) What platform OS / version / sed version is this on?\n>\n> 2) What's the output from \"set\" that's causing this error? Do we have an\n> isolated test case for that?\n\nA quick internet search suggests that this error message tends to appear\non Macs, when its 'sed' encounters an invalid UTF-8 character in its\ninput.\n\n> 3) There's other invocations of \"sed\" in the file, aren't those affected\n> as well?\n\nThe two 'sed' invocations in _git_stash() are suspect, as they process\nthe output of 'git stash list', which includes the stashes'\ndescriptions, which can contain whatever the users wished to store there\n(though they would probably get a \"Warning: commit message did not\nconform to UTF-8\" message from 9e83266525 (commit-tree: encourage UTF-8\ncommit messages., 2006-12-22) when doing so).\n\nThe 'sed' invocation in __git_complete_revlist_file() is suspect as\nwell, as it processes the output of 'git ls-tree'. Pathnames can\ncontain anything, and while 'git ls-tree' quotes pathnames with\n\"unusual\" characters, I don't know whether it quotes all characters that\ncan possibly upset Stephon's 'sed'.\n\nWhat about 'awk' on Stephon's platform? We already have one 'awk'\ninvocation in __git_match_ctag(), which we use for 'git grep ' and\n'git log -L:'. That 'awk' script uses a regexp to match the symbol\nname in a ctags file; does any programming language allow such unusual\ncharacters in symbol names?\n\nWhat about 'sed' and 'awk' scripts that don't use regexps at all? (I\nintend to add such an 'awk' script in a day or two...)\n\n> 4) Any reason we wouldn't just set LC_AlL=C for the whole file? I see we\n> already do it for our invocation to \"git merge\".\n\nThat would perhaps be the safest thing to do...\nBut how can we set it for a whole file, when the file is not executed as\na script but sourced into the user's shell environment?\n"},{"id":"344123","messageId":"xmqqd0z98ezk.fsf@gitster-ct.c.googlers.com","threadId":"48223","inReplyTo":"0102016293c8dca7-6626fcde-548d-476e-b61f-c83ecdeedfe1-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-08T23:17:51Z","receivedAt":"2018-04-08T23:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephon Harris writes:\n\n> Fixes issue with seeing `sed: RE error: illegal byte sequence` when running git-completion.bash\n> ---\n> contrib/completion/git-completion.bash | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index b09c8a23626b4..52a4ab5e2165a 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -282,7 +282,7 @@ __gitcomp ()\n> \n> # Clear the variables caching builtins' options when (re-)sourcing\n> # the completion script.\n> -unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> +unset $(set |LANG=C sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n\nShouldn't LC_ALL and LANG both set and exported to C, as\n\n (1) ancient systems understand only LANG but not LC_*; and\n\n (2) modern ones that understand both give precedence to LC_ALL over\n LANG?\n\nIf we were to set only one, it is probably more sensible to set\nLC_ALL, I suspect, but it may be better to set both, which sends a\nsign to the readers that we know what we are doing ;-)\n\n"},{"id":"344301","messageId":"DB5A1CE2-7844-4CB6-A03E-6AAC4EC13D66@gmail.com","threadId":"48223","inReplyTo":"87605616vr.fsf@evledraar.gmail.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Matt Coleman","fromEmail":"cat.moleman@gmail.com","sentAt":"2018-04-10T07:18:13Z","receivedAt":"2018-04-10T07:18:21Z","isPatch":true,"sender":{"key":"cat.moleman@gmail.com","avatar":null},"body":"> 1) What platform OS / version / sed version is this on?\nI'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.\n\nThe error goes away if I set LANG=C or LC_ALL=C or change it to use GNU sed (installed via homebrew as gsed).\n\n> 2) What's the output from \"set\" that's causing this error? Do we have an\n> isolated test case for that?\nOn my system, it's caused by whatever powerline is doing to Bash's PS1 variable:\n$ set | grep ^PS1= | sed 's/foo/bar/'\nsed: RE error: illegal byte sequence\n\nI'm not sure exactly which character is bad here:\n$ xxd <<<$PS1\n00000000: 5c5b 1b5b 303b 3338 3b32 3b32 3535 3b32 \\[.[0;38;2;255;2\n00000010: 3535 3b32 3535 3b34 383b 323b 303b 3133 55;255;48;2;0;13\n00000020: 353b 3137 353b 316d 5c5d c2a0 6d61 7474 5;175;1m\\]..matt\n00000030: c2a0 5c5b 1b5b 303b 3338 3b32 3b30 3b31 ..\\[.[0;38;2;0;1\n00000040: 3335 3b31 3735 3b34 383b 323b 3838 3b38 35;175;48;2;88;8\n00000050: 383b 3838 3b32 326d 5c5d ee82 b0c2 a05c 8;88;22m\\].....\\\n00000060: 5b1b 5b30 3b33 383b 323b 3230 383b 3230 [.[0;38;2;208;20\n00000070: 383b 3230 383b 3438 3b32 3b38 383b 3838 8;208;48;2;88;88\n00000080: 3b38 383b 316d 5c5d 7ec2 a05c 5b1b 5b30 ;88;1m\\]~..\\[.[0\n00000090: 3b33 383b 323b 3133 383b 3133 383b 3133 ;38;2;138;138;13\n000000a0: 383b 3438 3b32 3b38 383b 3838 3b38 383b 8;48;2;88;88;88;\n000000b0: 3232 6d5c 5dee 82b1 c2a0 5c5b 1b5b 303b 22m\\].....\\[.[0;\n000000c0: 3338 3b32 3b32 3038 3b32 3038 3b32 3038 38;2;208;208;208\n000000d0: 3b34 383b 323b 3838 3b38 383b 3838 3b31 ;48;2;88;88;88;1\n000000e0: 6d5c 5dc2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 m\\].............\n000000f0: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 ................\n00000100: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 ................\n00000110: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 ................\n00000120: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 ................\n00000130: a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a0c2 a05c ...............\\\n00000140: 5b1b 5b30 3b33 383b 323b 3438 3b34 383b [.[0;38;2;48;48;\n00000150: 3438 3b34 383b 323b 3838 3b38 383b 3838 48;48;2;88;88;88\n00000160: 3b32 326d 5c5d c2a0 ee82 b25c 5b1b 5b30 ;22m\\].....\\[.[0\n00000170: 3b33 383b 323b 3135 383b 3135 383b 3135 ;38;2;158;158;15\n00000180: 383b 3438 3b32 3b34 383b 3438 3b34 386d 8;48;2;48;48;48m\n00000190: 5c5d c2a0 3230 3138 2d30 342d 3130 5c5b \\]..2018-04-10\\[\n000001a0: 1b5b 303b 3338 3b32 3b39 383b 3938 3b39 .[0;38;2;98;98;9\n000001b0: 383b 3438 3b32 3b34 383b 3438 3b34 383b 8;48;2;48;48;48;\n000001c0: 3232 6d5c 5dc2 a0ee 82b3 5c5b 1b5b 303b 22m\\].....\\[.[0;\n000001d0: 3338 3b32 3b32 3038 3b32 3038 3b32 3038 38;2;208;208;208\n000001e0: 3b34 383b 323b 3438 3b34 383b 3438 3b31 ;48;2;48;48;48;1\n000001f0: 6d5c 5dc2 a030 333a 3136 c2a0 5c5b 1b5b m\\]..03:16..\\[.[\n00000200: 306d 5c5d 205c 5b1b 5b30 3b33 383b 323b 0m\\] \\[.[0;38;2;\n00000210: 3135 383b 3135 383b 3135 383b 3438 3b32 158;158;158;48;2\n00000220: 3b34 383b 3438 3b34 386d 5c5d c2a0 c2a0 ;48;48;48m\\]....\n00000230: 5c5b 1b5b 303b 3338 3b32 3b34 383b 3438 \\[.[0;38;2;48;48\n00000240: 3b34 383b 3439 3b32 326d 5c5d ee82 b0c2 ;48;49;22m\\]....\n00000250: a05c 5b1b 5b30 6d5c 5d0a .\\[.[0m\\]."},{"id":"344489","messageId":"3FE7BFB6-769A-4F11-9C3B-86D681B3502F@gmail.com","threadId":"48223","inReplyTo":"DB5A1CE2-7844-4CB6-A03E-6AAC4EC13D66@gmail.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Matt Coleman","fromEmail":"cat.moleman@gmail.com","sentAt":"2018-04-11T20:42:11Z","receivedAt":"2018-04-11T20:42:19Z","isPatch":true,"sender":{"key":"cat.moleman@gmail.com","avatar":null},"body":"I found another (possibly better) way to fix this:\n\n> On Apr 10, 2018, at 3:18 AM, Matt Coleman wrote:\n> \n>> 1) What platform OS / version / sed version is this on?\n> 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.\n> \n> 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).\n\nIf I change it to use awk instead of sed, it works with mawk, gawk, and macOS awk:\nunset $(set | awk -F '=' '/^__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*=/ {print $1}') 2>/dev/null\n\nI compared sed vs. awk on Linux and Mac and they all take about the same amount of time to run (within 0.002ms)."},{"id":"344568","messageId":"CBA1FB4A-C586-48E0-A64E-371CCD2F6AC4@1eanda.com","threadId":"48223","inReplyTo":"3FE7BFB6-769A-4F11-9C3B-86D681B3502F@gmail.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Matthew Coleman","fromEmail":"matt@1eanda.com","sentAt":"2018-04-12T22:12:12Z","receivedAt":"2018-04-12T22:20:10Z","isPatch":true,"sender":{"key":"matt@1eanda.com","avatar":"https://gravatar.com/avatar/7c91c5ac5a5a23cdb81c8d3be895de0b15aad043213568c6163e351b9ec57146?d=mp&s=160"},"body":"I did a little more digging into this issue today.\n\n> On Apr 11, 2018, at 4:42 PM, Matt Coleman wrote:\n> \n> I found another (possibly better) way to fix this:\n> \n>> On Apr 10, 2018, at 3:18 AM, Matt Coleman wrote:\n>> \n>>> 1) What platform OS / version / sed version is this on?\n>> 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.\n>> \n>> 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).\n> \n> If I change it to use awk instead of sed, it works with mawk, gawk, and macOS awk:\n> unset $(set | awk -F '=' '/^__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*=/ {print $1}') 2>/dev/null\n> \n> I compared sed vs. awk on Linux and Mac and they all take about the same amount of time to run (within 0.002ms).\n\nThe 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.\n\nPowerline uses several PUA code points to make some of its pretty text UI elements:\n\nCode point (hex value): description\nU+E0A0 (0xEE82A0): Version control branch\nU+E0A1 (0xEE82A1): LN (line) symbol\nU+E0A2 (0xEE82A2): Closed padlock\nU+E0B0 (0xEE82B0): Rightwards black arrowhead\nU+E0B1 (0xEE82B1): Rightwards arrowhead\nU+E0B2 (0xEE82B2): Leftwards black arrowhead\nU+E0B3 (0xEE82B3): Leftwards arrowhead\n\nmacOS 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.\n\nSo...\n\n> 3) There's other invocations of \"sed\" in the file, aren't those affected as well?\nThe short answer: no. Slightly longer: not by the same thing that's affecting the line in the patch, at least.\n\nLong: 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:\n\n$ echo $'\\xee\\x82\\xb0' | sed 's/./@/'\n@\n\nThe way that `set` is displaying the PS1 variable seems to be sending them to `sed` individually or somehow split up:\n\n$ for character in $'\\xee' $'\\x82' $'\\xb0' $'\\xee\\x82' $'\\x82\\xb0'; do echo $character | sed 's/./@/'; done\nsed: RE error: illegal byte sequence\nsed: RE error: illegal byte sequence\nsed: RE error: illegal byte sequence\nsed: RE error: illegal byte sequence\nsed: RE error: illegal byte sequence\n\nInterestingly, 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):\n\n$ PS1=$'\\xee\\x82\\xb0 '\n> set | grep PS1\nPS1=$'\\356\\202\\260 '\n\nI 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.\n\nI 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.\n\nDoes that sound like a good plan?\n"},{"id":"344586","messageId":"20180413000105.21708-1-szeder.dev@gmail.com","threadId":"48223","inReplyTo":"CBA1FB4A-C586-48E0-A64E-371CCD2F6AC4@1eanda.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-13T00:01:05Z","receivedAt":"2018-04-13T00:01:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> I did a little more digging into this issue today.\n\n> The issue isn't actually caused by `sed`: it's caused by the way that\n> the `set` builtin outputs characters in the Unicode Private Use Area\n> (PUA) in the build of Bash 3.2.57 that macOS Sierra ships with.\n> \n> Powerline uses several PUA code points to make some of its pretty text\n> UI elements:\n> \n> Code point (hex value): description\n> U+E0A0 (0xEE82A0): Version control branch\n> U+E0A1 (0xEE82A1): LN (line) symbol\n> U+E0A2 (0xEE82A2): Closed padlock\n> U+E0B0 (0xEE82B0): Rightwards black arrowhead\n> U+E0B1 (0xEE82B1): Rightwards arrowhead\n> U+E0B2 (0xEE82B2): Leftwards black arrowhead\n> U+E0B3 (0xEE82B3): Leftwards arrowhead\n> \n> macOS Bash 3.2.57's `set` builtin has garbled output where Powerline's\n> special symbols should be in the PS1 variable, but Bash 4.4.19\n> (installed on macOS via homebrew) and Bash 4.3.38 (Ubuntu 16.04) both\n> display it correctly in the output of `set`. `echo $PS1` does display\n> the symbols correctly on these versions of Bash.\n\nThanks for digging.\n\n> I think the best way to move forward with this is a new patch that uses\n> `awk` instead of `sed`: I tested several `awk` variants and the command\n> was portable without requiring any changes to LANG or LC_ALL.\n> \n> Does that sound like a good plan?\n\nNo ;)\nCould you please give the patch below a try?\n\nI intended it as a fix for a minor performance regression introduced in\nthe same commit that triggered this issue, but if it can work around\nthis issue, too, all the better.\n\nI'm just not sure whether we should burden this otherwise nice and short\ncommit message with the details of this Powerline-macOS-Bash-sed issue...\n\n\n -- >8 --\n\nSubject: [PATCH] completion: reduce overhead of clearing cached --options\n\nTo get the names of all '$__git_builtin_*' variables caching --options\nof builtin commands in order to unset them, 8b0eaa41f2 (completion:\nclear cached --options when sourcing the completion script,\n2018-03-22) runs a 'set |sed s///' pipeline. This works both in Bash\nand in ZSH, but has a higher than necessasry overhead with the extra\nprocesses.\n\nIn Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\nbuiltin command, which lists the same variables, but without a\npipeline and 'sed' it can do so with lower overhead.\n\nSigned-off-by: SZEDER Gábor \n\nTODO: as an unexpected bonus, this might work around the issue with\n'sed' and invalid UTF-8 characters in the environment as well, at least\nin Bash.\n\n https://public-inbox.org/git/0102016293c8dca7-6626fcde-548d-476e-b61f-c83ecdeedfe1-000000@eu-west-1.amazonses.com/\n---\n contrib/completion/git-completion.bash | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b09c8a2362..4ef59a51be 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -282,7 +282,11 @@ __gitcomp ()\n \n # Clear the variables caching builtins' options when (re-)sourcing\n # the completion script.\n-unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+if [[ -n ${ZSH_VERSION-} ]]; then\n+\tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+else\n+\tunset $(compgen -v __gitcomp_builtin_)\n+fi\n \n # This function is equivalent to\n #\n-- \n2.17.0.366.gbe216a3084\n\n"},{"id":"344587","messageId":"ACE8F169-7700-4D60-85CB-786C6BEBF0B7@1eanda.com","threadId":"48223","inReplyTo":"20180413000105.21708-1-szeder.dev@gmail.com","subject":"Re: [PATCH] specify encoding for sed command","fromName":"Matthew Coleman","fromEmail":"matt@1eanda.com","sentAt":"2018-04-13T03:00:24Z","receivedAt":"2018-04-13T03:00:46Z","isPatch":true,"sender":{"key":"matt@1eanda.com","avatar":"https://gravatar.com/avatar/7c91c5ac5a5a23cdb81c8d3be895de0b15aad043213568c6163e351b9ec57146?d=mp&s=160"},"body":">> I think the best way to move forward with this is a new patch that uses\n>> `awk` instead of `sed`: I tested several `awk` variants and the command\n>> was portable without requiring any changes to LANG or LC_ALL.\n>> \n>> Does that sound like a good plan?\n> \n> No ;)\n> Could you please give the patch below a try?\nYour patch works great and is clearly a way better approach than parsing the output of `set`.\n\n> I'm just not sure whether we should burden this otherwise nice and short\n> commit message with the details of this Powerline-macOS-Bash-sed issue...\nIt might be worth mentioning it briefly, in order to reduce the chances of a regression in the future. Maybe something like...\n\"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'.\"\n"},{"id":"344599","messageId":"20180413103005.671-1-szeder.dev@gmail.com","threadId":"48223","inReplyTo":"ACE8F169-7700-4D60-85CB-786C6BEBF0B7@1eanda.com","subject":"[PATCH] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-13T10:30:05Z","receivedAt":"2018-04-13T10:30:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To get the names of all '$__git_builtin_*' variables caching --options\nof builtin commands in order to unset them, 8b0eaa41f2 (completion:\nclear cached --options when sourcing the completion script,\n2018-03-22) runs a 'set |sed s///' pipeline. This works both in Bash\nand in ZSH, but has a higher than necessasry overhead with the extra\nprocesses.\n\nIn Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\nbuiltin command, which lists the same variables, but without a\npipeline and 'sed' it can do so with lower overhead.\n\nThis change also happens to work around an issue reported by users of\nthe Powerline shell prompt on macOS, which was triggered by the same\ncommit 8b0eaa41f2 as well. Powerline uses several Unicode Private Use\nArea code points to represent some of its pretty text UI elements\n(arrows and what not), and these are stored in the $PS1 variable.\nApparently the 'set' builtin command of the default Bash version shipped\nin macOS (3.2.57) has issues with these code points, and produces\ngarbled output where Powerline's special symbols should be in the $PS1\nvariable. This, in turn, triggers the following error message in the\ndownstream 'sed' process:\n\n sed: RE error: illegal byte sequence\n\nOther Bash versions, notably 4.4.19 on macOS (via homebrew) and 3.2.25\non CentOS don't seem to be affected.\n\nWith this patch neither the 'set' builtin is invoked to print garbage,\nnor 'sed' to choke on it.\n\nIssue-on-macOS-reported-by: Stephon Harris \nIssue-on-macOS-explained-by: Matthew Coleman \nSigned-off-by: SZEDER Gábor \n---\n contrib/completion/git-completion.bash | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b09c8a2362..4ef59a51be 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -282,7 +282,11 @@ __gitcomp ()\n \n # Clear the variables caching builtins' options when (re-)sourcing\n # the completion script.\n-unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+if [[ -n ${ZSH_VERSION-} ]]; then\n+\tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+else\n+\tunset $(compgen -v __gitcomp_builtin_)\n+fi\n \n # This function is equivalent to\n #\n-- \n2.17.0.366.gbe216a3084\n\n"},{"id":"344649","messageId":"86604un5mz.fsf@gmail.com","threadId":"48223","inReplyTo":"20180413103005.671-1-szeder.dev@gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-04-13T21:44:20Z","receivedAt":"2018-04-13T21:44:31Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"SZEDER Gábor writes:\n\n> To get the names of all '$__git_builtin_*' variables caching --options\n> of builtin commands in order to unset them, 8b0eaa41f2 (completion:\n> clear cached --options when sourcing the completion script,\n> 2018-03-22) runs a 'set |sed s///' pipeline. This works both in Bash\n> and in ZSH, but has a higher than necessasry overhead with the extra\n> processes.\n\nTypo: necessasry -> necessary\n\n>\n> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n> builtin command, which lists the same variables, but without a\n> pipeline and 'sed' it can do so with lower overhead.\n\nWhat about ZSH?\n\n-- \nJakub Narębski\n"},{"id":"344652","messageId":"CAM0VKjkKE695mMPgmfgzWJPaJThdLDSESV9KmupQX_bm-6MW-w@mail.gmail.com","threadId":"48223","inReplyTo":"86604un5mz.fsf@gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-13T22:23:32Z","receivedAt":"2018-04-13T22:23:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n> SZEDER Gábor writes:\n>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>> builtin command, which lists the same variables, but without a\n>> pipeline and 'sed' it can do so with lower overhead.\n>\n> What about ZSH?\n\nNothing, ZSH is unaffected by this patch.\n"},{"id":"344662","messageId":"86sh7xlxyp.fsf@gmail.com","threadId":"48223","inReplyTo":"CAM0VKjkKE695mMPgmfgzWJPaJThdLDSESV9KmupQX_bm-6MW-w@mail.gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-04-14T13:27:42Z","receivedAt":"2018-04-14T13:28:07Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"SZEDER Gábor writes:\n> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n>> SZEDER Gábor writes:\n>>>\n>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>>> builtin command, which lists the same variables, but without a\n>>> pipeline and 'sed' it can do so with lower overhead.\n>>\n>> What about ZSH?\n>\n> Nothing, ZSH is unaffected by this patch.\n\nAll right, so for ZSH we would need LC_ALL=C trick, or come with some\nequivalent of 'compgen -v __gitcomp_builtin_' for this shell.\n\nGood patch, though it does not solve whole of the problem.\n\nBest,\n-- \nJakub Narębski\n"},{"id":"344774","messageId":"xmqq7ep7ybw2.fsf@gitster-ct.c.googlers.com","threadId":"48223","inReplyTo":"CAM0VKjkKE695mMPgmfgzWJPaJThdLDSESV9KmupQX_bm-6MW-w@mail.gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-16T05:10:37Z","receivedAt":"2018-04-16T05:10:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n>> SZEDER Gábor writes:\n>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>>> builtin command, which lists the same variables, but without a\n>>> pipeline and 'sed' it can do so with lower overhead.\n>>\n>> What about ZSH?\n>\n> Nothing, ZSH is unaffected by this patch.\n\nOK, do we want to follow it up with [PATCH 2/1] to add the LC_ALL=C\nthing to the ZSH side to help that \"sed\", then?\n"},{"id":"344789","messageId":"CAM0VKjm=jyetT9ucNbOYxQ19BGYr8nWzAi+WUnsHPuRTrSG9SA@mail.gmail.com","threadId":"48223","inReplyTo":"xmqq7ep7ybw2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-16T13:15:50Z","receivedAt":"2018-04-16T13:15:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 16, 2018 at 7:10 AM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n>>> SZEDER Gábor writes:\n>>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>>>> builtin command, which lists the same variables, but without a\n>>>> pipeline and 'sed' it can do so with lower overhead.\n>>>\n>>> What about ZSH?\n>>\n>> Nothing, ZSH is unaffected by this patch.\n>\n> OK, do we want to follow it up with [PATCH 2/1] to add the LC_ALL=C\n> thing to the ZSH side to help that \"sed\", then?\n\nNo. 'sed' would only need need help when its input comes from a buggy\n'set' builtin of a particular version of Bash from a particular vendor.\n\nAs far as I can test this in Travis CI's OSX builds, ZSH doesn't seem to\nbe affected, neither the version Apple ships by default nor the version\ninstalled via homebrew.\n"},{"id":"344790","messageId":"CANQwDwf8sBFuKBXG6Yt5CH9+E2doqB6rB-_oQ4U7vjTOYBfu_g@mail.gmail.com","threadId":"48223","inReplyTo":"CAM0VKjm=jyetT9ucNbOYxQ19BGYr8nWzAi+WUnsHPuRTrSG9SA@mail.gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2018-04-16T13:29:19Z","receivedAt":"2018-04-16T13:30:05Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On 16 April 2018 at 15:15, SZEDER Gábor wrote:\n> On Mon, Apr 16, 2018 at 7:10 AM, Junio C Hamano wrote:\n> > SZEDER Gábor writes:\n> >> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n> >>> SZEDER Gábor writes:\n> >>>>\n> >>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n> >>>> builtin command, which lists the same variables, but without a\n> >>>> pipeline and 'sed' it can do so with lower overhead.\n> >>>\n> >>> What about ZSH?\n> >>\n> >> Nothing, ZSH is unaffected by this patch.\n> >\n> > OK, do we want to follow it up with [PATCH 2/1] to add the LC_ALL=C\n> > thing to the ZSH side to help that \"sed\", then?\n>\n> No. 'sed' would only need need help when its input comes from a buggy\n> 'set' builtin of a particular version of Bash from a particular vendor.\n>\n> As far as I can test this in Travis CI's OSX builds, ZSH doesn't seem to\n> be affected, neither the version Apple ships by default nor the version\n> installed via homebrew.\n\nThat's nice - this means that the patch fixes all of the issue.\nThe above information should be, in my opinion, included\nin the commit message, though.\n\nBest,\n-- \nJakub Narębski\n"},{"id":"344812","messageId":"CA+P7+xqVdhJ3PrGjQWcofHJV3E1UYWVHEXVt5yaV3Ycv1YvkTg@mail.gmail.com","threadId":"48223","inReplyTo":"86sh7xlxyp.fsf@gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-04-16T18:23:21Z","receivedAt":"2018-04-16T18:23:47Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Apr 14, 2018 at 6:27 AM, Jakub Narebski wrote:\n> SZEDER Gábor writes:\n>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n>>> SZEDER Gábor writes:\n>>>>\n>>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>>>> builtin command, which lists the same variables, but without a\n>>>> pipeline and 'sed' it can do so with lower overhead.\n>>>\n>>> What about ZSH?\n>>\n>> Nothing, ZSH is unaffected by this patch.\n>\n> All right, so for ZSH we would need LC_ALL=C trick, or come with some\n> equivalent of 'compgen -v __gitcomp_builtin_' for this shell.\n>\n> Good patch, though it does not solve whole of the problem.\n>\n> Best,\n> --\n> Jakub Narębski\n\nIs ZSH actually affected by the broken set behavior, though?\n\nThanks,\nJake\n"},{"id":"344819","messageId":"6A862FDA-1EBA-47BB-8C99-AB09DA91955D@1eanda.com","threadId":"48223","inReplyTo":"CA+P7+xqVdhJ3PrGjQWcofHJV3E1UYWVHEXVt5yaV3Ycv1YvkTg@mail.gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Matthew Coleman","fromEmail":"matt@1eanda.com","sentAt":"2018-04-16T20:35:31Z","receivedAt":"2018-04-16T20:35:41Z","isPatch":true,"sender":{"key":"matt@1eanda.com","avatar":"https://gravatar.com/avatar/7c91c5ac5a5a23cdb81c8d3be895de0b15aad043213568c6163e351b9ec57146?d=mp&s=160"},"body":"Disclaimer: I'm not a zsh user, so please correct anything I might have gotten wrong.\n\nI created a .zshrc with the following contents:\nautoload -Uz compinit\ncompinit\nsource /usr/local/lib/python3.6/site-packages/powerline/bindings/zsh/powerline.zsh\n\nzsh 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.\n\nSince 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.\n\nI 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?\nhttp://zsh.sourceforge.net/Doc/Release/Zsh-Modules.html#The-zsh_002fcomputil-Module\n\n> On Apr 16, 2018, at 2:23 PM, Jacob Keller wrote:\n> \n> On Sat, Apr 14, 2018 at 6:27 AM, Jakub Narebski wrote:\n>> SZEDER Gábor writes:\n>>> On Fri, Apr 13, 2018 at 11:44 PM, Jakub Narebski wrote:\n>>>> SZEDER Gábor writes:\n>>>>> \n>>>>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>>>>> builtin command, which lists the same variables, but without a\n>>>>> pipeline and 'sed' it can do so with lower overhead.\n>>>> \n>>>> What about ZSH?\n>>> \n>>> Nothing, ZSH is unaffected by this patch.\n>> \n>> All right, so for ZSH we would need LC_ALL=C trick, or come with some\n>> equivalent of 'compgen -v __gitcomp_builtin_' for this shell.\n>> \n>> Good patch, though it does not solve whole of the problem.\n>> \n>> Best,\n>> --\n>> Jakub Narębski\n> \n> Is ZSH actually affected by the broken set behavior, though?\n> \n> Thanks,\n> Jake\n\n"},{"id":"344843","messageId":"xmqqmuy2wz4j.fsf@gitster-ct.c.googlers.com","threadId":"48223","inReplyTo":"CANQwDwf8sBFuKBXG6Yt5CH9+E2doqB6rB-_oQ4U7vjTOYBfu_g@mail.gmail.com","subject":"Re: [PATCH] completion: reduce overhead of clearing cached --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-16T22:43:56Z","receivedAt":"2018-04-16T22:44:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski writes:\n\n> On 16 April 2018 at 15:15, SZEDER Gábor wrote:\n>> No. 'sed' would only need need help when its input comes from a buggy\n>> 'set' builtin of a particular version of Bash from a particular vendor.\n>>\n>> As far as I can test this in Travis CI's OSX builds, ZSH doesn't seem to\n>> be affected, neither the version Apple ships by default nor the version\n>> installed via homebrew.\n>\n> That's nice - this means that the patch fixes all of the issue.\n> The above information should be, in my opinion, included\n> in the commit message, though.\n\nYeah, I tend to agree. Thanks.\n"},{"id":"344949","messageId":"20180417220219.30445-1-szeder.dev@gmail.com","threadId":"48223","inReplyTo":"xmqqmuy2wz4j.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-04-17T22:02:19Z","receivedAt":"2018-04-17T22:02:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To get the names of all '$__git_builtin_*' variables caching --options\nof builtin commands in order to unset them, 8b0eaa41f2 (completion:\nclear cached --options when sourcing the completion script,\n2018-03-22) runs a 'set |sed s///' pipeline. This works both in Bash\nand in ZSH, but has a higher than necessary overhead with the extra\nprocesses.\n\nIn Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\nbuiltin command, which lists the same variables, but without a\npipeline and 'sed' it can do so with lower overhead.\nZSH will still continue to run that pipeline.\n\nThis change also happens to work around an issue in the default Bash\nversion shipped in macOS (3.2.57), reported by users of the Powerline\nshell prompt, which was triggered by the same commit 8b0eaa41f2 as\nwell. Powerline uses several Unicode Private Use Area code points to\nrepresent some of its pretty text UI elements (arrows and what not),\nand these are stored in the $PS1 variable. Apparently the 'set'\nbuiltin of said Bash version on macOS has issues with these code\npoints, and produces garbled output where Powerline's special symbols\nshould be in the $PS1 variable. This, in turn, triggers the following\nerror message in the downstream 'sed' process:\n\n sed: RE error: illegal byte sequence\n\nOther Bash versions, notably 4.4.19 on macOS via homebrew (i.e. a\nnewer version on the same platform) and 3.2.25 on CentOS (i.e. a\nslightly earlier version, though on a different platform) are not\naffected. ZSH in macOS (the versions shipped by default or installed\nvia homebrew) or on other platforms isn't affected either.\n\nWith this patch neither the 'set' builtin is invoked to print garbage,\nnor 'sed' to choke on it.\n\nIssue-on-macOS-reported-by: Stephon Harris \nIssue-on-macOS-explained-by: Matthew Coleman \nSigned-off-by: SZEDER Gábor \n---\n\nUpdated the commit message to explicitly mention that ZSH is\nunaffected. The patch is the same.\n\n contrib/completion/git-completion.bash | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex b09c8a2362..4ef59a51be 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -282,7 +282,11 @@ __gitcomp ()\n \n # Clear the variables caching builtins' options when (re-)sourcing\n # the completion script.\n-unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+if [[ -n ${ZSH_VERSION-} ]]; then\n+\tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n+else\n+\tunset $(compgen -v __gitcomp_builtin_)\n+fi\n \n # This function is equivalent to\n #\n-- \n2.17.0.366.gbe216a3084\n\n"},{"id":"344956","messageId":"xmqqh8o9l7n4.fsf@gitster-ct.c.googlers.com","threadId":"48223","inReplyTo":"20180417220219.30445-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-17T23:45:19Z","receivedAt":"2018-04-17T23:45:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> To get the names of all '$__git_builtin_*' variables caching --options\n> of builtin commands in order to unset them, 8b0eaa41f2 (completion:\n> clear cached --options when sourcing the completion script,\n> 2018-03-22) runs a 'set |sed s///' pipeline. This works both in Bash\n> and in ZSH, but has a higher than necessary overhead with the extra\n> processes.\n>\n> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n> builtin command, which lists the same variables, but without a\n> pipeline and 'sed' it can do so with lower overhead.\n> ZSH will still continue to run that pipeline.\n> \n> This change also happens to work around an issue in the default Bash\n> ...\n> Updated the commit message to explicitly mention that ZSH is\n> unaffected. The patch is the same.\n\nThanks.\n"},{"id":"346873","messageId":"21289682-5B46-4131-B7D7-57CE9F391B9D@1eanda.com","threadId":"48223","inReplyTo":"xmqqh8o9l7n4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Matthew Coleman","fromEmail":"matt@1eanda.com","sentAt":"2018-05-07T15:05:41Z","receivedAt":"2018-05-07T15:05:50Z","isPatch":true,"sender":{"key":"matt@1eanda.com","avatar":"https://gravatar.com/avatar/7c91c5ac5a5a23cdb81c8d3be895de0b15aad043213568c6163e351b9ec57146?d=mp&s=160"},"body":"I haven't seen any discussion about this recently. What are the chances of getting it merged? I'd like to see this included in 2.18.\n\n>> To get the names of all '$__git_builtin_*' variables caching --options\n>> of builtin commands in order to unset them, 8b0eaa41f2 (completion:\n>> clear cached --options when sourcing the completion script,\n>> 2018-03-22) runs a 'set |sed s///' pipeline. This works both in Bash\n>> and in ZSH, but has a higher than necessary overhead with the extra\n>> processes.\n>> \n>> In Bash we can do better: run the 'compgen -v __gitcomp_builtin_'\n>> builtin command, which lists the same variables, but without a\n>> pipeline and 'sed' it can do so with lower overhead.\n>> ZSH will still continue to run that pipeline.\n>> \n>> This change also happens to work around an issue in the default Bash\n>> ...\n>> Updated the commit message to explicitly mention that ZSH is\n>> unaffected. The patch is the same.\n> \n> Thanks.\n\n"},{"id":"346949","messageId":"20180508022842.GD26695@zaya.teonanacatl.net","threadId":"48223","inReplyTo":"21289682-5B46-4131-B7D7-57CE9F391B9D@1eanda.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2018-05-08T02:28:42Z","receivedAt":"2018-05-08T02:28:49Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi Matthew,\n\nMatthew Coleman wrote:\n> I haven't seen any discussion about this recently. What\n> are the chances of getting it merged? I'd like to see this\n> included in 2.18.\n\nJunio's last few \"What's cooking\" updates have mentioned it:\n\n> * sg/completion-clear-cached (2018-04-18) 1 commit\n> (merged to 'next' on 2018-04-25 at 9178da6c3d)\n> + completion: reduce overhead of clearing cached --options\n> \n> The completion script (in contrib/) learned to clear cached list of\n> command line options upon dot-sourcing it again in a more efficient\n> way.\n> \n> Will merge to 'master'.\n\nI imagine it will show up on master sometime soon, in time\nfor 2.18.0.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nI expected times like this -- but never thought they'd be so bad, so\nlong, and so frequent.\n -- Demotivators (www.despair.com)\n\n"},{"id":"346956","messageId":"xmqqr2mmn7v1.fsf@gitster-ct.c.googlers.com","threadId":"48223","inReplyTo":"20180508022842.GD26695@zaya.teonanacatl.net","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-08T03:28:50Z","receivedAt":"2018-05-08T03:28:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger writes:\n\n> Hi Matthew,\n>\n> Matthew Coleman wrote:\n>> I haven't seen any discussion about this recently. What\n>> are the chances of getting it merged? I'd like to see this\n>> included in 2.18.\n>\n> Junio's last few \"What's cooking\" updates have mentioned it:\n>\n>> * sg/completion-clear-cached (2018-04-18) 1 commit\n>> (merged to 'next' on 2018-04-25 at 9178da6c3d)\n>> + completion: reduce overhead of clearing cached --options\n>> \n>> The completion script (in contrib/) learned to clear cached list of\n>> command line options upon dot-sourcing it again in a more efficient\n>> way.\n>> \n>> Will merge to 'master'.\n>\n> I imagine it will show up on master sometime soon, in time\n> for 2.18.0.\n\nYup, I do not foresee at this moment why that shouldn't be the case.\n\nThanks.\n"},{"id":"349600","messageId":"20180607054834.GB6567@aiede.svl.corp.google.com","threadId":"48223","inReplyTo":"20180417220219.30445-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-06-07T05:48:34Z","receivedAt":"2018-06-07T05:48:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSZEDER Gábor wrote:\n\n> Other Bash versions, notably 4.4.19 on macOS via homebrew (i.e. a\n> newer version on the same platform) and 3.2.25 on CentOS (i.e. a\n> slightly earlier version, though on a different platform) are not\n> affected. ZSH in macOS (the versions shipped by default or installed\n> via homebrew) or on other platforms isn't affected either.\n[...]\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -282,7 +282,11 @@ __gitcomp ()\n> \n> # Clear the variables caching builtins' options when (re-)sourcing\n> # the completion script.\n> -unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> +if [[ -n ${ZSH_VERSION-} ]]; then\n> +\tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> +else\n> +\tunset $(compgen -v __gitcomp_builtin_)\n> +fi\n\nAs discussed at [1], this fails catastrophically when sourced by\ngit-completion.zsh, which explicitly clears ZSH_VERSION. By\ncatastrophically, I mean that Rick van Hattem and Dave Borowitz report\nthat it segfaults.\n\nI can't reproduce it since I am having trouble following the\ninstructions at the top of git-completion.zsh to get the recommended\nzsh completion script loading mechanism working at all. (I've\nsucceeded in using git's bash completion on zsh before, but that was\nmany years ago.) First attempt:\n\n 1. rm -fr ~/.zsh ~/.zshrc\n # you can move them out of the way instead for a less destructive\n # reproduction recipe\n 2. zsh\n 3. hit \"q\"\n 4. zstyle ':completion:*:*:git:*' script \\\n $(pwd)/contrib/completion/git-completion.zsh\n 5. git checkout \n\nExpected result: segfault\n\nActual result: succeeds, using zsh's standard completion script (not\ngit's).\n\nI also tried\n\n 1. rm -fr ~/.zsh ~/.zshrc\n # you can move them out of the way instead for a less destructive\n # reproduction recipe\n 2. zsh\n 3. hit \"2\"\n 4. mkdir ~/.zsh; cp contrib/completion/git-completion.zsh ~/.zsh/_git\n 5. echo 'fpath=(~/.zsh $fpath)' >>~/.zshrc\n 6. ^D; zsh\n 7. git checkout \n\nand a similar process with fpath earlier in the zshrc file. Whatever\nI try, I end up with one of two results: either it uses zsh's standard\ncompletion, or it spews a stream of errors about missing functions\nlike compgen. What am I doing wrong?\n\nRelated question: what would it take to add a zsh completion sanity\ncheck to t/t9902-completion.sh?\n\nWhen running with \"set -x\", here is what Dave Borowitz gets before the\nsegfault. I don't have a stacktrace because, as mentioned above, I\nhaven't been able to reproduce it.\n\n\tstr=git\n\t[[ -n git ]]\n\tservice=git\n\ti=\n\t_compskip=default\n\teval ''\n\tret=0\n\t[[ default = *patterns* ]]\n\t[[ default = all ]]\n\tstr=/usr/bin/git\n\t[[ -n /usr/bin/git ]]\n\tservice=_dispatch:70: bad math expression: operand expected at `/usr/bin/g...'\n\n\tzsh: segmentation fault zsh\n\nHere's a minimal fix, untested. As a followup, as Gábor suggests at [2],\nit would presumably make sense to stop overriding ZSH_VERSION, using\nthis GIT_ scoped variable everywhere instead.\n\nThoughts?\n\nReported-by: Rick van Hattem \nReported-by: Dave Borowitz \nSigned-off-by: Jonathan Nieder \n\n[1] https://public-inbox.org/git/01020163c683e753-04629405-15f8-4a30-9dc3-e4e3f2a5aa26-000000@eu-west-1.amazonses.com/\n[2] https://public-inbox.org/git/20180606114147.7753-1-szeder.dev@gmail.com/\n\ndiff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash\nindex 12814e9bbf..e4bcc435ea 100644\n--- i/contrib/completion/git-completion.bash\n+++ w/contrib/completion/git-completion.bash\n@@ -348,7 +348,7 @@ __gitcomp ()\n \n # Clear the variables caching builtins' options when (re-)sourcing\n # the completion script.\n-if [[ -n ${ZSH_VERSION-} ]]; then\n+if [[ -n ${ZSH_VERSION-} || -n ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then\n \tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n else\n \tunset $(compgen -v __gitcomp_builtin_)\ndiff --git i/contrib/completion/git-completion.zsh w/contrib/completion/git-completion.zsh\nindex 53cb0f934f..c7be9fd4dc 100644\n--- i/contrib/completion/git-completion.zsh\n+++ w/contrib/completion/git-completion.zsh\n@@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n \t\ttest -f $e && script=\"$e\" && break\n \tdone\n fi\n-ZSH_VERSION='' . \"$script\"\n+GIT_SOURCING_ZSH_COMPLETION=1 ZSH_VERSION='' . \"$script\"\n \n __gitcomp ()\n {\n"},{"id":"349608","messageId":"CAD0k6qS_VoX-Pzfh5MDgcUzAyARy13WHKvNqLShzafWL9JMkgw@mail.gmail.com","threadId":"48223","inReplyTo":"20180607054834.GB6567@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2018-06-07T12:07:40Z","receivedAt":"2018-06-07T12:07:55Z","isPatch":true,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Thu, Jun 7, 2018 at 1:48 AM Jonathan Nieder wrote:\n> Whatever\n> I try, I end up with one of two results: either it uses zsh's standard\n> completion, or it spews a stream of errors about missing functions\n> like compgen. What am I doing wrong?\n\nTry adding to the top of your .zshrc:\nautoload -U compinit; compinit\nautoload -U bashcompinit; bashcompinit\n\nThose are both in my .zshrc, and I think they are sufficient magic to\ndefine compgen.\n"},{"id":"349610","messageId":"CAJAwA=z2cwy7A3oG=0RPCrh4i41NxPk6jAFhjtisAPvndOKC9Q@mail.gmail.com","threadId":"48223","inReplyTo":"20180607054834.GB6567@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Rick van Hattem","fromEmail":"wolph@wol.ph","sentAt":"2018-06-07T12:41:47Z","receivedAt":"2018-06-07T12:42:15Z","isPatch":true,"sender":{"key":"wolph@wol.ph","avatar":"https://gravatar.com/avatar/3e86569f70d74646eb1f682e8d28f965114e44e067364be2154d74e7cb6b702a?d=mp&s=160"},"body":"The zsh script expects the bash completion script to be available so\nthat might be the issue here.\n\nTo reproduce this is what I do. First, a sparse checkout:\n# mkdir ~/git\n# cd ~/git\n# git init\n# git remote add origin git@github.com:git/git.git\n# git config core.sparseCheckout true\n# mkdir .git/info\n# echo contrib/completion >> .git/info/sparse-checkout\n# git pull origin master --depth 1\n\nAfter that I simply link the zsh script to _git:\n# cd git/contrib/completion\n# ln git-completion.zsh _git\n\nAnd add the following to a new .zshrc file:\nautoload -U compinit; compinit\nautoload -U bashcompinit; bashcompinit\nfpath=(~/git/contrib/completion $fpath)\n\nAnd that appears to be enough to reproduce:\n# git\ngit/contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX\nzsh:12: command not found: ___main\nzsh:15: _default: function definition file not found\n_dispatch:70: bad math expression: operand expected at `/usr/bin/g...'\nzsh: segmentation fault zsh\n\n~rick\n\nOn 7 June 2018 at 07:48, Jonathan Nieder wrote:\n> Hi,\n>\n> SZEDER Gábor wrote:\n>\n>> Other Bash versions, notably 4.4.19 on macOS via homebrew (i.e. a\n>> newer version on the same platform) and 3.2.25 on CentOS (i.e. a\n>> slightly earlier version, though on a different platform) are not\n>> affected. ZSH in macOS (the versions shipped by default or installed\n>> via homebrew) or on other platforms isn't affected either.\n> [...]\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -282,7 +282,11 @@ __gitcomp ()\n>>\n>> # Clear the variables caching builtins' options when (re-)sourcing\n>> # the completion script.\n>> -unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n>> +if [[ -n ${ZSH_VERSION-} ]]; then\n>> + unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n>> +else\n>> + unset $(compgen -v __gitcomp_builtin_)\n>> +fi\n>\n> As discussed at [1], this fails catastrophically when sourced by\n> git-completion.zsh, which explicitly clears ZSH_VERSION. By\n> catastrophically, I mean that Rick van Hattem and Dave Borowitz report\n> that it segfaults.\n>\n> I can't reproduce it since I am having trouble following the\n> instructions at the top of git-completion.zsh to get the recommended\n> zsh completion script loading mechanism working at all. (I've\n> succeeded in using git's bash completion on zsh before, but that was\n> many years ago.) First attempt:\n>\n> 1. rm -fr ~/.zsh ~/.zshrc\n> # you can move them out of the way instead for a less destructive\n> # reproduction recipe\n> 2. zsh\n> 3. hit \"q\"\n> 4. zstyle ':completion:*:*:git:*' script \\\n> $(pwd)/contrib/completion/git-completion.zsh\n> 5. git checkout \n>\n> Expected result: segfault\n>\n> Actual result: succeeds, using zsh's standard completion script (not\n> git's).\n>\n> I also tried\n>\n> 1. rm -fr ~/.zsh ~/.zshrc\n> # you can move them out of the way instead for a less destructive\n> # reproduction recipe\n> 2. zsh\n> 3. hit \"2\"\n> 4. mkdir ~/.zsh; cp contrib/completion/git-completion.zsh ~/.zsh/_git\n> 5. echo 'fpath=(~/.zsh $fpath)' >>~/.zshrc\n> 6. ^D; zsh\n> 7. git checkout \n>\n> and a similar process with fpath earlier in the zshrc file. Whatever\n> I try, I end up with one of two results: either it uses zsh's standard\n> completion, or it spews a stream of errors about missing functions\n> like compgen. What am I doing wrong?\n>\n> Related question: what would it take to add a zsh completion sanity\n> check to t/t9902-completion.sh?\n>\n> When running with \"set -x\", here is what Dave Borowitz gets before the\n> segfault. I don't have a stacktrace because, as mentioned above, I\n> haven't been able to reproduce it.\n>\n> str=git\n> [[ -n git ]]\n> service=git\n> i=\n> _compskip=default\n> eval ''\n> ret=0\n> [[ default = *patterns* ]]\n> [[ default = all ]]\n> str=/usr/bin/git\n> [[ -n /usr/bin/git ]]\n> service=_dispatch:70: bad math expression: operand expected at `/usr/bin/g...'\n>\n> zsh: segmentation fault zsh\n>\n> Here's a minimal fix, untested. As a followup, as Gábor suggests at [2],\n> it would presumably make sense to stop overriding ZSH_VERSION, using\n> this GIT_ scoped variable everywhere instead.\n>\n> Thoughts?\n>\n> Reported-by: Rick van Hattem \n> Reported-by: Dave Borowitz \n> Signed-off-by: Jonathan Nieder \n>\n> [1] https://public-inbox.org/git/01020163c683e753-04629405-15f8-4a30-9dc3-e4e3f2a5aa26-000000@eu-west-1.amazonses.com/\n> [2] https://public-inbox.org/git/20180606114147.7753-1-szeder.dev@gmail.com/\n>\n> diff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash\n> index 12814e9bbf..e4bcc435ea 100644\n> --- i/contrib/completion/git-completion.bash\n> +++ w/contrib/completion/git-completion.bash\n> @@ -348,7 +348,7 @@ __gitcomp ()\n>\n> # Clear the variables caching builtins' options when (re-)sourcing\n> # the completion script.\n> -if [[ -n ${ZSH_VERSION-} ]]; then\n> +if [[ -n ${ZSH_VERSION-} || -n ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then\n> unset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> else\n> unset $(compgen -v __gitcomp_builtin_)\n> diff --git i/contrib/completion/git-completion.zsh w/contrib/completion/git-completion.zsh\n> index 53cb0f934f..c7be9fd4dc 100644\n> --- i/contrib/completion/git-completion.zsh\n> +++ w/contrib/completion/git-completion.zsh\n> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n> test -f $e && script=\"$e\" && break\n> done\n> fi\n> -ZSH_VERSION='' . \"$script\"\n> +GIT_SOURCING_ZSH_COMPLETION=1 ZSH_VERSION='' . \"$script\"\n>\n> __gitcomp ()\n> {\n"},{"id":"349738","messageId":"20180608211639.7611-1-szeder.dev@gmail.com","threadId":"48223","inReplyTo":"20180607054834.GB6567@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-08T21:16:39Z","receivedAt":"2018-06-08T21:16:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> Related question: what would it take to add a zsh completion sanity\n> check to t/t9902-completion.sh?\n\nI don't know. What I do know is that we can't just run our tests with\nZSH, e.g. running 'zsh ./t0000-basic.sh' shows mostly failures. So it\nwon't be as simple as modifying 't/lib-bash.sh' to somehow support ZSH\nas well.\n\n> Here's a minimal fix, untested. As a followup, as G�bor suggests at [2],\n> it would presumably make sense to stop overriding ZSH_VERSION, using\n> this GIT_ scoped variable everywhere instead.\n> \n> Thoughts?\n> \n> Reported-by: Rick van Hattem \n> Reported-by: Dave Borowitz \n> Signed-off-by: Jonathan Nieder \n> \n> [1] https://public-inbox.org/git/01020163c683e753-04629405-15f8-4a30-9dc3-e4e3f2a5aa26-000000@eu-west-1.amazonses.com/\n> [2] https://public-inbox.org/git/20180606114147.7753-1-szeder.dev@gmail.com/\n> \n> diff --git i/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash\n> index 12814e9bbf..e4bcc435ea 100644\n> --- i/contrib/completion/git-completion.bash\n> +++ w/contrib/completion/git-completion.bash\n> @@ -348,7 +348,7 @@ __gitcomp ()\n> \n> # Clear the variables caching builtins' options when (re-)sourcing\n> # the completion script.\n> -if [[ -n ${ZSH_VERSION-} ]]; then\n> +if [[ -n ${ZSH_VERSION-} || -n ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then\n> \tunset $(set |sed -ne 's/^\\(__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*\\)=.*/\\1/p') 2>/dev/null\n> else\n> \tunset $(compgen -v __gitcomp_builtin_)\n> diff --git i/contrib/completion/git-completion.zsh w/contrib/completion/git-completion.zsh\n> index 53cb0f934f..c7be9fd4dc 100644\n> --- i/contrib/completion/git-completion.zsh\n> +++ w/contrib/completion/git-completion.zsh\n> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n> \t\ttest -f $e && script=\"$e\" && break\n> \tdone\n> fi\n> -ZSH_VERSION='' . \"$script\"\n> +GIT_SOURCING_ZSH_COMPLETION=1 ZSH_VERSION='' . \"$script\"\n> \n> __gitcomp ()\n> {\n> \n\nBeing in RC phase, I'm all for aiming for a minimal solution.\nHowever, I don't think that the better fix would be erm.. any \"less\nminimal\":\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex f2aa484758..7aeb575cd1 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3244,7 +3244,10 @@ __gitk_main ()\n \t__git_complete_revlist\n }\n \n-if [[ -n ${ZSH_VERSION-} ]]; then\n+if [[ -n ${ZSH_VERSION-} ]] &&\n+ # Don't define these functions when sourced from 'git-completion.zsh',\n+ # it has its own implementations.\n+ [[ -z \"${GIT_SOURCING_ZSH_COMPLETION}\" ]] ; then\n \techo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n \n \tautoload -U +X compinit && compinit\ndiff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\nindex 53cb0f934f..049d6b80f6 100644\n--- a/contrib/completion/git-completion.zsh\n+++ b/contrib/completion/git-completion.zsh\n@@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n \t\ttest -f $e && script=\"$e\" && break\n \tdone\n fi\n-ZSH_VERSION='' . \"$script\"\n+GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n \n __gitcomp ()\n {\n"},{"id":"349739","messageId":"20180608212348.GA42503@aiede.svl.corp.google.com","threadId":"48223","inReplyTo":"20180608211639.7611-1-szeder.dev@gmail.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-06-08T21:23:48Z","receivedAt":"2018-06-08T21:23:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSZEDER Gábor wrote:\n\n> Being in RC phase, I'm all for aiming for a minimal solution.\n> However, I don't think that the better fix would be erm.. any \"less\n> minimal\":\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index f2aa484758..7aeb575cd1 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -3244,7 +3244,10 @@ __gitk_main ()\n> \t__git_complete_revlist\n> }\n> \n> -if [[ -n ${ZSH_VERSION-} ]]; then\n> +if [[ -n ${ZSH_VERSION-} ]] &&\n> + # Don't define these functions when sourced from 'git-completion.zsh',\n> + # it has its own implementations.\n> + [[ -z \"${GIT_SOURCING_ZSH_COMPLETION}\" ]] ; then\n\nNeeds a - before the } to avoid errors in a shell where the user has\nchosen to use \"set -u\". See v1.7.4-rc0~159 (completion: fix zsh check\nunder bash with 'set -u', 2010-10-27) for more details.\n\n> \techo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n> \n> \tautoload -U +X compinit && compinit\n> diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\n> index 53cb0f934f..049d6b80f6 100644\n> --- a/contrib/completion/git-completion.zsh\n> +++ b/contrib/completion/git-completion.zsh\n> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n> \t\ttest -f $e && script=\"$e\" && break\n> \tdone\n> fi\n> -ZSH_VERSION='' . \"$script\"\n> +GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n> \n> __gitcomp ()\n> {\n\nExcept for that tweak,\nReviewed-by: Jonathan Nieder \nThanks.\n\nNow it just needs a commit message. :)\n"},{"id":"349740","messageId":"CAM0VKjkWkUmU9LGyKKFyvkOCFh1s+JJK4aZLPfoOJj26OkNQVg@mail.gmail.com","threadId":"48223","inReplyTo":"20180608212348.GA42503@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-08T21:41:46Z","receivedAt":"2018-06-08T21:41:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 8, 2018 at 11:23 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> SZEDER Gábor wrote:\n>\n>> Being in RC phase, I'm all for aiming for a minimal solution.\n>> However, I don't think that the better fix would be erm.. any \"less\n>> minimal\":\n>>\n>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n>> index f2aa484758..7aeb575cd1 100644\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -3244,7 +3244,10 @@ __gitk_main ()\n>> __git_complete_revlist\n>> }\n>>\n>> -if [[ -n ${ZSH_VERSION-} ]]; then\n>> +if [[ -n ${ZSH_VERSION-} ]] &&\n>> + # Don't define these functions when sourced from 'git-completion.zsh',\n>> + # it has its own implementations.\n>> + [[ -z \"${GIT_SOURCING_ZSH_COMPLETION}\" ]] ; then\n>\n> Needs a - before the } to avoid errors in a shell where the user has\n> chosen to use \"set -u\". See v1.7.4-rc0~159 (completion: fix zsh check\n> under bash with 'set -u', 2010-10-27) for more details.\n\nRight... I did remember that, but by the time I finished typing out\nthat long variable name I forgot about it... :)\n\nHowever, I'm not sure it's worth caring about, because the\nbash-competion scripts don't work with 'set -u' anyway...\n\n>> echo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n>>\n>> autoload -U +X compinit && compinit\n>> diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\n>> index 53cb0f934f..049d6b80f6 100644\n>> --- a/contrib/completion/git-completion.zsh\n>> +++ b/contrib/completion/git-completion.zsh\n>> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n>> test -f $e && script=\"$e\" && break\n>> done\n>> fi\n>> -ZSH_VERSION='' . \"$script\"\n>> +GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n>>\n>> __gitcomp ()\n>> {\n>\n> Except for that tweak,\n> Reviewed-by: Jonathan Nieder \n> Thanks.\n>\n> Now it just needs a commit message. :)\n"},{"id":"349741","messageId":"20180608215207.GB42503@aiede.svl.corp.google.com","threadId":"48223","inReplyTo":"CAM0VKjkWkUmU9LGyKKFyvkOCFh1s+JJK4aZLPfoOJj26OkNQVg@mail.gmail.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-06-08T21:52:07Z","receivedAt":"2018-06-08T21:52:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"SZEDER Gábor wrote:\n> On Fri, Jun 8, 2018 at 11:23 PM, Jonathan Nieder wrote:\n\n>>> + [[ -z \"${GIT_SOURCING_ZSH_COMPLETION}\" ]] ; then\n>>\n>> Needs a - before the } to avoid errors in a shell where the user has\n>> chosen to use \"set -u\". See v1.7.4-rc0~159 (completion: fix zsh check\n>> under bash with 'set -u', 2010-10-27) for more details.\n>\n> Right... I did remember that, but by the time I finished typing out\n> that long variable name I forgot about it... :)\n>\n> However, I'm not sure it's worth caring about, because the\n> bash-competion scripts don't work with 'set -u' anyway...\n\nI'd be happy to see these '-'s go away (it would make the code both\neasier to read and easier to maintain), but as a post-2.18 topic,\nplease.\n\nAlternatively, if there's a real need for them, I'd be happy to see\nthe completion tests start using 'set -u'.\n\nThanks,\nJonathan\n"},{"id":"349742","messageId":"CAM0VKjnJbHA8BXC_prSAj1QoNC2fDRX4WKmse028g9J1XZX+Qg@mail.gmail.com","threadId":"48223","inReplyTo":"20180608215207.GB42503@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-08T21:58:01Z","receivedAt":"2018-06-08T21:58:05Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 8, 2018 at 11:52 PM, Jonathan Nieder wrote:\n> SZEDER Gábor wrote:\n>> On Fri, Jun 8, 2018 at 11:23 PM, Jonathan Nieder wrote:\n>\n>>>> + [[ -z \"${GIT_SOURCING_ZSH_COMPLETION}\" ]] ; then\n>>>\n>>> Needs a - before the } to avoid errors in a shell where the user has\n>>> chosen to use \"set -u\". See v1.7.4-rc0~159 (completion: fix zsh check\n>>> under bash with 'set -u', 2010-10-27) for more details.\n>>\n>> Right... I did remember that, but by the time I finished typing out\n>> that long variable name I forgot about it... :)\n>>\n>> However, I'm not sure it's worth caring about, because the\n>> bash-competion scripts don't work with 'set -u' anyway...\n>\n> I'd be happy to see these '-'s go away (it would make the code both\n> easier to read and easier to maintain), but as a post-2.18 topic,\n> please.\n\nOf course.\n\n> Alternatively, if there's a real need for them, I'd be happy to see\n> the completion tests start using 'set -u'.\n\nOur test harness doesn't work with 'set -u' at all...\n"},{"id":"349921","messageId":"xmqqzi01b46g.fsf@gitster-ct.c.googlers.com","threadId":"48223","inReplyTo":"20180608212348.GA42503@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] completion: reduce overhead of clearing cached --options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-11T17:53:27Z","receivedAt":"2018-06-11T17:53:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> SZEDER Gábor wrote:\n>\n>> Being in RC phase, I'm all for aiming for a minimal solution.\n>> However, I don't think that the better fix would be erm.. any \"less\n>> minimal\":\n>>\n>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n>> index f2aa484758..7aeb575cd1 100644\n>> --- a/contrib/completion/git-completion.bash\n>> +++ b/contrib/completion/git-completion.bash\n>> @@ -3244,7 +3244,10 @@ __gitk_main ()\n>> \t__git_complete_revlist\n>> }\n>> \n>> -if [[ -n ${ZSH_VERSION-} ]]; then\n>> +if [[ -n ${ZSH_VERSION-} ]] &&\n>> + # Don't define these functions when sourced from 'git-completion.zsh',\n>> + # it has its own implementations.\n>> + [[ -z \"${GIT_SOURCING_ZSH_COMPLETION}\" ]] ; then\n>\n> Needs a - before the } to avoid errors in a shell where the user has\n> chosen to use \"set -u\". See v1.7.4-rc0~159 (completion: fix zsh check\n> under bash with 'set -u', 2010-10-27) for more details.\n>\n>> \techo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n>> \n>> \tautoload -U +X compinit && compinit\n>> diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\n>> index 53cb0f934f..049d6b80f6 100644\n>> --- a/contrib/completion/git-completion.zsh\n>> +++ b/contrib/completion/git-completion.zsh\n>> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n>> \t\ttest -f $e && script=\"$e\" && break\n>> \tdone\n>> fi\n>> -ZSH_VERSION='' . \"$script\"\n>> +GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n>> \n>> __gitcomp ()\n>> {\n>\n> Except for that tweak,\n> Reviewed-by: Jonathan Nieder \n> Thanks.\n>\n> Now it just needs a commit message. :)\n\nOK, can we see the final version of this patch so that we can work\nit around before 2.18.0-rc2? I am assuming that we are treating\nthis as a Git 2.18-rc regression that needs to be fixed before the\nfinal?\n\nThanks.\n"},{"id":"349927","messageId":"20180611182053.GA20665@aiede.svl.corp.google.com","threadId":"48223","inReplyTo":"20180608211639.7611-1-szeder.dev@gmail.com","subject":"[PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-06-11T18:20:53Z","receivedAt":"2018-06-11T18:21:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: SZEDER Gábor \nSubject: completion: correct zsh detection when run from git-completion.zsh\n\nv2.18.0-rc0~90^2 (completion: reduce overhead of clearing cached\n--options, 2018-04-18) worked around a bug in bash's \"set\" builtin on\nMacOS by using compgen instead. It was careful to avoid breaking zsh\nby guarding this workaround with\n\n\tif [[ -n ${ZSH_VERSION-}} ]]\n\nAlas, this interacts poorly with git-completion.zsh's bash emulation:\n\n\tZSH_VERSION='' . \"$script\"\n\nCorrect it by instead using a new GIT_SOURCING_ZSH_COMPLETION shell\nvariable to detect whether git-completion.bash is being sourced from\ngit-completion.zsh. This way, the zsh variant is used both when run\nfrom zsh directly and when run via git-completion.zsh.\n\nReproduction recipe:\n\n 1. cd git/contrib/completion && cp git-completion.zsh _git\n 2. Put the following in a new ~/.zshrc file:\n\n \tautoload -U compinit; compinit\n\tautoload -U bashcompinit; bashcompinit\n\tfpath=(~/src/git/contrib/completion $fpath)\n\n 3. Open zsh and \"git \".\n\nWith this patch:\nTriggers nice git-completion.bash based tab completion\n\nWithout:\n contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX\n zsh:12: command not found: ___main\n zsh:15: _default: function definition file not found\n _dispatch:70: bad math expression: operand expected at `/usr/bin/g...'\n Segmentation fault\n\nReported-by: Rick van Hattem \nReported-by: Dave Borowitz \nSigned-off-by: Jonathan Nieder \n---\nSZEDER Gábor wrote:\n\n> Being in RC phase, I'm all for aiming for a minimal solution.\n> However, I don't think that the better fix would be erm.. any \"less\n> minimal\":\n\nThanks again. May we have your sign-off?\n\n contrib/completion/git-completion.bash | 5 ++++-\n contrib/completion/git-completion.zsh | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 12814e9bbf..f4a2e6774b 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -3223,7 +3223,10 @@ __gitk_main ()\n \t__git_complete_revlist\n }\n \n-if [[ -n ${ZSH_VERSION-} ]]; then\n+if [[ -n ${ZSH_VERSION-} ]] &&\n+ # Don't define these functions when sourced from 'git-completion.zsh',\n+ # it has its own implementations.\n+ [[ -z ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then\n \techo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n \n \tautoload -U +X compinit && compinit\ndiff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\nindex 53cb0f934f..049d6b80f6 100644\n--- a/contrib/completion/git-completion.zsh\n+++ b/contrib/completion/git-completion.zsh\n@@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n \t\ttest -f $e && script=\"$e\" && break\n \tdone\n fi\n-ZSH_VERSION='' . \"$script\"\n+GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n \n __gitcomp ()\n {\n-- \n2.18.0.rc1.242.g61856ae69a\n\n"},{"id":"349972","messageId":"CAM0VKj==u0DgnHF0j+OOtyWMCsuGApHW65CN9S4d5iUxb2bRUg@mail.gmail.com","threadId":"48223","inReplyTo":"20180611182053.GA20665@aiede.svl.corp.google.com","subject":"Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-12T08:46:16Z","receivedAt":"2018-06-12T08:46:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jun 11, 2018 at 8:20 PM, Jonathan Nieder wrote:\n> From: SZEDER Gábor \n> Subject: completion: correct zsh detection when run from git-completion.zsh\n>\n> v2.18.0-rc0~90^2 (completion: reduce overhead of clearing cached\n> --options, 2018-04-18) worked around a bug in bash's \"set\" builtin on\n> MacOS by using compgen instead. It was careful to avoid breaking zsh\n> by guarding this workaround with\n>\n> if [[ -n ${ZSH_VERSION-}} ]]\n>\n> Alas, this interacts poorly with git-completion.zsh's bash emulation:\n>\n> ZSH_VERSION='' . \"$script\"\n>\n> Correct it by instead using a new GIT_SOURCING_ZSH_COMPLETION shell\n> variable to detect whether git-completion.bash is being sourced from\n> git-completion.zsh. This way, the zsh variant is used both when run\n> from zsh directly and when run via git-completion.zsh.\n>\n> Reproduction recipe:\n>\n> 1. cd git/contrib/completion && cp git-completion.zsh _git\n> 2. Put the following in a new ~/.zshrc file:\n>\n> autoload -U compinit; compinit\n> autoload -U bashcompinit; bashcompinit\n> fpath=(~/src/git/contrib/completion $fpath)\n>\n> 3. Open zsh and \"git \".\n>\n> With this patch:\n> Triggers nice git-completion.bash based tab completion\n>\n> Without:\n> contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX\n> zsh:12: command not found: ___main\n> zsh:15: _default: function definition file not found\n> _dispatch:70: bad math expression: operand expected at `/usr/bin/g...'\n> Segmentation fault\n>\n> Reported-by: Rick van Hattem \n> Reported-by: Dave Borowitz \n> Signed-off-by: Jonathan Nieder \n> ---\n> SZEDER Gábor wrote:\n>\n>> Being in RC phase, I'm all for aiming for a minimal solution.\n>> However, I don't think that the better fix would be erm.. any \"less\n>> minimal\":\n>\n> Thanks again. May we have your sign-off?\n\nSure:\n\nSigned-off-by: SZEDER Gábor \n\nThank you for the commit message; had a family visit the last couple\nof days and could not get around to it.\n\n\n> contrib/completion/git-completion.bash | 5 ++++-\n> contrib/completion/git-completion.zsh | 2 +-\n> 2 files changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 12814e9bbf..f4a2e6774b 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -3223,7 +3223,10 @@ __gitk_main ()\n> __git_complete_revlist\n> }\n>\n> -if [[ -n ${ZSH_VERSION-} ]]; then\n> +if [[ -n ${ZSH_VERSION-} ]] &&\n> + # Don't define these functions when sourced from 'git-completion.zsh',\n> + # it has its own implementations.\n> + [[ -z ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then\n> echo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n>\n> autoload -U +X compinit && compinit\n> diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\n> index 53cb0f934f..049d6b80f6 100644\n> --- a/contrib/completion/git-completion.zsh\n> +++ b/contrib/completion/git-completion.zsh\n> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n> test -f $e && script=\"$e\" && break\n> done\n> fi\n> -ZSH_VERSION='' . \"$script\"\n> +GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n>\n> __gitcomp ()\n> {\n> --\n> 2.18.0.rc1.242.g61856ae69a\n>\n"},{"id":"349976","messageId":"CAM0VKjm7Nap_tH-0Nd_QxabEPDL=71sVifo8cqhjMkqZOERJTA@mail.gmail.com","threadId":"48223","inReplyTo":"20180611182053.GA20665@aiede.svl.corp.google.com","subject":"Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-12T09:48:35Z","receivedAt":"2018-06-12T09:48:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jun 11, 2018 at 8:20 PM, Jonathan Nieder wrote:\n> From: SZEDER Gábor \n> Subject: completion: correct zsh detection when run from git-completion.zsh\n>\n> v2.18.0-rc0~90^2 (completion: reduce overhead of clearing cached\n> --options, 2018-04-18) worked around a bug in bash's \"set\" builtin on\n> MacOS by using compgen instead. It was careful to avoid breaking zsh\n> by guarding this workaround with\n>\n> if [[ -n ${ZSH_VERSION-}} ]]\n>\n> Alas, this interacts poorly with git-completion.zsh's bash emulation:\n>\n> ZSH_VERSION='' . \"$script\"\n>\n> Correct it by instead using a new GIT_SOURCING_ZSH_COMPLETION shell\n> variable to detect whether git-completion.bash is being sourced from\n> git-completion.zsh. This way, the zsh variant is used both when run\n> from zsh directly and when run via git-completion.zsh.\n>\n> Reproduction recipe:\n>\n> 1. cd git/contrib/completion && cp git-completion.zsh _git\n> 2. Put the following in a new ~/.zshrc file:\n>\n> autoload -U compinit; compinit\n> autoload -U bashcompinit; bashcompinit\n> fpath=(~/src/git/contrib/completion $fpath)\n>\n> 3. Open zsh and \"git \".\n>\n> With this patch:\n> Triggers nice git-completion.bash based tab completion\n>\n> Without:\n> contrib/completion/git-completion.bash:354: read-only variable: QISUFFIX\n> zsh:12: command not found: ___main\n> zsh:15: _default: function definition file not found\n> _dispatch:70: bad math expression: operand expected at `/usr/bin/g...'\n> Segmentation fault\n\nBtw, even if ZSH were not to segfault, the resulting shell would be\nnext to useless. The thing is that 'compgen -v foo' is supposed to\nlist only variable names starting with 'foo', at least that's what it\ndoes under Bash. In ZSH's Bash emulation, however, it lists _all_\nvariable names, including such fundamental env vars like PATH, HOME,\nand IFS. So when ZSH took the wrong if branch and run that\n\n unset $(compgen -v __gitcomp_builtin_)\n\nthen it would unset PATH and everything else as well.\n"},{"id":"349979","messageId":"CAJAwA=wQZ4b-wsy8yqURVK80hv9efGD7LZKCq89sVssBd5cgqA@mail.gmail.com","threadId":"48223","inReplyTo":"20180611182053.GA20665@aiede.svl.corp.google.com","subject":"Re: [PATCH] completion: correct zsh detection when run from git-completion.zsh (Re: [PATCH v2] completion: reduce overhead of clearing cached --options)","fromName":"Rick van Hattem","fromEmail":"wolph@wol.ph","sentAt":"2018-06-12T09:51:09Z","receivedAt":"2018-06-12T09:51:32Z","isPatch":true,"sender":{"key":"wolph@wol.ph","avatar":"https://gravatar.com/avatar/3e86569f70d74646eb1f682e8d28f965114e44e067364be2154d74e7cb6b702a?d=mp&s=160"},"body":"On 11 June 2018 at 20:20, Jonathan Nieder wrote:\n> SZEDER Gábor wrote:\n>\n>> Being in RC phase, I'm all for aiming for a minimal solution.\n>> However, I don't think that the better fix would be erm.. any \"less\n>> minimal\":\n>\n> Thanks again. May we have your sign-off?\n>\n> contrib/completion/git-completion.bash | 5 ++++-\n> contrib/completion/git-completion.zsh | 2 +-\n> 2 files changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 12814e9bbf..f4a2e6774b 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -3223,7 +3223,10 @@ __gitk_main ()\n> __git_complete_revlist\n> }\n>\n> -if [[ -n ${ZSH_VERSION-} ]]; then\n> +if [[ -n ${ZSH_VERSION-} ]] &&\n> + # Don't define these functions when sourced from 'git-completion.zsh',\n> + # it has its own implementations.\n> + [[ -z ${GIT_SOURCING_ZSH_COMPLETION-} ]]; then\n> echo \"WARNING: this script is deprecated, please see git-completion.zsh\" 1>&2\n>\n> autoload -U +X compinit && compinit\n> diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\n> index 53cb0f934f..049d6b80f6 100644\n> --- a/contrib/completion/git-completion.zsh\n> +++ b/contrib/completion/git-completion.zsh\n> @@ -39,7 +39,7 @@ if [ -z \"$script\" ]; then\n> test -f $e && script=\"$e\" && break\n> done\n> fi\n> -ZSH_VERSION='' . \"$script\"\n> +GIT_SOURCING_ZSH_COMPLETION=y . \"$script\"\n>\n> __gitcomp ()\n> {\n> --\n> 2.18.0.rc1.242.g61856ae69a\n\nThe change looks good to me :)\n"}]}