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

> 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.
> 		 */