Re: [PATCH 1/5] builtin-mailinfo.c infrastrcture changes
- From
Junio C Hamano <junkio@cox.net>
- Date
- Mar 7, 2007, 05:34 UTC
- Message-ID
- <7v8xe9k460.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <1173218263315-git-send-email-dzickus@redhat.com>
Don Zickus <dzickus@redhat.com> writes:
Show 14 quoted lines
> List of major changes/fixes: > - can't create empty patch files fix > - empty patch files don't fail, this failure will come inside git-am > - multipart boundaries are now handled > - only output inbody headers if a patch exists otherwise assume those > headers are part of the reply and instead output the original headers > - decode and filter base64 patches correctly > - various other accidental fixes > > I believe I didn't break any existing functionality or compatibility (other > than what I describe above, which is really only the empty patch file). > > I tested this through various mailing list archives and everything seemed to > parse correctly (a couple thousand emails).
Thanks.
Show 17 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...?
Show 9 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...
Show 7 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?
Show 17 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++)
...> + /* 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).
Show 9 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?
Show 19 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.
Show 7 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...
Show 12 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.