{"thread":{"id":"49710","subject":"[PATCH] completion: use builtin completion for format-patch","startedAt":"2018-10-29T17:58:01Z","lastAt":"2018-11-03T10:20:43Z","messageCount":13,"participants":["Denton Liu","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361933","messageId":"31160e8c63399e8c866eef40462d0b699bae9b59.1540835698.git.liu.denton@gmail.com","threadId":"49710","inReplyTo":null,"subject":"[PATCH] completion: use builtin completion for format-patch","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2018-10-29T17:57:55Z","receivedAt":"2018-10-29T17:58:01Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu <liu.denton@gmail.com>\n---\n contrib/completion/git-completion.bash | 11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex d63d2dffd..da77da481 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1531,15 +1531,6 @@ _git_fetch ()\n \t__git_complete_remote_or_refspec\n }\n \n-__git_format_patch_options=\"\n-\t--stdout --attach --no-attach --thread --thread= --no-thread\n-\t--numbered --start-number --numbered-files --keep-subject --signoff\n-\t--signature --no-signature --in-reply-to= --cc= --full-index --binary\n-\t--not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n-\t--inline --suffix= --ignore-if-in-upstream --subject-prefix=\n-\t--output-directory --reroll-count --to= --quiet --notes\n-\"\n-\n _git_format_patch ()\n {\n \tcase \"$cur\" in\n@@ -1550,7 +1541,7 @@ _git_format_patch ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"$__git_format_patch_options\"\n+\t\t__gitcomp_builtin format-patch\n \t\treturn\n \t\t;;\n \tesac\n-- \n2.19.1\n\n"},{"id":"361956","messageId":"xmqqbm7c2n0i.fsf@gitster-ct.c.googlers.com","threadId":"49710","inReplyTo":"31160e8c63399e8c866eef40462d0b699bae9b59.1540835698.git.liu.denton@gmail.com","subject":"Re: [PATCH] completion: use builtin completion for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-30T02:20:45Z","receivedAt":"2018-10-30T02:20:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 11 +----------\n>  1 file changed, 1 insertion(+), 10 deletions(-)\n\nWe saw a similar change proposed and then found out it was not such\na good idea in:\n\nhttps://public-inbox.org/git/CACsJy8DUrVJu0HN7kuCeo4iV5aimWbYtr+E-7kenPVDx90DpGw@mail.gmail.com/\n\nIt seems that this one loses options like --full-index, --no-prefix,\netc. compared to the earlier effort?\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index d63d2dffd..da77da481 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1531,15 +1531,6 @@ _git_fetch ()\n>  \t__git_complete_remote_or_refspec\n>  }\n>  \n> -__git_format_patch_options=\"\n> -\t--stdout --attach --no-attach --thread --thread= --no-thread\n> -\t--numbered --start-number --numbered-files --keep-subject --signoff\n> -\t--signature --no-signature --in-reply-to= --cc= --full-index --binary\n> -\t--not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n> -\t--inline --suffix= --ignore-if-in-upstream --subject-prefix=\n> -\t--output-directory --reroll-count --to= --quiet --notes\n> -\"\n> -\n>  _git_format_patch ()\n>  {\n>  \tcase \"$cur\" in\n> @@ -1550,7 +1541,7 @@ _git_format_patch ()\n>  \t\treturn\n>  \t\t;;\n>  \t--*)\n> -\t\t__gitcomp \"$__git_format_patch_options\"\n> +\t\t__gitcomp_builtin format-patch\n>  \t\treturn\n>  \t\t;;\n>  \tesac\n"},{"id":"361961","messageId":"20181030035036.GA14941@archbookpro.localdomain","threadId":"49710","inReplyTo":"xmqqbm7c2n0i.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] completion: use builtin completion for format-patch","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2018-10-30T03:50:36Z","receivedAt":"2018-10-30T03:50:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Tue, Oct 30, 2018 at 11:20:45AM +0900, Junio C Hamano wrote:\n> We saw a similar change proposed and then found out it was not such\n> a good idea in:\n> \n> https://public-inbox.org/git/CACsJy8DUrVJu0HN7kuCeo4iV5aimWbYtr+E-7kenPVDx90DpGw@mail.gmail.com/\n> \n> It seems that this one loses options like --full-index, --no-prefix,\n> etc. compared to the earlier effort?\n\nIn _git_send_email, we have the following lines:\n\n\t__gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n\t\t... more options ...\n\t\t$__git_format_patch_options\"\n\nWould it make sense to take the old `__git_format_patch_options` and\njust roll them into here, then make `_git_format_patch` use\n`__gitcomp_builtin format-patch`? That way, we'd be able to reap the\nbenefits of using `__gitcomp_builtin` where we can.\n"},{"id":"361967","messageId":"72331ce9275ce995009fe8dd3d586bb9d71f2cbf.1540881141.git.liu.denton@gmail.com","threadId":"49710","inReplyTo":"xmqqbm7c2n0i.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] completion: use builtin completion for format-patch","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2018-10-30T06:38:00Z","receivedAt":"2018-10-30T06:38:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"This patch offloads completion functionality for format-patch to\n__gitcomp_builtin. In addition to this, send-email was borrowing some\ncompletion options from format-patch so those options are moved into\nsend-email's completions.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n\nI ran t9902-completion.sh on this patch and it seems to pass. Thus, this\nshould address the concerns about losing some completion options in\nsend-email.\n\n---\n contrib/completion/git-completion.bash | 34 +++++++++++---------------\n 1 file changed, 14 insertions(+), 20 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex d63d2dffd..cb4ef6723 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1531,15 +1531,6 @@ _git_fetch ()\n \t__git_complete_remote_or_refspec\n }\n \n-__git_format_patch_options=\"\n-\t--stdout --attach --no-attach --thread --thread= --no-thread\n-\t--numbered --start-number --numbered-files --keep-subject --signoff\n-\t--signature --no-signature --in-reply-to= --cc= --full-index --binary\n-\t--not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n-\t--inline --suffix= --ignore-if-in-upstream --subject-prefix=\n-\t--output-directory --reroll-count --to= --quiet --notes\n-\"\n-\n _git_format_patch ()\n {\n \tcase \"$cur\" in\n@@ -1550,7 +1541,7 @@ _git_format_patch ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"$__git_format_patch_options\"\n+\t\t__gitcomp_builtin format-patch\n \t\treturn\n \t\t;;\n \tesac\n@@ -2080,16 +2071,19 @@ _git_send_email ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n-\t\t\t--compose --confirm= --dry-run --envelope-sender\n-\t\t\t--from --identity\n-\t\t\t--in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n-\t\t\t--no-suppress-from --no-thread --quiet --reply-to\n-\t\t\t--signed-off-by-cc --smtp-pass --smtp-server\n-\t\t\t--smtp-server-port --smtp-encryption= --smtp-user\n-\t\t\t--subject --suppress-cc= --suppress-from --thread --to\n-\t\t\t--validate --no-validate\n-\t\t\t$__git_format_patch_options\"\n+\t\t__gitcomp \"--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd\n+\t\t\t--chain-reply-to --compose --confirm= --cover-letter --dry-run\n+\t\t\t--dst-prefix= --envelope-sender --from --full-index --identity\n+\t\t\t--ignore-if-in-upstream --inline --in-reply-to --in-reply-to=\n+\t\t\t--keep-subject --no-attach --no-chain-reply-to --no-prefix\n+\t\t\t--no-signature --no-signed-off-by-cc --no-suppress-from --not --notes\n+\t\t\t--no-thread --no-validate --numbered --numbered-files\n+\t\t\t--output-directory --quiet --reply-to --reroll-count --signature\n+\t\t\t--signed-off-by-cc --signoff --smtp-encryption= --smtp-pass\n+\t\t\t--smtp-server --smtp-server-port --smtp-user --src-prefix=\n+\t\t\t--start-number --stdout --subject --subject-prefix= --suffix=\n+\t\t\t--suppress-cc= --suppress-from --thread --thread= --to --to=\n+\t\t\t--validate\"\n \t\treturn\n \t\t;;\n \tesac\n-- \n2.19.1\n\n"},{"id":"361968","messageId":"xmqqo9bb28jj.fsf@gitster-ct.c.googlers.com","threadId":"49710","inReplyTo":"72331ce9275ce995009fe8dd3d586bb9d71f2cbf.1540881141.git.liu.denton@gmail.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-30T07:33:20Z","receivedAt":"2018-10-30T07:33:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> This patch offloads completion functionality for format-patch to\n> __gitcomp_builtin. In addition to this, send-email was borrowing some\n> completion options from format-patch so those options are moved into\n> send-email's completions.\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>\n> I ran t9902-completion.sh on this patch and it seems to pass. Thus, this\n> should address the concerns about losing some completion options in\n> send-email.\n\nThanks.  I added those who were involved in the review thread of the\noriginal patch last week to CC.  Let's see how well this round is\nreceived.\n\n> ---\n>  contrib/completion/git-completion.bash | 34 +++++++++++---------------\n>  1 file changed, 14 insertions(+), 20 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index d63d2dffd..cb4ef6723 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1531,15 +1531,6 @@ _git_fetch ()\n>  \t__git_complete_remote_or_refspec\n>  }\n>  \n> -__git_format_patch_options=\"\n> -\t--stdout --attach --no-attach --thread --thread= --no-thread\n> -\t--numbered --start-number --numbered-files --keep-subject --signoff\n> -\t--signature --no-signature --in-reply-to= --cc= --full-index --binary\n> -\t--not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n> -\t--inline --suffix= --ignore-if-in-upstream --subject-prefix=\n> -\t--output-directory --reroll-count --to= --quiet --notes\n> -\"\n> -\n>  _git_format_patch ()\n>  {\n>  \tcase \"$cur\" in\n> @@ -1550,7 +1541,7 @@ _git_format_patch ()\n>  \t\treturn\n>  \t\t;;\n>  \t--*)\n> -\t\t__gitcomp \"$__git_format_patch_options\"\n> +\t\t__gitcomp_builtin format-patch\n>  \t\treturn\n>  \t\t;;\n>  \tesac\n> @@ -2080,16 +2071,19 @@ _git_send_email ()\n>  \t\treturn\n>  \t\t;;\n>  \t--*)\n> -\t\t__gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n> -\t\t\t--compose --confirm= --dry-run --envelope-sender\n> -\t\t\t--from --identity\n> -\t\t\t--in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n> -\t\t\t--no-suppress-from --no-thread --quiet --reply-to\n> -\t\t\t--signed-off-by-cc --smtp-pass --smtp-server\n> -\t\t\t--smtp-server-port --smtp-encryption= --smtp-user\n> -\t\t\t--subject --suppress-cc= --suppress-from --thread --to\n> -\t\t\t--validate --no-validate\n> -\t\t\t$__git_format_patch_options\"\n> +\t\t__gitcomp \"--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd\n> +\t\t\t--chain-reply-to --compose --confirm= --cover-letter --dry-run\n> +\t\t\t--dst-prefix= --envelope-sender --from --full-index --identity\n> +\t\t\t--ignore-if-in-upstream --inline --in-reply-to --in-reply-to=\n> +\t\t\t--keep-subject --no-attach --no-chain-reply-to --no-prefix\n> +\t\t\t--no-signature --no-signed-off-by-cc --no-suppress-from --not --notes\n> +\t\t\t--no-thread --no-validate --numbered --numbered-files\n> +\t\t\t--output-directory --quiet --reply-to --reroll-count --signature\n> +\t\t\t--signed-off-by-cc --signoff --smtp-encryption= --smtp-pass\n> +\t\t\t--smtp-server --smtp-server-port --smtp-user --src-prefix=\n> +\t\t\t--start-number --stdout --subject --subject-prefix= --suffix=\n> +\t\t\t--suppress-cc= --suppress-from --thread --thread= --to --to=\n> +\t\t\t--validate\"\n>  \t\treturn\n>  \t\t;;\n>  \tesac\n"},{"id":"361993","messageId":"CACsJy8Ajc0qWw3RLak1PRPXvQzMHjaFuWJ6BPkhiVg=7fQvCnA@mail.gmail.com","threadId":"49710","inReplyTo":"72331ce9275ce995009fe8dd3d586bb9d71f2cbf.1540881141.git.liu.denton@gmail.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-30T15:29:07Z","receivedAt":"2018-10-30T15:29:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 30, 2018 at 7:39 AM Denton Liu <liu.denton@gmail.com> wrote:\n>\n> This patch offloads completion functionality for format-patch to\n> __gitcomp_builtin. In addition to this, send-email was borrowing some\n> completion options from format-patch so those options are moved into\n> send-email's completions.\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>\n> I ran t9902-completion.sh on this patch and it seems to pass. Thus, this\n> should address the concerns about losing some completion options in\n> send-email.\n>\n> ---\n>  contrib/completion/git-completion.bash | 34 +++++++++++---------------\n>  1 file changed, 14 insertions(+), 20 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index d63d2dffd..cb4ef6723 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1531,15 +1531,6 @@ _git_fetch ()\n>         __git_complete_remote_or_refspec\n>  }\n>\n> -__git_format_patch_options=\"\n> -       --stdout --attach --no-attach --thread --thread= --no-thread\n> -       --numbered --start-number --numbered-files --keep-subject --signoff\n> -       --signature --no-signature --in-reply-to= --cc= --full-index --binary\n> -       --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n> -       --inline --suffix= --ignore-if-in-upstream --subject-prefix=\n> -       --output-directory --reroll-count --to= --quiet --notes\n> -\"\n> -\n>  _git_format_patch ()\n>  {\n>         case \"$cur\" in\n> @@ -1550,7 +1541,7 @@ _git_format_patch ()\n>                 return\n>                 ;;\n>         --*)\n> -               __gitcomp \"$__git_format_patch_options\"\n> +               __gitcomp_builtin format-patch\n\nWe do want to keep some extra options back because __gitcomp_builtin\ncannot show all supported options of format-patch. The reason is a\nsubset of options is handled by setup_revisions() which does not have\n--git-completion-helper support.\n\n> @@ -2080,16 +2071,19 @@ _git_send_email ()\n>                 return\n>                 ;;\n>         --*)\n> -               __gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n> -                       --compose --confirm= --dry-run --envelope-sender\n> -                       --from --identity\n> -                       --in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n> -                       --no-suppress-from --no-thread --quiet --reply-to\n> -                       --signed-off-by-cc --smtp-pass --smtp-server\n> -                       --smtp-server-port --smtp-encryption= --smtp-user\n> -                       --subject --suppress-cc= --suppress-from --thread --to\n> -                       --validate --no-validate\n> -                       $__git_format_patch_options\"\n> +               __gitcomp \"--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd\n> +                       --chain-reply-to --compose --confirm= --cover-letter --dry-run\n> +                       --dst-prefix= --envelope-sender --from --full-index --identity\n> +                       --ignore-if-in-upstream --inline --in-reply-to --in-reply-to=\n> +                       --keep-subject --no-attach --no-chain-reply-to --no-prefix\n> +                       --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes\n> +                       --no-thread --no-validate --numbered --numbered-files\n> +                       --output-directory --quiet --reply-to --reroll-count --signature\n> +                       --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass\n> +                       --smtp-server --smtp-server-port --smtp-user --src-prefix=\n> +                       --start-number --stdout --subject --subject-prefix= --suffix=\n> +                       --suppress-cc= --suppress-from --thread --thread= --to --to=\n> +                       --validate\"\n\nI have no comment about this. In an ideal world, sendemail.perl could\nbe taught to support --git-completion-helper but I don't think my\nlittle remaining Perl knowledge (or time) is enough to do it. Perhaps\nthis will do. I don't know.\n-- \nDuy\n"},{"id":"362141","messageId":"xmqqk1lxvaj6.fsf@gitster-ct.c.googlers.com","threadId":"49710","inReplyTo":"CACsJy8Ajc0qWw3RLak1PRPXvQzMHjaFuWJ6BPkhiVg=7fQvCnA@mail.gmail.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-01T01:42:05Z","receivedAt":"2018-11-01T01:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>> -__git_format_patch_options=\"\n>> -       --stdout --attach --no-attach --thread --thread= --no-thread\n>> -       --numbered --start-number --numbered-files --keep-subject --signoff\n>> -       --signature --no-signature --in-reply-to= --cc= --full-index --binary\n>> -       --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n>> -       --inline --suffix= --ignore-if-in-upstream --subject-prefix=\n>> -       --output-directory --reroll-count --to= --quiet --notes\n>> -\"\n>> -\n>>  _git_format_patch ()\n>>  {\n>>         case \"$cur\" in\n>> @@ -1550,7 +1541,7 @@ _git_format_patch ()\n>>                 return\n>>                 ;;\n>>         --*)\n>> -               __gitcomp \"$__git_format_patch_options\"\n>> +               __gitcomp_builtin format-patch\n>\n> We do want to keep some extra options back because __gitcomp_builtin\n> cannot show all supported options of format-patch. The reason is a\n> subset of options is handled by setup_revisions() which does not have\n> --git-completion-helper support.\n\nYeah, that is one of the differences I saw compared to the older\none; thanks for being a careful reviewer.\n\n\n>> @@ -2080,16 +2071,19 @@ _git_send_email ()\n>>                 return\n>>                 ;;\n>>         --*)\n>> -               __gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n>> -                       --compose --confirm= --dry-run --envelope-sender\n>> -                       --from --identity\n>> -                       --in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n>> -                       --no-suppress-from --no-thread --quiet --reply-to\n>> -                       --signed-off-by-cc --smtp-pass --smtp-server\n>> -                       --smtp-server-port --smtp-encryption= --smtp-user\n>> -                       --subject --suppress-cc= --suppress-from --thread --to\n>> -                       --validate --no-validate\n>> -                       $__git_format_patch_options\"\n>> +               __gitcomp \"--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd\n>> +                       --chain-reply-to --compose --confirm= --cover-letter --dry-run\n>> +                       --dst-prefix= --envelope-sender --from --full-index --identity\n>> +                       --ignore-if-in-upstream --inline --in-reply-to --in-reply-to=\n>> +                       --keep-subject --no-attach --no-chain-reply-to --no-prefix\n>> +                       --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes\n>> +                       --no-thread --no-validate --numbered --numbered-files\n>> +                       --output-directory --quiet --reply-to --reroll-count --signature\n>> +                       --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass\n>> +                       --smtp-server --smtp-server-port --smtp-user --src-prefix=\n>> +                       --start-number --stdout --subject --subject-prefix= --suffix=\n>> +                       --suppress-cc= --suppress-from --thread --thread= --to --to=\n>> +                       --validate\"\n>\n> I have no comment about this. In an ideal world, sendemail.perl could\n> be taught to support --git-completion-helper but I don't think my\n> little remaining Perl knowledge (or time) is enough to do it. Perhaps\n> this will do. I don't know.\n\nSo \"all\", \"attach\", etc. are added to this list while these similar\noptions are lost from the other variable?  Is this a good trade-off?\n"},{"id":"362195","messageId":"CACsJy8AowaAW91wGMYbC8aTOB1NoqQN-5NGx=qUCHp0i6zQRDA@mail.gmail.com","threadId":"49710","inReplyTo":"xmqqk1lxvaj6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-01T15:40:54Z","receivedAt":"2018-11-01T15:41:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 1, 2018 at 2:42 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >> @@ -2080,16 +2071,19 @@ _git_send_email ()\n> >>                 return\n> >>                 ;;\n> >>         --*)\n> >> -               __gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n> >> -                       --compose --confirm= --dry-run --envelope-sender\n> >> -                       --from --identity\n> >> -                       --in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n> >> -                       --no-suppress-from --no-thread --quiet --reply-to\n> >> -                       --signed-off-by-cc --smtp-pass --smtp-server\n> >> -                       --smtp-server-port --smtp-encryption= --smtp-user\n> >> -                       --subject --suppress-cc= --suppress-from --thread --to\n> >> -                       --validate --no-validate\n> >> -                       $__git_format_patch_options\"\n> >> +               __gitcomp \"--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd\n> >> +                       --chain-reply-to --compose --confirm= --cover-letter --dry-run\n> >> +                       --dst-prefix= --envelope-sender --from --full-index --identity\n> >> +                       --ignore-if-in-upstream --inline --in-reply-to --in-reply-to=\n> >> +                       --keep-subject --no-attach --no-chain-reply-to --no-prefix\n> >> +                       --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes\n> >> +                       --no-thread --no-validate --numbered --numbered-files\n> >> +                       --output-directory --quiet --reply-to --reroll-count --signature\n> >> +                       --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass\n> >> +                       --smtp-server --smtp-server-port --smtp-user --src-prefix=\n> >> +                       --start-number --stdout --subject --subject-prefix= --suffix=\n> >> +                       --suppress-cc= --suppress-from --thread --thread= --to --to=\n> >> +                       --validate\"\n> >\n> > I have no comment about this. In an ideal world, sendemail.perl could\n> > be taught to support --git-completion-helper but I don't think my\n> > little remaining Perl knowledge (or time) is enough to do it. Perhaps\n> > this will do. I don't know.\n>\n> So \"all\", \"attach\", etc. are added to this list while these similar\n> options are lost from the other variable?  Is this a good trade-off?\n\nNot sure if I understand you correctly, but it looks to me that the\noptions in git-send-email.perl are well organized, so we could add\n--git-completon-helper in that script to print all send-email specific\noptions, then call \"git format-patch --git-completion-helper\" to add a\nbunch more. The options that are handled by setup_revisions() will\nhave to be maintained manually here like $__git_format_patch_options\nand added on top in both _git_send_email () and _git_format_patch ().\n\nSo, nothing option is lost and by the time setup_revisions() supports\n-git-completion-helper, we can get rid of the manual shell variable\ntoo. The downside is, lots of work, probably.\n-- \nDuy\n"},{"id":"362218","messageId":"xmqqzhuss6dd.fsf@gitster-ct.c.googlers.com","threadId":"49710","inReplyTo":"CACsJy8AowaAW91wGMYbC8aTOB1NoqQN-5NGx=qUCHp0i6zQRDA@mail.gmail.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-01T23:52:30Z","receivedAt":"2018-11-01T23:52:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>> > I have no comment about this. In an ideal world, sendemail.perl could\n>> > be taught to support --git-completion-helper but I don't think my\n>> > little remaining Perl knowledge (or time) is enough to do it. Perhaps\n>> > this will do. I don't know.\n>>\n>> So \"all\", \"attach\", etc. are added to this list while these similar\n>> options are lost from the other variable?  Is this a good trade-off?\n>\n> Not sure if I understand you correctly, but it looks to me that the\n> options in git-send-email.perl are well organized, so we could...\n\nYes, but I wasn't commenting on your \"sendemail should also be able\nto help completion by supporting --completion-helper option\" (I think\nthat is a sensible approach).  My comment was about Denton's patch,\nwhich reduced the hard-coded list of format-patch options (i.e. the\nfirst hunk) but had to add back many of them to send-email's\ncompletion (i.e. the last hunk)---overall, it did not help reducing\nthe number of options hardcoded in the script.\n\nIf it makes sense to complete all options to format-patch to\nsend-email, then as you outlined, grabbing them out of format-patch\nwith the --completion-helper option at runtime, and using them to\ncomplete both format-patch and send-email would be a good idea.  And\nthat should be doable even before send-email learns how to list its\nsupported options to help the completion.\n\n> --git-completon-helper in that script to print all send-email specific\n> options, then call \"git format-patch --git-completion-helper\" to add a\n> bunch more. The options that are handled by setup_revisions() will\n> have to be maintained manually here like $__git_format_patch_options\n> and added on top in both _git_send_email () and _git_format_patch ().\n>\n> So, nothing option is lost and by the time setup_revisions() supports\n> -git-completion-helper, we can get rid of the manual shell variable\n> too. The downside is, lots of work, probably.\n"},{"id":"362334","messageId":"20181103060317.GA5432@duynguyen.home","threadId":"49710","inReplyTo":"xmqqzhuss6dd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-03T06:03:18Z","receivedAt":"2018-11-03T06:03:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 02, 2018 at 08:52:30AM +0900, Junio C Hamano wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n> >> > I have no comment about this. In an ideal world, sendemail.perl could\n> >> > be taught to support --git-completion-helper but I don't think my\n> >> > little remaining Perl knowledge (or time) is enough to do it. Perhaps\n> >> > this will do. I don't know.\n> >>\n> >> So \"all\", \"attach\", etc. are added to this list while these similar\n> >> options are lost from the other variable?  Is this a good trade-off?\n> >\n> > Not sure if I understand you correctly, but it looks to me that the\n> > options in git-send-email.perl are well organized, so we could...\n> \n> Yes, but I wasn't commenting on your \"sendemail should also be able\n> to help completion by supporting --completion-helper option\" (I think\n> that is a sensible approach).  My comment was about Denton's patch,\n> which reduced the hard-coded list of format-patch options (i.e. the\n> first hunk) but had to add back many of them to send-email's\n> completion (i.e. the last hunk)---overall, it did not help reducing\n> the number of options hardcoded in the script.\n> \n> If it makes sense to complete all options to format-patch to\n> send-email, then as you outlined, grabbing them out of format-patch\n> with the --completion-helper option at runtime, and using them to\n> complete both format-patch and send-email would be a good idea.  And\n> that should be doable even before send-email learns how to list its\n> supported options to help the completion.\n\nOK how about this?\n\nMinimal changes in send-email.perl and no duplication in\n_git_send_email(). I could do $(git format-patch\n--git-completion-helper) directly from _git_send_email() too but we\nlose caching.\n\n-- 8< --\nSubject: [PATCH] completion: use __gitcomp_builtin for format-patch\n\nThis helps format-patch gain completion for a couple new options,\nnotably --range-diff.\n\nSince send-email completion relies on $__git_format_patch_options\nwhich is now reduced, we need to do something not to regress\nsend-email completion.\n\nThe workaround here is implement --git-completion-helper in\nsend-email.perl just as a bridge to \"format-patch --git-completion-helper\".\nThis is enough to use __gitcomp_builtin on send-email (to take\nadvantage of caching).\n\nIn the end, send-email.perl can probably reuse the same info it passes\nto GetOptions() to generate full --git-completion-helper output so\nthat we don't need to keep track of its options in git-completion.bash\nanymore. But that's something for another boring day.\n\nHelped-by: Denton Liu <liu.denton@gmail.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n contrib/completion/git-completion.bash | 16 ++++++----------\n git-send-email.perl                    |  8 ++++++++\n 2 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex db7fd87b6b..8409978793 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1532,13 +1532,9 @@ _git_fetch ()\n \t__git_complete_remote_or_refspec\n }\n \n-__git_format_patch_options=\"\n-\t--stdout --attach --no-attach --thread --thread= --no-thread\n-\t--numbered --start-number --numbered-files --keep-subject --signoff\n-\t--signature --no-signature --in-reply-to= --cc= --full-index --binary\n-\t--not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n-\t--inline --suffix= --ignore-if-in-upstream --subject-prefix=\n-\t--output-directory --reroll-count --to= --quiet --notes\n+__git_format_patch_extra_options=\"\n+\t--full-index --not --all --no-prefix --src-prefix=\n+\t--dst-prefix= --notes\n \"\n \n _git_format_patch ()\n@@ -1551,7 +1547,7 @@ _git_format_patch ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"$__git_format_patch_options\"\n+\t\t__gitcomp_builtin format-patch \"$__git_format_patch_extra_options\"\n \t\treturn\n \t\t;;\n \tesac\n@@ -2081,7 +2077,7 @@ _git_send_email ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n+\t\t__gitcomp_builtin send-email \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n \t\t\t--compose --confirm= --dry-run --envelope-sender\n \t\t\t--from --identity\n \t\t\t--in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n@@ -2090,7 +2086,7 @@ _git_send_email ()\n \t\t\t--smtp-server-port --smtp-encryption= --smtp-user\n \t\t\t--subject --suppress-cc= --suppress-from --thread --to\n \t\t\t--validate --no-validate\n-\t\t\t$__git_format_patch_options\"\n+\t\t\t$__git_format_patch_extra_options\"\n \t\treturn\n \t\t;;\n \tesac\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2be5dac337..ed0714eaaa 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -119,6 +119,11 @@ sub usage {\n \texit(1);\n }\n \n+sub completion_helper {\n+    print Git::command('format-patch', '--git-completion-helper');\n+    exit(0);\n+}\n+\n # most mail servers generate the Date: header, but not all...\n sub format_2822_time {\n \tmy ($time) = @_;\n@@ -311,6 +316,7 @@ sub signal_handler {\n # needing, first, from the command line:\n \n my $help;\n+my $git_completion_helper;\n my $rc = GetOptions(\"h\" => \\$help,\n                     \"dump-aliases\" => \\$dump_aliases);\n usage() unless $rc;\n@@ -373,9 +379,11 @@ sub signal_handler {\n \t\t    \"no-xmailer\" => sub {$use_xmailer = 0},\n \t\t    \"batch-size=i\" => \\$batch_size,\n \t\t    \"relogin-delay=i\" => \\$relogin_delay,\n+\t\t    \"git-completion-helper\" => \\$git_completion_helper,\n \t );\n \n usage() if $help;\n+completion_helper() if $git_completion_helper;\n unless ($rc) {\n     usage();\n }\n-- \n2.19.1.1005.gac84295441\n\n-- 8< --\n--\nDuy\n"},{"id":"362336","messageId":"20181103075918.GA905@archbookpro.localdomain","threadId":"49710","inReplyTo":"20181103060317.GA5432@duynguyen.home","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2018-11-03T07:59:18Z","receivedAt":"2018-11-03T07:59:24Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sat, Nov 03, 2018 at 07:03:18AM +0100, Duy Nguyen wrote:\n> Subject: [PATCH] completion: use __gitcomp_builtin for format-patch\n> \n> This helps format-patch gain completion for a couple new options,\n> notably --range-diff.\n> \n> Since send-email completion relies on $__git_format_patch_options\n> which is now reduced, we need to do something not to regress\n> send-email completion.\n> \n> The workaround here is implement --git-completion-helper in\n> send-email.perl just as a bridge to \"format-patch --git-completion-helper\".\n> This is enough to use __gitcomp_builtin on send-email (to take\n> advantage of caching).\n> \n> In the end, send-email.perl can probably reuse the same info it passes\n> to GetOptions() to generate full --git-completion-helper output so\n> that we don't need to keep track of its options in git-completion.bash\n> anymore. But that's something for another boring day.\n> \n> Helped-by: Denton Liu <liu.denton@gmail.com>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 16 ++++++----------\n>  git-send-email.perl                    |  8 ++++++++\n>  2 files changed, 14 insertions(+), 10 deletions(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index db7fd87b6b..8409978793 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1532,13 +1532,9 @@ _git_fetch ()\n>  \t__git_complete_remote_or_refspec\n>  }\n>  \n> -__git_format_patch_options=\"\n> -\t--stdout --attach --no-attach --thread --thread= --no-thread\n> -\t--numbered --start-number --numbered-files --keep-subject --signoff\n> -\t--signature --no-signature --in-reply-to= --cc= --full-index --binary\n> -\t--not --all --cover-letter --no-prefix --src-prefix= --dst-prefix=\n> -\t--inline --suffix= --ignore-if-in-upstream --subject-prefix=\n> -\t--output-directory --reroll-count --to= --quiet --notes\n> +__git_format_patch_extra_options=\"\n> +\t--full-index --not --all --no-prefix --src-prefix=\n> +\t--dst-prefix= --notes\n>  \"\n>  \n>  _git_format_patch ()\n> @@ -1551,7 +1547,7 @@ _git_format_patch ()\n>  \t\treturn\n>  \t\t;;\n>  \t--*)\n> -\t\t__gitcomp \"$__git_format_patch_options\"\n> +\t\t__gitcomp_builtin format-patch \"$__git_format_patch_extra_options\"\n>  \t\treturn\n>  \t\t;;\n>  \tesac\n> @@ -2081,7 +2077,7 @@ _git_send_email ()\n>  \t\treturn\n>  \t\t;;\n>  \t--*)\n> -\t\t__gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n> +\t\t__gitcomp_builtin send-email \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n>  \t\t\t--compose --confirm= --dry-run --envelope-sender\n>  \t\t\t--from --identity\n>  \t\t\t--in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n\nWould it make sense to make send-email's completion helper print these\nout directly? That way, if someone were to modify send-email in the\nfuture, they'd only have to look through one file instead of both\nsend-email and the completions script.\n\n> @@ -2090,7 +2086,7 @@ _git_send_email ()\n>  \t\t\t--smtp-server-port --smtp-encryption= --smtp-user\n>  \t\t\t--subject --suppress-cc= --suppress-from --thread --to\n>  \t\t\t--validate --no-validate\n> -\t\t\t$__git_format_patch_options\"\n> +\t\t\t$__git_format_patch_extra_options\"\n>  \t\treturn\n>  \t\t;;\n>  \tesac\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2be5dac337..ed0714eaaa 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -119,6 +119,11 @@ sub usage {\n>  \texit(1);\n>  }\n>  \n> +sub completion_helper {\n> +    print Git::command('format-patch', '--git-completion-helper');\n> +    exit(0);\n> +}\n> +\n>  # most mail servers generate the Date: header, but not all...\n>  sub format_2822_time {\n>  \tmy ($time) = @_;\n> @@ -311,6 +316,7 @@ sub signal_handler {\n>  # needing, first, from the command line:\n>  \n>  my $help;\n> +my $git_completion_helper;\n>  my $rc = GetOptions(\"h\" => \\$help,\n>                      \"dump-aliases\" => \\$dump_aliases);\n>  usage() unless $rc;\n> @@ -373,9 +379,11 @@ sub signal_handler {\n>  \t\t    \"no-xmailer\" => sub {$use_xmailer = 0},\n>  \t\t    \"batch-size=i\" => \\$batch_size,\n>  \t\t    \"relogin-delay=i\" => \\$relogin_delay,\n> +\t\t    \"git-completion-helper\" => \\$git_completion_helper,\n>  \t );\n>  \n>  usage() if $help;\n> +completion_helper() if $git_completion_helper;\n>  unless ($rc) {\n>      usage();\n>  }\n> -- \n> 2.19.1.1005.gac84295441\n> \n> -- 8< --\n> --\n> Duy\n\nAside from that one comment, it looks good to me. Thanks for helping me\nclean up my earlier patch!\n"},{"id":"362338","messageId":"CACsJy8B4vrtsBu79J2kYNmcNQfRkgetdbi8XOPjm7j1kNDJ6Yg@mail.gmail.com","threadId":"49710","inReplyTo":"20181103075918.GA905@archbookpro.localdomain","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-03T08:29:31Z","receivedAt":"2018-11-03T08:30:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Nov 3, 2018 at 8:59 AM Denton Liu <liu.denton@gmail.com> wrote:\n> > @@ -2081,7 +2077,7 @@ _git_send_email ()\n> >               return\n> >               ;;\n> >       --*)\n> > -             __gitcomp \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n> > +             __gitcomp_builtin send-email \"--annotate --bcc --cc --cc-cmd --chain-reply-to\n> >                       --compose --confirm= --dry-run --envelope-sender\n> >                       --from --identity\n> >                       --in-reply-to --no-chain-reply-to --no-signed-off-by-cc\n>\n> Would it make sense to make send-email's completion helper print these\n> out directly? That way, if someone were to modify send-email in the\n> future, they'd only have to look through one file instead of both\n> send-email and the completions script.\n\nI did think about that and decided not to do it (in fact the first\nrevision of this patch did exactly that).\n\nIf we have to maintain this list manually, we might as well leave to\nthe place that matters: the completion script. I don't think the\nperson who updates send-email.perl would be always interested in\ncompletion support. And the one that is interested usually only looks\nat the completion script. Putting this list in send-email.perl just\nmakes it harder to find.\n-- \nDuy\n"},{"id":"362355","messageId":"xmqqmuqqmphl.fsf@gitster-ct.c.googlers.com","threadId":"49710","inReplyTo":"CACsJy8B4vrtsBu79J2kYNmcNQfRkgetdbi8XOPjm7j1kNDJ6Yg@mail.gmail.com","subject":"Re: [PATCH v2] completion: use builtin completion for format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-03T10:20:38Z","receivedAt":"2018-11-03T10:20:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>> Would it make sense to make send-email's completion helper print these\n>> out directly? That way, if someone were to modify send-email in the\n>> future, they'd only have to look through one file instead of both\n>> send-email and the completions script.\n>\n> I did think about that and decided not to do it (in fact the first\n> revision of this patch did exactly that).\n>\n> If we have to maintain this list manually, we might as well leave to\n> the place that matters: the completion script. I don't think the\n> person who updates send-email.perl would be always interested in\n> completion support. And the one that is interested usually only looks\n> at the completion script. Putting this list in send-email.perl just\n> makes it harder to find.\n\nI do not necessarily disagree with the conclusion, but I am not sure\nif I agree with the last paragraph.  If the definition used to list\ncompletable options was in the send-email command, it is more likely\nthat a patch to send-email.perl that would add/modify an option\nwithout making a matching change to the definition in the same file\ngets noticed, whether the developer who is doing the feature is or\nis not interested in maintaining the completion script working, no?\nSimilarly, if one notices that an option the command supports that\nought to get completed does not get completed, and gets motivated\nenough to try finding where in the completion script other existing\noptions that do get completed are handled, wouldn't that lead one to\nthe right solution, i.e. discovery of the definition in the\nsend-email script?  \n\nQuite honestly, I would expect that our developers and user base are\nmuch more competent than one who\n\n - wants to add completion of the option Y to the command A, which\n   has known-to-be-working completion of the option X, and yet\n\n - fails to imagine that it could be a possible good first step to\n   figure out how the option X is completed, so that a new support\n   for the option Y might be able to emulate it.\n\nNow, once we start going in the direction of having both the\nimplementation of options *and* the definition of the list of\ncompletable options in send-email.perl script, I would agree with\nyour initial assessment in a message much earlier in the thread.  It\nwould be very tempting to use the data we feed Getopt::Long as the\nsource of the list of completable options to reduce the longer-term\nmaintenance load, which would mean it will involve more work.  And\nin order to avoid having to invest more work upfront (which I do not\nthink is necessarily a bad thing), having the definition in the\ncompletion script might be easier to manage---it is closer to the\nstatus quo, especially the state before you taught parse-options API\nto give the list of completable options.\n\nThanks.\n"}]}