Re: [PATCH v8 0/2] send-email: shell completion improvements
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Oct 26, 2021, 00:48 UTC
- Message-ID
- <211026.86k0i01u3c.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <211026.86o87c1zry.gmgdl@evledraar.gmail.com>
On Tue, Oct 26 2021, Ævar Arnfjörð Bjarmason wrote:
Show 101 quoted lines
> On Mon, Oct 25 2021, Thiago Perrotta wrote:
>
>>> ...I think that re-indentation is better left alone for the patch
>>> readability.
>>
>> Reverted the `GetOptions` indentation. Noise is now gone :-)
>
> Thanks.
>
>>> First, in your 1/3 you're adding a \n, but in 2/3 we end up with \n\n. I
>>> think you can just skip 1/3, maybe mention "how it also has a "\n" in
>>> the commit message.
>>
>> I don't quite see how this would fit into the commit message. A comment in the
>> code seems to fit better to account for this detail. That's what I did, but if
>> you still disagree, please elaborate where in the commit message this sentence
>> should be added.
>
> Makes sense I think, will take a look.
>
>>> You then strip out "--" arguments from the combined list, but isn't this
>>> something we do need to emit? I.e. it's used as syntax by the bash
>>> completion isn't it? (I just skimmed the relevant C code in
>>> parse-options.c).
>>
>> I interpreted that standalone `--` as an extraneous / useless token. If it's
>> there intentionally, then I am reverting my stripping of it. At the end of the
>> day whether to include it or not is a small detail, but FYI, when I do:
>>
>> $ git clone -<TAB>
>>
>> in bash, nothing happens. I would have expected it to be expanded to "--"
>> because of the explicit "--", but it doesn't. Therefore my conclusion is that
>> "--" in the output of "--git-completion-helper" is useless. Am I missing
>> something? What would be the function of the standalone "--" then?
>>
>> From my local testing, whether the options are sorted or not, whether
>> they are repeated or not, whether they follow a specific order with
>> respect to "--" or not, all of those details seem not to matter. Bash
>> completion seems to handle all of those cases just fine interactively.
>
> Digging a bit more: It's for folding away options that are negated, not
> for completing "-<TAB>". See the examples at b221b5ab9b9 (completion:
> collapse extra --no-.. options, 2018-06-06).
>
>> In fact, here's another example:
>>
>> $ git init --git-completion-helper | tr ' ' '\n' | grep -C1 '^--$'
>> --no-template
>> --
>> --no-bare
>>
>> ...there are --no-* options both _before_ and _after_ the --. I really
>> cannot see the point of the -- in the output, it seems to be just noise.
>
> Right, because some --no-whatever we define because we've got a
> --whatever and it's boolean, but for others we've got a --no-whatever as
> the primary, as in th case of --no-template..
>
>> I readded -- to the output anyway since you requested it, but if it
>> needs to follow a certain spec w.r.t. ordering, we should have tests for
>> it. This specific part (the -- and the --no- order thing) of the commit
>> is something I am not keen to doing though, at least not in this patch
>> series; sorry, it already goes far beyond the scope of my original
>> intent. Anything else you ask for that is inline with the original
>> intent (like generating options programatically instead of hard-coding
>> them) I am fine with though, and in fact I believe I have addressed all
>> comments so far, if there's anything else I may have missed let me know.
>
> Yeah sorry about the confusion so far, it's also been a voyage of
> discovery for me :)
>
> This time around I tested with:
>
> diff --git a/parse-options.c b/parse-options.c
> index 6e0535bdaad..d659309c5e7 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -515,8 +515,6 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
> static void show_negated_gitcomp(const struct option *opts, int show_all,
> int nr_noopts)
> {
> - int printed_dashdash = 0;
> -
> for (; opts->type != OPTION_END; opts++) {
> int has_unset_form = 0;
> const char *name;
> @@ -551,10 +549,6 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,
> if (nr_noopts < 0)
> printf(" --%s", name);
> } else if (nr_noopts >= 0) {
> - if (nr_noopts && !printed_dashdash) {
> - printf(" --");
> - printed_dashdash = 1;
> - }
> printf(" --no-%s", opts->long_name);
> nr_noopts++;
> }
>
> Which will fail a test in t/t9902-completion.sh showing this specific
> behavior.FWIW I came up with the below on top while testing this. I think your patch series is fine and we should just take it as-is.
This edge case of "--no" behavior isn't something we support in any sensible way already, so it can just be left for later.
But since I think I gave you some bad advice and WIP code (sorry!) early on I think this is closer to a 1=1 mapping to parse-options.c behavior, i.e. we split up "no" and "non-no" options, put them on either side of the "--", but are careful to put /some/ "--no" options on the RHS.
This has at least one bug: A few options have e.g. --no-to= and --no-to, so both "=" and "" forms.
There's also some unrelated fixes there, e.g. the existing --dump-aliases behavior was kind of dumb for what it was trying to do, and I changed it since it got in the way of hacking this up.
diff --git a/git-send-email.perl b/git-send-email.perl index 04087221aa7..9d6cdf52cc3 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -120,32 +120,87 @@ sub uniq { } sub completion_helper { - my ($original_opts) = @_; - my %not_for_completion = ( - "git-completion-helper" => undef, - "h" => undef, + my ($options, $no_opt) = @_; + + my %fp_opt_types; + my (@fp_pos_opt, @fp_neg_opt); + my $saw_dashdash = 0; + my $fp_gch = Git::command('format-patch', '--git-completion-helper'); + for my $opt (split ' ', $fp_gch) { + if ($opt eq '--') { + $saw_dashdash = 1; + next; + } + $opt =~ s/^--//; + my $str = $opt =~ s/=$//; + if ($saw_dashdash) { + push @fp_neg_opt => $opt; + } else { + push @fp_pos_opt => $opt; + } + $fp_opt_types{$opt} = $str ? '=' : ''; + } + + ## Parse options according to Getopt::Long specifications. See + ## "perldoc Getopt::Long". + my %se_opt_types; + my (@se_pos_opt, @se_neg_opt); + for my $item ([$options, \@se_pos_opt], [$no_opt, \@se_neg_opt]) { + my ($orig, $new) = @$item; + @$new = map { + my $type; + $type = '' if !$type && s/\!$//; + $type = '=' if !$type && s/[=:][sid]$//; + $type = '' if !$type; # the default + # We don't handle all possible Getopt::Long argument specifications + die "BUG: unknown argument specification for $_" if /[=:+]/; + $se_opt_types{$_} = $type for split /\|/; + split /\|/; + } keys %$orig; + } + + ## Sanity check that format-patch options and send-email + ## options don't clash. We have existing conflicts, but should + ## not be adding more. + my %conflicting; + my @conflicting = qw( + cc + from + in-reply-to + no-cc + no-thread + no-to + quiet + thread + to ); - my @send_email_opts = (); - - foreach my $key (keys %$original_opts) { - unless (exists $not_for_completion{$key}) { - $key =~ s/!$//; + @conflicting{@conflicting} = (); + for my $opt (@fp_neg_opt, @fp_pos_opt) { + next unless exists $se_opt_types{$opt}; + next if exists $conflicting{$opt}; + warn "BUG: have a format-patch option '$opt' clashing with send-email"; + } - if ($key =~ /[:=][si]$/) { - $key =~ s/[:=][si]$//; - push (@send_email_opts, "--$_=") foreach (split (/\|/, $key)); - } else { - push (@send_email_opts, "--$_") foreach (split (/\|/, $key)); - } - } + ## Sanity check that for a --no-foo we have a positive --foo + my @se_bool_neg_opt = grep { /^no-/ and $se_opt_types{$_} eq '' } keys %se_opt_types; + for my $opt (sort @se_bool_neg_opt) { + my $pos = $opt; $pos =~ s/^no-//; + next if exists $se_opt_types{$pos}; + die "BUG: Should have a '$pos' corresponding to '$opt'"; } - my @format_patch_opts = split(/ /, Git::command('format-patch', '--git-completion-helper')); - my @opts = (@send_email_opts, @format_patch_opts); - @opts = uniq (grep !/^$/, @opts); - # There's an implicit '\n' here already, no need to add an explicit one. - print "@opts"; - exit(0); + my @pos_opts = sort( + map({ "--$_$fp_opt_types{$_}" } @fp_pos_opt), + map({ "--$_$se_opt_types{$_}" } @se_pos_opt), + ); + my @neg_opts = sort( + map({ "--$_$fp_opt_types{$_}" } @fp_neg_opt), + map({ "--$_$se_opt_types{$_}" } @se_neg_opt), + ); + my @all_opts = (uniq(@pos_opts), "--", uniq(@neg_opts)); + + print "@all_opts\n"; + exit; } # most mail servers generate the Date: header, but not all... @@ -470,20 +525,14 @@ sub config_regexp { read_config(\%known_config_keys, \%configured, "sendemail"); } -# Begin by accumulating all the variables (defined above), that we will end up -# needing, first, from the command line: - -my $help; -my $git_completion_helper; -my %dump_aliases_options = ( - "h" => \$help, - "dump-aliases" => \$dump_aliases, +my $help = 0; +my $git_completion_helper = 0; +my %options_no_completion = ( + "h" => \$help, + "git-completion-helper" => \$git_completion_helper, ); -$rc = GetOptions(%dump_aliases_options); -usage() unless $rc; -die __("--dump-aliases incompatible with other options\n") - if !$help and $dump_aliases and @ARGV; my %options = ( + "dump-aliases" => \$dump_aliases, "sender|from=s" => \$sender, "in-reply-to=s" => \$initial_in_reply_to, "reply-to=s" => \$reply_to, @@ -528,9 +577,7 @@ sub config_regexp { "dry-run" => \$dry_run, "envelope-sender=s" => \$envelope_sender, "thread!" => \$thread, - "no-thread" => sub {$thread = 0}, "validate!" => \$validate, - "no-validate" => sub {$validate = 0}, "transfer-encoding=s" => \$target_xfer_encoding, "format-patch!" => \$format_patch, "no-format-patch" => sub {$format_patch = 0}, @@ -538,12 +585,23 @@ sub config_regexp { "compose-encoding=s" => \$compose_encoding, "force" => \$force, "xmailer!" => \$use_xmailer, - "no-xmailer" => sub {$use_xmailer = 0}, "batch-size=i" => \$batch_size, "relogin-delay=i" => \$relogin_delay, - "git-completion-helper" => \$git_completion_helper, ); -$rc = GetOptions(%options); +# --no-* options that disable a default that's otherwise enabled by +# default. Matters for --git-completion-helper. +my %no_options = ( + "no-thread" => sub {$thread = 0}, + "no-validate" => sub {$validate = 0}, + "no-xmailer" => sub {$use_xmailer = 0}, +); +my @ORIG_ARGV = @ARGV; +$rc = GetOptions(%options_no_completion, %options); +usage() unless $rc; + +# Option compatibility +die __("--dump-aliases incompatible with other options\n") + if $dump_aliases and @ORIG_ARGV > 1; # Munge any "either config or getopt, not both" variables my @initial_to = @getopt_to ? @getopt_to : ($no_to ? () : @config_to); @@ -551,11 +609,7 @@ sub config_regexp { my @initial_bcc = @getopt_bcc ? @getopt_bcc : ($no_bcc ? () : @config_bcc); usage() if $help; -my %all_options = (%options, %dump_aliases_options, %identity_options); -completion_helper(\%all_options) if $git_completion_helper; -unless ($rc) { - usage(); -} +completion_helper({%options, %identity_options}, \%no_options) if $git_completion_helper; if ($forbid_sendmail_variables && grep { /^sendmail/s } keys %known_config_keys) { die __("fatal: found configuration options for 'sendmail'\n" .