Re: [PATCH 06/14] imap-send.c: remove some unused fields from struct store
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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/14] imap-send.c: remove some unused fields from struct store
Signed-off-by: Michael Haggerty mhag...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/14] imap-send.c: remove some unused fields from struct store
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html