Re: [PATCH 2/5] add the ability to select more email header fields to output

["Don Zickus" <dzickus@gmail.com>]

On 3/12/07, Junio C Hamano <junkio@cox.net> wrote:
> Don Zickus <dzickus@redhat.com> writes:
>
> > This is useful when scripts need more than just the basic email headers to
> > parse.  By specifying the "-x=" option, one can search and output any header
> > field they want.
>
> It probably is useful, but that is rather difficult to judge,
> unless you have a specific use in the scripts (am/applymox).

I have my own custom clone of git-am I am using to extract the
Message-id, In-Reply-To, and References fields.  For awhile I had the
fields hardcoded in my version of git-mailinfo.  But instead of
maintaining it, I was hoping I could push it upstream.  One less thing
to maintain on my own.  :-)

>
> > @@ -870,6 +871,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
> >       def_charset = (git_commit_encoding ? git_commit_encoding : "utf-8");
> >       metainfo_charset = def_charset;
> >
> > +     for (top=0; header[top]; top++){ ; }
> > +
> >       while (1 < argc && argv[1][0] == '-') {
> >               if (!strcmp(argv[1], "-k"))
> >                       keep_subject = 1;
> > @@ -879,7 +882,10 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
> >                       metainfo_charset = NULL;
> >               else if (!prefixcmp(argv[1], "--encoding="))
> >                       metainfo_charset = argv[1] + 11;
> > -             else
> > +             else if (!prefixcmp(argv[1], "-x=")) {
> > +                     header[top] = xmalloc(256*sizeof(char));
> > +                     strncpy(header[top++], argv[1]+3, 256);
>
> Return "-ETOOMANYEXTRAHEADERSHEADERS" when top overflows,
> perhaps?

yup.  i'll fix that.

>
> You seem to omit SP around '=' in initializers (the first part
> of for loop and "typename var=init" at the beginning of block)
> but not in an assignment expression used as a freestanding
> statement.  Is this recommended by some coding style I am not
> aware of, or it it just your habit?  It is somewhat irritating

mainly habit.  I don't have a preference.  I'll clean up what I find and repost.

> to my eyes, although they might be syntactically different class
> and you might be using one from the other consistently (but in
> [1/5] some SP around '=' in assignments are omitted, and there
> does not seem to be any such consistency).
>
> And a micronit on [1/5] in the series.  I do not think "less
> than zero" comment applies to what is being done, and I do not
> think it needs to be explained what the code is doing by
> checking return value from strcasestr() with NULL.
>
> +       char boundary[256];
> +
> +       /* the only time this return less than zero is when
> +          /line/ does not contain "text/"
>          */
> -       if (strcasestr(line, "boundary=")) {
> -               fprintf(stderr, "Not handling nested multipart message.\n");
> -               exit(1);
> +       if (strcasestr(line, "text/") == NULL)
> +                message_type = TYPE_OTHER;
>

oops.  left over comment from old code.  I'll clean that up.

Cheers,
Don