[PATCH] completion: use builtin completion for format-patch

[PATCH] completion: use builtin completion for format-patch

[Denton Liu]

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

Re: [PATCH] completion: use builtin completion for format-patch

[Junio C Hamano]

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

Re: [PATCH] completion: use builtin completion for format-patch

[Denton Liu]

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.

[PATCH v2] completion: use builtin completion for format-patch

[Denton Liu]

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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Junio C Hamano]

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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Junio C Hamano]

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?

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Junio C Hamano]

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.

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Denton Liu]

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!

Re: [PATCH v2] completion: use builtin completion for format-patch

[Duy Nguyen]

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

Re: [PATCH v2] completion: use builtin completion for format-patch

[Junio C Hamano]

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.