Re: [PATCH 1/5] builtin-mailinfo.c infrastrcture changes
- From
- Don Zickus <dzickus@redhat.com>
- Date
- Mar 7, 2007, 17:17 UTC
- Message-ID
- <20070307171744.GL11029@redhat.com>
- In-Reply-To
- <7v8xe9k460.fsf@assigned-by-dhcp.cox.net>
On Tue, Mar 06, 2007 at 09:34:31PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
>
> > @@ -177,17 +175,35 @@ static int slurp_attr(const char *line, const char *name, char *attr)
> > return 1;
> > }
> >
> > -static int handle_subcontent_type(char *line)
> > +struct content_type {
> > + char *boundary;
> > + int boundary_len;
> > +};
> > +
> > +static struct content_type content[]={
> > + { NULL },
> > + { NULL },
> > + { NULL },
> > + { NULL },
> > + { NULL },
> > +};
>
> The reason why this array has 5 elements (not 4, not 6) is...?a flip of a coin. I got lazy and didn't feel like implementing linked lists. So instead I decided to use static char arrays. I can create a #define <arraysize> with a comment on its intended use. Or do you prefer a different approach.
Show 18 quoted lines
>
> > +
> > +static struct content_type *content_top = content;
> > +
> > +static int handle_content_type(char *line)
> > {
> > + char boundary[256];
> > +
> > + if (strcasestr(line, "text/") < 0)
> > + message_type = TYPE_OTHER;
>
> Did you mean
>
> if (strncasecmp(line, "text/", 5))
>
> It makes me wonder how a couple thousand mails (or for that
> matter our own testsuite) could have passed the test with a
> thing like this...Ooops. Thanks for catching that. The default TYPE_TEXT works for almost all situations, which is why it passes my tests. The only reason why I put that code in there is to handle very large base64 binary blobs (that could overflow the char arrays over 2k bytes). I didn't have any binary blobs that large to test so I never ran into this bug. I'll fix it up.
Show 10 quoted lines
>
> > + if (slurp_attr(line, "boundary=", boundary + 2)) {
> > + memcpy(boundary, "--", 2);
> > + content_top++;
> > + content_top->boundary_len = strlen(boundary);
> > + content_top->boundary = xmalloc(content_top->boundary_len+1);
> > + strcpy(content_top->boundary, boundary);
> > }
>
> Does anybody check the content[] stack overflow?nope. meant too though. I'll fix that too.
Show 38 quoted lines
>
> > @@ -341,57 +290,65 @@ static void cleanup_space(char *buf)
> > }
> >
> > static void decode_header(char *it);
> > -typedef int (*header_fn_t)(char *);
> > -struct header_def {
> > - const char *name;
> > - header_fn_t func;
> > - int namelen;
> > +static char *header[10] = {
> > + "From",
> > + "Subject",
> > + "Date",
> > + NULL,
> > + NULL,
> > + NULL,
> > };
>
> Why initialize only three with NULL when you have four more?
> What are the 7 entries other than From/Subject/Date for? Future
> extension? Wouldn't
>
> static char *header[] = {
> "From", "Subject", "Date", NULL,
> };
>
> or even:
>
> static char *header[] = {
> "From", "Subject", "Date",
> };
>
> easier to handle (for the latter, you would need your loop with:
>
> for (i = 0; i < ARRAY_SIZE(header); i++)
> ...
> see patch 2/5 for a better idea of the direction I was going with this. Again another lazy trick to avoid linked lists and instead use static char arrays to easily handle new content. Probably wouldn't hurt for me to clean this up too.
Show 5 quoted lines
> > + /* Content stuff */
> > + if (!strncasecmp(line, "Content-Type: ", 14)) {
>
> I'd rather not insist SP after colon (I do not think it even has
> to exist, but I think we check for isspace() in the current code).ok.
Show 12 quoted lines
>
> > +static int handle_boundary(void)
> > +{
> > +again:
> > + if (!memcmp(line+content_top->boundary_len, "--", 2)) {
> > + /* we hit an end boundary */
> > + /* pop the current boundary off the stack */
> > + free(content_top->boundary);
> > + content_top--;
> > + handle_filter("\n");
>
> Stack underflow?yup. i'll fix this.
Show 22 quoted lines
>
> > +static void handle_body(void)
> > {
> > ...
> > + switch (transfer_encoding) {
> > + case TE_BASE64:
> > + {
> > + /* binary data most likely doesn't have newlines */
> > + if (message_type != TYPE_TEXT) {
> > + rc=handle_filter(line);
> > + break;
> > + }
> > +
> > + /* this is a decoded line that may contain
> > + * multiple new lines. Pass only one chunk
> > + * at a time to handle_filter()
> > + */
> > +
> > + char *op=line;
> > +
>
> builtin-mailinfo.c:786: warning: ISO C90 forbids mixed declarations and code.hmm. don't know why gcc 4.1.1 didn't catch that. anyway, the binary data part was added much later. should be easy to fix.
Show 10 quoted lines
> > > @@ -809,18 +833,16 @@ int mailinfo(FILE *in, FILE *out, int ks, const char *encoding, > > fclose(cmitmsg); > > return -1; > > } > > + > > + p_hdr_data = xcalloc(10, sizeof(char *)); > > + s_hdr_data = xcalloc(10, sizeof(char *)); > > These tens look unexplained magic...
i'll create a #define for those too.
Show 16 quoted lines
> > > diff --git a/git-am.sh b/git-am.sh > > index 2c73d11..8fa466a 100755 > > --- a/git-am.sh > > +++ b/git-am.sh > > @@ -290,6 +290,7 @@ do > > git-mailinfo $keep $utf8 "$dotest/msg" "$dotest/patch" \ > > <"$dotest/$msgnum" >"$dotest/info" || > > stop_here $this > > + test -s $dotest/patch || stop_here $this > > git-stripspace < "$dotest/msg" > "$dotest/msg-clean" > > ;; > > esac > > I think this interface change probably is a good thing. I > wonder if we need a matching change to applymbox, though.
that and git-quiltimport too.
I'll send out another draft this afternoon.
Cheers, Don