# [PATCH v4 0/4] completion: couple of cleanups 11 messages from 2012-02-02 to 2012-02-06. Participants: Felipe Contreras, Junio C Hamano, SZEDER Gábor. Thread: https://gitlist.dev/t/29528 ## Felipe Contreras, 2012-02-02 20:30 Subject: [PATCH v4 0/4] completion: couple of cleanups Message-ID: <1328214625-3576-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1328214625-3576-1-git-send-email-felipe.contreras%40gmail.com ``` And an improvement for zsh. v4: Same as v3, but with an improved commit message with input from Thomas Rast and sending directly to Junio. v3: Junio: I see you already picked most of them for 'pu', but I've made further changes based on the feedback: * completion: be nicer with zsh Improved the code-style * completion: simplify __gitcomp* Fix Improved commit message Felipe Contreras (4): completion: work around zsh option propagation bug completion: simplify __git_remotes completion: remove unused code completion: simplify __gitcomp* contrib/completion/git-completion.bash | 66 +++++--------------------------- 1 files changed, 10 insertions(+), 56 deletions(-) -- 1.7.9 ``` ## Junio C Hamano, 2012-02-03 20:23 Subject: Re: [PATCH v4 1/4] completion: work around zsh option propagation bug Message-ID: <7v1uqbpsyh.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1uqbpsyh.fsf%40alter.siamese.dyndns.org In-Reply-To: <1328214625-3576-2-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > Right now when listing commands in zsh (git ), all of them > will show up, instead of only porcelain ones. Jonathan's rewrite goes straight to the root cause instead, which is another way to describe the problem. Explaining user-visible symptoms at the beginning like you did is a good strategy that I would want to see more contributors follow, though. > Basically, in zsh, this: > > for i in $__git_all_commands > > Should be: > > for i in ${=__git_all_commands} > > Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is > set). sh emulation should take care of that, but the subshell is messing > up with that. So __git_list_porcelain_commands does not do any > filtering. Let me step back a bit and see if we are on the same page wrt the root cause of the problem and for whom we are explaining the change. The adaptation of the bash completion script to zsh is done by asking zsh to obey POSIXy word splitting rules to honor $IFS that is in effect when the words are split. However zsh does not do a good job at it in some cases, and your patch works it around by avoiding a construct known to be troublesome to zsh. Am I correct so far? If so, especially if the first sentence of the above paragraph is correct, then how would it help others to teach "this is the right way to do a word-split if we were writing in native zsh" when we are not? While it probably is a good description to have in a bug report given to zsh folks, it is useless for people who read the history of Git. Unless you are writing zsh-native completion script and this patch is not about git-completion.bash, that is. The readers need to read the solution described in order to understand why the updated construct is written in an unnatural (to people who write to POSIXy shells) way, or to avoid reintroducing a similar problem elsewhere in the future. I find that what Jonathan gave you helps them much better: ... fn () { var='one two' printf '%s\n' $var } x=$(fn) : ${y=$(fn)} printing "$x" results in two lines as expected, but printing "$y" results in a single line because $var is expanded as a single word when evaluating fn to compute y. So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)' instead. So I'll take the first two lines of the message (good bits), and simplify the "This fixes a bug tht caused..." from the last paragraph (or perhaps even drop it). Thanks. ``` ## Junio C Hamano, 2012-02-03 20:23 Subject: Re: [PATCH v4 4/4] completion: simplify __gitcomp* Message-ID: <7vty37oedr.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vty37oedr.fsf%40alter.siamese.dyndns.org In-Reply-To: <1328214625-3576-5-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > @@ -495,11 +495,7 @@ fi > # 4: A suffix to be appended to each possible completion word (optional). > __gitcomp () > { > - local cur_="$cur" > - > - if [ $# -gt 2 ]; then > - cur_="$3" > - fi > + local cur_="${3:-$cur}" > case "$cur_" in > --*=) > COMPREPLY=() I think this rewrite is wrong, even though it may not make a difference to the current callers (I didn't check). Drop the colon from ${3:-...}. > @@ -524,18 +520,8 @@ __gitcomp () > # appended. > __gitcomp_nl () > { > - local s=$'\n' IFS=' '$'\t'$'\n' > - local cur_="$cur" suffix=" " > - > - if [ $# -gt 2 ]; then > - cur_="$3" > - if [ $# -gt 3 ]; then > - suffix="$4" > - fi > - fi > - > - IFS=$s > - COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_")) > + local IFS=$'\n' > + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3:-$cur}")) So is this. Fixing the above two gives me what I've already sent in $gmane/189683, so... ``` ## SZEDER Gábor, 2012-02-04 13:54 Subject: Re: [PATCH v4 4/4] completion: simplify __gitcomp* Message-ID: <20120204135404.GF16099@goldbirke> URL: https://gitlist.dev/e/20120204135404.GF16099%40goldbirke In-Reply-To: <7vty37oedr.fsf@alter.siamese.dyndns.org> ``` Hi, On Fri, Feb 03, 2012 at 12:23:12PM -0800, Junio C Hamano wrote: > Felipe Contreras writes: > > > @@ -495,11 +495,7 @@ fi > > # 4: A suffix to be appended to each possible completion word (optional). > > __gitcomp () > > { > > - local cur_="$cur" > > - > > - if [ $# -gt 2 ]; then > > - cur_="$3" > > - fi > > + local cur_="${3:-$cur}" > > case "$cur_" in > > --*=) > > COMPREPLY=() > > I think this rewrite is wrong, even though it may not make a difference to > the current callers (I didn't check). Drop the colon from ${3:-...}. > > > @@ -524,18 +520,8 @@ __gitcomp () > > # appended. > > __gitcomp_nl () > > { > > - local s=$'\n' IFS=' '$'\t'$'\n' > > - local cur_="$cur" suffix=" " > > - > > - if [ $# -gt 2 ]; then > > - cur_="$3" > > - if [ $# -gt 3 ]; then > > - suffix="$4" > > - fi > > - fi > > - > > - IFS=$s > > - COMPREPLY=($(compgen -P "${2-}" -S "$suffix" -W "$1" -- "$cur_")) > > + local IFS=$'\n' > > + COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3:-$cur}")) > > So is this. > > Fixing the above two gives me what I've already sent in $gmane/189683, > so... > Good point, I missed this when pointed out the similar issue with $4 earlier. And it does make a difference, it breaks the completion of a single word in multiple steps, e.g. git log --pretty= master... In such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third argument to __gitcomp() and __gitcomp_nl(), which can be empty strings when the user hits TAB right after the '=' and '..'. Replacing that empty string with $cur is bad, because none of the possible completion words (i.e. $1) will match it, and bash will fall back to filename completion. Without the colon, i.e. using "${3-$cur}", it works as expected. Best, Gábor ``` ## Junio C Hamano, 2012-02-05 20:45 Subject: Re: [PATCH v4 4/4] completion: simplify __gitcomp* Message-ID: <7vmx8xggan.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmx8xggan.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120204135404.GF16099@goldbirke> ``` SZEDER Gábor writes: > And it does make a difference, it breaks the completion of a single > word in multiple steps, e.g. git log --pretty= master... In > such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third > argument to __gitcomp() and __gitcomp_nl(), which can be empty strings > when the user hits TAB right after the '=' and '..'. After saying "this rewrite is wrong", I was actually wondering if I should have said "this rewrite is not faithful to the original". Based on your analysis, the difference does break the callers, so the rewrite is indeed wrong. Thanks for following up. ``` ## Felipe Contreras, 2012-02-05 21:14 Subject: Re: [PATCH v4 4/4] completion: simplify __gitcomp* Message-ID: URL: https://gitlist.dev/e/CAMP44s3ECM2e08wsjMzu9Y4xDRw%2BKVNbMdCeAmxiPn3jNtC59Q%40mail.gmail.com In-Reply-To: <7vmx8xggan.fsf@alter.siamese.dyndns.org> ``` 2012/2/5 Junio C Hamano : > SZEDER Gábor writes: > >> And it does make a difference, it breaks the completion of a single >> word in multiple steps, e.g. git log --pretty= master...  In >> such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third >> argument to __gitcomp() and __gitcomp_nl(), which can be empty strings >> when the user hits TAB right after the '=' and '..'. > > After saying "this rewrite is wrong", I was actually wondering if I should > have said "this rewrite is not faithful to the original".  Based on your > analysis, the difference does break the callers, so the rewrite is indeed > wrong. That's why we need tests for the completion stuff as well. I was thinking on doing that, but if I have to write a peer-reviewed essay with an introduction for the people that are not familiar with the code in each of the patches, I'd rather not. -- Felipe Contreras ``` ## SZEDER Gábor, 2012-02-06 20:53 Subject: Re: [PATCH v4 2/4] completion: simplify __git_remotes Message-ID: <20120206205315.GI16099@goldbirke> URL: https://gitlist.dev/e/20120206205315.GI16099%40goldbirke In-Reply-To: <1328214625-3576-3-git-send-email-felipe.contreras@gmail.com> ``` Hi, On Thu, Feb 02, 2012 at 10:30:23PM +0200, Felipe Contreras wrote: > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index b435b6d..f86b734 100755 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -644,12 +644,7 @@ __git_refs_remotes () > __git_remotes () > { > local i ngoff IFS=$'\n' d="$(__gitdir)" You could also remove the ngoff variable, because with this patch it's not used anymore. > - __git_shopt -q nullglob || ngoff=1 > - __git_shopt -s nullglob > - for i in "$d/remotes"/*; do > - echo ${i#$d/remotes/} > - done > - [ "$ngoff" ] && __git_shopt -u nullglob > + test -d "$d/remotes" && ls -1 "$d/remotes" > for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do > i="${i#remote.}" > echo "${i/.url*/}" > -- > 1.7.9 > ``` ## Felipe Contreras, 2012-02-06 21:04 Subject: Re: [PATCH v4 2/4] completion: simplify __git_remotes Message-ID: URL: https://gitlist.dev/e/CAMP44s2vwQK4Mp%3DqJN-_60NRBY42fxzFLHbh9e9j%3DvL5mrMryg%40mail.gmail.com In-Reply-To: <20120206205315.GI16099@goldbirke> ``` 2012/2/6 SZEDER Gábor : > On Thu, Feb 02, 2012 at 10:30:23PM +0200, Felipe Contreras wrote: >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash >> index b435b6d..f86b734 100755 >> --- a/contrib/completion/git-completion.bash >> +++ b/contrib/completion/git-completion.bash >> @@ -644,12 +644,7 @@ __git_refs_remotes () >>  __git_remotes () >>  { >>       local i ngoff IFS=$'\n' d="$(__gitdir)" > > You could also remove the ngoff variable, because with this patch it's > not used anymore. Right, I thought I did that... The change must have been lost in one of the many revisions =/ -- Felipe Contreras ``` ## Felipe Contreras, 2012-02-06 22:59 Subject: Re: [PATCH v4 1/4] completion: work around zsh option propagation bug Message-ID: URL: https://gitlist.dev/e/CAMP44s3SruBpM74BjWuTLfS%3D_66p7r6rkjJ%2BObLr4bLq0nERNA%40mail.gmail.com In-Reply-To: <7v1uqbpsyh.fsf@alter.siamese.dyndns.org> ``` On Fri, Feb 3, 2012 at 10:23 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Right now when listing commands in zsh (git ), all of them >> will show up, instead of only porcelain ones. > > Jonathan's rewrite goes straight to the root cause instead, which is > another way to describe the problem. > > Explaining user-visible symptoms at the beginning like you did is a good > strategy that I would want to see more contributors follow, though. > >> Basically, in zsh, this: >> >>  for i in $__git_all_commands >> >> Should be: >> >>  for i in ${=__git_all_commands} >> >> Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is >> set). sh emulation should take care of that, but the subshell is messing >> up with that. So __git_list_porcelain_commands does not do any >> filtering. > > Let me step back a bit and see if we are on the same page wrt the root > cause of the problem and for whom we are explaining the change. > > The adaptation of the bash completion script to zsh is done by asking zsh > to obey POSIXy word splitting rules to honor $IFS that is in effect when > the words are split.  However zsh does not do a good job at it in some > cases, and your patch works it around by avoiding a construct known to be > troublesome to zsh. Troublesome to zsh emulation, yeah. > Am I correct so far?  If so, especially if the first sentence of the above > paragraph is correct, then how would it help others to teach "this is the > right way to do a word-split if we were writing in native zsh" when we are > not? Because without that explanation it's quite difficult to know what part of the code would behave differently in zsh, and how. Most people are not familiar with shell features, and would have no idea what "word splitting" means in a practical context. > While it probably is a good description to have in a bug report given to > zsh folks, it is useless for people who read the history of Git. Of course it's not. It tells you that there is indeed an issue in zsh, and not in the way we are using it, as it has been acknowledged by zsh developers. > The readers need to read the solution described in order to understand why > the updated construct is written in an unnatural (to people who write to > POSIXy shells) way, or to avoid reintroducing a similar problem elsewhere > in the future. Doesn't this explain that? --- sh emulation should take care of that, but the subshell is messing up with that. --- Granted, for people not familiar with shell features "subshell" should be accompanied with $(foo). > I find that what Jonathan gave you helps them much better: >        ... >        fn () { >                var='one two' >                printf '%s\n' $var >        } >        x=$(fn) >        : ${y=$(fn)} > >    printing "$x" results in two lines as expected, but printing "$y" results >    in a single line because $var is expanded as a single word when evaluating >    fn to compute y. > >    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)' >    instead. > > So I'll take the first two lines of the message (good bits), and simplify > the "This fixes a bug tht caused..." from the last paragraph (or perhaps > even drop it). I'm not sure about it, because this relies on knowledge of how printf works, and it's not used that often; an example with 'for' would be much more clear IMO. Cheers. -- Felipe Contreras ``` ## Junio C Hamano, 2012-02-06 23:20 Subject: Re: [PATCH v4 1/4] completion: work around zsh option propagation bug Message-ID: <7vvcnj4kif.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvcnj4kif.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Felipe Contreras writes: >> I find that what Jonathan gave you helps them much better: >> ... >> fn () { >> var='one two' >> printf '%s\n' $var >> } >> x=$(fn) >> : ${y=$(fn)} >> >> printing "$x" results in two lines as expected, but printing "$y" results >> in a single line because $var is expanded as a single word when evaluating >> fn to compute y. >> >> So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)' >> instead. >> >> So I'll take the first two lines of the message (good bits), and simplify >> the "This fixes a bug tht caused..." from the last paragraph (or perhaps >> even drop it). > > I'm not sure about it, because this relies on knowledge of how printf > works, and it's not used that often; an example with 'for' would be > much more clear IMO. Meaning, replace the fn() definition with something like: fn () { var='one two' for v in $var do echo "$v" done } I can see that may make the issue easier to see; as you pointed out, it requires no implicit knowledge that printf "loops" over the arguments and applies the format string as manu times as needed to eat them. Let me update the log message before I merge it to 'next'. My main point was to illustrate the problematic pattern for people who write for bash, not for zsh, and that does not change with the above improvement, though ;-). ``` ## Felipe Contreras, 2012-02-06 23:57 Subject: Re: [PATCH v4 1/4] completion: work around zsh option propagation bug Message-ID: URL: https://gitlist.dev/e/CAMP44s1urg0-1GZqLytg0h%3DqT77_zajwsKwNt9HnVfRZoOECqA%40mail.gmail.com In-Reply-To: <7vvcnj4kif.fsf@alter.siamese.dyndns.org> ``` On Tue, Feb 7, 2012 at 1:20 AM, Junio C Hamano wrote: > Felipe Contreras writes: > >>> I find that what Jonathan gave you helps them much better: >>>        ... >>>        fn () { >>>                var='one two' >>>                printf '%s\n' $var >>>        } >>>        x=$(fn) >>>        : ${y=$(fn)} >>> >>>    printing "$x" results in two lines as expected, but printing "$y" results >>>    in a single line because $var is expanded as a single word when evaluating >>>    fn to compute y. >>> >>>    So avoid the construct, and use an explicit 'test -n "$foo" || foo=$(bar)' >>>    instead. >>> >>> So I'll take the first two lines of the message (good bits), and simplify >>> the "This fixes a bug tht caused..." from the last paragraph (or perhaps >>> even drop it). >> >> I'm not sure about it, because this relies on knowledge of how printf >> works, and it's not used that often; an example with 'for' would be >> much more clear IMO. > > Meaning, replace the fn() definition with something like: > >        fn () { >                var='one two' >                for v in $var >                do >                        echo "$v" >                done >        } Yes. > I can see that may make the issue easier to see; as you pointed out, it > requires no implicit knowledge that printf "loops" over the arguments > and applies the format string as manu times as needed to eat them. > Let me update the log message before I merge it to 'next'. > > My main point was to illustrate the problematic pattern for people who > write for bash, not for zsh, and that does not change with the above > improvement, though ;-). True, but I think that's an addendum. Most likely the people working on this script will be thinking on bash terms, and would not notice if they introduce code that is difficult for zsh, even if it's carefully explained in this commit message; it would be zsh people that find the bug, thus that's why I address the zsh side _first_. So, IMO it should look like (looks like a fix has been submitted, so I've updated accordingly): --- completion: work around zsh option propagation bug Right now when listing commands in zsh (git ), all of them will show up, instead of only porcelain ones. This is caused by a bug in zsh[1] that causes subshells to loose the word splitting option (SH_WORD_SPLIT) since 4.3.0[2]. It will probably be fixed in the next release (4.3.16). Basically, in zsh, this: for i in $__git_all_commands Should be: for i in ${=__git_all_commands} Otherwise there's no word-splitting expansion (unless SH_WORD_SPLIT is set). sh emulation should take care of that, but the subshell is messing things up. The visible result is that __git_list_porcelain_commands don't do any filtering. Specifically, the issue is with subshells in parmeter expansion (e.g. '${var=$(func)'): fn () { var='one two' for v in $var; do echo "$v" done } x=$(fn) : ${y=$(fn)} Printing "$x" results in two lines as expected, but printing "$y" results in a single line because $var is expanded as a single word (there's no word-splitting expansion). So avoid the construct, and use an explicit '[ -n "$foo" ] || foo=$(bar)' instead. [1] http://article.gmane.org/gmane.comp.shells.zsh.devel/24296 [2] http://article.gmane.org/gmane.comp.shells.zsh.devel/24338 --- Cheers. -- Felipe Contreras ```