From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 26 Oct 2026 09:42:13 GMT
Subject: Re: [PATCH v3 1/4] imap-send: wait for the tunnel command to finish
Message-ID: <at8gbEmbW8b5cDjo@pks.im>
In-Reply-To: <0f446c3c12aa5963259fed2617da51c31b992321.1792652185.git.gitgitgadget@gmail.com>

On Thu, Oct 22, 2026 at 06:56:22AM +0000, Harald Nordgren via GitGitGadget wrote:
> diff --git a/imap-send.c b/imap-send.c
> index bf1d2cf74d..fa326c4ae2 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -118,6 +118,7 @@ struct imap {
>  	/* command queue */
>  	int nexttag, num_in_progress, literal_pending;
>  	struct imap_cmd *in_progress, **in_progress_append;
> +	struct child_process tunnel;
>  	struct imap_buffer buf; /* this is BIG, so put it last */
>  };
>  
> @@ -858,6 +859,8 @@ static void imap_close_server(struct imap_store *ictx)
>  		imap_exec(ictx, NULL, "LOGOUT");
>  		socket_shutdown(&imap->buf.sock);
>  	}
> +	if (ictx->cfg->tunnel)
> +		finish_command(&imap->tunnel);

Hmm, okay. Do we _know_ that `ictx->cfg->tunnel` is always properly
initialized in case we call `imap_close_server()`? Or are there error
paths where it might not be initialized yet? If the latter I think we
might see weird behaviour here as the function does not seem to be safe
when called on an uninitialized command.

Maybe we should make `imap->tunnel` an allocated pointer instead so that
we know to only finish it when allocated and initialized?

> @@ -1163,19 +1166,20 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>  	/* open connection to IMAP server */
>  
>  	if (srvc->tunnel) {
> -		struct child_process tunnel = CHILD_PROCESS_INIT;
> +		struct child_process *tunnel = &imap->tunnel;

Nit: I would have avoided this local variable, it doesn't help us anyway
to shrink the diff.

Patrick