Re: [PATCH 1/2] mailinfo: support rfc3676 (format=flowed) text/plain messages

["Jay Soffian" <jaysoffian@gmail.com>]

On Fri, Feb 15, 2008 at 5:41 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

>  Why not call it "enum message_format"?

It's only applicable to text/plain messages. Then again, this will be
set to FORMAT_NONE for non-text/plain messages so I guess that's just as
good a name.

BTW, since all I care about is format=flowed, I could just as easily use
an int and call it something like message_is_flowed. This would actually
simplify the code a bit but I was trying to follow the existing code
which uses an enum for message_type that could similarly have been an
int called message_type_is_text.

>  Why not call it "enum message_is_delsp"?

Sure, my reasoning is same as above.

>  > @@ -193,6 +200,18 @@ static int handle_content_type(char *line)
>  >
>  >       if (strcasestr(line, "text/") == NULL)
>  >                message_type = TYPE_OTHER;
>  > +     else if (strcasestr(line, "text/plain")) {
>  > +             char attr[256];
>  > +             if (slurp_attr(line, "format=", attr) && !strcasecmp(attr, "flowed")) {
>  > +                     tp_format = FORMAT_FLOWED;
>  > +                     if (slurp_attr(line, "delsp=", attr) && !strcasecmp(attr, "yes"))
>  > +                             tp_delsp = DELSP_YES;
>  > +                     else
>  > +                             tp_delsp = DELSP_NO;
>  > +             }
>  > +             else
>  > +                     tp_format = FORMAT_FIXED;
>
>  Does that mean that the format is only set if the content type is
>  "text/plain"?

tp_format is initially set to FORMAT_NONE. Only for text/plain is it
them set to FORMAT_FIXED or FORMAT_FLOWED. Again, though, all I care
about is format=flowed so I could be using an int called
message_is_flowed but was following the enum message_type example.

>  > +     if (tp_format == FORMAT_FLOWED && !!strcmp(line, "-- \n")) {
>
>  The !! is unnecessary; please skip it.

Heh. I'd never seen that before I started perusing the git code, I was
just following along. But looking more carefully now I see that the
other code only does that if it's assigning to an int. I see that it's
not needed in a boolean context.

>  How about using strchrnul()?

Cool. I'd never heard of it.

>  > +                     if (cp > line && *cp == '\n' && *(cp-1) == ' ') {
>  > +                             if (tp_delsp == DELSP_YES)
>  > +                                     *(cp-1) = '\0';
>  > +                             else
>  > +                                     *cp = '\0';
>  > +                     }
>
>  Or maybe
>                                 cp[0 - (tp_delsp == DELSP_YES)] = '\0';
>
>  But maybe that is too cute.

Heh, I think that's too clever by half.

>  But another thing struck me here: why setting *cp = '\0'; only if *(cp-1)
>  == ' ', even if tp_delsp != DELSP_YES?

" \n$" indicates the line is flowed, meaning the MUA added the '\n' to
the line and we need to remove it. The next question is whether the
space before the '\n' was also inserted by the MUA. If DELSP_YES, it
was, so we want to remove it, else it was an existing space that we want
to keep.

>  > @@ -818,6 +857,7 @@ static void handle_body(void)
>  >
>  >               switch (transfer_encoding) {
>  >               case TE_BASE64:
>  > +             case TE_QP:
>  >               {
>  >                       char *op = line;
>
>  Did that just slip in, or was this intended.  If the latter, is this
>  related to format=flawed, or is it a bug fix in its own right?

It was intended. The patch doesn't have enough context to have included
this comment inside the TE_BASE64 case:

	/* this is a decoded line that may contain
	 * multiple new lines.  Pass only one chunk
	 * at a time to handle_filter()
	 */

It turns out that decoding QP can also cause a decoded line to contain
extra newlines. So it's a bug fix, but I'm not sure it mattered before.
I'll break it out into a separate patch though to make that clear. And I
know I should add a test first for that fix, but ugh, figuring out the
test case for a one-line code change is painful.

Thanks for the comments, I'll follow up with a revised patch.

j.