Re: [PATCH v8 0/2] send-email: shell completion improvements

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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" .