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

From: Jeff King <peff@peff.net>
Date: Oct 23, 2023, 18:40 UTC
Message-ID: <20231023184010.GA1537181@coredump.intra.peff.net>
In-Reply-To: <ZTJaVzt75r0iHPzR@ugly>

On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:

Show 12 quoted lines

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

Show 15 quoted lines

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