From: Michael Haggerty <mhagger@alum.mit.edu>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store
Date: Wed, 16 Jan 2013 09:23:03 +0100 [thread overview]
Message-ID: <50F66367.1010106@alum.mit.edu> (raw)
In-Reply-To: <20130115203204.GA12524@google.com>
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@alum.mit.edu
http://softwareswirl.blogspot.com/
next prev parent reply other threads:[~2013-01-16 8:23 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-15 8:06 [PATCH v2 00/14] Remove unused code from imap-send.c Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 01/14] imap-send.c: remove msg_data::flags, which was always zero Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 02/14] imap-send.c: remove struct msg_data Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 03/14] iamp-send.c: remove unused struct imap_store_conf Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 04/14] imap-send.c: remove struct store_conf Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 05/14] imap-send.c: remove struct message Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 06/14] imap-send.c: remove some unused fields from struct store Michael Haggerty
2013-01-15 20:32 ` Jonathan Nieder
2013-01-15 22:30 ` Junio C Hamano
2013-01-15 22:59 ` Jonathan Nieder
2013-01-16 8:23 ` Michael Haggerty [this message]
2013-01-15 8:06 ` [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list() Michael Haggerty
2013-01-15 18:51 ` Matt Kraai
2013-01-16 8:26 ` Michael Haggerty
2013-01-16 15:34 ` Junio C Hamano
2013-01-17 4:43 ` Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 08/14] imap-send.c: remove struct imap argument to parse_imap_list_l() Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 09/14] imap-send.c: remove namespace fields from struct imap Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 10/14] imap-send.c: remove unused field imap_store::trashnc Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 11/14] imap-send.c: use struct imap_store instead of struct store Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 12/14] imap-send.c: remove unused field imap_store::uidvalidity Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 13/14] imap-send.c: fold struct store into struct imap_store Michael Haggerty
2013-01-15 8:06 ` [PATCH v2 14/14] imap-send.c: simplify logic in lf_to_crlf() Michael Haggerty
2013-01-15 14:42 ` [PATCH v2 00/14] Remove unused code from imap-send.c Jeff King
2013-01-15 20:49 ` Jonathan Nieder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50F66367.1010106@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.