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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 15, 2013, 20:32 UTC
Message-ID: <20130115203204.GA12524@google.com>
In-Reply-To: <1358237193-8887-7-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty wrote:

Show 7 quoted lines

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