Tony Finch <d...@dotat.at> 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 <d...@dotat.at>
> ---

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

> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to