{"thread":{"id":"55821","subject":"[PATCH v3] contrib/completion: fix zsh completion regression from 59d85a2a05","startedAt":"2021-06-01T16:53:00Z","lastAt":"2021-06-01T23:33:44Z","messageCount":4,"participants":["David Aguilar","Felipe Contreras"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"426150","messageId":"20210601165254.18136-1-davvid@gmail.com","threadId":"55821","inReplyTo":null,"subject":"[PATCH v3] contrib/completion: fix zsh completion regression from 59d85a2a05","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-06-01T16:52:54Z","receivedAt":"2021-06-01T16:53:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"A recent change to make git-completion.bash use $__git_cmd_idx\nin more places broke a number of completions on zsh because it\nmodified __git_main but did not update __git_zsh_main.\n\nNotably, completions for \"add\", \"branch\", \"mv\" and \"push\" were\nbroken as a result of this change.\n\nIn addition to the undefined variable usage, \"git mv <tab>\" also\nprints the following error:\n\n\t__git_count_arguments:7: bad math expression:\n\toperand expected at `\"1\"'\n\n\t_git_mv:[:7: unknown condition: -gt\n\nRemove the quotes around $__git_cmd_idx in __git_count_arguments\nand set __git_cmd_idx=1 early in __git_zsh_main to fix the\nregressions from 59d85a2a05.\n\nAdd \"git\" to the \"words\" array in _git_zsh_main to guarantee\nthat \"git\" is at least always in the completion list. It may not\nbe completely correct because __git_cmd_idx is not always 1,\nbut it is enough to fix the regression.\n\nThis was tested on zsh 5.7.1 (x86_64-apple-darwin19.0).\n\nHelped-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n contrib/completion/git-completion.bash | 2 +-\n contrib/completion/git-completion.zsh  | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3c5739b905..b50c5d0ea3 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1306,7 +1306,7 @@ __git_count_arguments ()\n \tlocal word i c=0\n \n \t# Skip \"git\" (first argument)\n-\tfor ((i=\"$__git_cmd_idx\"; i < ${#words[@]}; i++)); do\n+\tfor ((i=$__git_cmd_idx; i < ${#words[@]}; i++)); do\n \t\tword=\"${words[i]}\"\n \n \t\tcase \"$word\" in\ndiff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh\nindex 6c56296997..6405d7ba30 100644\n--- a/contrib/completion/git-completion.zsh\n+++ b/contrib/completion/git-completion.zsh\n@@ -251,7 +251,7 @@ __git_zsh_main ()\n \t\tdone\n \t\t;;\n \t(arg)\n-\t\tlocal command=\"${words[1]}\" __git_dir\n+\t\tlocal command=\"${words[1]}\" __git_dir __git_cmd_idx=1\n \n \t\tif (( $+opt_args[--bare] )); then\n \t\t\t__git_dir='.'\n@@ -261,7 +261,7 @@ __git_zsh_main ()\n \n \t\t(( $+opt_args[--help] )) && command='help'\n \n-\t\twords=( ${orig_words[@]} )\n+\t\twords=( git ${orig_words[@]} )\n \n \t\t__git_zsh_bash_func $command\n \t\t;;\n-- \n2.32.0.rc2.1.gf67de2b3ac\n\n"},{"id":"426155","messageId":"60b6873624c6f_1a702085e@natae.notmuch","threadId":"55821","inReplyTo":"20210601165254.18136-1-davvid@gmail.com","subject":"RE: [PATCH v3] contrib/completion: fix zsh completion regression from 59d85a2a05","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-01T19:15:02Z","receivedAt":"2021-06-01T19:15:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"David Aguilar wrote:\n> A recent change to make git-completion.bash use $__git_cmd_idx\n> Add \"git\" to the \"words\" array in _git_zsh_main to guarantee\n> that \"git\" is at least always in the completion list.\n\nHm, no. The current code already guarantees \"git\" is always at the start\nof the completion list. In [1] I suggested to add git *if* $words is\nused instead of $orig_words.\n\nIf you add \"git\" to $orig_words you end up with something like\n\"git git mv\", so the __git_cmd_idx is definitely not 1.\n\nYou should probably try to test yourself:\n\n  words=( git ${words[@]} )\n  echo \"$words\" >> /tmp/words-log.txt\n\nThe problem is that zsh's _arguments eats all the words it finds, so for\nexample if you type:\n\n  git mv --force <tab>\n\n$words will be 'mv --force'.\n\nIt's better to use $words because in case there's arguments beforehand,\nlike:\n\n  git --git-dir=/tmp/test/.git mv --force\n\n$words will be 'mv --force', so we can get the proper index by just\nadding 'git' beforehand.\n\nBut it doesn't work for arguments not in _arguments, like:\n\n  git --foo mv --force\n\nWhich returns:\n\n  --foo mv --force\n\nAnd unfortunately upstream's version of the wrapper doesn't understand\nmany arguments, like -c, or -C. git-completion does have all of them\n\nIt's better to just leave the code as it is and just fix the regression\nby adding __git_cmd_idx=1.\n\n> Helped-by: Felipe Contreras <felipe.contreras@gmail.com>\n\nI mean I kind of wrote 2 of the 3 lines you sent, can I get a\nSuggested-by?\n\n> --- a/contrib/completion/git-completion.zsh\n> +++ b/contrib/completion/git-completion.zsh\n> @@ -251,7 +251,7 @@ __git_zsh_main ()\n>  \t\tdone\n>  \t\t;;\n>  \t(arg)\n> -\t\tlocal command=\"${words[1]}\" __git_dir\n> +\t\tlocal command=\"${words[1]}\" __git_dir __git_cmd_idx=1\n\nThis is needed.\n\n>  \n>  \t\tif (( $+opt_args[--bare] )); then\n>  \t\t\t__git_dir='.'\n> @@ -261,7 +261,7 @@ __git_zsh_main ()\n>  \n>  \t\t(( $+opt_args[--help] )) && command='help'\n>  \n> -\t\twords=( ${orig_words[@]} )\n> +\t\twords=( git ${orig_words[@]} )\n\nThis is wrong. The current code is fine.\n\nCheers.\n\n[1] https://lore.kernel.org/git/60b3c2d7557bd_be762089a@natae.notmuch/\n\n-- \nFelipe Contreras\n"},{"id":"426161","messageId":"CAJDDKr630n7=8FqsSRXpN7fNy9ZB8FOzO=LxvfHFVrMR1=L3_A@mail.gmail.com","threadId":"55821","inReplyTo":"60b6873624c6f_1a702085e@natae.notmuch","subject":"Re: [PATCH v3] contrib/completion: fix zsh completion regression from 59d85a2a05","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-06-01T20:59:54Z","receivedAt":"2021-06-01T21:00:34Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jun 1, 2021 at 12:15 PM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> David Aguilar wrote:\n> > A recent change to make git-completion.bash use $__git_cmd_idx\n> > Add \"git\" to the \"words\" array in _git_zsh_main to guarantee\n> > that \"git\" is at least always in the completion list.\n>\n> Hm, no. The current code already guarantees \"git\" is always at the start\n> of the completion list. In [1] I suggested to add git *if* $words is\n> used instead of $orig_words.\n>\n> If you add \"git\" to $orig_words you end up with something like\n> \"git git mv\", so the __git_cmd_idx is definitely not 1.\n>\n> You should probably try to test yourself:\n>\n>   words=( git ${words[@]} )\n>   echo \"$words\" >> /tmp/words-log.txt\n>\n> The problem is that zsh's _arguments eats all the words it finds, so for\n> example if you type:\n>\n>   git mv --force <tab>\n>\n> $words will be 'mv --force'.\n>\n> It's better to use $words because in case there's arguments beforehand,\n> like:\n>\n>   git --git-dir=/tmp/test/.git mv --force\n>\n> $words will be 'mv --force', so we can get the proper index by just\n> adding 'git' beforehand.\n>\n> But it doesn't work for arguments not in _arguments, like:\n>\n>   git --foo mv --force\n>\n> Which returns:\n>\n>   --foo mv --force\n>\n> And unfortunately upstream's version of the wrapper doesn't understand\n> many arguments, like -c, or -C. git-completion does have all of them\n>\n> It's better to just leave the code as it is and just fix the regression\n> by adding __git_cmd_idx=1.\n>\n> > Helped-by: Felipe Contreras <felipe.contreras@gmail.com>\n>\n> I mean I kind of wrote 2 of the 3 lines you sent, can I get a\n> Suggested-by?\n\n\nThe v4 I just sent is now basically the same as v2 modulo the\nSuggested-by: trailer update.\n\n\n>\n> > --- a/contrib/completion/git-completion.zsh\n> > +++ b/contrib/completion/git-completion.zsh\n> > @@ -251,7 +251,7 @@ __git_zsh_main ()\n> >               done\n> >               ;;\n> >       (arg)\n> > -             local command=\"${words[1]}\" __git_dir\n> > +             local command=\"${words[1]}\" __git_dir __git_cmd_idx=1\n>\n> This is needed.\n>\n> >\n> >               if (( $+opt_args[--bare] )); then\n> >                       __git_dir='.'\n> > @@ -261,7 +261,7 @@ __git_zsh_main ()\n> >\n> >               (( $+opt_args[--help] )) && command='help'\n> >\n> > -             words=( ${orig_words[@]} )\n> > +             words=( git ${orig_words[@]} )\n>\n> This is wrong. The current code is fine.\n>\n> Cheers.\n>\n> [1] https://lore.kernel.org/git/60b3c2d7557bd_be762089a@natae.notmuch/\n\nThanks for the detailed explanation.\n\nJust so I'm understanding this correctly.. if this was instead..\n\n    words=( git ${words[@]} )\n\n(instead of orig_words like I mistakenly included in v3) would that be\nan improvement, no-op or would it be worse? It sounds like additional\nchanges are needed to make it properly support options between \"git\"\nand the sub-command name, hence the patch is fine as-is in v4,\ncorrect?\n\nHopefully in the future it can be extended to cover eg. \"git -c\nfoo.bar -C some-dir <sub-command>\" as well. Thanks for your patience.\n\ncheers,\n-- \nDavid\n"},{"id":"426176","messageId":"60b6c3d4d432b_4044208a6@natae.notmuch","threadId":"55821","inReplyTo":"CAJDDKr630n7=8FqsSRXpN7fNy9ZB8FOzO=LxvfHFVrMR1=L3_A@mail.gmail.com","subject":"Re: [PATCH v3] contrib/completion: fix zsh completion regression from 59d85a2a05","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-01T23:33:40Z","receivedAt":"2021-06-01T23:33:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"David Aguilar wrote:\n> On Tue, Jun 1, 2021 at 12:15 PM Felipe Contreras\n> <felipe.contreras@gmail.com> wrote:\n> > > @@ -261,7 +261,7 @@ __git_zsh_main ()\n> > >\n> > >               (( $+opt_args[--help] )) && command='help'\n> > >\n> > > -             words=( ${orig_words[@]} )\n> > > +             words=( git ${orig_words[@]} )\n> >\n> > This is wrong. The current code is fine.\n\n> Thanks for the detailed explanation.\n> \n> Just so I'm understanding this correctly.. if this was instead..\n> \n>     words=( git ${words[@]} )\n> \n> (instead of orig_words like I mistakenly included in v3) would that be\n> an improvement, no-op or would it be worse?\n\nIt would be an improvement, but it's orthogonal to the regression you\nare trying to fix.\n\nI would just fix the regression for v2.32, and then afterwards try to do\nthe improvement.\n\nI have a testing framework for the zsh completion in my git-completion\nproject, so I would be much more confident about this change if all the\ntests pass. Alas I have not yet merged any v2.32.0-rc* so it's not\nstraightforward to run the tests now.\n\n> It sounds like additional changes are needed to make it properly\n> support options between \"git\" and the sub-command name, hence the\n> patch is fine as-is in v4, correct?\n\nI mean there's git options, and git command options. I don't know how\nmany changes are needed to make all the interactions work correctly, but\nI wouldn't have confidence in any of them so close to a release,\nespecially considering git.git doesn't have any zsh tests.\n\nSo yes, v4 is fine.\n\n> Hopefully in the future it can be extended to cover eg. \"git -c\n> foo.bar -C some-dir <sub-command>\" as well. Thanks for your patience.\n\nThat will work correctly on git-completion once I merge your v4 patch\n(and v2.32).\n\n-- \nFelipe Contreras\n"}]}