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

From: Michael Haggerty <[email-masked]>
Date: Wed, 16 Jan 2013 09:23:03 +0100

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
[email-masked]
http://softwareswirl.blogspot.com/