Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"

From: Todd Zullinger <tmz@pobox.com>
Date: Oct 26, 2023, 14:46 UTC
Message-ID: <ZTp7xvXDw1GF-NUB@pobox.com>
In-Reply-To: <a71f2f1f-b5f0-4628-a4f3-6fd1319062a3@amd.com>

Michael Strawbridge wrote:

Show 17 quoted lines

> On 10/26/23 08:41, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>>> Note that the bug will only trigger if Email::Valid is installed.
>> 
>> I recall we chased a different bug that depends on the use/non-use
>> of this package a few years ago.  Is the difference significant
>> enough that we may want to install on one but not in another CI
>> environment, like we have a separate CI jobs with exotic settings, I
>> wonder.
> 
> That would make sense to me.  We have had 3 regressions threads
> recently for git send email where Email::Valid was important.
> 
> - [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas - (this thread)
> - [REGRESSION] uninitialized value $address in git send-email - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#m9e0211a8ad387adbbadf31dcfcd7982d4046633d
> - Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address" - https://public-inbox.org/git/68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com/T/#m1411c155e11ad9c5d913d22d1d11180ed56eabc7

Alternately, perhaps having Email::Valid as an optional dependency is worth reconsidering. If it's truly important to validation, make it a requirement. If it's not, then drop it to simplify the code and avoid these sort of issues.

As a (former) distribution packager, having these optional dependencies which change the behavior is always a tough position to be in.

If I make the git package require it to ensure consistent behavior then some folks will -quite rightly- complain that it should not be a requirement. If I keep it an optional dependency, then debugging becomes more difficult for the reasons we've seen in these recent (and not-so-recent) threads.

I'd lean toward dropping the dependency entirely and leave the more basic validation of git-send-email in place. That may not catch every type of address error, but I would argue that what we do without Email::Valid is perfectly reasonable for checking basic email address syntax sanity.

Further validation will happen along the path of mail transfer agents and failures should be reported to the sender in the same way as any other invalid email address.

On a related note, one issue¹ we had reported in Fedora after making Email::Valid a requirement was that it rejected messages where the local part was too long, per the relevant RFC's. But these were generated addresses from GitLab. The addresses worked in practice. While Email::Valid was technically correct in rejecting such addresses, it didn't improve the experience of git send-email users.

¹ https://bugzilla.redhat.com/2046203

-- 
Todd