Re: [PATCH 2/2] imap-send: create target mailbox if it is missing

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2014, 18:57 UTC
Message-ID: <xmqqy4v9we3k.fsf@gitster.dls.corp.google.com>
In-Reply-To: <alpine.LSU.2.00.1407310914320.13901@hermes-1.csi.cam.ac.uk>

Tony Finch <dot@dotat.at> writes:

Show 14 quoted lines

> Some MUAs delete their "drafts" folder when it is empty, so
> git imap-send should be able to create it if necessary.
>
> This change checks that the folder exists immediately after
> login and tries to create it if it is missing.
>
> There was some vestigial code to handle a [TRYCREATE] response
> from the server when an APPEND target is missing. However this
> code never ran (the create and trycreate flags were never set)
> and when I tried to make it run I found that the code had already
> thrown away the contents of the message it was trying to append.
>
> Signed-off-by: Tony Finch <dot@dotat.at>
> ---

The basic idea looks good, but I have doubts on one point.

Show 26 quoted lines

> diff --git a/imap-send.c b/imap-send.c
> index 524fbab..5e4a24e 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1156,6 +1133,25 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
>  		credential_approve(&cred);
>  	credential_clear(&cred);
>
> +	/* check the target mailbox exists */
> +	ctx->name = folder;
> +	switch (imap_exec(ctx, NULL, "EXAMINE \"%s\"", ctx->name)) {
> +	case RESP_OK:
> +		/* ok */
> +		break;
> +	case RESP_BAD:
> +		fprintf(stderr, "IMAP error: could not check mailbox\n");
> +		goto bail;
> +	case RESP_NO:
> +		if (imap_exec(ctx, NULL, "CREATE \"%s\"", ctx->name) == RESP_OK) {
> +			imap_info("Created missing mailbox\n");
> +		} else {
> +			fprintf(stderr, "IMAP error: could not create missing mailbox\n");
> +			goto bail;
> +		}
> +		break;
> +	}

At any and all the existing places that "goto bail" in the function, we know we failed to authenticate. I think they are all sensible places to call credential_reject().

On the other hand, at this point before you try to "check the target mailbox exists", we have authenticated sucessfully, we know the credential used was good, and called credential_approve() to mark it as such. I do agree that you would want to signal an error to the caller upon these two failures, but I do not think you want to "goto bail" and reject the credential. The error you observed in the new codepath is caused by something else, not authentication failure, and in such a case you do not want to cause the credential helper to evict the user/pass pair from the keyring, no?

Thanks.