On Tue, Oct 26 2021, Ævar Arnfjörð Bjarmason wrote:
> 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" .
