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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2023, 17:32 UTC
Message-ID: <xmqqjzqkxmuk.fsf@gitster.g>
In-Reply-To: <20231114163826.207267-1-tmz@pobox.com>

Todd Zullinger <tmz@pobox.com> writes:

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

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.