Re: [PATCH] git-send-email: Add --no-validate-email option

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 4036 bytes --]

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.

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

> 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=2046203

Email::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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]