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

Alejandro Colomar via Mutt-dev <[email protected]> Fri, 15 May 2026 15:08:51 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<agcZCkipb8T6hl4E@devuan>
Hi Kevin,

On 2026-05-15T20:28:08+0800, Kevin J. McCarthy wrote:
> Move the final mutt_prepare_envelope() before the last
> mutt_env_to_intl().  This is standard practice elsewhere in the code.
> Additionally, mutt_prepare_envelope(..., 1) sets Mail-Followup-To, so
> it's prudent to ensure the value is IDNA encoded.
> 
> Remove a comment that no longer has any relevance to the current
> state of the code.
> ---
>  send.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/send.c b/send.c
> index d7f71733..34f29019 100644
> --- a/send.c
> +++ b/send.c
> @@ -2447,16 +2447,6 @@ main_loop:
>      }
>    }
>  
> -  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))
> -      goto main_loop;
> -    else
> -      goto cleanup;
> -  }
> -
>    if (!sctx->msg->env->subject && ! (sctx->flags & SENDBATCH) &&
>        (i = query_quadoption(OPT_SUBJECT, _("No subject, abort sending?"))) != MUTT_NO)
>    {
> @@ -2495,13 +2485,24 @@ main_loop:
>    if (sctx->msg->content->next)
>      sctx->msg->content = mutt_make_multipart_mixed(sctx->msg->content);
>  
> -  /*
> -   * Ok, we need to do it this way instead of handling all fcc stuff in
> -   * one place in order to avoid going to main_loop with encoded "env"
> -   * in case of error.  Ugh.
> -   */
> -
> +  /* Do intl encoding.  Set Message-ID, Mail-Followup-To. */
>    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;

The benefits of this are multiple:

-  Reduce negative logic; the condition of the 'if' is simpler, as we
   don't need to think about negating the effects of &.  This makes it
   cognitively simpler.

-  'else' is not necessary after 'goto'.  That was already true in the
   existing code, but to be fair, with the negated logic it was more
   readable with the 'else'.  If we make the logic positive, I think
   it's more readable without the 'else'.  That reduces mental branches.

-  It's easier to find out which code is triggered by SENDBATCH:
   whatever is under the 'if'.  Previously, it was the 'else' path.

(Maybe better as a follow-up cosmetic patch.  Or maybe even better as
 a cosmetic patch preceding this one.)

> +  }
>  
>    /*
>     * Make sure that clear_content and free_clear_content are
> @@ -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?


Have a lovely day!
Alex

>          decode_descriptions(sctx->msg->content);
> +        mutt_unprepare_envelope(sctx->msg->env);
>          goto main_loop;
>        }
>        mutt_encode_descriptions(sctx->msg->content, 0);
> @@ -2553,8 +2553,6 @@ main_loop:
>    if (!option(OPTNOCURSES) && !(sctx->flags & SENDMAILX))
>      mutt_message _("Sending message...");
>  
> -  mutt_prepare_envelope(sctx->msg->env, 1);
> -
>    if (option(OPTFCCBEFORESEND))
>    {
>      if (save_fcc(sctx, clear_content, pgpkeylist, sctx->flags) &&
> -- 
> 2.54.0
> 

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