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

2013-01-15 Thread Jonathan Nieder 
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.
 */
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-15 Thread Junio C Hamano 
Jonathan Nieder jrnie...@gmail.com writes:

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

Hmph; it seems that it is not worth rerolling the whole thing only
for this, so let me squash this in, replacing the /* unused */ with
the large comment, and then merge the result to 'next'.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-15 Thread Jonathan Nieder 
Junio C Hamano wrote:
 Jonathan Nieder jrnie...@gmail.com writes:

 Since actually fixing that is probably too aggressive for this patch,
 how about a FIXME comment like the following?
[...]
 Hmph; it seems that it is not worth rerolling the whole thing only
 for this, so let me squash this in, replacing the /* unused */ with
 the large comment, and then merge the result to 'next'.

Sounds good to me.  Next time, I'll include a 'fixup!' patch instead of
making you do the copy/pasting.

Thanks, both.
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html