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

[Jonathan Nieder <jrnieder@gmail.com>]

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