[PATCH] send-email: avoid duplicate specification warnings

[PATCH] send-email: avoid duplicate specification warnings

[Todd Zullinger]

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.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

I've run this through the full test suite.  I also compared the output of
--help to ensure it only differs in the removal of the "Duplicate
specification" warnings.  I _think_ that's a good sign that no other changes
will result.  But I would be grateful to anyone who can confirm or reject that
theory.

 git-send-email.perl | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index cacdbd6bb2..13d9c47fe5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -491,7 +491,6 @@ sub config_regexp {
 		    "bcc=s" => \@getopt_bcc,
 		    "no-bcc" => \$no_bcc,
 		    "chain-reply-to!" => \$chain_reply_to,
-		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
 		    "sendmail-cmd=s" => \$sendmail_cmd,
 		    "smtp-server=s" => \$smtp_server,
 		    "smtp-server-option=s" => \@smtp_server_options,
@@ -506,36 +505,27 @@ sub config_regexp {
 		    "smtp-auth=s" => \$smtp_auth,
 		    "no-smtp-auth" => sub {$smtp_auth = 'none'},
 		    "annotate!" => \$annotate,
-		    "no-annotate" => sub {$annotate = 0},
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "header-cmd=s" => \$header_cmd,
 		    "no-header-cmd" => \$no_header_cmd,
 		    "suppress-from!" => \$suppress_from,
-		    "no-suppress-from" => sub {$suppress_from = 0},
 		    "suppress-cc=s" => \@suppress_cc,
-		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
-		    "no-signed-off-cc|no-signed-off-by-cc" => sub {$signed_off_by_cc = 0},
-		    "cc-cover|cc-cover!" => \$cover_cc,
-		    "no-cc-cover" => sub {$cover_cc = 0},
-		    "to-cover|to-cover!" => \$cover_to,
-		    "no-to-cover" => sub {$cover_to = 0},
+		    "signed-off-by-cc!" => \$signed_off_by_cc,
+		    "cc-cover!" => \$cover_cc,
+		    "to-cover!" => \$cover_to,
 		    "confirm=s" => \$confirm,
 		    "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},
 		    "8bit-encoding=s" => \$auto_8bit_encoding,
 		    "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,

Re: [PATCH] send-email: avoid duplicate specification warnings

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

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

As long as these manual implementation of "no-" are doing true
opposite of the positive one, it should be sufficient to remove
them, so I'd prefer to see you explicitly say that you did audit
them all to make sure.

For example,

>  		    "annotate!" => \$annotate,
> -		    "no-annotate" => sub {$annotate = 0},

this is an example of good pair.  With the former, "--no-annotate"
and "--annotate" result in $annotate set to false and true, and the
latter attempts to set $annotate to false upon "--no-annotate", so
the net result of removing the latter should be a no-op.

>  		    "suppress-from!" => \$suppress_from,
> -		    "no-suppress-from" => sub {$suppress_from = 0},

Ditto.

As it is very late at night here, I didn't do a though job to scan
and validate all of them (some did not have their positive
counterparts in the context), though.  Thanks for woking on this.

Re: [PATCH] send-email: avoid duplicate specification warnings

[Jeff King]

On Tue, Nov 14, 2023 at 11:38:19AM -0500, Todd Zullinger wrote:

> 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:

This made me wonder if the warnings are new, or if the duplicated
auto-negated options are new. I.e., were the manual "--no-foo" option
specs doing something useful in the older versions (in which case we'd
need to do something more complicated)?

But I think the answer is no.  We've explicitly marked these with "!" to
indicate that they're negatable. And certainly running with Getopt::Long
2.52 (from perl 5.36, which is the current in Debian unstable) seems to
support them.

It does make me wonder why some boolean options are not marked as
negatable (even if just to countermand an earlier option), but that is
outside the scope of your patch.

> I've run this through the full test suite.  I also compared the output of
> --help to ensure it only differs in the removal of the "Duplicate
> specification" warnings.  I _think_ that's a good sign that no other changes
> will result.  But I would be grateful to anyone who can confirm or reject that
> theory.

I guess you meant "-h", not "--help", since the latter will just show
the manpage. But isn't "-h" just dumping a static usage message we
wrote, and not auto-generated by the code?

The changes look good to me (even after double-checking Junio's question
that they are all appropriately matched with their "positive" sides).
This one is curious:

> -		    "cc-cover|cc-cover!" => \$cover_cc,

It was an alternate name for itself? I think somebody just misunderstood
how the API was supposed to work. The "!" would applies to all names, if
I understand correctly, so this really is doing nothing beyond just
"cc-cover!", which is what your patch switches it to.

-Peff

Re: [PATCH] send-email: avoid duplicate specification warnings

[Todd Zullinger]

Jeff King wrote:
> On Tue, Nov 14, 2023 at 11:38:19AM -0500, Todd Zullinger wrote:
>> I've run this through the full test suite.  I also compared the output of
>> --help to ensure it only differs in the removal of the "Duplicate
>> specification" warnings.  I _think_ that's a good sign that no other changes
>> will result.  But I would be grateful to anyone who can confirm or reject that
>> theory.
> 
> I guess you meant "-h", not "--help", since the latter will just show
> the manpage. But isn't "-h" just dumping a static usage message we
> wrote, and not auto-generated by the code?

Yes to both.  This is why I shouldn't submit patches within
a few hours of waking up.

> The changes look good to me (even after double-checking Junio's question
> that they are all appropriately matched with their "positive" sides).

Indeed.  I need to go through them each to test that the
results match before and after.  With the fallback to
passing options to format-patch, testing outside of a git
repo makes this rather convenient.  If I've dropped an
option it will result in the "Cannot run git format-patch
from outside a repository" error.  That's a good start to
ensure the changes don't cause any regressions.

I did notice that I mistakenly dropped --[no-]signed-off-cc.
I need to keep:

    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,

as is.

> This one is curious:
> 
>> -		    "cc-cover|cc-cover!" => \$cover_cc,
> 
> It was an alternate name for itself? I think somebody just misunderstood
> how the API was supposed to work. The "!" would applies to all names, if
> I understand correctly, so this really is doing nothing beyond just
> "cc-cover!", which is what your patch switches it to.

I wondered about those as well.  Perhaps this is needed in
some older version of Getopt::Long?  I'll try to look
through the history of the module to see if that's the case.

Since this isn't anything new with 2.43, it doesn't need to
be fixed with much urgency.

Thanks both,

-- 
Todd

Re: [PATCH] send-email: avoid duplicate specification warnings

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> Since this isn't anything new with 2.43, it doesn't need to
> be fixed with much urgency.

True.  Unless the new version of Getopt::Long is quickly spreading
through our user base, that is.

> Thanks both,

Thanks for spotting the issue and acting on it quickly.

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

[Todd Zullinger]

Changes since v1:

    * Teach `--git-completion-helper` to output the '--no-' options.
      They are not included in the options hash and would otherwise be
      lost.

    * Restore the `--signed-off-cc` alias which was mistakenly removed.

Todd Zullinger (2):
  send-email: avoid duplicate specification warnings
  send-email: remove stray characters from usage

 git-send-email.perl | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

-- 
2.43.0.rc2

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

[Todd Zullinger]

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.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
I compared the output from

    git send-email --git-completion-helper | tr ' ' ' '\n' | sort

before and after the change to ensure no options were lost (or added).

I also confirmed that each of the options changed did not result in any
error.  Unrecognized options result in an error from `git format-patch`,
e.g.:

    $ git send-email --foo
    fatal: unrecognized argument: --foo
    format-patch -o /tmp/PaqtbH3jCw --foo: command returned error: 128

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

Another option to avoid the warning from Getopt::Long >= 2.55 would be
to remove the '!' negation, but that would drop support for the 'no'
prefix variants (e.g.: `--nocc-cover`).  While these are not documented
(and I don't think they ever were[2]), they have worked for a long, long
time.  Odds are good that some scripts rely on them and we don't want
anyone yelling at Junio.

I lean toward dropping support for the 21-year-old 5.8.0.

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;`.

[1] http://perlpunks.de/corelist/mversion?module=Getopt%3A%3ALong

[2] The 'no-' opts were added in f471494303 (git-send-email.perl:
    support no- prefix with older GetOptions, 2015-01-30).  The commit
    message says "the help only mentions the 'no-' prefix and not the
    'no' prefix, add explicit support for the 'no-' prefix to support
    older GetOptions versions."

 git-send-email.perl | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

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/!$//);
 
 			if ($key =~ /[:=][si]$/) {
 				$key =~ s/[:=][si]$//;
 				push (@send_email_opts, "--$_=") foreach (split (/\|/, $key));
 			} else {
 				push (@send_email_opts, "--$_") foreach (split (/\|/, $key));
+				if ($negate) {
+					push (@send_email_opts, "--no-$_") foreach (split (/\|/, $key));
+				}
 			}
 		}
 	}
@@ -491,7 +494,6 @@ sub config_regexp {
 		    "bcc=s" => \@getopt_bcc,
 		    "no-bcc" => \$no_bcc,
 		    "chain-reply-to!" => \$chain_reply_to,
-		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
 		    "sendmail-cmd=s" => \$sendmail_cmd,
 		    "smtp-server=s" => \$smtp_server,
 		    "smtp-server-option=s" => \@smtp_server_options,
@@ -506,36 +508,27 @@ sub config_regexp {
 		    "smtp-auth=s" => \$smtp_auth,
 		    "no-smtp-auth" => sub {$smtp_auth = 'none'},
 		    "annotate!" => \$annotate,
-		    "no-annotate" => sub {$annotate = 0},
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "header-cmd=s" => \$header_cmd,
 		    "no-header-cmd" => \$no_header_cmd,
 		    "suppress-from!" => \$suppress_from,
-		    "no-suppress-from" => sub {$suppress_from = 0},
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
-		    "no-signed-off-cc|no-signed-off-by-cc" => sub {$signed_off_by_cc = 0},
-		    "cc-cover|cc-cover!" => \$cover_cc,
-		    "no-cc-cover" => sub {$cover_cc = 0},
-		    "to-cover|to-cover!" => \$cover_to,
-		    "no-to-cover" => sub {$cover_to = 0},
+		    "cc-cover!" => \$cover_cc,
+		    "to-cover!" => \$cover_to,
 		    "confirm=s" => \$confirm,
 		    "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},
 		    "8bit-encoding=s" => \$auto_8bit_encoding,
 		    "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,
-- 
2.43.0.rc2

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

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

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

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

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

[PATCH v3 0/2] send-email: avoid duplicate specification warnings

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
[...]
>> 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.

That is much better.  I've adjusted the commit message similarly and
hopefully kept your improved wording largely intact.

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

Indeed it is.  I should have mentioned that more explicitly.  I added
the RFC tag to this round because I was unsure whether we'd want to go
the route of bumping the Perl requirement.  But I managed to not
actually say as much.

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

I think the new commit messages describe the changes better.  I didn't
include anything in RelNotes as I was presuming we'd leave this for
2.44 rather than risk causing any problems this late in the 2.43 cycle.
If you think the risk is low and/or the benefit is high, I can add it to
the 2.43.0 RelNotes.

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

Sounds good.

> 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?

Exactly.  We could bundle the two no* options together, but that's a
trivial style issue, i.e.:

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

Thanks for a very helpful review, as always.

Todd Zullinger (2):
  perl: bump the required Perl version to 5.8.1 from 5.8.0
  send-email: avoid duplicate specification warnings

 Documentation/CodingGuidelines          |  2 +-
 INSTALL                                 |  2 +-
 contrib/diff-highlight/DiffHighlight.pm |  2 +-
 contrib/mw-to-git/Git/Mediawiki.pm      |  2 +-
 git-archimport.perl                     |  2 +-
 git-cvsexportcommit.perl                |  2 +-
 git-cvsimport.perl                      |  2 +-
 git-cvsserver.perl                      |  2 +-
 git-send-email.perl                     | 23 ++++++++---------------
 git-svn.perl                            |  2 +-
 gitweb/INSTALL                          |  2 +-
 gitweb/gitweb.perl                      |  2 +-
 perl/Git.pm                             |  2 +-
 perl/Git/I18N.pm                        |  2 +-
 perl/Git/LoadCPAN.pm                    |  2 +-
 perl/Git/LoadCPAN/Error.pm              |  2 +-
 perl/Git/LoadCPAN/Mail/Address.pm       |  2 +-
 perl/Git/Packet.pm                      |  2 +-
 t/t0202/test.pl                         |  2 +-
 t/t5562/invoke-with-content-length.pl   |  2 +-
 t/t9700/test.pl                         |  2 +-
 t/test-terminal.perl                    |  2 +-
 22 files changed, 29 insertions(+), 36 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  b276216a53 perl: bump the required Perl version to 5.8.1 from 5.8.0
1:  59e2c79085 ! 2:  e076a2ede5 send-email: avoid duplicate specification warnings
    @@ Metadata
      ## Commit message ##
         send-email: avoid duplicate specification warnings
     
    -    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:
    +    A warning is issued for options which are specified more than once
    +    beginning with perl-Getopt-Long >= 2.55.  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
    @@ Commit message
           | error: last command exited with $?=1
           | not ok 37 - reject long lines
     
    -    Remove the duplicate option specs.
    +    Remove the duplicate option specs.  These are primarily the explicit
    +    '--no-' prefix opts which were added in f471494303 (git-send-email.perl:
    +    support no- prefix with older GetOptions, 2015-01-30).  This was done
    +    specifically to support perl-5.8.0 which includes Getopt::Long 2.32[1].
    +
    +    Getopt::Long 2.33 added support for the '--no-' prefix natively by
    +    appending '!' to the option specification string, which was included in
    +    perl-5.8.1 and is not present in perl-5.8.0.  The previous commit bumped
    +    the minimum supported Perl version to 5.8.1 so we no longer need to
    +    provide the '--no-' variants for negatable options manually.
     
         Teach `--git-completion-helper` to output the '--no-' options.  They are
         not included in the options hash and would otherwise be lost.
     
         Signed-off-by: Todd Zullinger <tmz@pobox.com>
     
      ## git-send-email.perl ##
     @@ git-send-email.perl: sub completion_helper {
      
      	foreach my $key (keys %$original_opts) {
      		unless (exists $not_for_completion{$key}) {
     -			$key =~ s/!$//;
    -+			my $negate = ($key =~ s/!$//);
    ++			my $negatable = ($key =~ s/!$//);
      
      			if ($key =~ /[:=][si]$/) {
      				$key =~ s/[:=][si]$//;
      				push (@send_email_opts, "--$_=") foreach (split (/\|/, $key));
      			} else {
      				push (@send_email_opts, "--$_") foreach (split (/\|/, $key));
    -+				if ($negate) {
    ++				if ($negatable) {
     +					push (@send_email_opts, "--no-$_") foreach (split (/\|/, $key));
     +				}
      			}
2:  c1f37d4395 < -:  ---------- send-email: remove stray characters from usage
-- 
2.43.0.rc2

[PATCH v3 1/2] perl: bump the required Perl version to 5.8.1 from 5.8.0

[Todd Zullinger]

The following commit will make use of a Getopt::Long feature which is
only present in Perl >= 5.8.1.  Document that as the minimum version we
support.

Many of our Perl scripts will continue to run with 5.8.0 but this change
allows us to adjust them as needed without breaking any promises to our
users.

The Perl requirement was last changed in d48b284183 (perl: bump the
required Perl version to 5.8 from 5.6.[21], 2010-09-24).  At that time,
5.8.0 was 8 years old.  It is now over 21 years old.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
I debated changing all the 'use 5.008;' lines here, as most don't
actually require a newer Perl, but the previous bump did the same.

I can see the merit in either direction.

Changing it allows future contributors to be confident in relying on
5.8.1 features.

Not changing it allows anyone stuck on 5.8.0 to continue using the perl
scripts which don't actually require 5.8.1.

Tangentially, the Perl docs for 'use' function recommend against the
5.008001 form[1]:

    Specifying VERSION as a numeric argument of the form 5.024001 should
    generally be avoided as older less readable syntax compared to
    v5.24.1. Before perl 5.8.0 released in 2002 the more verbose numeric
    form was the only supported syntax, which is why you might see it in
    older code.

        use v5.24.1;    # compile time version check
        use 5.24.1;     # ditto
        use 5.024_001;  # ditto; older syntax compatible with perl 5.6

I'm not enough of a Perl coder to have a strong preference or desire to
push for such a change, but I thought it was worth mentioning in case
others wonder why we're using the 5.008001 form.

[1] https://perldoc.perl.org/functions/use#use-VERSION

 Documentation/CodingGuidelines          | 2 +-
 INSTALL                                 | 2 +-
 contrib/diff-highlight/DiffHighlight.pm | 2 +-
 contrib/mw-to-git/Git/Mediawiki.pm      | 2 +-
 git-archimport.perl                     | 2 +-
 git-cvsexportcommit.perl                | 2 +-
 git-cvsimport.perl                      | 2 +-
 git-cvsserver.perl                      | 2 +-
 git-send-email.perl                     | 4 ++--
 git-svn.perl                            | 2 +-
 gitweb/INSTALL                          | 2 +-
 gitweb/gitweb.perl                      | 2 +-
 perl/Git.pm                             | 2 +-
 perl/Git/I18N.pm                        | 2 +-
 perl/Git/LoadCPAN.pm                    | 2 +-
 perl/Git/LoadCPAN/Error.pm              | 2 +-
 perl/Git/LoadCPAN/Mail/Address.pm       | 2 +-
 perl/Git/Packet.pm                      | 2 +-
 t/t0202/test.pl                         | 2 +-
 t/t5562/invoke-with-content-length.pl   | 2 +-
 t/t9700/test.pl                         | 2 +-
 t/test-terminal.perl                    | 2 +-
 22 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 8d3a467c01..39b9b7260f 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -490,7 +490,7 @@ For Perl programs:
 
  - Most of the C guidelines above apply.
 
- - We try to support Perl 5.8 and later ("use Perl 5.008").
+ - We try to support Perl 5.8.1 and later ("use Perl 5.008001").
 
  - use strict and use warnings are strongly preferred.
 
diff --git a/INSTALL b/INSTALL
index 4b42288882..06f29a8ae7 100644
--- a/INSTALL
+++ b/INSTALL
@@ -119,7 +119,7 @@ Issues of note:
 	- A POSIX-compliant shell is required to run some scripts needed
 	  for everyday use (e.g. "bisect", "request-pull").
 
-	- "Perl" version 5.8 or later is needed to use some of the
+	- "Perl" version 5.8.1 or later is needed to use some of the
 	  features (e.g. sending patches using "git send-email",
 	  interacting with svn repositories with "git svn").  If you can
 	  live without these, use NO_PERL.  Note that recent releases of
diff --git a/contrib/diff-highlight/DiffHighlight.pm b/contrib/diff-highlight/DiffHighlight.pm
index 376f577737..636add6968 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -1,6 +1,6 @@
 package DiffHighlight;
 
-use 5.008;
+use 5.008001;
 use warnings FATAL => 'all';
 use strict;
 
diff --git a/contrib/mw-to-git/Git/Mediawiki.pm b/contrib/mw-to-git/Git/Mediawiki.pm
index 917d9e2d32..ff7811225e 100644
--- a/contrib/mw-to-git/Git/Mediawiki.pm
+++ b/contrib/mw-to-git/Git/Mediawiki.pm
@@ -1,6 +1,6 @@
 package Git::Mediawiki;
 
-use 5.008;
+use 5.008001;
 use strict;
 use POSIX;
 use Git;
diff --git a/git-archimport.perl b/git-archimport.perl
index b7c173c345..f5a317b899 100755
--- a/git-archimport.perl
+++ b/git-archimport.perl
@@ -54,7 +54,7 @@ =head1 Devel Notes
 
 =cut
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 use Getopt::Std;
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 289d4bc684..1e03ba94d1 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -1,6 +1,6 @@
 #!/usr/bin/perl
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 use Getopt::Std;
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 7bf3c12d67..07ea3443f7 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -13,7 +13,7 @@
 # The head revision is on branch "origin" by default.
 # You can change that with the '-o' option.
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 use Getopt::Long;
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 7b757360e2..124f598bdc 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -15,7 +15,7 @@
 ####
 ####
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 use bytes;
diff --git a/git-send-email.perl b/git-send-email.perl
index cacdbd6bb2..d75a4a33dd 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -16,7 +16,7 @@
 #    and second line is the subject of the message.
 #
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 use Getopt::Long;
@@ -228,7 +228,7 @@ sub system_or_msg {
 	my @sprintf_args = ($cmd_name ? $cmd_name : $args->[0], $exit_code);
 	if (defined $msg) {
 		# Quiet the 'redundant' warning category, except we
-		# need to support down to Perl 5.8, so we can't do a
+		# need to support down to Perl 5.8.1, so we can't do a
 		# "no warnings 'redundant'", since that category was
 		# introduced in perl 5.22, and asking for it will die
 		# on older perls.
diff --git a/git-svn.perl b/git-svn.perl
index 4e8878f035..b0d0a50984 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1,7 +1,7 @@
 #!/usr/bin/perl
 # Copyright (C) 2006, Eric Wong <normalperson@yhbt.net>
 # License: GPL v2 or later
-use 5.008;
+use 5.008001;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 use strict;
 use vars qw/	$AUTHOR $VERSION
diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index a58e6b3c44..dadc6efa81 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -29,7 +29,7 @@ Requirements
 ------------
 
  - Core git tools
- - Perl 5.8
+ - Perl 5.8.1
  - Perl modules: CGI, Encode, Fcntl, File::Find, File::Basename.
  - web server
 
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e66eb3d9ba..55e7c6567e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7,7 +7,7 @@
 #
 # This program is licensed under the GPLv2
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 # handle ACL in file access tests
diff --git a/perl/Git.pm b/perl/Git.pm
index 117765dc73..03bf570bf4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -7,7 +7,7 @@ =head1 NAME
 
 package Git;
 
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index 895e759c57..5454c3a6d2 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -1,5 +1,5 @@
 package Git::I18N;
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 BEGIN {
diff --git a/perl/Git/LoadCPAN.pm b/perl/Git/LoadCPAN.pm
index 0c360bc799..8c7fa805f9 100644
--- a/perl/Git/LoadCPAN.pm
+++ b/perl/Git/LoadCPAN.pm
@@ -1,5 +1,5 @@
 package Git::LoadCPAN;
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 
diff --git a/perl/Git/LoadCPAN/Error.pm b/perl/Git/LoadCPAN/Error.pm
index 5d84c20288..5cecb0fcd6 100644
--- a/perl/Git/LoadCPAN/Error.pm
+++ b/perl/Git/LoadCPAN/Error.pm
@@ -1,5 +1,5 @@
 package Git::LoadCPAN::Error;
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 use Git::LoadCPAN (
diff --git a/perl/Git/LoadCPAN/Mail/Address.pm b/perl/Git/LoadCPAN/Mail/Address.pm
index 340e88a7a5..9f808090a6 100644
--- a/perl/Git/LoadCPAN/Mail/Address.pm
+++ b/perl/Git/LoadCPAN/Mail/Address.pm
@@ -1,5 +1,5 @@
 package Git::LoadCPAN::Mail::Address;
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 use Git::LoadCPAN (
diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
index d144f5168f..d896e69523 100644
--- a/perl/Git/Packet.pm
+++ b/perl/Git/Packet.pm
@@ -1,5 +1,5 @@
 package Git::Packet;
-use 5.008;
+use 5.008001;
 use strict;
 use warnings $ENV{GIT_PERL_FATAL_WARNINGS} ? qw(FATAL all) : ();
 BEGIN {
diff --git a/t/t0202/test.pl b/t/t0202/test.pl
index 2cbf7b9590..47d96a2a13 100755
--- a/t/t0202/test.pl
+++ b/t/t0202/test.pl
@@ -1,5 +1,5 @@
 #!/usr/bin/perl
-use 5.008;
+use 5.008001;
 use lib (split(/:/, $ENV{GITPERLLIB}));
 use strict;
 use warnings;
diff --git a/t/t5562/invoke-with-content-length.pl b/t/t5562/invoke-with-content-length.pl
index 718dd9b49d..9babb9a375 100644
--- a/t/t5562/invoke-with-content-length.pl
+++ b/t/t5562/invoke-with-content-length.pl
@@ -1,4 +1,4 @@
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 6d753708d2..d8e85482ab 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -1,7 +1,7 @@
 #!/usr/bin/perl
 use lib (split(/:/, $ENV{GITPERLLIB}));
 
-use 5.008;
+use 5.008001;
 use warnings;
 use strict;
 
diff --git a/t/test-terminal.perl b/t/test-terminal.perl
index 1bcf01a9a4..3810e9bb43 100755
--- a/t/test-terminal.perl
+++ b/t/test-terminal.perl
@@ -1,5 +1,5 @@
 #!/usr/bin/perl
-use 5.008;
+use 5.008001;
 use strict;
 use warnings;
 use IO::Pty;
-- 
2.43.0.rc2

Re: [PATCH v3 1/2] perl: bump the required Perl version to 5.8.1 from 5.8.0

[Jeff King]

On Thu, Nov 16, 2023 at 02:30:10PM -0500, Todd Zullinger wrote:

> The following commit will make use of a Getopt::Long feature which is
> only present in Perl >= 5.8.1.  Document that as the minimum version we
> support.
> 
> Many of our Perl scripts will continue to run with 5.8.0 but this change
> allows us to adjust them as needed without breaking any promises to our
> users.
> 
> The Perl requirement was last changed in d48b284183 (perl: bump the
> required Perl version to 5.8 from 5.6.[21], 2010-09-24).  At that time,
> 5.8.0 was 8 years old.  It is now over 21 years old.

Thanks, IMHO this is long overdue. You mentioned 5.10 elsewhere in the
thread, and it came up recently in a discussion (it would allow the use
of "//" for defined-or). So we could perhaps go a bit farther. But I am
also fine with 5.8.1 for now, if that is all it takes for this fix (and
I expect the chance that it causes a problem for anybody to be close to
zero).

> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> I debated changing all the 'use 5.008;' lines here, as most don't
> actually require a newer Perl, but the previous bump did the same.
> 
> I can see the merit in either direction.
> 
> Changing it allows future contributors to be confident in relying on
> 5.8.1 features.
> 
> Not changing it allows anyone stuck on 5.8.0 to continue using the perl
> scripts which don't actually require 5.8.1.

Yeah, I can see both sides of the argument. I think I'd err on the side
of bumping (as you did here). That lets somebody who will be affected
know immediately, rather than only finding out when we randomly depend
on a feature later.

All of this discussion could likewise go in the commit message. :)

> Tangentially, the Perl docs for 'use' function recommend against the
> 5.008001 form[1]:
> 
>     Specifying VERSION as a numeric argument of the form 5.024001 should
>     generally be avoided as older less readable syntax compared to
>     v5.24.1. Before perl 5.8.0 released in 2002 the more verbose numeric
>     form was the only supported syntax, which is why you might see it in
>     older code.
> 
>         use v5.24.1;    # compile time version check
>         use 5.24.1;     # ditto
>         use 5.024_001;  # ditto; older syntax compatible with perl 5.6
> 
> I'm not enough of a Perl coder to have a strong preference or desire to
> push for such a change, but I thought it was worth mentioning in case
> others wonder why we're using the 5.008001 form.

I doubt it matters too much either way. I suspect at the time we moved
to v5.8 the nicer syntax was still pretty new (having only been
introduced by v5.6, which we were moving off of) and that older versions
of perl might not give as nice a message when they see it. But given
that 5.6 is now 23 years old, we can probably assume nobody will use it
(or at least they will be accustomed to whatever ugly message it
produces).

But IMHO that should be done as a separate patch anyway.

-Peff

[PATCH v3 2/2] send-email: avoid duplicate specification warnings

[Todd Zullinger]

A warning is issued for options which are specified more than once
beginning with perl-Getopt-Long >= 2.55.  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.  These are primarily the explicit
'--no-' prefix opts which were added in f471494303 (git-send-email.perl:
support no- prefix with older GetOptions, 2015-01-30).  This was done
specifically to support perl-5.8.0 which includes Getopt::Long 2.32[1].

Getopt::Long 2.33 added support for the '--no-' prefix natively by
appending '!' to the option specification string, which was included in
perl-5.8.1 and is not present in perl-5.8.0.  The previous commit bumped
the minimum supported Perl version to 5.8.1 so we no longer need to
provide the '--no-' variants for negatable options manually.

Teach `--git-completion-helper` to output the '--no-' options.  They are
not included in the options hash and would otherwise be lost.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 git-send-email.perl | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index d75a4a33dd..f214bd4521 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 $negatable = ($key =~ s/!$//);
 
 			if ($key =~ /[:=][si]$/) {
 				$key =~ s/[:=][si]$//;
 				push (@send_email_opts, "--$_=") foreach (split (/\|/, $key));
 			} else {
 				push (@send_email_opts, "--$_") foreach (split (/\|/, $key));
+				if ($negatable) {
+					push (@send_email_opts, "--no-$_") foreach (split (/\|/, $key));
+				}
 			}
 		}
 	}
@@ -491,7 +494,6 @@ sub config_regexp {
 		    "bcc=s" => \@getopt_bcc,
 		    "no-bcc" => \$no_bcc,
 		    "chain-reply-to!" => \$chain_reply_to,
-		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
 		    "sendmail-cmd=s" => \$sendmail_cmd,
 		    "smtp-server=s" => \$smtp_server,
 		    "smtp-server-option=s" => \@smtp_server_options,
@@ -506,36 +508,27 @@ sub config_regexp {
 		    "smtp-auth=s" => \$smtp_auth,
 		    "no-smtp-auth" => sub {$smtp_auth = 'none'},
 		    "annotate!" => \$annotate,
-		    "no-annotate" => sub {$annotate = 0},
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
 		    "header-cmd=s" => \$header_cmd,
 		    "no-header-cmd" => \$no_header_cmd,
 		    "suppress-from!" => \$suppress_from,
-		    "no-suppress-from" => sub {$suppress_from = 0},
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
-		    "no-signed-off-cc|no-signed-off-by-cc" => sub {$signed_off_by_cc = 0},
-		    "cc-cover|cc-cover!" => \$cover_cc,
-		    "no-cc-cover" => sub {$cover_cc = 0},
-		    "to-cover|to-cover!" => \$cover_to,
-		    "no-to-cover" => sub {$cover_to = 0},
+		    "cc-cover!" => \$cover_cc,
+		    "to-cover!" => \$cover_to,
 		    "confirm=s" => \$confirm,
 		    "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},
 		    "8bit-encoding=s" => \$auto_8bit_encoding,
 		    "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,
-- 
2.43.0.rc2

Re: [PATCH v3 0/2] send-email: avoid duplicate specification warnings

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

>> This sounds like something that is worth describing in the log
>> message (and Release Notes).
>
> I think the new commit messages describe the changes better.  I didn't
> include anything in RelNotes as I was presuming we'd leave this for
> 2.44 rather than risk causing any problems this late in the 2.43 cycle.
> If you think the risk is low and/or the benefit is high, I can add it to
> the 2.43.0 RelNotes.

Please don't worry about the release notes, which I'll do only when
the topic hits the 'master' branch.  It was meant primarily as a
note to myself.  And I agree that this would be material for 2.44
and later.

Both patches, plus the stray single quote fix patch, looked good.

Thanks.

[RFC PATCH v2 2/2] send-email: remove stray characters from usage

[Todd Zullinger]

A few stray single quotes crept into the usage string in a2ce608244
(send-email docs: add format-patch options, 2021-10-25).  Remove them.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
This is not scrictly tied to the previous commit.  It just stood out
while I was reviewing the usage output.

 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 94046e0fb7..cd2f0ae14e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,8 +28,8 @@
 
 sub usage {
 	print <<EOT;
-git send-email' [<options>] <file|directory>
-git send-email' [<options>] <format-patch options>
+git send-email [<options>] <file|directory>
+git send-email [<options>] <format-patch options>
 git send-email --dump-aliases
 
   Composing:
-- 
2.43.0.rc2

Re: [RFC PATCH v2 2/2] send-email: remove stray characters from usage

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> A few stray single quotes crept into the usage string in a2ce608244
> (send-email docs: add format-patch options, 2021-10-25).  Remove them.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> This is not scrictly tied to the previous commit.  It just stood out
> while I was reviewing the usage output.

Thanks.  Let's split this out as a docfix patch and handle it
separately.


>
>  git-send-email.perl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 94046e0fb7..cd2f0ae14e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -28,8 +28,8 @@
>  
>  sub usage {
>  	print <<EOT;
> -git send-email' [<options>] <file|directory>
> -git send-email' [<options>] <format-patch options>
> +git send-email [<options>] <file|directory>
> +git send-email [<options>] <format-patch options>
>  git send-email --dump-aliases
>  
>    Composing:

[PATCH] send-email: remove stray characters from usage

[Todd Zullinger]

A few stray single quotes crept into the usage string in a2ce608244
(send-email docs: add format-patch options, 2021-10-25).  Remove them.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
[I trimmed the Cc: list]

Junio C Hamano wrote:
> Thanks.  Let's split this out as a docfix patch and handle it
> separately.

Done. :)

 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index cacdbd6bb2..d24e981d61 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,8 +28,8 @@
 
 sub usage {
 	print <<EOT;
-git send-email' [<options>] <file|directory>
-git send-email' [<options>] <format-patch options>
+git send-email [<options>] <file|directory>
+git send-email [<options>] <format-patch options>
 git send-email --dump-aliases
 
   Composing:
-- 
2.43.0.rc2