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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 21, 2022, 22:12 UTC
Message-ID: <220622.864k0dmzl9.gmgdl@evledraar.gmail.com>
In-Reply-To: <YrEMq+slLOHqw/hz@camp.crustytoothpaste.net>

On Tue, Jun 21 2022, brian m. carlson wrote:

Show 81 quoted lines

> [[PGP Signed Part:Undecided]]
> 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."

That's fair, but that rationale is quite disconnected from how the code works now. You happen to get that check if you have Email::Valid installed, otherwise not.

So if it's a use-case we care about we should make it a hard dependency.

Show 6 quoted lines

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

Would this be addressed by instead opening a connection to the server, and seeing if it is willing to accept these addresess on a "RCPT TO" line?

Which I think is what would happen anyway as you try to send the E-Mail, I'm not sure what distinction you're drawing here (but I haven't looked deeply into the control flow here).

I.e. if your MTA isn't going to accept an address that we regex match, isn't it going to error when you try to send the mail, why isn't it better and more reliable to offload more of that sort of validation to the MTA?

As this report notes that can lead to cases where there's a mismatch between the two, i.e. we can't format the E-Mail, but the MTA would be happy to send it for us.