Re: [9front] [PATCH] upas: allow send to parse e-mail descriptions like marshal

[Romano <me+unobe@fallglow.com>]

On Wed Jun 19 15:55:59 -0700 2024, moody@posixcafe.org wrote:
> On 6/18/24 13:05, Romano wrote:
> > Ping. Here's a cleaned up patch:
> > http://okturing.com/src/20104/body
> > 
> > Similarly with the ramfs patch, any comments/concerns?
> 
> The general idea seems fine I think.
> I don't use upas, but I did find some
> issues with the implementation purposed:
> 
> +char *
> Typical style is to make the return type 'char*' here

Got it. So parameters and declarations look to have 'char *',
but return types remove the space.

> +userfrom(char *cp)
> +{
> +	char *s;
> +	int n;
> +
> +	if((n = strlen(cp)) > 4 && cp[n-1] == '>'){
> +		if((s = strrchr(cp, '<')) != nil && s != cp && isspace(s[-1])) {
> +			s++;
> +			cp[n-1] = '\0';
> +			strcpy(cp, s);
> 
> I don't love this loop, we're essentially walking the string three times here.
> Once for the first strlen, again for strrchr, and a third time with the strcpy.
> Because we return a pointer here we could get rid of this strcpy entirely and just
> make this inner block two lines like so:
> +			cp[n-1] = '\0';
> +			return s + 1;
> 
> However at this point we'd still be walking the string twice. I wanted to see if
> we could just walk the string once without having the code suck and this is what
> I came up with:
> 
> char*
> userfrom(char *cp)
> {
> 	char *p, *e;
> 
> 	if((p = strchr(cp, '<')) == nil)
> 		return cp;
> 	if(p == cp || !isspace(p[-1]))
> 		return cp;
> 	p++;
> 	if((e = strchr(p, '>')) == nil)
> 		return cp;
> 	*e = '\0';
> 	return p;
> }
> 
> Not that the length is going to be super long here, but being a bit more efficient doesn't
> make the code any more complicated (imo).

I like it--easier to follow. Admittedly, I had cargo-culted from the marshal code (you
link to below). I did that poorly.

> Also I believe the commit message is incorrect given it's understanding of what
> printform is doing. I pulled the code out to play with it, and from what I see
> when it is given a "A name <a@example.com>" it will print out the full thing,
> not just the email. The point of this code seems to ensure that the "name" portion
> of this description is passed through the rfc2047fmt function (installed as %U).
> 
> /sys/src/cmd/upas/marshal/marshal.c:820
> Specifically prints both portions there, with the only difference from the default
> case being that it passes the name through %U.
> 
> So I think the description of what is going on in the commit message is not quite
> correct either.

Yeah, my description is flat out wrong. Is this better?

 /sys/src/cmd/upas/marshal/marshal.c:/^printfrom parses $upasname (or
 the login name) as an e-mail with description (e.g., "A Name
 <a.name@example.com>") to set the From: line. This does similarly for
 upas/send, but only parses the e-mail portion so that
 upasname='A name <a.name@example.com>' can be used to both set the
 From: in marshal with a description and to match the correct from in
 upas/send for sending via smtp.

> Thanks,
> moody

Thank you for taking the time to give a better implementation (imo).