From: Junio C Hamano <gitster@pobox.com>
To: Todd Zullinger <tmz@pobox.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Ondřej Pohořelský" <opohorel@redhat.com>
Subject: Re: [RFC PATCH v2 1/2] send-email: avoid duplicate specification warnings
Date: Thu, 16 Nov 2023 13:58:51 +0900 [thread overview]
Message-ID: <xmqq4jhmthtg.fsf@gitster.g> (raw)
In-Reply-To: <20231115173952.339303-2-tmz@pobox.com> (Todd Zullinger's message of "Wed, 15 Nov 2023 12:39:43 -0500")
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.
next prev parent reply other threads:[~2023-11-16 4:59 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-14 16:38 [PATCH] send-email: avoid duplicate specification warnings Todd Zullinger
2023-11-14 17:32 ` Junio C Hamano
2023-11-14 20:00 ` Jeff King
2023-11-14 20:59 ` Todd Zullinger
2023-11-15 0:48 ` Junio C Hamano
2023-11-15 17:39 ` [RFC PATCH v2 0/2] " Todd Zullinger
2023-11-15 17:39 ` [RFC PATCH v2 1/2] " Todd Zullinger
2023-11-16 4:58 ` Junio C Hamano [this message]
2023-11-16 19:30 ` [PATCH v3 0/2] " Todd Zullinger
2023-11-16 19:30 ` [PATCH v3 1/2] perl: bump the required Perl version to 5.8.1 from 5.8.0 Todd Zullinger
2023-11-16 20:16 ` Jeff King
2023-11-16 19:30 ` [PATCH v3 2/2] send-email: avoid duplicate specification warnings Todd Zullinger
2023-11-16 22:32 ` [PATCH v3 0/2] " Junio C Hamano
2023-11-15 17:39 ` [RFC PATCH v2 2/2] send-email: remove stray characters from usage Todd Zullinger
2023-11-16 4:59 ` Junio C Hamano
2023-11-16 19:36 ` [PATCH] " Todd Zullinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4jhmthtg.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=opohorel@redhat.com \
--cc=peff@peff.net \
--cc=tmz@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).