Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Oct 23, 2023, 19:50 UTC
Message-ID: <ZTbOnsxBFERPLN3F@ugly>
In-Reply-To: <20231023184010.GA1537181@coredump.intra.peff.net>

On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:

Show 7 quoted lines

>On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
>> that seems like a rather significant point, doesn't it?
>
>Maybe. It depends on whether anybody is interested in adding
>continuation support. Nobody has in the previous 18 years, and nobody
>has asked for it.
>

dunno, it seems like a bug to me. so if i cared at all about this functionality, i'd fix it just because. so at least it doesn't seem nice to make it harder for a potential volunteer.

Show 14 quoted lines

>> > So another option is to just fix the individual bugs separately.
>> > 
>> ... so that seems preferable to me, given that the necessary fixes 
>> seem
>> rather trivial.
>
>They're not too bad. Probably:
>
>  1. lc() the keys we put into the hash
>
>  2. match to/cc/bcc and dereference their arrays
>
>  3. maybe handle 'body' separately from headers to avoid confusion
>

with the header keys lowercased, one could simply use BODY as the key and be done with it.

>But there may be other similar bugs lurking.

>One I didn't mention: the
>hash-based version randomly reorders headers!
>

hmm, yeah, that would mean using Tie::IxHash if one wanted to do it elegantly, at the cost of depending on another non-core module.

also, it means that another hash with non-lowercased versions of the keys would have to be kept.

ok, that's stupid. it would be easier to just keep an additional array of the original keys for iteration, and check the hash before emitting them.

Show 9 quoted lines

>> > I guess "readable" is up for debate here, but I find the inline handling
>> > a lot easier to follow
>> > 
>> any particular reason for that?
>
>For the reasons I gave in the commit message: namely that the matching
>and logic is in one place and doesn't need to be duplicated (e.g., the
>special handling of to/cc/bcc, which caused a bug here).
>

from what i can see, there isn't really anything to "match", apart from agreeing on the data structure (which the code partially failed to do, but that's trivial enough). and layering/abstracting things is usually considered a good thing, unless the cost/benefit ratio is completely backwards.

>The "//" one would
>work, but we support perl versions old enough that they don't have it.
>

according to my grepping, that ship has sailed. also, why _would_ you support such ancient perl versions? that makes even less sense to me than supporting ancient c compilers.

regards