Re: [PATCH] git-send-email: Add --no-validate-email option
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 20, 2022, 08:28 UTC
- Message-ID
- <220620.867d5bwx19.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220620004427.3586240-1-trawets@amazon.com>
On Sun, Jun 19 2022, Stewart Smith wrote:
Show 76 quoted lines
> 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
>
> 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
> 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
> + return 1 if not $validate_email;
> +
> $address =~ s/^\s*<(.*)>\s*$/$1/;
> my $have_email_valid = eval { require Email::Valid; 1 };
> if ($have_email_valid) {
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 31526e6b64..6e363c46f3 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2302,6 +2302,7 @@ test_expect_success PERL 'send-email' '
> EOF
> test_completion "git send-email --val" <<-\EOF &&
> --validate Z
> + --validate-email Z
> EOF
> test_completion "git send-email ma" "main "
> 'I don't think this patch is what we want to fix this problem: The git-send-email script should ultimately be trying to pass an address to a MTA. If you look into its history of Email::Valid use you'll see that we initially used it unconditionally, then later conditionally to get rid of the dependency.
I think a better change is to simply get rid of the Email::Valid dependency, but I'm not 100% sure if there aren't edge cases where our parsing there isn't something we rely on in other cases.
But in the meantime a more narrow change that I believe solves the issue
for you is:
diff --git a/git-send-email.perl b/git-send-email.perl
index 5861e99a6eb..1168da43ef2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1135,7 +1135,11 @@ sub extract_valid_address {
$address =~ s/^\s*<(.*)>\s*$/$1/;
my $have_email_valid = eval { require Email::Valid; 1 };
if ($have_email_valid) {
- return scalar Email::Valid->address($address);
+ my $email = Email::Valid->address(
+ -address => $address,
+ -localpart => 0,
+ );
+ return $email if $email;
}
# less robust/correct than the monster regexp in Email::Valid,Of that we just need that "-localpart => 1" part to fix this specific problem, but I think having the "return $email if $email" is also more correct, and would solve this bug even without the "-localpart => 0", i.e. we'll always fall through to trying to parse the address with our regex.
In any case I think this could really use a corresponding update to the t/t9001-send-email.sh script, i.e. to test an address with a long local part.