Re: [PATCH 2/2] imap-send: create target mailbox if it is missing
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:06
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH 2/2] imap-send: create target mailbox if it is missing · Tony Finch <dot@dotat.at>
Tony Finch [off-list ref] writes:
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.
quoted hunk
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.