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

[Oswald Buddenhagen <oswald.buddenhagen@gmx.de>]

On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:
>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.

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

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