Re: [PATCH 1/2] Rearrange and consolidate final encoding steps when sending.

Alejandro Colomar via Mutt-dev <[email protected]> Sat, 16 May 2026 00:47:16 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<ageh_0zYM2F8tXcM@devuan>
Hi Kevin,

On 2026-05-16T06:30:11+0800, Kevin J. McCarthy wrote:
> On Fri, May 15, 2026 at 03:08:51PM +0200, Alejandro Colomar via Mutt-dev wrote:
> > On 2026-05-15T20:28:08+0800, Kevin J. McCarthy wrote:
> > >    mutt_encode_descriptions(sctx->msg->content, 1);
> > > +  mutt_prepare_envelope(sctx->msg->env, 1);
> > > +  if (mutt_env_to_intl(sctx->msg->env, &tag, &err))
> > > +  {
> > > +    mutt_error(_("Bad IDN in \"%s\": '%s'"), tag, err);
> > > +    FREE(&err);
> > > +    if (!(sctx->flags & SENDBATCH))
> > > +    {
> > > +      sctx->msg->content = mutt_remove_multipart_mixed(sctx->msg->content);
> > > +      sctx->msg->content = mutt_remove_multipart_alternative(sctx->msg->content);
> > > +      decode_descriptions(sctx->msg->content);
> > > +      mutt_unprepare_envelope(sctx->msg->env);
> > > +      goto main_loop;
> > > +    }
> > > +    else
> > > +      goto cleanup;
> > 
> > I see why you've done it this way: minimizing changes to existing code.
> > However, I suggest rewriting this to invert the logic:
> > 
> > 	if (sctx->flags & SENDBATCH)
> > 	  goto cleanup;
> > 
> > 	sctx->msg->content = mutt_remove_multipart_mixed(sctx->msg->content);
> > 	sctx->msg->content = mutt_remove_multipart_alternative(sctx->msg->content);
> > 	decode_descriptions(sctx->msg->content);
> > 	mutt_unprepare_envelope(sctx->msg->env);
> > 	goto main_loop;
> 
> Hi Alex,
> 
> Okay, I'll do a follow up error handling cleanup patch.

Thanks!

> 
> > > @@ -2523,12 +2524,11 @@ main_loop:
> > >        if ((crypt_get_keys(sctx->msg, &pgpkeylist, 0) == -1) ||
> > >            mutt_protect(sctx, pgpkeylist, 0) == -1)
> > >        {
> > > +        FREE(&pgpkeylist);
> > >          sctx->msg->content = mutt_remove_multipart_mixed(sctx->msg->content);
> > >          sctx->msg->content = mutt_remove_multipart_alternative(sctx->msg->content);
> > > -
> > > -        FREE(&pgpkeylist);
> > > -
> > 
> > The movement of this FREE() seems no obvious from the commit message.
> > Is it intrinsic to this patch?
> 
> Only from the standpoint that I was also doing a little bit of cleanup :D.

:)

> I moved the pgpkeylist free to the beginning to match the order in the error
> handling blocks below and above.

I guess if you comment that in the commit message, it would be fine.
Otherwise, I find it a bit confusing.  Maybe better as a separate patch,
though.

> The important part was the introduction of the call to
> mutt_unprepare_envelope():


Cheers,
Alex

> 
> > >          decode_descriptions(sctx->msg->content);
> > > +        mutt_unprepare_envelope(sctx->msg->env);
> > >          goto main_loop;
> 
> -- 
> Kevin J. McCarthy
> GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA



-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)