Re: [PATCH] git-send-email: Add --no-validate-email option
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Jun 21, 2022, 00:11 UTC
- Message-ID
- <YrEMq+slLOHqw/hz@camp.crustytoothpaste.net>
- In-Reply-To
- <20220620004427.3586240-1-trawets@amazon.com>
On 2022-06-20 at 00:44:27, Stewart Smith wrote:
> The perl Email::Valid module gets things right, but this may not always > be what you want, as can be seen in > https://bugzilla.redhat.com/show_bug.cgi?id=2046203
You should explain this in the body of the message, since we generally want to know the rationale behind the change even if RedHat moves away from Bugzilla in the future.
You could say something like this:
The Perl Email::Valid module correctly checks whether an email address is syntactically valid. However, in some cases, people have email addresses which are not syntactically valid, such as those where the local-part is more than 64 octets, and would like to use those addresses despite that fact.
Show 5 quoted lines
> So, add a --validate-email (default, current behavior) and > the inverse --no-validate-email option to be able to skip the check > while still having the Email::Valid perl module installed. > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2046203
I don't believe we generally include Fixes headers for external bugs.
Show 50 quoted lines
> Suggested-by: Todd Zullinger <tmz@pobox.com>
> Signed-off-by: Stewart Smith <trawets@amazon.com>
> ---
> git-send-email.perl | 9 +++++++++
> t/t9902-completion.sh | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5861e99a6e..c75b08f9ce 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -103,6 +103,7 @@ sub usage {
> --quiet * Output one line of info per email.
> --dry-run * Don't actually send the emails.
> --[no-]validate * Perform patch sanity checks. Default on.
> + --[no-]validate-email * Perform email address sanity checks. Default on.
> --[no-]format-patch * understand any non optional arguments as
> `git format-patch` ones.
> --force * Send even if safety checks would prevent it.
> @@ -281,6 +282,7 @@ sub do_edit {
> my $chain_reply_to = 0;
> my $use_xmailer = 1;
> my $validate = 1;
> +my $validate_email = 1;
> my $target_xfer_encoding = 'auto';
> my $forbid_sendmail_variables = 1;
>
> @@ -293,6 +295,7 @@ sub do_edit {
> "tocover" => \$cover_to,
> "signedoffcc" => \$signed_off_by_cc,
> "validate" => \$validate,
> + "validateemail" => \$validate_email,
> "multiedit" => \$multiedit,
> "annotate" => \$annotate,
> "xmailer" => \$use_xmailer,
> @@ -531,6 +534,8 @@ sub config_regexp {
> "no-thread" => sub {$thread = 0},
> "validate!" => \$validate,
> "no-validate" => sub {$validate = 0},
> + "validate-email!" => \$validate_email,
> + "no-validate-email" => sub {$validate_email = 0},
> "transfer-encoding=s" => \$target_xfer_encoding,
> "format-patch!" => \$format_patch,
> "no-format-patch" => sub {$format_patch = 0},
> @@ -1132,6 +1137,10 @@ sub extract_valid_address {
> # check for a local address:
> return $address if ($address =~ /^($local_part_regexp)$/);
>
> + # Email::Valid isn't always correct, so support a way to bypass
> + # See https://bugzilla.redhat.com/show_bug.cgi?id=2046203Email::Valid is in fact correct. However, the email which you want to use doesn't conform to the RFC and isn't valid. So this should probably say something like, "Allow people to use an email address which is not valid according to the RFCs if the server accepts it."
I think this patch would be fine as it stands with those changes. Unlike Ævar, I don't think we should get rid of Email::Valid, just like I don't think we should get rid of the transfer encoding checks. I support warning people before sending invalid emails, especially since I believe the address in question would not be deliverable through some mail servers (such as mine).
-- brian m. carlson (he/him or they/them) Toronto, Ontario, CA