| Message ID | 20231114163826.207267-1-tmz@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: avoid duplicate specification warnings | expand |
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.
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
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 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.
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,
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(-)