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

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

[Michael Haggerty]

As discussed before [1], imap-send.c was copied from isync, including
a lot of code that is not used within the git project.  This patch
series rips a bunch of it out.

[1] http://comments.gmane.org/gmane.comp.version-control.git/210355

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: simplify logic in lf_to_crlf()
  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 | 286 +++++++++---------------------------------------------------
 1 file changed, 39 insertions(+), 247 deletions(-)

-- 
1.8.0.3

[PATCH 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..451d502 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

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

[Jonathan Nieder]

Hi,

Michael Haggerty wrote:

> This removes the need for function imap_make_flags(), so delete it,
> too.
[...]
> --- a/imap-send.c
> +++ b/imap-send.c
[...]
>  	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);

Before this change, the command is

	"APPEND" SP mailbox SP "{" msglen "}" CRLF

.  After this change, it leaves out the space before the brace.  If I
understand RFC3501 correctly, the space is required.  Intentional?

With the below squashed in,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/imap-send.c w/imap-send.c
index 451d5027..f1c8f5a5 100644
--- i/imap-send.c
+++ w/imap-send.c
@@ -1296,7 +1296,7 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
 	box = gctx->name;
 	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
 	cb.create = 0;
-	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\"", prefix, box);
+	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
 		return ret;

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

[Michael Haggerty]

On 01/14/2013 06:57 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> This removes the need for function imap_make_flags(), so delete it,
>> too.
> [...]
>> --- a/imap-send.c
>> +++ b/imap-send.c
> [...]
>>  	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);
> 
> Before this change, the command is
> 
> 	"APPEND" SP mailbox SP "{" msglen "}" CRLF
> 
> .  After this change, it leaves out the space before the brace.  If I
> understand RFC3501 correctly, the space is required.  Intentional?

No, not intentional.  I simply didn't follow far enough how the string
was used and mistakenly thought it was an entire command.  Thanks for
finding and fixing this.

(It probably would be less error-prone if v_issue_imap_cmd() would be
responsible for adding the extra space in the second branch of the
following if statement:

    if (!cmd->cb.data)
            bufl = nfsnprintf(buf, sizeof(buf), "%d %s\r\n", cmd->tag,
cmd->cmd);
    else
            bufl = nfsnprintf(buf, sizeof(buf), "%d %s{%d%s}\r\n",
                              cmd->tag, cmd->cmd, cmd->cb.dlen,
                              CAP(LITERALPLUS) ? "+" : "");

but that's a design choice that was already in the original version and
I don't care enough to change it.)

> With the below squashed in,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> diff --git i/imap-send.c w/imap-send.c
> index 451d5027..f1c8f5a5 100644
> --- i/imap-send.c
> +++ w/imap-send.c
> @@ -1296,7 +1296,7 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)
>  	box = gctx->name;
>  	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
>  	cb.create = 0;
> -	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\"", prefix, box);
> +	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" ", prefix, box);
>  	imap->caps = imap->rcaps;
>  	if (ret != DRV_OK)
>  		return ret;
> 

ACK.

Michael

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

[PATCH 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 451d502..a8cb66a 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 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 a8cb66a..d675e70 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 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 d675e70..3177361 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 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 3177361..a47008b 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 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 a47008b..fe2bfab 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 06/14] imap-send.c: remove some unused fields from struct store

[Jonathan Nieder]

Michael Haggerty wrote:

> --- a/imap-send.c
> +++ b/imap-send.c
[...]
> @@ -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 */

Neat.  Let me try to understand what was going on here:

When opening a mailbox with the SELECT command, an IMAP server
responds with tagged data indicating how many messages exist and how
many are marked Recent.  But git imap-send never reads mailboxes and
in particular never uses the SELECT command, so there is no need for
us to parse or record such responses.

Out of paranoia we are keeping the parsing for now, but the parsed
response is unused, hence the comment above.

If I've understood correctly so far (a big assumption), I still am not
sure what it would mean if we hit this ((arg1 = next_arg(&cmd))) case.
Does it mean:

 A. The server has gone haywire and given a tagged response where
    one is not allowed, but let's tolerate it because we always have
    done so?  Or

 B. This is a perfectly normal response to some of the commands we
    send, and we have always been deliberately ignoring it because it
    is not important for what imap-send does?

Curious,
Jonathan

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

[Michael Haggerty]

On 01/14/2013 07:19 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> --- a/imap-send.c
>> +++ b/imap-send.c
> [...]
>> @@ -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 */
> 
> Neat.  Let me try to understand what was going on here:
> 
> When opening a mailbox with the SELECT command, an IMAP server
> responds with tagged data indicating how many messages exist and how
> many are marked Recent.  But git imap-send never reads mailboxes and
> in particular never uses the SELECT command, so there is no need for
> us to parse or record such responses.
> 
> Out of paranoia we are keeping the parsing for now, but the parsed
> response is unused, hence the comment above.
> 
> If I've understood correctly so far (a big assumption), I still am not
> sure what it would mean if we hit this ((arg1 = next_arg(&cmd))) case.
> Does it mean:
> 
>  A. The server has gone haywire and given a tagged response where
>     one is not allowed, but let's tolerate it because we always have
>     done so?  Or
> 
>  B. This is a perfectly normal response to some of the commands we
>     send, and we have always been deliberately ignoring it because it
>     is not important for what imap-send does?

Honestly, I didn't bother to look into this.  I was just doing some
brainless elimination of obviously unused code.

No doubt a deeper analysis (like yours) could find more code to discard,
but I didn't want to invest that much time and this code has absolutely
no tests, so I stuck to the obvious (and even then you found a mistake
in my changes :-( ).

Michael

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

[PATCH 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 fe2bfab..452e73e 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

[PATCH 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 452e73e..5238c74 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 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.

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

diff --git a/imap-send.c b/imap-send.c
index 5238c74..9616e80 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,9 @@ 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);
+				skip_list(&cmd);
+				skip_list(&cmd);
+				skip_list(&cmd);
 			} 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 +779,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

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

[Jonathan Nieder]

Michael Haggerty wrote:

[...]
> @@ -722,9 +667,9 @@ 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);
> +				skip_list(&cmd);
> +				skip_list(&cmd);
> +				skip_list(&cmd);

This codepath would only be triggered if we emit a NAMESPACE command,
right?  If I am understanding correctly, a comment could make this
less mystifying:

				/* rfc2342 NAMESPACE response. */
				skip_list(&cmd);	/* Personal mailboxes */
				skip_list(&cmd);	/* Others' mailboxes */
				skip_list(&cmd);	/* Shared mailboxes */

Though that's probably academic since hopefully this if branch
will die soon. :)

The rest of the patch is clear and pleasant and also looks correct.

With the above change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

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

[Michael Haggerty]

On 01/14/2013 07:43 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
> [...]
>> @@ -722,9 +667,9 @@ 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);
>> +				skip_list(&cmd);
>> +				skip_list(&cmd);
>> +				skip_list(&cmd);
> 
> This codepath would only be triggered if we emit a NAMESPACE command,
> right?  If I am understanding correctly, a comment could make this
> less mystifying:
> 
> 				/* rfc2342 NAMESPACE response. */
> 				skip_list(&cmd);	/* Personal mailboxes */
> 				skip_list(&cmd);	/* Others' mailboxes */
> 				skip_list(&cmd);	/* Shared mailboxes */

Yes, the comments are useful.

> Though that's probably academic since hopefully this if branch
> will die soon. :)

Not by my hand :-)  Maybe somebody more familiar with the IMAP protocol
wants to take the code culling further...

Michael

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

[PATCH 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 9616e80..70abe9b 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 {
@@ -1079,7 +1078,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 11/14] imap-send.c: simplify logic in lf_to_crlf()

[Michael Haggerty]

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

diff --git a/imap-send.c b/imap-send.c
index 70abe9b..3167dcc 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1089,36 +1089,21 @@ static void lf_to_crlf(struct strbuf *msg)
 {
 	size_t new_len;
 	char *new;
-	int i, j, lfnum = 0;
+	int i, j = 0, lfnum = 0;
+	char lastc;
 
-	if (msg->buf[0] == '\n')
-		lfnum++;
-	for (i = 1; i < msg->len; i++) {
-		if (msg->buf[i - 1] != '\r' && msg->buf[i] == '\n')
+	for (i = 0, lastc = '\0'; i < msg->len; i++) {
+		if (msg->buf[i] == '\n' && lastc != '\r')
 			lfnum++;
+		lastc = msg->buf[i];
 	}
 
 	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')
+	for (i = 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);
 }
-- 
1.8.0.3

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

[Jonathan Nieder]

Michael Haggerty wrote:

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

I like the code reduction, but my nighttime brain doesn't have the
patience to understand the preimage or postimage, and the above
description doesn't give any hints.  Maybe a comment right above the
function definition describing its contract would help to prepare the
daunted reader (and to let the uninterested understand call sites
without reading the implementation).

Thanks,
Jonathan

[PATCH 12/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 3167dcc..31fdbf3 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -781,9 +781,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);
 }
 
@@ -868,7 +868,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;
@@ -1078,10 +1078,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;
 }
 
@@ -1112,9 +1112,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;
@@ -1126,7 +1125,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);
@@ -1282,7 +1281,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;
@@ -1338,7 +1337,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

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

[Jonathan Nieder]

Michael Haggerty wrote:

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

Nice.

It is tempting to fold "struct store" into "struct imap_store" at the same
time, as in

	struct imap_store {
		struct {
			struct store_conf *conf; /* foreign */

			/* 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 */
			int count; /* # of messages */
			int recent; /* # of recent messages - don't trust this beyond the initial read */
		} gen;
		int uidvalidity;
		struct imap *imap;
		const char *prefix;
		unsigned /*currentnc:1,*/ trashnc:1;
	};

to help the reader verify that objects of type "struct store" are not
used except through the gen field of imap_store after this change.
But verifying directly worked fine, so never mind. ;-)

[PATCH 13/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 31fdbf3..4d24faf 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 14/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 4d24faf..1b665bb 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;
@@ -1124,7 +1120,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);
@@ -1336,7 +1332,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

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

[Jeff King]

On Mon, Jan 14, 2013 at 06:32:32AM +0100, Michael Haggerty wrote:

> As discussed before [1], imap-send.c was copied from isync, including
> a lot of code that is not used within the git project.  This patch
> series rips a bunch of it out.

Thanks, this looks like a good direction.

I did not notice any problems reading through the patches, but my brain
is frazzled from a day of flying. I missed the problem Jonathan noticed.
:)

Some of the things you are removing are for advanced IMAP features that
imap-send does not need. In theory, somebody might extend it to use them
in the future. But since it has not seen any active feature development
in years, and since anybody could resurrect the code by reverting your
commits, I don't think it is a big risk.

I suspect you could go even further in ripping things out (e.g., I do
not think a server will generate an untagged namespace response at all
if we do not ask for it by issuing a namespace command). But you've
certainly grabbed the low-hanging fruit that can mostly be verified by
the compiler, and I don't know if it's worth the effort to go much
further, as it would require a lot of manual verification (and
understanding of IMAP, which will rot your brain).

-Peff

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

[Jonathan Nieder]

Michael Haggerty wrote:

>  imap-send.c | 286 +++++++++---------------------------------------------------
>  1 file changed, 39 insertions(+), 247 deletions(-)

See my replies for comments on patches 1, 6, 9, 11, and 12.  The rest
are

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

The series is tasteful and easy to follow and it's hard to argue with
the resulting code reduction.  Thanks for a pleasant read.

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

[Michael Haggerty]

On 01/14/2013 07:57 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>>  imap-send.c | 286 +++++++++---------------------------------------------------
>>  1 file changed, 39 insertions(+), 247 deletions(-)
> 
> See my replies for comments on patches 1, 6, 9, 11, and 12.  The rest
> are
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> The series is tasteful and easy to follow and it's hard to argue with
> the resulting code reduction.  Thanks for a pleasant read.

Thanks for your careful review.  I will re-roll the patch series as soon
as I get the chance.

Michael

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

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

[Junio C Hamano]

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

> On 01/14/2013 07:57 AM, Jonathan Nieder wrote:
>> Michael Haggerty wrote:
>> 
>>>  imap-send.c | 286 +++++++++---------------------------------------------------
>>>  1 file changed, 39 insertions(+), 247 deletions(-)
>> 
>> See my replies for comments on patches 1, 6, 9, 11, and 12.  The rest
>> are
>> 
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>> 
>> The series is tasteful and easy to follow and it's hard to argue with
>> the resulting code reduction.  Thanks for a pleasant read.
>
> Thanks for your careful review.  I will re-roll the patch series as soon
> as I get the chance.

Thanks, all of you.  The numbers and the graph look very nice indeed
;-)