{"thread":{"id":"49626","subject":"[PATCH] completion: fix __gitcomp_builtin no longer consider extra options","startedAt":"2018-10-21T08:37:47Z","lastAt":"2018-10-22T17:49:50Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361085","messageId":"20181021083731.8009-1-pclouds@gmail.com","threadId":"49626","inReplyTo":null,"subject":"[PATCH] completion: fix __gitcomp_builtin no longer consider extra options","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-21T08:37:31Z","receivedAt":"2018-10-21T08:37:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"__gitcomp_builtin() has the main completion list provided by\n\n git xxx --git-completion-helper\n\nbut the caller can also add extra options that is not provided by\n--git-completion-helper. The only call site that does this is \"git\ndifftool\" completion.\n\nThis support is broken by b221b5ab9b (completion: collapse extra\n--no-.. options - 2018-06-06), which adds a special value \"--\" to mark\nthat the rest of the options can be hidden by default. The commit\nforgets the fact that extra options are appended after\n\"$(git xxx --git-completion-helper)\", i.e. after this \"--\", and will\nbe incorrectly hidden as well.\n\nPrepend the extra options before \"$(git xxx --git-completion-helper)\"\nto avoid this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \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 db7fd87b6b..c8fdcf8644 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -400,7 +400,7 @@ __gitcomp_builtin ()\n \tif [ -z \"$options\" ]; then\n \t\t# leading and trailing spaces are significant to make\n \t\t# option removal work correctly.\n-\t\toptions=\" $(__git ${cmd/_/ } --git-completion-helper) $incl \"\n+\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n \t\tfor i in $excl; do\n \t\t\toptions=\"${options/ $i / }\"\n \t\tdone\n-- \n2.19.1.647.g708186aaf9\n\n"},{"id":"361130","messageId":"xmqqr2giwsep.fsf@gitster-ct.c.googlers.com","threadId":"49626","inReplyTo":"20181021083731.8009-1-pclouds@gmail.com","subject":"Re: [PATCH] completion: fix __gitcomp_builtin no longer consider extra options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T03:51:10Z","receivedAt":"2018-10-22T03:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> __gitcomp_builtin() has the main completion list provided by\n>\n> git xxx --git-completion-helper\n>\n> but the caller can also add extra options that is not provided by\n> --git-completion-helper. The only call site that does this is \"git\n> difftool\" completion.\n>\n> This support is broken by b221b5ab9b (completion: collapse extra\n> --no-.. options - 2018-06-06), which adds a special value \"--\" to mark\n> that the rest of the options can be hidden by default. The commit\n> forgets the fact that extra options are appended after\n> \"$(git xxx --git-completion-helper)\", i.e. after this \"--\", and will\n> be incorrectly hidden as well.\n>\n> Prepend the extra options before \"$(git xxx --git-completion-helper)\"\n> to avoid this.\n\nThanks for a clear analysis. How did you find it? Got annoyed that\ncompletion of difftool got broken, or discovered while trying to\napply the same trick as difftool completion uses to another one and\nseeing that the technique does not work?\n\nWill queue. Thanks.\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \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 db7fd87b6b..c8fdcf8644 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -400,7 +400,7 @@ __gitcomp_builtin ()\n> \tif [ -z \"$options\" ]; then\n> \t\t# leading and trailing spaces are significant to make\n> \t\t# option removal work correctly.\n> -\t\toptions=\" $(__git ${cmd/_/ } --git-completion-helper) $incl \"\n> +\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n> \t\tfor i in $excl; do\n> \t\t\toptions=\"${options/ $i / }\"\n> \t\tdone\n"},{"id":"361162","messageId":"CACsJy8B-NtKb1sk_CxwB-bffMsy+nDGL2-MpkScBF5vvGHFBjA@mail.gmail.com","threadId":"49626","inReplyTo":"xmqqr2giwsep.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] completion: fix __gitcomp_builtin no longer consider extra options","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-22T14:34:16Z","receivedAt":"2018-10-22T14:34:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 22, 2018 at 5:51 AM Junio C Hamano wrote:\n>\n> Nguyễn Thái Ngọc Duy writes:\n>\n> > __gitcomp_builtin() has the main completion list provided by\n> >\n> > git xxx --git-completion-helper\n> >\n> > but the caller can also add extra options that is not provided by\n> > --git-completion-helper. The only call site that does this is \"git\n> > difftool\" completion.\n> >\n> > This support is broken by b221b5ab9b (completion: collapse extra\n> > --no-.. options - 2018-06-06), which adds a special value \"--\" to mark\n> > that the rest of the options can be hidden by default. The commit\n> > forgets the fact that extra options are appended after\n> > \"$(git xxx --git-completion-helper)\", i.e. after this \"--\", and will\n> > be incorrectly hidden as well.\n> >\n> > Prepend the extra options before \"$(git xxx --git-completion-helper)\"\n> > to avoid this.\n>\n> Thanks for a clear analysis. How did you find it? Got annoyed that\n> completion of difftool got broken, or discovered while trying to\n> apply the same trick as difftool completion uses to another one and\n> seeing that the technique does not work?\n\nI was fixing format-patch completion and was surprised it did not work\nas expected. Never really used difftool myself :P I only found out\nabout it when I asked myself \"why wasn't this breakage found earlier?\"\n-- \nDuy\n"},{"id":"361173","messageId":"20181022174944.GH30222@szeder.dev","threadId":"49626","inReplyTo":"CACsJy8B-NtKb1sk_CxwB-bffMsy+nDGL2-MpkScBF5vvGHFBjA@mail.gmail.com","subject":"Re: [PATCH] completion: fix __gitcomp_builtin no longer consider extra options","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T17:49:44Z","receivedAt":"2018-10-22T17:49:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 22, 2018 at 04:34:16PM +0200, Duy Nguyen wrote:\n> On Mon, Oct 22, 2018 at 5:51 AM Junio C Hamano wrote:\n> >\n> > Nguyễn Thái Ngọc Duy writes:\n> >\n> > > __gitcomp_builtin() has the main completion list provided by\n> > >\n> > > git xxx --git-completion-helper\n> > >\n> > > but the caller can also add extra options that is not provided by\n> > > --git-completion-helper. The only call site that does this is \"git\n> > > difftool\" completion.\n> > >\n> > > This support is broken by b221b5ab9b (completion: collapse extra\n> > > --no-.. options - 2018-06-06), which adds a special value \"--\" to mark\n> > > that the rest of the options can be hidden by default. The commit\n> > > forgets the fact that extra options are appended after\n> > > \"$(git xxx --git-completion-helper)\", i.e. after this \"--\", and will\n> > > be incorrectly hidden as well.\n> > >\n> > > Prepend the extra options before \"$(git xxx --git-completion-helper)\"\n> > > to avoid this.\n> >\n> > Thanks for a clear analysis. How did you find it? Got annoyed that\n> > completion of difftool got broken, or discovered while trying to\n> > apply the same trick as difftool completion uses to another one and\n> > seeing that the technique does not work?\n> \n> I was fixing format-patch completion and was surprised it did not work\n> as expected. Never really used difftool myself :P I only found out\n> about it when I asked myself \"why wasn't this breakage found earlier?\"\n\nErm... maybe because there are no tests covering the combination of\nextra options and '--no-..' options in 't9902-completion.sh'?\n\n(Hint, hint... :)\n\n"}]}