[PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

[PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

[Tony Finch]

Explicitly mention that leaving imap.authMethod unset makes
git imap-send use the basic IMAP plaintext LOGIN command.
---
 Documentation/git-imap-send.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 875d283..770cbe8 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -76,7 +76,8 @@ imap.preformattedHTML::

 imap.authMethod::
 	Specify authenticate method for authentication with IMAP server.
-	Current supported method is 'CRAM-MD5' only.
+	Current supported method is 'CRAM-MD5' only. If this is not set
+	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

 Examples
 ~~~~~~~~
-- 
2.0.3.dirty

Re: [PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

[Junio C Hamano]

Tony Finch <dot@dotat.at> writes:

> Explicitly mention that leaving imap.authMethod unset makes
> git imap-send use the basic IMAP plaintext LOGIN command.
> ---
>  Documentation/git-imap-send.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 875d283..770cbe8 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -76,7 +76,8 @@ imap.preformattedHTML::
>
>  imap.authMethod::
>  	Specify authenticate method for authentication with IMAP server.
> -	Current supported method is 'CRAM-MD5' only.
> +	Current supported method is 'CRAM-MD5' only. If this is not set
> +	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
>
>  Examples
>  ~~~~~~~~

Both patches make sense to me, but can you please sign-off your
patches?  See Documentation/SubmittingPatches for details.

Thanks.

Re: [PATCH] imap-send: clarify CRAM-MD5 vs LOGIN documentation

[Tony Finch]

Junio C Hamano <gitster@pobox.com> wrote:
>
> Both patches make sense to me, but can you please sign-off your
> patches?

Oops, sorry about that. Re-roll on its way...

Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
Thames, Dover: Southwest 4 or 5, increasing 6 at times. Slight or moderate.
Fair. Good.

[PATCH 1/2] imap-send: clarify CRAM-MD5 vs LOGIN documentation

[Tony Finch]

Explicitly mention that leaving imap.authMethod unset makes
git imap-send use the basic IMAP plaintext LOGIN command.

Signed-off-by: Tony Finch <dot@dotat.at>
---
 Documentation/git-imap-send.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 875d283..770cbe8 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -76,7 +76,8 @@ imap.preformattedHTML::

 imap.authMethod::
 	Specify authenticate method for authentication with IMAP server.
-	Current supported method is 'CRAM-MD5' only.
+	Current supported method is 'CRAM-MD5' only. If this is not set
+	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

 Examples
 ~~~~~~~~
-- 
2.1.0.rc0.229.gaee38de

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

[Tony Finch]

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>
---
 imap-send.c | 56 +++++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 524fbab..5e4a24e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -128,7 +128,6 @@ struct imap_cmd_cb {
 	char *data;
 	int dlen;
 	int uid;
-	unsigned create:1, trycreate:1;
 };

 struct imap_cmd {
@@ -714,8 +713,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 {
 	struct imap *imap = ctx->imap;
-	struct imap_cmd *cmdp, **pcmdp, *ncmdp;
-	char *cmd, *arg, *arg1, *p;
+	struct imap_cmd *cmdp, **pcmdp;
+	char *cmd, *arg, *arg1;
 	int n, resp, resp2, tag;

 	for (;;) {
@@ -801,30 +800,9 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			if (!strcmp("OK", arg))
 				resp = DRV_OK;
 			else {
-				if (!strcmp("NO", arg)) {
-					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
-						p = strchr(cmdp->cmd, '"');
-						if (!issue_imap_cmd(ctx, NULL, "CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						/* not waiting here violates the spec, but a server that does not
-						   grok this nonetheless violates it too. */
-						cmdp->cb.create = 0;
-						if (!(ncmdp = issue_imap_cmd(ctx, &cmdp->cb, "%s", cmdp->cmd))) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						free(cmdp->cmd);
-						free(cmdp);
-						if (!tcmd)
-							return 0;	/* ignored */
-						if (cmdp == tcmd)
-							tcmd = ncmdp;
-						continue;
-					}
+				if (!strcmp("NO", arg))
 					resp = RESP_NO;
-				} else /*if (!strcmp("BAD", arg))*/
+				else /*if (!strcmp("BAD", arg))*/
 					resp = RESP_BAD;
 				fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
 					 memcmp(cmdp->cmd, "LOGIN", 5) ?
@@ -833,7 +811,6 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			}
 			if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
 				resp = resp2;
-		normal:
 			if (cmdp->cb.done)
 				cmdp->cb.done(ctx, cmdp, resp);
 			free(cmdp->cb.data);
@@ -944,7 +921,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }

-static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
+static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;
 	struct imap_store *ctx;
@@ -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;
+	}
+
 	ctx->prefix = "";
 	return ctx;

@@ -1219,7 +1215,6 @@ static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)

 	box = ctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
-	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
@@ -1418,14 +1413,13 @@ int main(int argc, char **argv)
 	}

 	/* write it to the imap server */
-	ctx = imap_open_store(&server);
+	ctx = imap_open_store(&server, imap_folder);
 	if (!ctx) {
 		fprintf(stderr, "failed to open store\n");
 		return 1;
 	}

 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
-	ctx->name = imap_folder;
 	while (1) {
 		unsigned percent = n * 100 / total;

-- 
2.1.0.rc0.229.gaee38de

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

[Junio C Hamano]

Tony Finch <dot@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 <dot@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.

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

[Tony Finch]

Junio C Hamano <gitster@pobox.com> wrote:
>
> The basic idea looks good, but I have doubts on one point.

Thanks for spotting the mistake in the error handling. I'll send an update
with a fix.

Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
South Utsire: Southwesterly 4 or 5, occasionally 6 at first in south, backing
southeasterly later. Moderate. Thundery showers. Good.

[PATCH v3 1/2] imap-send: clarify CRAM-MD5 vs LOGIN documentation

[Tony Finch]

Explicitly mention that leaving imap.authMethod unset makes
git imap-send use the basic IMAP plaintext LOGIN command.

Signed-off-by: Tony Finch <dot@dotat.at>
---
 Documentation/git-imap-send.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 875d283..770cbe8 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -76,7 +76,8 @@ imap.preformattedHTML::

 imap.authMethod::
 	Specify authenticate method for authentication with IMAP server.
-	Current supported method is 'CRAM-MD5' only.
+	Current supported method is 'CRAM-MD5' only. If this is not set
+	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

 Examples
 ~~~~~~~~
-- 
2.1.0.rc0.60.gb5b18e5

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

[Tony Finch]

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>
---
 imap-send.c | 57 ++++++++++++++++++++++++++-------------------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 524fbab..a29d03e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -128,7 +128,6 @@ struct imap_cmd_cb {
 	char *data;
 	int dlen;
 	int uid;
-	unsigned create:1, trycreate:1;
 };

 struct imap_cmd {
@@ -714,8 +713,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 {
 	struct imap *imap = ctx->imap;
-	struct imap_cmd *cmdp, **pcmdp, *ncmdp;
-	char *cmd, *arg, *arg1, *p;
+	struct imap_cmd *cmdp, **pcmdp;
+	char *cmd, *arg, *arg1;
 	int n, resp, resp2, tag;

 	for (;;) {
@@ -801,30 +800,9 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			if (!strcmp("OK", arg))
 				resp = DRV_OK;
 			else {
-				if (!strcmp("NO", arg)) {
-					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
-						p = strchr(cmdp->cmd, '"');
-						if (!issue_imap_cmd(ctx, NULL, "CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						/* not waiting here violates the spec, but a server that does not
-						   grok this nonetheless violates it too. */
-						cmdp->cb.create = 0;
-						if (!(ncmdp = issue_imap_cmd(ctx, &cmdp->cb, "%s", cmdp->cmd))) {
-							resp = RESP_BAD;
-							goto normal;
-						}
-						free(cmdp->cmd);
-						free(cmdp);
-						if (!tcmd)
-							return 0;	/* ignored */
-						if (cmdp == tcmd)
-							tcmd = ncmdp;
-						continue;
-					}
+				if (!strcmp("NO", arg))
 					resp = RESP_NO;
-				} else /*if (!strcmp("BAD", arg))*/
+				else /*if (!strcmp("BAD", arg))*/
 					resp = RESP_BAD;
 				fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
 					 memcmp(cmdp->cmd, "LOGIN", 5) ?
@@ -833,7 +811,6 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			}
 			if ((resp2 = parse_response_code(ctx, &cmdp->cb, cmd)) > resp)
 				resp = resp2;
-		normal:
 			if (cmdp->cb.done)
 				cmdp->cb.done(ctx, cmdp, resp);
 			free(cmdp->cb.data);
@@ -944,7 +921,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }

-static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
+static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;
 	struct imap_store *ctx;
@@ -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 out;
+	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 out;
+		}
+		break;
+	}
+
 	ctx->prefix = "";
 	return ctx;

@@ -1164,6 +1160,7 @@ bail:
 		credential_reject(&cred);
 	credential_clear(&cred);

+ out:
 	imap_close_store(ctx);
 	return NULL;
 }
@@ -1219,7 +1216,6 @@ static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)

 	box = ctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
-	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
@@ -1418,14 +1414,13 @@ int main(int argc, char **argv)
 	}

 	/* write it to the imap server */
-	ctx = imap_open_store(&server);
+	ctx = imap_open_store(&server, imap_folder);
 	if (!ctx) {
 		fprintf(stderr, "failed to open store\n");
 		return 1;
 	}

 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
-	ctx->name = imap_folder;
 	while (1) {
 		unsigned percent = n * 100 / total;

-- 
2.1.0.rc0.60.gb5b18e5

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

[Junio C Hamano]

Tony Finch <dot@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.

Thanks for an update; queued.

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

[Ramsay Jones]

On 01/08/14 23:30, Junio C Hamano wrote:
> Tony Finch <dot@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.
> 
> Thanks for an update; queued.

BTW, this provokes a warning from gcc (v4.8.2) for me:

        CC imap-send.o
    imap-send.c:561:25: warning: ‘issue_imap_cmd’ defined but not used [-Wunused-function]
     static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,
                             ^
It seems the change to get_cmd_result(), in commit 7ebac713, removes
the only remaining (two) calls to issue_imap_cmd().

HTH

ATB,
Ramsay Jones

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

[Junio C Hamano]

Yup. I think I also queued a fix-up on top.
Thanks for reporting.

On Sat, Aug 2, 2014 at 6:44 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:
> On 01/08/14 23:30, Junio C Hamano wrote:
>> Tony Finch <dot@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.
>>
>> Thanks for an update; queued.
>
> BTW, this provokes a warning from gcc (v4.8.2) for me:
>
>         CC imap-send.o
>     imap-send.c:561:25: warning: ‘issue_imap_cmd’ defined but not used [-Wunused-function]
>      static struct imap_cmd *issue_imap_cmd(struct imap_store *ctx,
>                              ^
> It seems the change to get_cmd_result(), in commit 7ebac713, removes
> the only remaining (two) calls to issue_imap_cmd().
>
> HTH
>
> ATB,
> Ramsay Jones
>
>
>