Junio C Hamano wrote:
> Todd Zullinger <tmz@xxxxxxxxx> 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@xxxxxxxxx>
## 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
