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

From: Jeff King <peff@peff.net>
Date: Nov 14, 2023, 20:00 UTC
Message-ID: <20231114200009.GD2092538@coredump.intra.peff.net>
In-Reply-To: <20231114163826.207267-1-tmz@pobox.com>

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.

Show 5 quoted lines

> 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