| Message ID | 72331ce9275ce995009fe8dd3d586bb9d71f2cbf.1540881141.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] completion: use builtin completion for format-patch | expand |
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
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 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?
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 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.
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(); }
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!
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 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.
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
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(-)