* [PATCH] completion: use builtin completion for format-patch @ 2018-10-29 17:57 Denton Liu 2018-10-30 2:20 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Denton Liu @ 2018-10-29 17:57 UTC (permalink / raw) To: git; +Cc: liu.denton, anmolmago, briankyho, david.lu97, shirui.wang Signed-off-by: Denton Liu <liu.denton@gmail.com> --- contrib/completion/git-completion.bash | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index d63d2dffd..da77da481 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1531,15 +1531,6 @@ _git_fetch () __git_complete_remote_or_refspec } -__git_format_patch_options=" - --stdout --attach --no-attach --thread --thread= --no-thread - --numbered --start-number --numbered-files --keep-subject --signoff - --signature --no-signature --in-reply-to= --cc= --full-index --binary - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= - --inline --suffix= --ignore-if-in-upstream --subject-prefix= - --output-directory --reroll-count --to= --quiet --notes -" - _git_format_patch () { case "$cur" in @@ -1550,7 +1541,7 @@ _git_format_patch () return ;; --*) - __gitcomp "$__git_format_patch_options" + __gitcomp_builtin format-patch return ;; esac -- 2.19.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] completion: use builtin completion for format-patch 2018-10-29 17:57 [PATCH] completion: use builtin completion for format-patch Denton Liu @ 2018-10-30 2:20 ` Junio C Hamano 2018-10-30 3:50 ` Denton Liu 2018-10-30 6:38 ` [PATCH v2] " Denton Liu 0 siblings, 2 replies; 13+ messages in thread From: Junio C Hamano @ 2018-10-30 2:20 UTC (permalink / raw) To: Denton Liu; +Cc: git, anmolmago, briankyho, david.lu97, shirui.wang Denton Liu <liu.denton@gmail.com> writes: > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > contrib/completion/git-completion.bash | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) We saw a similar change proposed and then found out it was not such a good idea in: https://public-inbox.org/git/CACsJy8DUrVJu0HN7kuCeo4iV5aimWbYtr+E-7kenPVDx90DpGw@mail.gmail.com/ It seems that this one loses options like --full-index, --no-prefix, etc. compared to the earlier effort? > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index d63d2dffd..da77da481 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1531,15 +1531,6 @@ _git_fetch () > __git_complete_remote_or_refspec > } > > -__git_format_patch_options=" > - --stdout --attach --no-attach --thread --thread= --no-thread > - --numbered --start-number --numbered-files --keep-subject --signoff > - --signature --no-signature --in-reply-to= --cc= --full-index --binary > - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > - --inline --suffix= --ignore-if-in-upstream --subject-prefix= > - --output-directory --reroll-count --to= --quiet --notes > -" > - > _git_format_patch () > { > case "$cur" in > @@ -1550,7 +1541,7 @@ _git_format_patch () > return > ;; > --*) > - __gitcomp "$__git_format_patch_options" > + __gitcomp_builtin format-patch > return > ;; > esac ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] completion: use builtin completion for format-patch 2018-10-30 2:20 ` Junio C Hamano @ 2018-10-30 3:50 ` Denton Liu 2018-10-30 6:38 ` [PATCH v2] " Denton Liu 1 sibling, 0 replies; 13+ messages in thread From: Denton Liu @ 2018-10-30 3:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, anmolmago, briankyho, david.lu97, shirui.wang On Tue, Oct 30, 2018 at 11:20:45AM +0900, Junio C Hamano wrote: > We saw a similar change proposed and then found out it was not such > a good idea in: > > https://public-inbox.org/git/CACsJy8DUrVJu0HN7kuCeo4iV5aimWbYtr+E-7kenPVDx90DpGw@mail.gmail.com/ > > It seems that this one loses options like --full-index, --no-prefix, > etc. compared to the earlier effort? In _git_send_email, we have the following lines: __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to ... more options ... $__git_format_patch_options" Would it make sense to take the old `__git_format_patch_options` and just roll them into here, then make `_git_format_patch` use `__gitcomp_builtin format-patch`? That way, we'd be able to reap the benefits of using `__gitcomp_builtin` where we can. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] completion: use builtin completion for format-patch 2018-10-30 2:20 ` Junio C Hamano 2018-10-30 3:50 ` Denton Liu @ 2018-10-30 6:38 ` Denton Liu 2018-10-30 7:33 ` Junio C Hamano 2018-10-30 15:29 ` Duy Nguyen 1 sibling, 2 replies; 13+ messages in thread From: Denton Liu @ 2018-10-30 6:38 UTC (permalink / raw) To: gitster; +Cc: git, anmolmago, briankyho, david.lu97, shirui.wang This patch offloads completion functionality for format-patch to __gitcomp_builtin. In addition to this, send-email was borrowing some completion options from format-patch so those options are moved into send-email's completions. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- I ran t9902-completion.sh on this patch and it seems to pass. Thus, this should address the concerns about losing some completion options in send-email. --- contrib/completion/git-completion.bash | 34 +++++++++++--------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index d63d2dffd..cb4ef6723 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1531,15 +1531,6 @@ _git_fetch () __git_complete_remote_or_refspec } -__git_format_patch_options=" - --stdout --attach --no-attach --thread --thread= --no-thread - --numbered --start-number --numbered-files --keep-subject --signoff - --signature --no-signature --in-reply-to= --cc= --full-index --binary - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= - --inline --suffix= --ignore-if-in-upstream --subject-prefix= - --output-directory --reroll-count --to= --quiet --notes -" - _git_format_patch () { case "$cur" in @@ -1550,7 +1541,7 @@ _git_format_patch () return ;; --*) - __gitcomp "$__git_format_patch_options" + __gitcomp_builtin format-patch return ;; esac @@ -2080,16 +2071,19 @@ _git_send_email () return ;; --*) - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to - --compose --confirm= --dry-run --envelope-sender - --from --identity - --in-reply-to --no-chain-reply-to --no-signed-off-by-cc - --no-suppress-from --no-thread --quiet --reply-to - --signed-off-by-cc --smtp-pass --smtp-server - --smtp-server-port --smtp-encryption= --smtp-user - --subject --suppress-cc= --suppress-from --thread --to - --validate --no-validate - $__git_format_patch_options" + __gitcomp "--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd + --chain-reply-to --compose --confirm= --cover-letter --dry-run + --dst-prefix= --envelope-sender --from --full-index --identity + --ignore-if-in-upstream --inline --in-reply-to --in-reply-to= + --keep-subject --no-attach --no-chain-reply-to --no-prefix + --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes + --no-thread --no-validate --numbered --numbered-files + --output-directory --quiet --reply-to --reroll-count --signature + --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass + --smtp-server --smtp-server-port --smtp-user --src-prefix= + --start-number --stdout --subject --subject-prefix= --suffix= + --suppress-cc= --suppress-from --thread --thread= --to --to= + --validate" return ;; esac -- 2.19.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-10-30 6:38 ` [PATCH v2] " Denton Liu @ 2018-10-30 7:33 ` Junio C Hamano 2018-10-30 15:29 ` Duy Nguyen 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2018-10-30 7:33 UTC (permalink / raw) To: Denton Liu Cc: SZEDER Gábor, Eric Sunshine, Nguyễn Thái Ngọc Duy, git, anmolmago, briankyho, david.lu97, shirui.wang Denton Liu <liu.denton@gmail.com> writes: > This patch offloads completion functionality for format-patch to > __gitcomp_builtin. In addition to this, send-email was borrowing some > completion options from format-patch so those options are moved into > send-email's completions. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > > I ran t9902-completion.sh on this patch and it seems to pass. Thus, this > should address the concerns about losing some completion options in > send-email. Thanks. I added those who were involved in the review thread of the original patch last week to CC. Let's see how well this round is received. > --- > contrib/completion/git-completion.bash | 34 +++++++++++--------------- > 1 file changed, 14 insertions(+), 20 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index d63d2dffd..cb4ef6723 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1531,15 +1531,6 @@ _git_fetch () > __git_complete_remote_or_refspec > } > > -__git_format_patch_options=" > - --stdout --attach --no-attach --thread --thread= --no-thread > - --numbered --start-number --numbered-files --keep-subject --signoff > - --signature --no-signature --in-reply-to= --cc= --full-index --binary > - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > - --inline --suffix= --ignore-if-in-upstream --subject-prefix= > - --output-directory --reroll-count --to= --quiet --notes > -" > - > _git_format_patch () > { > case "$cur" in > @@ -1550,7 +1541,7 @@ _git_format_patch () > return > ;; > --*) > - __gitcomp "$__git_format_patch_options" > + __gitcomp_builtin format-patch > return > ;; > esac > @@ -2080,16 +2071,19 @@ _git_send_email () > return > ;; > --*) > - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to > - --compose --confirm= --dry-run --envelope-sender > - --from --identity > - --in-reply-to --no-chain-reply-to --no-signed-off-by-cc > - --no-suppress-from --no-thread --quiet --reply-to > - --signed-off-by-cc --smtp-pass --smtp-server > - --smtp-server-port --smtp-encryption= --smtp-user > - --subject --suppress-cc= --suppress-from --thread --to > - --validate --no-validate > - $__git_format_patch_options" > + __gitcomp "--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd > + --chain-reply-to --compose --confirm= --cover-letter --dry-run > + --dst-prefix= --envelope-sender --from --full-index --identity > + --ignore-if-in-upstream --inline --in-reply-to --in-reply-to= > + --keep-subject --no-attach --no-chain-reply-to --no-prefix > + --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes > + --no-thread --no-validate --numbered --numbered-files > + --output-directory --quiet --reply-to --reroll-count --signature > + --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass > + --smtp-server --smtp-server-port --smtp-user --src-prefix= > + --start-number --stdout --subject --subject-prefix= --suffix= > + --suppress-cc= --suppress-from --thread --thread= --to --to= > + --validate" > return > ;; > esac ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-10-30 6:38 ` [PATCH v2] " Denton Liu 2018-10-30 7:33 ` Junio C Hamano @ 2018-10-30 15:29 ` Duy Nguyen 2018-11-01 1:42 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Duy Nguyen @ 2018-10-30 15:29 UTC (permalink / raw) To: liu.denton, SZEDER Gábor Cc: Junio C Hamano, Git Mailing List, anmolmago, briankyho, david.lu97, shirui.wang On Tue, Oct 30, 2018 at 7:39 AM Denton Liu <liu.denton@gmail.com> wrote: > > This patch offloads completion functionality for format-patch to > __gitcomp_builtin. In addition to this, send-email was borrowing some > completion options from format-patch so those options are moved into > send-email's completions. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > > I ran t9902-completion.sh on this patch and it seems to pass. Thus, this > should address the concerns about losing some completion options in > send-email. > > --- > contrib/completion/git-completion.bash | 34 +++++++++++--------------- > 1 file changed, 14 insertions(+), 20 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index d63d2dffd..cb4ef6723 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1531,15 +1531,6 @@ _git_fetch () > __git_complete_remote_or_refspec > } > > -__git_format_patch_options=" > - --stdout --attach --no-attach --thread --thread= --no-thread > - --numbered --start-number --numbered-files --keep-subject --signoff > - --signature --no-signature --in-reply-to= --cc= --full-index --binary > - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > - --inline --suffix= --ignore-if-in-upstream --subject-prefix= > - --output-directory --reroll-count --to= --quiet --notes > -" > - > _git_format_patch () > { > case "$cur" in > @@ -1550,7 +1541,7 @@ _git_format_patch () > return > ;; > --*) > - __gitcomp "$__git_format_patch_options" > + __gitcomp_builtin format-patch We do want to keep some extra options back because __gitcomp_builtin cannot show all supported options of format-patch. The reason is a subset of options is handled by setup_revisions() which does not have --git-completion-helper support. > @@ -2080,16 +2071,19 @@ _git_send_email () > return > ;; > --*) > - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to > - --compose --confirm= --dry-run --envelope-sender > - --from --identity > - --in-reply-to --no-chain-reply-to --no-signed-off-by-cc > - --no-suppress-from --no-thread --quiet --reply-to > - --signed-off-by-cc --smtp-pass --smtp-server > - --smtp-server-port --smtp-encryption= --smtp-user > - --subject --suppress-cc= --suppress-from --thread --to > - --validate --no-validate > - $__git_format_patch_options" > + __gitcomp "--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd > + --chain-reply-to --compose --confirm= --cover-letter --dry-run > + --dst-prefix= --envelope-sender --from --full-index --identity > + --ignore-if-in-upstream --inline --in-reply-to --in-reply-to= > + --keep-subject --no-attach --no-chain-reply-to --no-prefix > + --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes > + --no-thread --no-validate --numbered --numbered-files > + --output-directory --quiet --reply-to --reroll-count --signature > + --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass > + --smtp-server --smtp-server-port --smtp-user --src-prefix= > + --start-number --stdout --subject --subject-prefix= --suffix= > + --suppress-cc= --suppress-from --thread --thread= --to --to= > + --validate" I have no comment about this. In an ideal world, sendemail.perl could be taught to support --git-completion-helper but I don't think my little remaining Perl knowledge (or time) is enough to do it. Perhaps this will do. I don't know. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-10-30 15:29 ` Duy Nguyen @ 2018-11-01 1:42 ` Junio C Hamano 2018-11-01 15:40 ` Duy Nguyen 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2018-11-01 1:42 UTC (permalink / raw) To: Duy Nguyen Cc: liu.denton, SZEDER Gábor, Git Mailing List, anmolmago, briankyho, david.lu97, shirui.wang Duy Nguyen <pclouds@gmail.com> writes: >> -__git_format_patch_options=" >> - --stdout --attach --no-attach --thread --thread= --no-thread >> - --numbered --start-number --numbered-files --keep-subject --signoff >> - --signature --no-signature --in-reply-to= --cc= --full-index --binary >> - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= >> - --inline --suffix= --ignore-if-in-upstream --subject-prefix= >> - --output-directory --reroll-count --to= --quiet --notes >> -" >> - >> _git_format_patch () >> { >> case "$cur" in >> @@ -1550,7 +1541,7 @@ _git_format_patch () >> return >> ;; >> --*) >> - __gitcomp "$__git_format_patch_options" >> + __gitcomp_builtin format-patch > > We do want to keep some extra options back because __gitcomp_builtin > cannot show all supported options of format-patch. The reason is a > subset of options is handled by setup_revisions() which does not have > --git-completion-helper support. Yeah, that is one of the differences I saw compared to the older one; thanks for being a careful reviewer. >> @@ -2080,16 +2071,19 @@ _git_send_email () >> return >> ;; >> --*) >> - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to >> - --compose --confirm= --dry-run --envelope-sender >> - --from --identity >> - --in-reply-to --no-chain-reply-to --no-signed-off-by-cc >> - --no-suppress-from --no-thread --quiet --reply-to >> - --signed-off-by-cc --smtp-pass --smtp-server >> - --smtp-server-port --smtp-encryption= --smtp-user >> - --subject --suppress-cc= --suppress-from --thread --to >> - --validate --no-validate >> - $__git_format_patch_options" >> + __gitcomp "--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd >> + --chain-reply-to --compose --confirm= --cover-letter --dry-run >> + --dst-prefix= --envelope-sender --from --full-index --identity >> + --ignore-if-in-upstream --inline --in-reply-to --in-reply-to= >> + --keep-subject --no-attach --no-chain-reply-to --no-prefix >> + --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes >> + --no-thread --no-validate --numbered --numbered-files >> + --output-directory --quiet --reply-to --reroll-count --signature >> + --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass >> + --smtp-server --smtp-server-port --smtp-user --src-prefix= >> + --start-number --stdout --subject --subject-prefix= --suffix= >> + --suppress-cc= --suppress-from --thread --thread= --to --to= >> + --validate" > > I have no comment about this. In an ideal world, sendemail.perl could > be taught to support --git-completion-helper but I don't think my > little remaining Perl knowledge (or time) is enough to do it. Perhaps > this will do. I don't know. So "all", "attach", etc. are added to this list while these similar options are lost from the other variable? Is this a good trade-off? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-11-01 1:42 ` Junio C Hamano @ 2018-11-01 15:40 ` Duy Nguyen 2018-11-01 23:52 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Duy Nguyen @ 2018-11-01 15:40 UTC (permalink / raw) To: Junio C Hamano Cc: Denton Liu, SZEDER Gábor, Git Mailing List, Anmol Mago, briankyho, david.lu97, shirui.wang On Thu, Nov 1, 2018 at 2:42 AM Junio C Hamano <gitster@pobox.com> wrote: > >> @@ -2080,16 +2071,19 @@ _git_send_email () > >> return > >> ;; > >> --*) > >> - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to > >> - --compose --confirm= --dry-run --envelope-sender > >> - --from --identity > >> - --in-reply-to --no-chain-reply-to --no-signed-off-by-cc > >> - --no-suppress-from --no-thread --quiet --reply-to > >> - --signed-off-by-cc --smtp-pass --smtp-server > >> - --smtp-server-port --smtp-encryption= --smtp-user > >> - --subject --suppress-cc= --suppress-from --thread --to > >> - --validate --no-validate > >> - $__git_format_patch_options" > >> + __gitcomp "--all --annotate --attach --bcc --binary --cc --cc= --cc-cmd > >> + --chain-reply-to --compose --confirm= --cover-letter --dry-run > >> + --dst-prefix= --envelope-sender --from --full-index --identity > >> + --ignore-if-in-upstream --inline --in-reply-to --in-reply-to= > >> + --keep-subject --no-attach --no-chain-reply-to --no-prefix > >> + --no-signature --no-signed-off-by-cc --no-suppress-from --not --notes > >> + --no-thread --no-validate --numbered --numbered-files > >> + --output-directory --quiet --reply-to --reroll-count --signature > >> + --signed-off-by-cc --signoff --smtp-encryption= --smtp-pass > >> + --smtp-server --smtp-server-port --smtp-user --src-prefix= > >> + --start-number --stdout --subject --subject-prefix= --suffix= > >> + --suppress-cc= --suppress-from --thread --thread= --to --to= > >> + --validate" > > > > I have no comment about this. In an ideal world, sendemail.perl could > > be taught to support --git-completion-helper but I don't think my > > little remaining Perl knowledge (or time) is enough to do it. Perhaps > > this will do. I don't know. > > So "all", "attach", etc. are added to this list while these similar > options are lost from the other variable? Is this a good trade-off? Not sure if I understand you correctly, but it looks to me that the options in git-send-email.perl are well organized, so we could add --git-completon-helper in that script to print all send-email specific options, then call "git format-patch --git-completion-helper" to add a bunch more. The options that are handled by setup_revisions() will have to be maintained manually here like $__git_format_patch_options and added on top in both _git_send_email () and _git_format_patch (). So, nothing option is lost and by the time setup_revisions() supports -git-completion-helper, we can get rid of the manual shell variable too. The downside is, lots of work, probably. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-11-01 15:40 ` Duy Nguyen @ 2018-11-01 23:52 ` Junio C Hamano 2018-11-03 6:03 ` Duy Nguyen 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2018-11-01 23:52 UTC (permalink / raw) To: Duy Nguyen Cc: Denton Liu, SZEDER Gábor, Git Mailing List, Anmol Mago, briankyho, david.lu97, shirui.wang Duy Nguyen <pclouds@gmail.com> writes: >> > I have no comment about this. In an ideal world, sendemail.perl could >> > be taught to support --git-completion-helper but I don't think my >> > little remaining Perl knowledge (or time) is enough to do it. Perhaps >> > this will do. I don't know. >> >> So "all", "attach", etc. are added to this list while these similar >> options are lost from the other variable? Is this a good trade-off? > > Not sure if I understand you correctly, but it looks to me that the > options in git-send-email.perl are well organized, so we could... Yes, but I wasn't commenting on your "sendemail should also be able to help completion by supporting --completion-helper option" (I think that is a sensible approach). My comment was about Denton's patch, which reduced the hard-coded list of format-patch options (i.e. the first hunk) but had to add back many of them to send-email's completion (i.e. the last hunk)---overall, it did not help reducing the number of options hardcoded in the script. If it makes sense to complete all options to format-patch to send-email, then as you outlined, grabbing them out of format-patch with the --completion-helper option at runtime, and using them to complete both format-patch and send-email would be a good idea. And that should be doable even before send-email learns how to list its supported options to help the completion. > --git-completon-helper in that script to print all send-email specific > options, then call "git format-patch --git-completion-helper" to add a > bunch more. The options that are handled by setup_revisions() will > have to be maintained manually here like $__git_format_patch_options > and added on top in both _git_send_email () and _git_format_patch (). > > So, nothing option is lost and by the time setup_revisions() supports > -git-completion-helper, we can get rid of the manual shell variable > too. The downside is, lots of work, probably. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-11-01 23:52 ` Junio C Hamano @ 2018-11-03 6:03 ` Duy Nguyen 2018-11-03 7:59 ` Denton Liu 0 siblings, 1 reply; 13+ messages in thread From: Duy Nguyen @ 2018-11-03 6:03 UTC (permalink / raw) To: Junio C Hamano Cc: Denton Liu, SZEDER Gábor, Git Mailing List, Anmol Mago, briankyho, david.lu97, shirui.wang On Fri, Nov 02, 2018 at 08:52:30AM +0900, Junio C Hamano wrote: > Duy Nguyen <pclouds@gmail.com> writes: > > >> > I have no comment about this. In an ideal world, sendemail.perl could > >> > be taught to support --git-completion-helper but I don't think my > >> > little remaining Perl knowledge (or time) is enough to do it. Perhaps > >> > this will do. I don't know. > >> > >> So "all", "attach", etc. are added to this list while these similar > >> options are lost from the other variable? Is this a good trade-off? > > > > Not sure if I understand you correctly, but it looks to me that the > > options in git-send-email.perl are well organized, so we could... > > Yes, but I wasn't commenting on your "sendemail should also be able > to help completion by supporting --completion-helper option" (I think > that is a sensible approach). My comment was about Denton's patch, > which reduced the hard-coded list of format-patch options (i.e. the > first hunk) but had to add back many of them to send-email's > completion (i.e. the last hunk)---overall, it did not help reducing > the number of options hardcoded in the script. > > If it makes sense to complete all options to format-patch to > send-email, then as you outlined, grabbing them out of format-patch > with the --completion-helper option at runtime, and using them to > complete both format-patch and send-email would be a good idea. And > that should be doable even before send-email learns how to list its > supported options to help the completion. OK how about this? Minimal changes in send-email.perl and no duplication in _git_send_email(). I could do $(git format-patch --git-completion-helper) directly from _git_send_email() too but we lose caching. -- 8< -- Subject: [PATCH] completion: use __gitcomp_builtin for format-patch This helps format-patch gain completion for a couple new options, notably --range-diff. Since send-email completion relies on $__git_format_patch_options which is now reduced, we need to do something not to regress send-email completion. The workaround here is implement --git-completion-helper in send-email.perl just as a bridge to "format-patch --git-completion-helper". This is enough to use __gitcomp_builtin on send-email (to take advantage of caching). In the end, send-email.perl can probably reuse the same info it passes to GetOptions() to generate full --git-completion-helper output so that we don't need to keep track of its options in git-completion.bash anymore. But that's something for another boring day. Helped-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- contrib/completion/git-completion.bash | 16 ++++++---------- git-send-email.perl | 8 ++++++++ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index db7fd87b6b..8409978793 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1532,13 +1532,9 @@ _git_fetch () __git_complete_remote_or_refspec } -__git_format_patch_options=" - --stdout --attach --no-attach --thread --thread= --no-thread - --numbered --start-number --numbered-files --keep-subject --signoff - --signature --no-signature --in-reply-to= --cc= --full-index --binary - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= - --inline --suffix= --ignore-if-in-upstream --subject-prefix= - --output-directory --reroll-count --to= --quiet --notes +__git_format_patch_extra_options=" + --full-index --not --all --no-prefix --src-prefix= + --dst-prefix= --notes " _git_format_patch () @@ -1551,7 +1547,7 @@ _git_format_patch () return ;; --*) - __gitcomp "$__git_format_patch_options" + __gitcomp_builtin format-patch "$__git_format_patch_extra_options" return ;; esac @@ -2081,7 +2077,7 @@ _git_send_email () return ;; --*) - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to + __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd --chain-reply-to --compose --confirm= --dry-run --envelope-sender --from --identity --in-reply-to --no-chain-reply-to --no-signed-off-by-cc @@ -2090,7 +2086,7 @@ _git_send_email () --smtp-server-port --smtp-encryption= --smtp-user --subject --suppress-cc= --suppress-from --thread --to --validate --no-validate - $__git_format_patch_options" + $__git_format_patch_extra_options" return ;; esac diff --git a/git-send-email.perl b/git-send-email.perl index 2be5dac337..ed0714eaaa 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -119,6 +119,11 @@ sub usage { exit(1); } +sub completion_helper { + print Git::command('format-patch', '--git-completion-helper'); + exit(0); +} + # most mail servers generate the Date: header, but not all... sub format_2822_time { my ($time) = @_; @@ -311,6 +316,7 @@ sub signal_handler { # needing, first, from the command line: my $help; +my $git_completion_helper; my $rc = GetOptions("h" => \$help, "dump-aliases" => \$dump_aliases); usage() unless $rc; @@ -373,9 +379,11 @@ sub signal_handler { "no-xmailer" => sub {$use_xmailer = 0}, "batch-size=i" => \$batch_size, "relogin-delay=i" => \$relogin_delay, + "git-completion-helper" => \$git_completion_helper, ); usage() if $help; +completion_helper() if $git_completion_helper; unless ($rc) { usage(); } -- 2.19.1.1005.gac84295441 -- 8< -- -- Duy ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-11-03 6:03 ` Duy Nguyen @ 2018-11-03 7:59 ` Denton Liu 2018-11-03 8:29 ` Duy Nguyen 0 siblings, 1 reply; 13+ messages in thread From: Denton Liu @ 2018-11-03 7:59 UTC (permalink / raw) To: Duy Nguyen Cc: Junio C Hamano, SZEDER Gábor, Git Mailing List, Anmol Mago, briankyho, david.lu97, shirui.wang On Sat, Nov 03, 2018 at 07:03:18AM +0100, Duy Nguyen wrote: > Subject: [PATCH] completion: use __gitcomp_builtin for format-patch > > This helps format-patch gain completion for a couple new options, > notably --range-diff. > > Since send-email completion relies on $__git_format_patch_options > which is now reduced, we need to do something not to regress > send-email completion. > > The workaround here is implement --git-completion-helper in > send-email.perl just as a bridge to "format-patch --git-completion-helper". > This is enough to use __gitcomp_builtin on send-email (to take > advantage of caching). > > In the end, send-email.perl can probably reuse the same info it passes > to GetOptions() to generate full --git-completion-helper output so > that we don't need to keep track of its options in git-completion.bash > anymore. But that's something for another boring day. > > Helped-by: Denton Liu <liu.denton@gmail.com> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > contrib/completion/git-completion.bash | 16 ++++++---------- > git-send-email.perl | 8 ++++++++ > 2 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index db7fd87b6b..8409978793 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1532,13 +1532,9 @@ _git_fetch () > __git_complete_remote_or_refspec > } > > -__git_format_patch_options=" > - --stdout --attach --no-attach --thread --thread= --no-thread > - --numbered --start-number --numbered-files --keep-subject --signoff > - --signature --no-signature --in-reply-to= --cc= --full-index --binary > - --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > - --inline --suffix= --ignore-if-in-upstream --subject-prefix= > - --output-directory --reroll-count --to= --quiet --notes > +__git_format_patch_extra_options=" > + --full-index --not --all --no-prefix --src-prefix= > + --dst-prefix= --notes > " > > _git_format_patch () > @@ -1551,7 +1547,7 @@ _git_format_patch () > return > ;; > --*) > - __gitcomp "$__git_format_patch_options" > + __gitcomp_builtin format-patch "$__git_format_patch_extra_options" > return > ;; > esac > @@ -2081,7 +2077,7 @@ _git_send_email () > return > ;; > --*) > - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to > + __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd --chain-reply-to > --compose --confirm= --dry-run --envelope-sender > --from --identity > --in-reply-to --no-chain-reply-to --no-signed-off-by-cc Would it make sense to make send-email's completion helper print these out directly? That way, if someone were to modify send-email in the future, they'd only have to look through one file instead of both send-email and the completions script. > @@ -2090,7 +2086,7 @@ _git_send_email () > --smtp-server-port --smtp-encryption= --smtp-user > --subject --suppress-cc= --suppress-from --thread --to > --validate --no-validate > - $__git_format_patch_options" > + $__git_format_patch_extra_options" > return > ;; > esac > diff --git a/git-send-email.perl b/git-send-email.perl > index 2be5dac337..ed0714eaaa 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -119,6 +119,11 @@ sub usage { > exit(1); > } > > +sub completion_helper { > + print Git::command('format-patch', '--git-completion-helper'); > + exit(0); > +} > + > # most mail servers generate the Date: header, but not all... > sub format_2822_time { > my ($time) = @_; > @@ -311,6 +316,7 @@ sub signal_handler { > # needing, first, from the command line: > > my $help; > +my $git_completion_helper; > my $rc = GetOptions("h" => \$help, > "dump-aliases" => \$dump_aliases); > usage() unless $rc; > @@ -373,9 +379,11 @@ sub signal_handler { > "no-xmailer" => sub {$use_xmailer = 0}, > "batch-size=i" => \$batch_size, > "relogin-delay=i" => \$relogin_delay, > + "git-completion-helper" => \$git_completion_helper, > ); > > usage() if $help; > +completion_helper() if $git_completion_helper; > unless ($rc) { > usage(); > } > -- > 2.19.1.1005.gac84295441 > > -- 8< -- > -- > Duy Aside from that one comment, it looks good to me. Thanks for helping me clean up my earlier patch! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-11-03 7:59 ` Denton Liu @ 2018-11-03 8:29 ` Duy Nguyen 2018-11-03 10:20 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Duy Nguyen @ 2018-11-03 8:29 UTC (permalink / raw) To: Denton Liu Cc: Junio C Hamano, SZEDER Gábor, Git Mailing List, Anmol Mago, briankyho, david.lu97, shirui.wang On Sat, Nov 3, 2018 at 8:59 AM Denton Liu <liu.denton@gmail.com> wrote: > > @@ -2081,7 +2077,7 @@ _git_send_email () > > return > > ;; > > --*) > > - __gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to > > + __gitcomp_builtin send-email "--annotate --bcc --cc --cc-cmd --chain-reply-to > > --compose --confirm= --dry-run --envelope-sender > > --from --identity > > --in-reply-to --no-chain-reply-to --no-signed-off-by-cc > > Would it make sense to make send-email's completion helper print these > out directly? That way, if someone were to modify send-email in the > future, they'd only have to look through one file instead of both > send-email and the completions script. I did think about that and decided not to do it (in fact the first revision of this patch did exactly that). If we have to maintain this list manually, we might as well leave to the place that matters: the completion script. I don't think the person who updates send-email.perl would be always interested in completion support. And the one that is interested usually only looks at the completion script. Putting this list in send-email.perl just makes it harder to find. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] completion: use builtin completion for format-patch 2018-11-03 8:29 ` Duy Nguyen @ 2018-11-03 10:20 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2018-11-03 10:20 UTC (permalink / raw) To: Duy Nguyen Cc: Denton Liu, SZEDER Gábor, Git Mailing List, Anmol Mago, briankyho, david.lu97, shirui.wang Duy Nguyen <pclouds@gmail.com> writes: >> Would it make sense to make send-email's completion helper print these >> out directly? That way, if someone were to modify send-email in the >> future, they'd only have to look through one file instead of both >> send-email and the completions script. > > I did think about that and decided not to do it (in fact the first > revision of this patch did exactly that). > > If we have to maintain this list manually, we might as well leave to > the place that matters: the completion script. I don't think the > person who updates send-email.perl would be always interested in > completion support. And the one that is interested usually only looks > at the completion script. Putting this list in send-email.perl just > makes it harder to find. I do not necessarily disagree with the conclusion, but I am not sure if I agree with the last paragraph. If the definition used to list completable options was in the send-email command, it is more likely that a patch to send-email.perl that would add/modify an option without making a matching change to the definition in the same file gets noticed, whether the developer who is doing the feature is or is not interested in maintaining the completion script working, no? Similarly, if one notices that an option the command supports that ought to get completed does not get completed, and gets motivated enough to try finding where in the completion script other existing options that do get completed are handled, wouldn't that lead one to the right solution, i.e. discovery of the definition in the send-email script? Quite honestly, I would expect that our developers and user base are much more competent than one who - wants to add completion of the option Y to the command A, which has known-to-be-working completion of the option X, and yet - fails to imagine that it could be a possible good first step to figure out how the option X is completed, so that a new support for the option Y might be able to emulate it. Now, once we start going in the direction of having both the implementation of options *and* the definition of the list of completable options in send-email.perl script, I would agree with your initial assessment in a message much earlier in the thread. It would be very tempting to use the data we feed Getopt::Long as the source of the list of completable options to reduce the longer-term maintenance load, which would mean it will involve more work. And in order to avoid having to invest more work upfront (which I do not think is necessarily a bad thing), having the definition in the completion script might be easier to manage---it is closer to the status quo, especially the state before you taught parse-options API to give the list of completable options. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-11-03 10:20 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-29 17:57 [PATCH] completion: use builtin completion for format-patch Denton Liu 2018-10-30 2:20 ` Junio C Hamano 2018-10-30 3:50 ` Denton Liu 2018-10-30 6:38 ` [PATCH v2] " Denton Liu 2018-10-30 7:33 ` Junio C Hamano 2018-10-30 15:29 ` Duy Nguyen 2018-11-01 1:42 ` Junio C Hamano 2018-11-01 15:40 ` Duy Nguyen 2018-11-01 23:52 ` Junio C Hamano 2018-11-03 6:03 ` Duy Nguyen 2018-11-03 7:59 ` Denton Liu 2018-11-03 8:29 ` Duy Nguyen 2018-11-03 10:20 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).