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
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html