On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
> On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
> > But one thing that gives me pause is that the neither before or after
> > this patch do we handle continuation lines like:
> >
> > Subject: this is the beginning
> > and this is more subject
> >
> > And it would probably be a lot easier to add when storing the headers in
> > a hash (it's not impossible to do it the other way, but you basically
> > have to delay processing each line with a small state machine).
> >
> 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.
> > 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
But there may be other similar bugs lurking. One I didn't mention: the
hash-based version randomly reorders headers!
> > 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).
> > (and it's half as many lines; most of the diffstat is the new tests).
>
> > - if ($parsed_email{'From'}) {
> > - $sender = delete($parsed_email{'From'});
> > - }
>
> this verbosity could be cut down somewhat using just
>
> $sender = delete($parsed_email{'From'});
>
> and if the value can be pre-set and needs to be preserved,
>
> $sender = delete($parsed_email{'From'}) // $sender;
>
> but this seems kind of counter-productive legibility-wise.
We do need to avoid overwriting the pre-set value. The "//" one would
work, but we support perl versions old enough that they don't have it.
-Peff
