Re: [RFC PATCH v2 1/2] send-email: avoid duplicate specification warnings

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 16, 2023, 04:58 UTC
Message-ID: <xmqq4jhmthtg.fsf@gitster.g>
In-Reply-To: <20231115173952.339303-2-tmz@pobox.com>

Todd Zullinger <tmz@pobox.com> writes:

Show 23 quoted lines

> With perl-Getopt-Long >= 2.55 a warning is issued for options which are
> specified more than once.  In addition to causing users to see warnings,
> this results in test failures which compare the output.  An example,
> from t9001-send-email.37:
>
>   | +++ diff -u expect actual
>   | --- expect      2023-11-14 10:38:23.854346488 +0000
>   | +++ actual      2023-11-14 10:38:23.848346466 +0000
>   | @@ -1,2 +1,7 @@
>   | +Duplicate specification "no-chain-reply-to" for option "no-chain-reply-to"
>   | +Duplicate specification "to-cover|to-cover!" for option "to-cover"
>   | +Duplicate specification "cc-cover|cc-cover!" for option "cc-cover"
>   | +Duplicate specification "no-thread" for option "no-thread"
>   | +Duplicate specification "no-to-cover" for option "no-to-cover"
>   |  fatal: longline.patch:35 is longer than 998 characters
>   |  warning: no patches were sent
>   | error: last command exited with $?=1
>   | not ok 37 - reject long lines
>
> Remove the duplicate option specs.
>
> Teach `--git-completion-helper` to output the '--no-' options.  They are
> not included in the options hash and would otherwise be lost.

Nice to see a careful handling of potential fallouts.

Show 14 quoted lines

> A little history:
>
>   Support for the '--no-' prefix was added in Getopt::Long >= 2.33, in
>   commit 8ca8b48 (Negatable options (with "!") now also support the
>   "no-" prefix., 2003-04-04).  Getopt::Long 2.34 was included in
>   perl-5.8.1 (2003-09-25), per Module::CoreList[1].
>   
>   We list perl-5.8 as the minimum version in INSTALL.  This would leave
>   users with perl-5.8.0 (2002-07-18) with non-working arguments for
>   options where we're removing the explicit 'no-' variant.
>   
>   The explicit 'no-' opts were added in f471494303 (git-send-email.perl:
>   support no- prefix with older GetOptions, 2015-01-30), specifically to
>   support perl-5.8.0 which includes the older Getopt::Long.

These are all very much relevant and deserve to be in the log message, not hidden under the three-dash line, I would think. Thanks for digging the history. The first paragraph was a bit hard to read as it wasn't clear "support" on which side is being discussed, though. If it were written perhaps like so:

   Getopt::Long >= 2.33 started supporting the '--no-' prefix
   natively by appending '!' to the option specification string,
   which was shipped with perl-5.8.1 and not present in perl-5.8.0

it would have been clear that it was talking about the support given by Getopt module, not on our side.

> It may be time to bump the Perl requirement to 5.8.1 (2003-09-25) or
> even 5.10.0 (2007-12-18).  We last bumped the requirement from 5.6 to
> 5.8 in d48b284183 (perl: bump the required Perl version to 5.8 from
> 5.6.[21], 2010-09-24).

Isn't the position this patch takes a lot stronger than "It may be time"? If we applied this patch, it drops the support for folks with Perl 5.8.0 (which I do not think is a bad thing, by the way).

This sounds like something that is worth describing in the log message (and Release Notes).

> If there is a way to have our cake without any consequence, I'm happy to
> hear it.  If not, I'll add a commit which bumps the requirement in
> general or notes that some git-send-email requires perl >= 5.8.1 and
> adjusts the 'use' line there to `use 5.008001;`.

Sounds like a plan.

Show 10 quoted lines

> diff --git a/git-send-email.perl b/git-send-email.perl
> index cacdbd6bb2..94046e0fb7 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -119,13 +119,16 @@ sub completion_helper {
>  
>  	foreach my $key (keys %$original_opts) {
>  		unless (exists $not_for_completion{$key}) {
> -			$key =~ s/!$//;
> +			my $negate = ($key =~ s/!$//);

A very minor nit, but I'd call this $negatable if I were doing this patch.

Just to make sure I did not misunderstand what you said below the three-dash line, if we were to take the other option that allows us to live with 5.8.0, we would make this hunk ...

>  		    "chain-reply-to!" => \$chain_reply_to,
> -		    "no-chain-reply-to" => sub {$chain_reply_to = 0},

... look more like this?

> -		    "chain-reply-to!" => \$chain_reply_to,
> +		    "chain-reply-to" => \$chain_reply_to,
>  		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
> +		    "nochain-reply-to" => sub {$chain_reply_to = 0},

That is, by removing the "!" suffix, we reject the native support of "--no-*" offered by Getopt::Long, and implement the negated variants ourselves?

Thanks.