Don Zickus <dzickus@xxxxxxxxxx> writes:
> 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.
> @@ -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...?
> +
> +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...
> + 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?
> @@ -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).
> +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?
> +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.
> @@ -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...
> 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html