[PATCH v2 00/14] Remove unused code from imap-send.c

[PATCH v2 00/14] Remove unused code from imap-send.c

[Michael Haggerty]

This is a re-roll, incorporating the feedback of Jonathan Nieder
(thanks!).

Differences from v1:

* Added comments to get_cmd_result() at the place where the
  "NAMESPACE" response is skipped over.

* Added some comments to lf_to_crlf(), simplified the code a bit
  further, and expanded the commit message.

* Replaced erroneously-deleted space in "APPEND" command in
  imap_store_msg().

I also moved the patch rewriting lf_to_crlf() to the end of the
series, because it is not just dead-code elimination like the others.

Michael Haggerty (14):
  imap-send.c: remove msg_data::flags, which was always zero
  imap-send.c: remove struct msg_data
  iamp-send.c: remove unused struct imap_store_conf
  imap-send.c: remove struct store_conf
  imap-send.c: remove struct message
  imap-send.c: remove some unused fields from struct store
  imap-send.c: inline imap_parse_list() in imap_list()
  imap-send.c: remove struct imap argument to parse_imap_list_l()
  imap-send.c: remove namespace fields from struct imap
  imap-send.c: remove unused field imap_store::trashnc
  imap-send.c: use struct imap_store instead of struct store
  imap-send.c: remove unused field imap_store::uidvalidity
  imap-send.c: fold struct store into struct imap_store
  imap-send.c: simplify logic in lf_to_crlf()

 imap-send.c | 308 +++++++++++-------------------------------------------------
 1 file changed, 55 insertions(+), 253 deletions(-)

-- 
1.8.0.3

[PATCH v2 01/14] imap-send.c: remove msg_data::flags, which was always zero

[Michael Haggerty]

This removes the need for function imap_make_flags(), so delete it,
too.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 40 +++-------------------------------------
 1 file changed, 3 insertions(+), 37 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index e521e2f..f1c8f5a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -70,7 +70,6 @@ struct store {
 
 struct msg_data {
 	struct strbuf data;
-	unsigned char flags;
 };
 
 static const char imap_send_usage[] = "git imap-send < <mbox>";
@@ -225,14 +224,6 @@ static const char *cap_list[] = {
 static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd);
 
 
-static const char *Flags[] = {
-	"Draft",
-	"Flagged",
-	"Answered",
-	"Seen",
-	"Deleted",
-};
-
 #ifndef NO_OPENSSL
 static void ssl_socket_perror(const char *func)
 {
@@ -1246,23 +1237,6 @@ bail:
 	return NULL;
 }
 
-static int imap_make_flags(int flags, char *buf)
-{
-	const char *s;
-	unsigned i, d;
-
-	for (i = d = 0; i < ARRAY_SIZE(Flags); i++)
-		if (flags & (1 << i)) {
-			buf[d++] = ' ';
-			buf[d++] = '\\';
-			for (s = Flags[i]; *s; s++)
-				buf[d++] = *s;
-		}
-	buf[0] = '(';
-	buf[d++] = ')';
-	return d;
-}
-
 static void lf_to_crlf(struct strbuf *msg)
 {
 	size_t new_len;
@@ -1311,8 +1285,7 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
 	struct imap *imap = ctx->imap;
 	struct imap_cmd_cb cb;
 	const char *prefix, *box;
-	int ret, d;
-	char flagstr[128];
+	int ret;
 
 	lf_to_crlf(&msg->data);
 	memset(&cb, 0, sizeof(cb));
@@ -1320,17 +1293,10 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
 	cb.dlen = msg->data.len;
 	cb.data = strbuf_detach(&msg->data, NULL);
 
-	d = 0;
-	if (msg->flags) {
-		d = imap_make_flags(msg->flags, flagstr);
-		flagstr[d++] = ' ';
-	}
-	flagstr[d] = 0;
-
 	box = gctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
 	cb.create = 0;
-	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" %s", prefix, box, flagstr);
+	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
 		return ret;
@@ -1483,7 +1449,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)
 int main(int argc, char **argv)
 {
 	struct strbuf all_msgs = STRBUF_INIT;
-	struct msg_data msg = {STRBUF_INIT, 0};
+	struct msg_data msg = {STRBUF_INIT};
 	struct store *ctx = NULL;
 	int ofs = 0;
 	int r;
-- 
1.8.0.3

[PATCH v2 02/14] imap-send.c: remove struct msg_data

[Michael Haggerty]

Now that its flags member has been deleted, all that is left is a
strbuf.  So use a strbuf directly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index f1c8f5a..29c10a4 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -68,10 +68,6 @@ struct store {
 	int recent; /* # of recent messages - don't trust this beyond the initial read */
 };
 
-struct msg_data {
-	struct strbuf data;
-};
-
 static const char imap_send_usage[] = "git imap-send < <mbox>";
 
 #undef DRV_OK
@@ -1279,7 +1275,7 @@ static void lf_to_crlf(struct strbuf *msg)
  * Store msg to IMAP.  Also detach and free the data from msg->data,
  * leaving msg->data empty.
  */
-static int imap_store_msg(struct store *gctx, struct msg_data *msg)
+static int imap_store_msg(struct store *gctx, struct strbuf *msg)
 {
 	struct imap_store *ctx = (struct imap_store *)gctx;
 	struct imap *imap = ctx->imap;
@@ -1287,11 +1283,11 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
 	const char *prefix, *box;
 	int ret;
 
-	lf_to_crlf(&msg->data);
+	lf_to_crlf(msg);
 	memset(&cb, 0, sizeof(cb));
 
-	cb.dlen = msg->data.len;
-	cb.data = strbuf_detach(&msg->data, NULL);
+	cb.dlen = msg->len;
+	cb.data = strbuf_detach(msg, NULL);
 
 	box = gctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
@@ -1449,7 +1445,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)
 int main(int argc, char **argv)
 {
 	struct strbuf all_msgs = STRBUF_INIT;
-	struct msg_data msg = {STRBUF_INIT};
+	struct strbuf msg = STRBUF_INIT;
 	struct store *ctx = NULL;
 	int ofs = 0;
 	int r;
@@ -1511,10 +1507,10 @@ int main(int argc, char **argv)
 		unsigned percent = n * 100 / total;
 
 		fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
-		if (!split_msg(&all_msgs, &msg.data, &ofs))
+		if (!split_msg(&all_msgs, &msg, &ofs))
 			break;
 		if (server.use_html)
-			wrap_in_html(&msg.data);
+			wrap_in_html(&msg);
 		r = imap_store_msg(ctx, &msg);
 		if (r != DRV_OK)
 			break;
-- 
1.8.0.3

[PATCH v2 03/14] iamp-send.c: remove unused struct imap_store_conf

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 29c10a4..dbe0546 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -130,11 +130,6 @@ static struct imap_server_conf server = {
 	NULL,	/* auth_method */
 };
 
-struct imap_store_conf {
-	struct store_conf gen;
-	struct imap_server_conf *server;
-};
-
 #define NIL	(void *)0x1
 #define LIST	(void *)0x2
 
-- 
1.8.0.3

[PATCH v2 04/14] imap-send.c: remove struct store_conf

[Michael Haggerty]

It was never used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index dbe0546..b8a7ff9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,15 +33,6 @@ typedef void *SSL;
 #include <openssl/hmac.h>
 #endif
 
-struct store_conf {
-	char *name;
-	const char *path; /* should this be here? its interpretation is driver-specific */
-	char *map_inbox;
-	char *trash;
-	unsigned max_size; /* off_t is overkill */
-	unsigned trash_remote_new:1, trash_only_new:1;
-};
-
 /* For message->status */
 #define M_RECENT       (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
 #define M_DEAD         (1<<1) /* expunged */
@@ -55,8 +46,6 @@ struct message {
 };
 
 struct store {
-	struct store_conf *conf; /* foreign */
-
 	/* currently open mailbox */
 	const char *name; /* foreign! maybe preset? */
 	char *path; /* own */
-- 
1.8.0.3

[PATCH v2 05/14] imap-send.c: remove struct message

[Michael Haggerty]

It was never used.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index b8a7ff9..9e181e0 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,23 +33,10 @@ typedef void *SSL;
 #include <openssl/hmac.h>
 #endif
 
-/* For message->status */
-#define M_RECENT       (1<<0) /* unsyncable flag; maildir_* depend on this being 1<<0 */
-#define M_DEAD         (1<<1) /* expunged */
-#define M_FLAGS        (1<<2) /* flags fetched */
-
-struct message {
-	struct message *next;
-	size_t size; /* zero implies "not fetched" */
-	int uid;
-	unsigned char flags, status;
-};
-
 struct store {
 	/* currently open mailbox */
 	const char *name; /* foreign! maybe preset? */
 	char *path; /* own */
-	struct message *msgs; /* own */
 	int uidvalidity;
 	unsigned char opts; /* maybe preset? */
 	/* note that the following do _not_ reflect stats from msgs, but mailbox totals */
@@ -74,8 +61,6 @@ static void imap_warn(const char *, ...);
 
 static char *next_arg(char **);
 
-static void free_generic_messages(struct message *);
-
 __attribute__((format (printf, 3, 4)))
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
 
@@ -447,16 +432,6 @@ static char *next_arg(char **s)
 	return ret;
 }
 
-static void free_generic_messages(struct message *msgs)
-{
-	struct message *tmsg;
-
-	for (; msgs; msgs = tmsg) {
-		tmsg = msgs->next;
-		free(msgs);
-	}
-}
-
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 {
 	int ret;
@@ -914,7 +889,6 @@ static void imap_close_server(struct imap_store *ictx)
 static void imap_close_store(struct store *ctx)
 {
 	imap_close_server((struct imap_store *)ctx);
-	free_generic_messages(ctx->msgs);
 	free(ctx);
 }
 
-- 
1.8.0.3

[PATCH v2 06/14] imap-send.c: remove some unused fields from struct store

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 9e181e0..f193211 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -36,12 +36,7 @@ typedef void *SSL;
 struct store {
 	/* currently open mailbox */
 	const char *name; /* foreign! maybe preset? */
-	char *path; /* own */
 	int uidvalidity;
-	unsigned char opts; /* maybe preset? */
-	/* note that the following do _not_ reflect stats from msgs, but mailbox totals */
-	int count; /* # of messages */
-	int recent; /* # of recent messages - don't trust this beyond the initial read */
 };
 
 static const char imap_send_usage[] = "git imap-send < <mbox>";
@@ -772,13 +767,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 				   !strcmp("NO", arg) || !strcmp("BYE", arg)) {
 				if ((resp = parse_response_code(ctx, NULL, cmd)) != RESP_OK)
 					return resp;
-			} else if (!strcmp("CAPABILITY", arg))
+			} else if (!strcmp("CAPABILITY", arg)) {
 				parse_capability(imap, cmd);
-			else if ((arg1 = next_arg(&cmd))) {
-				if (!strcmp("EXISTS", arg1))
-					ctx->gen.count = atoi(arg);
-				else if (!strcmp("RECENT", arg1))
-					ctx->gen.recent = atoi(arg);
+			} else if ((arg1 = next_arg(&cmd))) {
+				/* unused */
 			} else {
 				fprintf(stderr, "IMAP error: unable to parse untagged response\n");
 				return RESP_BAD;
@@ -1254,7 +1246,6 @@ static int imap_store_msg(struct store *gctx, struct strbuf *msg)
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
 		return ret;
-	gctx->count++;
 
 	return DRV_OK;
 }
-- 
1.8.0.3

Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store

[Jonathan Nieder]

Michael Haggerty wrote:

> -			else if ((arg1 = next_arg(&cmd))) {
> -				if (!strcmp("EXISTS", arg1))
> -					ctx->gen.count = atoi(arg);
> -				else if (!strcmp("RECENT", arg1))
> -					ctx->gen.recent = atoi(arg);
> +			} else if ((arg1 = next_arg(&cmd))) {
> +				/* unused */

The above is just the right thing to do to ensure no behavior change.
Let's take a look at the resulting code, though:

			if (... various reasonable things ...) {
				...
			} else if ((arg1 = next_arg(&cmd))) {
				/* unused */
			} else {
				fprintf(stderr, "IMAP error: unable to parse untagged response\n");
				return RESP_BAD;

Anyone forced by some bug to examine this "/* unused */" case is going
to have no clue what's going on.  In that respect, the old code was
much better, since it at least made it clear that one case where this
code gets hit is handling "<num> EXISTS" and "<num> RECENT" untagged
responses.

I suspect that original code did not have an implicit and intended
missing

				else
					; /* negligible response; ignore it */

but the intent was rather 

				else {
					fprintf(stderr, "IMAP error: I can't parse this\n");
					return RESP_BAD;
				}

Since actually fixing that is probably too aggressive for this patch,
how about a FIXME comment like the following?

		/*
		 * Unhandled response-data with at least two words.
		 * Ignore it.
		 *
		 * NEEDSWORK: Previously this case handled '<num> EXISTS'
		 * and '<num> RECENT' but as a probably-unintended side
		 * effect it ignores other unrecognized two-word
		 * responses.  imap-send doesn't ever try to read
		 * messages or mailboxes these days, so consider
		 * eliminating this case.
		 */

Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Michael Haggerty wrote:
>
>> -			else if ((arg1 = next_arg(&cmd))) {
>> -				if (!strcmp("EXISTS", arg1))
>> -					ctx->gen.count = atoi(arg);
>> -				else if (!strcmp("RECENT", arg1))
>> -					ctx->gen.recent = atoi(arg);
>> +			} else if ((arg1 = next_arg(&cmd))) {
>> +				/* unused */
>
> The above is just the right thing to do to ensure no behavior change.
> Let's take a look at the resulting code, though:
>
> 			if (... various reasonable things ...) {
> 				...
> 			} else if ((arg1 = next_arg(&cmd))) {
> 				/* unused */
> 			} else {
> 				fprintf(stderr, "IMAP error: unable to parse untagged response\n");
> 				return RESP_BAD;
>
> Anyone forced by some bug to examine this "/* unused */" case is going
> to have no clue what's going on.  In that respect, the old code was
> much better, since it at least made it clear that one case where this
> code gets hit is handling "<num> EXISTS" and "<num> RECENT" untagged
> responses.
>
> I suspect that original code did not have an implicit and intended
> missing
>
> 				else
> 					; /* negligible response; ignore it */
>
> but the intent was rather 
>
> 				else {
> 					fprintf(stderr, "IMAP error: I can't parse this\n");
> 					return RESP_BAD;
> 				}
>
> Since actually fixing that is probably too aggressive for this patch,
> how about a FIXME comment like the following?
>
> 		/*
> 		 * Unhandled response-data with at least two words.
> 		 * Ignore it.
> 		 *
> 		 * NEEDSWORK: Previously this case handled '<num> EXISTS'
> 		 * and '<num> RECENT' but as a probably-unintended side
> 		 * effect it ignores other unrecognized two-word
> 		 * responses.  imap-send doesn't ever try to read
> 		 * messages or mailboxes these days, so consider
> 		 * eliminating this case.
> 		 */

Hmph; it seems that it is not worth rerolling the whole thing only
for this, so let me squash this in, replacing the /* unused */ with
the large comment, and then merge the result to 'next'.

Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Since actually fixing that is probably too aggressive for this patch,
>> how about a FIXME comment like the following?
[...]
> Hmph; it seems that it is not worth rerolling the whole thing only
> for this, so let me squash this in, replacing the /* unused */ with
> the large comment, and then merge the result to 'next'.

Sounds good to me.  Next time, I'll include a 'fixup!' patch instead of
making you do the copy/pasting.

Thanks, both.
Jonathan

Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store

[Michael Haggerty]

On 01/15/2013 09:32 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> -			else if ((arg1 = next_arg(&cmd))) {
>> -				if (!strcmp("EXISTS", arg1))
>> -					ctx->gen.count = atoi(arg);
>> -				else if (!strcmp("RECENT", arg1))
>> -					ctx->gen.recent = atoi(arg);
>> +			} else if ((arg1 = next_arg(&cmd))) {
>> +				/* unused */
> 
> The above is just the right thing to do to ensure no behavior change.
> Let's take a look at the resulting code, though:
> 
> 			if (... various reasonable things ...) {
> 				...
> 			} else if ((arg1 = next_arg(&cmd))) {
> 				/* unused */
> 			} else {
> 				fprintf(stderr, "IMAP error: unable to parse untagged response\n");
> 				return RESP_BAD;
> 
> Anyone forced by some bug to examine this "/* unused */" case is going
> to have no clue what's going on.  In that respect, the old code was
> much better, since it at least made it clear that one case where this
> code gets hit is handling "<num> EXISTS" and "<num> RECENT" untagged
> responses.
> 
> I suspect that original code did not have an implicit and intended
> missing
> 
> 				else
> 					; /* negligible response; ignore it */
> 
> but the intent was rather 
> 
> 				else {
> 					fprintf(stderr, "IMAP error: I can't parse this\n");
> 					return RESP_BAD;
> 				}
> 
> Since actually fixing that is probably too aggressive for this patch,
> how about a FIXME comment like the following?
> 
> 		/*
> 		 * Unhandled response-data with at least two words.
> 		 * Ignore it.
> 		 *
> 		 * NEEDSWORK: Previously this case handled '<num> EXISTS'
> 		 * and '<num> RECENT' but as a probably-unintended side
> 		 * effect it ignores other unrecognized two-word
> 		 * responses.  imap-send doesn't ever try to read
> 		 * messages or mailboxes these days, so consider
> 		 * eliminating this case.
> 		 */

Yes, this sounds reasonable to me.  Thanks for the improvement.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()

[Michael Haggerty]

The function is only called from here.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index f193211..cbbf845 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -669,21 +669,16 @@ bail:
 	return -1;
 }
 
-static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
+static struct imap_list *parse_list(char **sp)
 {
 	struct imap_list *head;
 
-	if (!parse_imap_list_l(imap, sp, &head, 0))
+	if (!parse_imap_list_l(NULL, sp, &head, 0))
 		return head;
 	free_list(head);
 	return NULL;
 }
 
-static struct imap_list *parse_list(char **sp)
-{
-	return parse_imap_list(NULL, sp);
-}
-
 static void parse_capability(struct imap *imap, char *cmd)
 {
 	char *arg;
-- 
1.8.0.3

Re: [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()

[Matt Kraai]

On Tue, Jan 15, 2013 at 09:06:25AM +0100, Michael Haggerty wrote:
> -static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
> +static struct imap_list *parse_list(char **sp)

The commit subject refers to imap_parse_list and imap_list whereas the
code refers to parse_imap_list and parse_list.

Re: [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()

[Michael Haggerty]

On 01/15/2013 07:51 PM, Matt Kraai wrote:
> On Tue, Jan 15, 2013 at 09:06:25AM +0100, Michael Haggerty wrote:
>> -static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
>> +static struct imap_list *parse_list(char **sp)
> 
> The commit subject refers to imap_parse_list and imap_list whereas the
> code refers to parse_imap_list and parse_list.

Yes, you're right.  Thanks.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 01/15/2013 07:51 PM, Matt Kraai wrote:
>> On Tue, Jan 15, 2013 at 09:06:25AM +0100, Michael Haggerty wrote:
>>> -static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
>>> +static struct imap_list *parse_list(char **sp)
>> 
>> The commit subject refers to imap_parse_list and imap_list whereas the
>> code refers to parse_imap_list and parse_list.
>
> Yes, you're right.  Thanks.

I think I've fixed this (and some other minor points in other
patches in the series) while queuing; please check master..3691031c
after fetching from me.

Thanks.

Re: [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()

[Michael Haggerty]

On 01/16/2013 04:34 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> On 01/15/2013 07:51 PM, Matt Kraai wrote:
>>> On Tue, Jan 15, 2013 at 09:06:25AM +0100, Michael Haggerty wrote:
>>>> -static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
>>>> +static struct imap_list *parse_list(char **sp)
>>>
>>> The commit subject refers to imap_parse_list and imap_list whereas the
>>> code refers to parse_imap_list and parse_list.
>>
>> Yes, you're right.  Thanks.
> 
> I think I've fixed this (and some other minor points in other
> patches in the series) while queuing; please check master..3691031c
> after fetching from me.

Looks good.  Thanks.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v2 08/14] imap-send.c: remove struct imap argument to parse_imap_list_l()

[Michael Haggerty]

It was always set to NULL.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 39 +++------------------------------------
 1 file changed, 3 insertions(+), 36 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index cbbf845..29e4037 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -578,11 +578,10 @@ static void free_list(struct imap_list *list)
 	}
 }
 
-static int parse_imap_list_l(struct imap *imap, char **sp, struct imap_list **curp, int level)
+static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
 {
 	struct imap_list *cur;
 	char *s = *sp, *p;
-	int n, bytes;
 
 	for (;;) {
 		while (isspace((unsigned char)*s))
@@ -598,39 +597,7 @@ static int parse_imap_list_l(struct imap *imap, char **sp, struct imap_list **cu
 			/* sublist */
 			s++;
 			cur->val = LIST;
-			if (parse_imap_list_l(imap, &s, &cur->child, level + 1))
-				goto bail;
-		} else if (imap && *s == '{') {
-			/* literal */
-			bytes = cur->len = strtol(s + 1, &s, 10);
-			if (*s != '}')
-				goto bail;
-
-			s = cur->val = xmalloc(cur->len);
-
-			/* dump whats left over in the input buffer */
-			n = imap->buf.bytes - imap->buf.offset;
-
-			if (n > bytes)
-				/* the entire message fit in the buffer */
-				n = bytes;
-
-			memcpy(s, imap->buf.buf + imap->buf.offset, n);
-			s += n;
-			bytes -= n;
-
-			/* mark that we used part of the buffer */
-			imap->buf.offset += n;
-
-			/* now read the rest of the message */
-			while (bytes > 0) {
-				if ((n = socket_read(&imap->buf.sock, s, bytes)) <= 0)
-					goto bail;
-				s += n;
-				bytes -= n;
-			}
-
-			if (buffer_gets(&imap->buf, &s))
+			if (parse_imap_list_l(&s, &cur->child, level + 1))
 				goto bail;
 		} else if (*s == '"') {
 			/* quoted string */
@@ -673,7 +640,7 @@ static struct imap_list *parse_list(char **sp)
 {
 	struct imap_list *head;
 
-	if (!parse_imap_list_l(NULL, sp, &head, 0))
+	if (!parse_imap_list_l(sp, &head, 0))
 		return head;
 	free_list(head);
 	return NULL;
-- 
1.8.0.3

[PATCH v2 09/14] imap-send.c: remove namespace fields from struct imap

[Michael Haggerty]

They are unused, and their removal means that a bunch of list-related
infrastructure can be disposed of.

It might be that the "NAMESPACE" response that is now skipped over in
get_cmd_result() should never be sent by the server.  But somebody
would have to check the IMAP protocol and how we interact with the
server to be sure.  So for now I am leaving that branch of the "if"
statement there.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 75 ++++++++-----------------------------------------------------
 1 file changed, 9 insertions(+), 66 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 29e4037..ff44013 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -99,15 +99,6 @@ static struct imap_server_conf server = {
 	NULL,	/* auth_method */
 };
 
-#define NIL	(void *)0x1
-#define LIST	(void *)0x2
-
-struct imap_list {
-	struct imap_list *next, *child;
-	char *val;
-	int len;
-};
-
 struct imap_socket {
 	int fd[2];
 	SSL *ssl;
@@ -124,7 +115,6 @@ struct imap_cmd;
 
 struct imap {
 	int uidnext; /* from SELECT responses */
-	struct imap_list *ns_personal, *ns_other, *ns_shared; /* NAMESPACE info */
 	unsigned caps, rcaps; /* CAPABILITY results */
 	/* command queue */
 	int nexttag, num_in_progress, literal_pending;
@@ -554,34 +544,9 @@ static int imap_exec_m(struct imap_store *ctx, struct imap_cmd_cb *cb,
 	}
 }
 
-static int is_atom(struct imap_list *list)
-{
-	return list && list->val && list->val != NIL && list->val != LIST;
-}
-
-static int is_list(struct imap_list *list)
-{
-	return list && list->val == LIST;
-}
-
-static void free_list(struct imap_list *list)
-{
-	struct imap_list *tmp;
-
-	for (; list; list = tmp) {
-		tmp = list->next;
-		if (is_list(list))
-			free_list(list->child);
-		else if (is_atom(list))
-			free(list->val);
-		free(list);
-	}
-}
-
-static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
+static int skip_imap_list_l(char **sp, int level)
 {
-	struct imap_list *cur;
-	char *s = *sp, *p;
+	char *s = *sp;
 
 	for (;;) {
 		while (isspace((unsigned char)*s))
@@ -590,36 +555,23 @@ static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
 			s++;
 			break;
 		}
-		*curp = cur = xmalloc(sizeof(*cur));
-		curp = &cur->next;
-		cur->val = NULL; /* for clean bail */
 		if (*s == '(') {
 			/* sublist */
 			s++;
-			cur->val = LIST;
-			if (parse_imap_list_l(&s, &cur->child, level + 1))
+			if (skip_imap_list_l(&s, level + 1))
 				goto bail;
 		} else if (*s == '"') {
 			/* quoted string */
 			s++;
-			p = s;
 			for (; *s != '"'; s++)
 				if (!*s)
 					goto bail;
-			cur->len = s - p;
 			s++;
-			cur->val = xmemdupz(p, cur->len);
 		} else {
 			/* atom */
-			p = s;
 			for (; *s && !isspace((unsigned char)*s); s++)
 				if (level && *s == ')')
 					break;
-			cur->len = s - p;
-			if (cur->len == 3 && !memcmp("NIL", p, 3))
-				cur->val = NIL;
-			else
-				cur->val = xmemdupz(p, cur->len);
 		}
 
 		if (!level)
@@ -628,22 +580,15 @@ static int parse_imap_list_l(char **sp, struct imap_list **curp, int level)
 			goto bail;
 	}
 	*sp = s;
-	*curp = NULL;
 	return 0;
 
 bail:
-	*curp = NULL;
 	return -1;
 }
 
-static struct imap_list *parse_list(char **sp)
+static void skip_list(char **sp)
 {
-	struct imap_list *head;
-
-	if (!parse_imap_list_l(sp, &head, 0))
-		return head;
-	free_list(head);
-	return NULL;
+	skip_imap_list_l(sp, 0);
 }
 
 static void parse_capability(struct imap *imap, char *cmd)
@@ -722,9 +667,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 			}
 
 			if (!strcmp("NAMESPACE", arg)) {
-				imap->ns_personal = parse_list(&cmd);
-				imap->ns_other = parse_list(&cmd);
-				imap->ns_shared = parse_list(&cmd);
+				/* rfc2342 NAMESPACE response. */
+				skip_list(&cmd); /* Personal mailboxes */
+				skip_list(&cmd); /* Others' mailboxes */
+				skip_list(&cmd); /* Shared mailboxes */
 			} else if (!strcmp("OK", arg) || !strcmp("BAD", arg) ||
 				   !strcmp("NO", arg) || !strcmp("BYE", arg)) {
 				if ((resp = parse_response_code(ctx, NULL, cmd)) != RESP_OK)
@@ -834,9 +780,6 @@ static void imap_close_server(struct imap_store *ictx)
 		imap_exec(ictx, NULL, "LOGOUT");
 		socket_shutdown(&imap->buf.sock);
 	}
-	free_list(imap->ns_personal);
-	free_list(imap->ns_other);
-	free_list(imap->ns_shared);
 	free(imap);
 }
 
-- 
1.8.0.3

[PATCH v2 10/14] imap-send.c: remove unused field imap_store::trashnc

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ff44013..909e4db 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -127,7 +127,6 @@ struct imap_store {
 	int uidvalidity;
 	struct imap *imap;
 	const char *prefix;
-	unsigned /*currentnc:1,*/ trashnc:1;
 };
 
 struct imap_cmd_cb {
@@ -1080,7 +1079,6 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	} /* !preauth */
 
 	ctx->prefix = "";
-	ctx->trashnc = 1;
 	return (struct store *)ctx;
 
 bail:
-- 
1.8.0.3

[PATCH v2 11/14] imap-send.c: use struct imap_store instead of struct store

[Michael Haggerty]

In fact, all struct store instances are upcasts of struct imap_store
anyway, so stop making the distinction.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 909e4db..48c646c 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -782,9 +782,9 @@ static void imap_close_server(struct imap_store *ictx)
 	free(imap);
 }
 
-static void imap_close_store(struct store *ctx)
+static void imap_close_store(struct imap_store *ctx)
 {
-	imap_close_server((struct imap_store *)ctx);
+	imap_close_server(ctx);
 	free(ctx);
 }
 
@@ -869,7 +869,7 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }
 
-static struct store *imap_open_store(struct imap_server_conf *srvc)
+static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
 {
 	struct imap_store *ctx;
 	struct imap *imap;
@@ -1079,10 +1079,10 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	} /* !preauth */
 
 	ctx->prefix = "";
-	return (struct store *)ctx;
+	return ctx;
 
 bail:
-	imap_close_store(&ctx->gen);
+	imap_close_store(ctx);
 	return NULL;
 }
 
@@ -1128,9 +1128,8 @@ static void lf_to_crlf(struct strbuf *msg)
  * Store msg to IMAP.  Also detach and free the data from msg->data,
  * leaving msg->data empty.
  */
-static int imap_store_msg(struct store *gctx, struct strbuf *msg)
+static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)
 {
-	struct imap_store *ctx = (struct imap_store *)gctx;
 	struct imap *imap = ctx->imap;
 	struct imap_cmd_cb cb;
 	const char *prefix, *box;
@@ -1142,7 +1141,7 @@ static int imap_store_msg(struct store *gctx, struct strbuf *msg)
 	cb.dlen = msg->len;
 	cb.data = strbuf_detach(msg, NULL);
 
-	box = gctx->name;
+	box = ctx->gen.name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
 	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
@@ -1298,7 +1297,7 @@ int main(int argc, char **argv)
 {
 	struct strbuf all_msgs = STRBUF_INIT;
 	struct strbuf msg = STRBUF_INIT;
-	struct store *ctx = NULL;
+	struct imap_store *ctx = NULL;
 	int ofs = 0;
 	int r;
 	int total, n = 0;
@@ -1354,7 +1353,7 @@ int main(int argc, char **argv)
 	}
 
 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
-	ctx->name = imap_folder;
+	ctx->gen.name = imap_folder;
 	while (1) {
 		unsigned percent = n * 100 / total;
 
-- 
1.8.0.3

[PATCH v2 12/14] imap-send.c: remove unused field imap_store::uidvalidity

[Michael Haggerty]

I suspect that the existence of both imap_store::uidvalidity and
store::uidvalidity was an accident.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 48c646c..a0f42bb 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -124,7 +124,6 @@ struct imap {
 
 struct imap_store {
 	struct store gen;
-	int uidvalidity;
 	struct imap *imap;
 	const char *prefix;
 };
-- 
1.8.0.3

[PATCH v2 13/14] imap-send.c: fold struct store into struct imap_store

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index a0f42bb..f2933e9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -33,12 +33,6 @@ typedef void *SSL;
 #include <openssl/hmac.h>
 #endif
 
-struct store {
-	/* currently open mailbox */
-	const char *name; /* foreign! maybe preset? */
-	int uidvalidity;
-};
-
 static const char imap_send_usage[] = "git imap-send < <mbox>";
 
 #undef DRV_OK
@@ -123,7 +117,9 @@ struct imap {
 };
 
 struct imap_store {
-	struct store gen;
+	/* currently open mailbox */
+	const char *name; /* foreign! maybe preset? */
+	int uidvalidity;
 	struct imap *imap;
 	const char *prefix;
 };
@@ -618,7 +614,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 	*p++ = 0;
 	arg = next_arg(&s);
 	if (!strcmp("UIDVALIDITY", arg)) {
-		if (!(arg = next_arg(&s)) || !(ctx->gen.uidvalidity = atoi(arg))) {
+		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
 			fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
 			return RESP_BAD;
 		}
@@ -636,7 +632,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
 		for (; isspace((unsigned char)*p); p++);
 		fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
 	} else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
-		if (!(arg = next_arg(&s)) || !(ctx->gen.uidvalidity = atoi(arg)) ||
+		if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
 		    !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
 			fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
 			return RESP_BAD;
@@ -1140,7 +1136,7 @@ static int imap_store_msg(struct imap_store *ctx, struct strbuf *msg)
 	cb.dlen = msg->len;
 	cb.data = strbuf_detach(msg, NULL);
 
-	box = ctx->gen.name;
+	box = ctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
 	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
@@ -1352,7 +1348,7 @@ int main(int argc, char **argv)
 	}
 
 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
-	ctx->gen.name = imap_folder;
+	ctx->name = imap_folder;
 	while (1) {
 		unsigned percent = n * 100 / total;
 
-- 
1.8.0.3

[PATCH v2 14/14] imap-send.c: simplify logic in lf_to_crlf()

[Michael Haggerty]

* The first character in the string used to be special-cased to get
  around the fact that msg->buf[i - 1] is not defined for i == 0.
  Instead, keep track of the previous character in a separate
  variable, "lastc", initialized in such a way to let the loop handle
  i == 0 correctly.

* Make the two loops over the string look as similar as possible to
  make it more obvious that the count computed in the first pass
  agrees with the true length of the new string written in the second
  pass.  As a side effect, this makes it possible to use the "j"
  counter in place of lfnum and new_len.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 imap-send.c | 52 +++++++++++++++++++++++-----------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index f2933e9..1d40207 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1081,42 +1081,36 @@ bail:
 	return NULL;
 }
 
+/*
+ * Insert CR characters as necessary in *msg to ensure that every LF
+ * character in *msg is preceded by a CR.
+ */
 static void lf_to_crlf(struct strbuf *msg)
 {
-	size_t new_len;
 	char *new;
-	int i, j, lfnum = 0;
-
-	if (msg->buf[0] == '\n')
-		lfnum++;
-	for (i = 1; i < msg->len; i++) {
-		if (msg->buf[i - 1] != '\r' && msg->buf[i] == '\n')
-			lfnum++;
+	size_t i, j;
+	char lastc;
+
+	/* First pass: tally, in j, the size of the new string: */
+	for (i = j = 0, lastc = '\0'; i < msg->len; i++) {
+		if (msg->buf[i] == '\n' && lastc != '\r')
+			j++; /* a CR will need to be added here */
+		lastc = msg->buf[i];
+		j++;
 	}
 
-	new_len = msg->len + lfnum;
-	new = xmalloc(new_len + 1);
-	if (msg->buf[0] == '\n') {
-		new[0] = '\r';
-		new[1] = '\n';
-		i = 1;
-		j = 2;
-	} else {
-		new[0] = msg->buf[0];
-		i = 1;
-		j = 1;
-	}
-	for ( ; i < msg->len; i++) {
-		if (msg->buf[i] != '\n') {
-			new[j++] = msg->buf[i];
-			continue;
-		}
-		if (msg->buf[i - 1] != '\r')
+	new = xmalloc(j + 1);
+
+	/*
+	 * Second pass: write the new string.  Note that this loop is
+	 * otherwise identical to the first pass.
+	 */
+	for (i = j = 0, lastc = '\0'; i < msg->len; i++) {
+		if (msg->buf[i] == '\n' && lastc != '\r')
 			new[j++] = '\r';
-		/* otherwise it already had CR before */
-		new[j++] = '\n';
+		lastc = new[j++] = msg->buf[i];
 	}
-	strbuf_attach(msg, new, new_len, new_len + 1);
+	strbuf_attach(msg, new, j, j + 1);
 }
 
 /*
-- 
1.8.0.3

Re: [PATCH v2 00/14] Remove unused code from imap-send.c

[Jeff King]

On Tue, Jan 15, 2013 at 09:06:18AM +0100, Michael Haggerty wrote:

> This is a re-roll, incorporating the feedback of Jonathan Nieder
> (thanks!).

Thanks, I don't see anything wrong with this from a cursory reading.

> * Added some comments to lf_to_crlf(), simplified the code a bit
>   further, and expanded the commit message.

I found this version pretty easy to read (the comments helped a lot).

-Peff

Re: [PATCH v2 00/14] Remove unused code from imap-send.c

[Jonathan Nieder]

Michael Haggerty wrote:

>  imap-send.c | 308 +++++++++++-------------------------------------------------
>  1 file changed, 55 insertions(+), 253 deletions(-)

Patch 14 is lovely.  Except for patch 6, for what it's worth these are
all

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Nicely done.