{"thread":{"id":"32192","subject":"[PATCH 0/8] Add function strbuf_addstr_xml_quoted() and more","startedAt":"2012-11-25T11:08:33Z","lastAt":"2012-12-03T15:06:32Z","messageCount":18,"participants":["Michael Haggerty","Junio C Hamano","Jeff King","Thiago Farina"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"203797","messageId":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":null,"subject":"[PATCH 0/8] Add function strbuf_addstr_xml_quoted() and more","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:33Z","receivedAt":"2012-11-25T11:08:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There were two functions doing almost the same XML quoting of\ncharacter entities, so implement a library function\nstrbuf_addstr_xml_quoted() and use that in both places.\n\nAlong the way, do a lot of simplification within imap-send.c, which\nwas doing a lot of its own string management instead of using strbuf.\n\nPlease note that \"git imap-send\" is utterly absent from the test\nsuite, probably due to the difficulty of testing without a real IMAP\nserver. I ran some manual tests after my changes and didn't find any\nproblems.\n\nThe bug that I reported on 2012-11-12, namely that\n\n git format-patch --signoff --stdout --attach origin | git imap-send\n\nis broken, is not addressed by these patches.\n\nMichael Haggerty (8):\n Add new function strbuf_add_xml_quoted()\n xml_entities(): use function strbuf_addstr_xml_quoted()\n lf_to_crlf(): NUL-terminate msg_data::data\n imap-send: store all_msgs as a strbuf\n imap-send: correctly report errors reading from stdin\n imap-send: change msg_data from storing (char *, len) to storing\n strbuf\n wrap_in_html(): use strbuf_addstr_xml_quoted()\n wrap_in_html(): process message in bulk rather than line-by-line\n\n http-push.c | 23 +--------\n imap-send.c | 157 +++++++++++++++++++++++++++---------------------------------\n strbuf.c | 26 ++++++++++\n strbuf.h | 6 +++\n 4 files changed, 104 insertions(+), 108 deletions(-)\n\n-- \n1.8.0\n"},{"id":"203798","messageId":"1353841721-16269-2-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/8] Add new function strbuf_add_xml_quoted()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:34Z","receivedAt":"2012-11-25T11:08:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Substantially the same code is present in http-push.c and imap-send.c,\nso make a library function out of it.\n\nSigned-off-by: Michael Haggerty \n---\n strbuf.c | 26 ++++++++++++++++++++++++++\n strbuf.h | 6 ++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 05d0693..9a373be 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -425,6 +425,32 @@ void strbuf_add_lines(struct strbuf *out, const char *prefix,\n \tstrbuf_complete_line(out);\n }\n \n+void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)\n+{\n+\twhile (*s) {\n+\t\tsize_t len = strcspn(s, \"\\\"<>&\");\n+\t\tstrbuf_add(buf, s, len);\n+\t\ts += len;\n+\t\tswitch (*s) {\n+\t\tcase '\"':\n+\t\t\tstrbuf_addstr(buf, \""\");\n+\t\t\tbreak;\n+\t\tcase '<':\n+\t\t\tstrbuf_addstr(buf, \"<\");\n+\t\t\tbreak;\n+\t\tcase '>':\n+\t\t\tstrbuf_addstr(buf, \">\");\n+\t\t\tbreak;\n+\t\tcase '&':\n+\t\t\tstrbuf_addstr(buf, \"&\");\n+\t\t\tbreak;\n+\t\tcase 0:\n+\t\t\treturn;\n+\t\t}\n+\t\ts++;\n+\t}\n+}\n+\n static int is_rfc3986_reserved(char ch)\n {\n \tswitch (ch) {\ndiff --git a/strbuf.h b/strbuf.h\nindex aa386c6..ecae4e2 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -136,6 +136,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n \n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n+/*\n+ * Append s to sb, with the characters '<', '>', '&' and '\"' converted\n+ * into XML entities.\n+ */\n+extern void strbuf_addstr_xml_quoted(struct strbuf *sb, const char *s);\n+\n static inline void strbuf_complete_line(struct strbuf *sb)\n {\n \tif (sb->len && sb->buf[sb->len - 1] != '\\n')\n-- \n1.8.0\n"},{"id":"203802","messageId":"1353841721-16269-3-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/8] xml_entities(): use function strbuf_addstr_xml_quoted()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:35Z","receivedAt":"2012-11-25T11:08:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n http-push.c | 23 +----------------------\n 1 file changed, 1 insertion(+), 22 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 8701c12..9923441 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -172,28 +172,7 @@ enum dav_header_flag {\n static char *xml_entities(const char *s)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\twhile (*s) {\n-\t\tsize_t len = strcspn(s, \"\\\"<>&\");\n-\t\tstrbuf_add(&buf, s, len);\n-\t\ts += len;\n-\t\tswitch (*s) {\n-\t\tcase '\"':\n-\t\t\tstrbuf_addstr(&buf, \""\");\n-\t\t\tbreak;\n-\t\tcase '<':\n-\t\t\tstrbuf_addstr(&buf, \"<\");\n-\t\t\tbreak;\n-\t\tcase '>':\n-\t\t\tstrbuf_addstr(&buf, \">\");\n-\t\t\tbreak;\n-\t\tcase '&':\n-\t\t\tstrbuf_addstr(&buf, \"&\");\n-\t\t\tbreak;\n-\t\tcase 0:\n-\t\t\treturn strbuf_detach(&buf, NULL);\n-\t\t}\n-\t\ts++;\n-\t}\n+\tstrbuf_addstr_xml_quoted(&buf, s);\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-- \n1.8.0\n"},{"id":"203801","messageId":"1353841721-16269-4-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/8] lf_to_crlf(): NUL-terminate msg_data::data","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:36Z","receivedAt":"2012-11-25T11:08:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Through the rest of the file, the data member of struct msg_data is\nkept NUL-terminated, and that fact is relied upon in a couple of\nplaces. Change lf_to_crlf() to preserve this invariant.\n\nIn fact, there are no execution paths in which lf_to_crlf() is called\nand then its data member is required to be NUL-terminated, but it is\nbetter to be consistent to prevent future confusion.\n\nDocument the invariant in the struct msg_data definition.\n\nSigned-off-by: Michael Haggerty \n---\n imap-send.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex d42e471..c818b0c 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -69,8 +69,12 @@ struct store {\n };\n \n struct msg_data {\n+\t/* NUL-terminated data: */\n \tchar *data;\n+\n+\t/* length of data (not including NUL): */\n \tint len;\n+\n \tunsigned char flags;\n };\n \n@@ -1276,7 +1280,7 @@ static void lf_to_crlf(struct msg_data *msg)\n \t\t\tlfnum++;\n \t}\n \n-\tnew = xmalloc(msg->len + lfnum);\n+\tnew = xmalloc(msg->len + lfnum + 1);\n \tif (msg->data[0] == '\\n') {\n \t\tnew[0] = '\\r';\n \t\tnew[1] = '\\n';\n@@ -1297,6 +1301,7 @@ static void lf_to_crlf(struct msg_data *msg)\n \t\t/* otherwise it already had CR before */\n \t\tnew[j++] = '\\n';\n \t}\n+\tnew[j] = '\\0';\n \tmsg->len += lfnum;\n \tfree(msg->data);\n \tmsg->data = new;\n-- \n1.8.0\n"},{"id":"203800","messageId":"1353841721-16269-5-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/8] imap-send: store all_msgs as a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:37Z","receivedAt":"2012-11-25T11:08:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"all_msgs is only used as a glorified string, therefore there is no\nreason to declare it as a struct msg_data.\n\nSigned-off-by: Michael Haggerty \n---\n imap-send.c | 23 +++++++++--------------\n 1 file changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex c818b0c..50e223a 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1391,26 +1391,20 @@ static void wrap_in_html(struct msg_data *msg)\n \n #define CHUNKSIZE 0x1000\n \n-static int read_message(FILE *f, struct msg_data *msg)\n+static int read_message(FILE *f, struct strbuf *all_msgs)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\tmemset(msg, 0, sizeof(*msg));\n-\n \tdo {\n-\t\tif (strbuf_fread(&buf, CHUNKSIZE, f) <= 0)\n+\t\tif (strbuf_fread(all_msgs, CHUNKSIZE, f) <= 0)\n \t\t\tbreak;\n \t} while (!feof(f));\n \n-\tmsg->len = buf.len;\n-\tmsg->data = strbuf_detach(&buf, NULL);\n-\treturn msg->len;\n+\treturn all_msgs->len;\n }\n \n-static int count_messages(struct msg_data *msg)\n+static int count_messages(struct strbuf *all_msgs)\n {\n \tint count = 0;\n-\tchar *p = msg->data;\n+\tchar *p = all_msgs->buf;\n \n \twhile (1) {\n \t\tif (!prefixcmp(p, \"From \")) {\n@@ -1431,7 +1425,7 @@ static int count_messages(struct msg_data *msg)\n \treturn count;\n }\n \n-static int split_msg(struct msg_data *all_msgs, struct msg_data *msg, int *ofs)\n+static int split_msg(struct strbuf *all_msgs, struct msg_data *msg, int *ofs)\n {\n \tchar *p, *data;\n \n@@ -1439,7 +1433,7 @@ static int split_msg(struct msg_data *all_msgs, struct msg_data *msg, int *ofs)\n \tif (*ofs >= all_msgs->len)\n \t\treturn 0;\n \n-\tdata = &all_msgs->data[*ofs];\n+\tdata = &all_msgs->buf[*ofs];\n \tmsg->len = all_msgs->len - *ofs;\n \n \tif (msg->len < 5 || prefixcmp(data, \"From \"))\n@@ -1509,7 +1503,8 @@ static int git_imap_config(const char *key, const char *val, void *cb)\n \n int main(int argc, char **argv)\n {\n-\tstruct msg_data all_msgs, msg;\n+\tstruct strbuf all_msgs = STRBUF_INIT;\n+\tstruct msg_data msg;\n \tstruct store *ctx = NULL;\n \tint ofs = 0;\n \tint r;\n-- \n1.8.0\n"},{"id":"203799","messageId":"1353841721-16269-6-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/8] imap-send: correctly report errors reading from stdin","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:38Z","receivedAt":"2012-11-25T11:08:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, read_message() didn't distinguish between an error and eof\nwhen reading its input. This could have resulted in incorrect\nbehavior if there was an error: (1) reporting \"nothing to send\" if no\nbytes were read or (2) sending an incomplete message if some bytes\nwere read before the error.\n\nChange read_message() to return -1 on ferror()s and 0 on success, so\nthat the caller can recognize that an error occurred. (The return\nvalue used to be the length of the input read, which was redundant\nbecause that is already available as the strbuf length.\n\nChange the caller to report errors correctly.\n\nSigned-off-by: Michael Haggerty \n---\n imap-send.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 50e223a..86cf603 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1398,7 +1398,7 @@ static int read_message(FILE *f, struct strbuf *all_msgs)\n \t\t\tbreak;\n \t} while (!feof(f));\n \n-\treturn all_msgs->len;\n+\treturn ferror(f) ? -1 : 0;\n }\n \n static int count_messages(struct strbuf *all_msgs)\n@@ -1537,7 +1537,12 @@ int main(int argc, char **argv)\n \t}\n \n \t/* read the messages */\n-\tif (!read_message(stdin, &all_msgs)) {\n+\tif (read_message(stdin, &all_msgs)) {\n+\t\tfprintf(stderr, \"error reading input\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tif (all_msgs.len == 0) {\n \t\tfprintf(stderr, \"nothing to send\\n\");\n \t\treturn 1;\n \t}\n-- \n1.8.0\n"},{"id":"203803","messageId":"1353841721-16269-7-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:39Z","receivedAt":"2012-11-25T11:08:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"struct msg_data stored (char *, len) of the data to be included in a\nmessage, kept the character data NUL-terminated, etc., much like a\nstrbuf would do. So change it to use a struct strbuf. This makes the\ncode clearer and reduces copying a little bit.\n\nA side effect of this change is that the memory for each message is\nfreed after it is used rather than leaked, though that detail is\nunimportant given that imap-send is a top-level command.\n\n--\n\nFor some reason, there is a bunch of infrastructure in this file for\ndealing with IMAP flags, although there is nothing in the code that\nactually allows any flags to be set. If there is no plan to add\nsupport for flags in the future, a bunch of code could be ripped out\nand \"struct msg_data\" could be completely replaced with strbuf.\n\nSigned-off-by: Michael Haggerty \n---\n imap-send.c | 92 +++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 47 insertions(+), 45 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 86cf603..a5e0e33 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -69,12 +69,7 @@ struct store {\n };\n \n struct msg_data {\n-\t/* NUL-terminated data: */\n-\tchar *data;\n-\n-\t/* length of data (not including NUL): */\n-\tint len;\n-\n+\tstruct strbuf data;\n \tunsigned char flags;\n };\n \n@@ -1268,46 +1263,49 @@ static int imap_make_flags(int flags, char *buf)\n \treturn d;\n }\n \n-static void lf_to_crlf(struct msg_data *msg)\n+static void lf_to_crlf(struct strbuf *msg)\n {\n+\tsize_t new_len;\n \tchar *new;\n \tint i, j, lfnum = 0;\n \n-\tif (msg->data[0] == '\\n')\n+\tif (msg->buf[0] == '\\n')\n \t\tlfnum++;\n \tfor (i = 1; i < msg->len; i++) {\n-\t\tif (msg->data[i - 1] != '\\r' && msg->data[i] == '\\n')\n+\t\tif (msg->buf[i - 1] != '\\r' && msg->buf[i] == '\\n')\n \t\t\tlfnum++;\n \t}\n \n-\tnew = xmalloc(msg->len + lfnum + 1);\n-\tif (msg->data[0] == '\\n') {\n+\tnew_len = msg->len + lfnum;\n+\tnew = xmalloc(new_len + 1);\n+\tif (msg->buf[0] == '\\n') {\n \t\tnew[0] = '\\r';\n \t\tnew[1] = '\\n';\n \t\ti = 1;\n \t\tj = 2;\n \t} else {\n-\t\tnew[0] = msg->data[0];\n+\t\tnew[0] = msg->buf[0];\n \t\ti = 1;\n \t\tj = 1;\n \t}\n \tfor ( ; i < msg->len; i++) {\n-\t\tif (msg->data[i] != '\\n') {\n-\t\t\tnew[j++] = msg->data[i];\n+\t\tif (msg->buf[i] != '\\n') {\n+\t\t\tnew[j++] = msg->buf[i];\n \t\t\tcontinue;\n \t\t}\n-\t\tif (msg->data[i - 1] != '\\r')\n+\t\tif (msg->buf[i - 1] != '\\r')\n \t\t\tnew[j++] = '\\r';\n \t\t/* otherwise it already had CR before */\n \t\tnew[j++] = '\\n';\n \t}\n-\tnew[j] = '\\0';\n-\tmsg->len += lfnum;\n-\tfree(msg->data);\n-\tmsg->data = new;\n+\tstrbuf_attach(msg, new, new_len, new_len + 1);\n }\n \n-static int imap_store_msg(struct store *gctx, struct msg_data *data)\n+/*\n+ * Store msg to IMAP. Also detach and free the data from msg->data,\n+ * leaving msg->data empty.\n+ */\n+static int imap_store_msg(struct store *gctx, struct msg_data *msg)\n {\n \tstruct imap_store *ctx = (struct imap_store *)gctx;\n \tstruct imap *imap = ctx->imap;\n@@ -1316,16 +1314,15 @@ static int imap_store_msg(struct store *gctx, struct msg_data *data)\n \tint ret, d;\n \tchar flagstr[128];\n \n-\tlf_to_crlf(data);\n+\tlf_to_crlf(&msg->data);\n \tmemset(&cb, 0, sizeof(cb));\n \n-\tcb.dlen = data->len;\n-\tcb.data = xmalloc(cb.dlen);\n-\tmemcpy(cb.data, data->data, data->len);\n+\tcb.dlen = msg->data.len;\n+\tcb.data = strbuf_detach(&msg->data, NULL);\n \n \td = 0;\n-\tif (data->flags) {\n-\t\td = imap_make_flags(data->flags, flagstr);\n+\tif (msg->flags) {\n+\t\td = imap_make_flags(msg->flags, flagstr);\n \t\tflagstr[d++] = ' ';\n \t}\n \tflagstr[d] = 0;\n@@ -1356,7 +1353,8 @@ static void encode_html_chars(struct strbuf *p)\n \t\t\tstrbuf_splice(p, i, 1, \""\", 6);\n \t}\n }\n-static void wrap_in_html(struct msg_data *msg)\n+\n+static void wrap_in_html(struct strbuf *msg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf **lines;\n@@ -1366,9 +1364,7 @@ static void wrap_in_html(struct msg_data *msg)\n \tstatic char *pre_close = \"\\n\";\n \tint added_header = 0;\n \n-\tstrbuf_attach(&buf, msg->data, msg->len, msg->len);\n-\tlines = strbuf_split(&buf, '\\n');\n-\tstrbuf_release(&buf);\n+\tlines = strbuf_split(msg, '\\n');\n \tfor (p = lines; *p; p++) {\n \t\tif (! added_header) {\n \t\t\tif ((*p)->len == 1 && *((*p)->buf) == '\\n') {\n@@ -1385,8 +1381,8 @@ static void wrap_in_html(struct msg_data *msg)\n \t}\n \tstrbuf_addstr(&buf, pre_close);\n \tstrbuf_list_free(lines);\n-\tmsg->len = buf.len;\n-\tmsg->data = strbuf_detach(&buf, NULL);\n+\tstrbuf_release(msg);\n+\t*msg = buf;\n }\n \n #define CHUNKSIZE 0x1000\n@@ -1425,34 +1421,39 @@ static int count_messages(struct strbuf *all_msgs)\n \treturn count;\n }\n \n-static int split_msg(struct strbuf *all_msgs, struct msg_data *msg, int *ofs)\n+/*\n+ * Copy the next message from all_msgs, starting at offset *ofs, to\n+ * msg. Update *ofs to the start of the following message. Return\n+ * true iff a message was successfully copied.\n+ */\n+static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)\n {\n \tchar *p, *data;\n+\tsize_t len;\n \n-\tmemset(msg, 0, sizeof *msg);\n \tif (*ofs >= all_msgs->len)\n \t\treturn 0;\n \n \tdata = &all_msgs->buf[*ofs];\n-\tmsg->len = all_msgs->len - *ofs;\n+\tlen = all_msgs->len - *ofs;\n \n-\tif (msg->len < 5 || prefixcmp(data, \"From \"))\n+\tif (len < 5 || prefixcmp(data, \"From \"))\n \t\treturn 0;\n \n \tp = strchr(data, '\\n');\n \tif (p) {\n-\t\tp = &p[1];\n-\t\tmsg->len -= p-data;\n-\t\t*ofs += p-data;\n+\t\tp++;\n+\t\tlen -= p - data;\n+\t\t*ofs += p - data;\n \t\tdata = p;\n \t}\n \n \tp = strstr(data, \"\\nFrom \");\n \tif (p)\n-\t\tmsg->len = &p[1] - data;\n+\t\tlen = &p[1] - data;\n \n-\tmsg->data = xmemdupz(data, msg->len);\n-\t*ofs += msg->len;\n+\tstrbuf_add(msg, data, len);\n+\t*ofs += len;\n \treturn 1;\n }\n \n@@ -1504,7 +1505,7 @@ static int git_imap_config(const char *key, const char *val, void *cb)\n int main(int argc, char **argv)\n {\n \tstruct strbuf all_msgs = STRBUF_INIT;\n-\tstruct msg_data msg;\n+\tstruct msg_data msg = {STRBUF_INIT, 0};\n \tstruct store *ctx = NULL;\n \tint ofs = 0;\n \tint r;\n@@ -1564,11 +1565,12 @@ int main(int argc, char **argv)\n \tctx->name = imap_folder;\n \twhile (1) {\n \t\tunsigned percent = n * 100 / total;\n+\n \t\tfprintf(stderr, \"%4u%% (%d/%d) done\\r\", percent, n, total);\n-\t\tif (!split_msg(&all_msgs, &msg, &ofs))\n+\t\tif (!split_msg(&all_msgs, &msg.data, &ofs))\n \t\t\tbreak;\n \t\tif (server.use_html)\n-\t\t\twrap_in_html(&msg);\n+\t\t\twrap_in_html(&msg.data);\n \t\tr = imap_store_msg(ctx, &msg);\n \t\tif (r != DRV_OK)\n \t\t\tbreak;\n-- \n1.8.0\n"},{"id":"203804","messageId":"1353841721-16269-8-git-send-email-mhagger@alum.mit.edu","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 7/8] wrap_in_html(): use strbuf_addstr_xml_quoted()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-25T11:08:40Z","receivedAt":"2012-11-25T11:08:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use the new function to quote characters as they are being added to\nbuf, rather than quoting them in *p and then copying them into buf.\nThis increases code sharing, and changes the algorithm from O(N^2) to\nO(N) in the number of characters in a line.\n\nSigned-off-by: Michael Haggerty \n---\n imap-send.c | 23 ++++-------------------\n 1 file changed, 4 insertions(+), 19 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex a5e0e33..b73c913 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1339,21 +1339,6 @@ static int imap_store_msg(struct store *gctx, struct msg_data *msg)\n \treturn DRV_OK;\n }\n \n-static void encode_html_chars(struct strbuf *p)\n-{\n-\tint i;\n-\tfor (i = 0; i < p->len; i++) {\n-\t\tif (p->buf[i] == '&')\n-\t\t\tstrbuf_splice(p, i, 1, \"&\", 5);\n-\t\tif (p->buf[i] == '<')\n-\t\t\tstrbuf_splice(p, i, 1, \"<\", 4);\n-\t\tif (p->buf[i] == '>')\n-\t\t\tstrbuf_splice(p, i, 1, \">\", 4);\n-\t\tif (p->buf[i] == '\"')\n-\t\t\tstrbuf_splice(p, i, 1, \""\", 6);\n-\t}\n-}\n-\n static void wrap_in_html(struct strbuf *msg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1372,12 +1357,12 @@ static void wrap_in_html(struct strbuf *msg)\n \t\t\t\tstrbuf_addbuf(&buf, *p);\n \t\t\t\tstrbuf_addstr(&buf, pre_open);\n \t\t\t\tadded_header = 1;\n-\t\t\t\tcontinue;\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addbuf(&buf, *p);\n \t\t\t}\n+\t\t} else {\n+\t\t\tstrbuf_addstr_xml_quoted(&buf, (*p)->buf);\n \t\t}\n-\t\telse\n-\t\t\tencode_html_chars(*p);\n-\t\tstrbuf_addbuf(&buf, *p);\n \t}\n \tstrbuf_addstr(&buf, pre_close);\n \tstrbuf_list_free(lines);\n-- \n1.8.0\n"},{"id":"204296","messageId":"7vboegp04x.fsf@alter.siamese.dyndns.org","threadId":"32192","inReplyTo":"1353841721-16269-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T21:30:54Z","receivedAt":"2012-11-29T21:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> struct msg_data stored (char *, len) of the data to be included in a\n\nThat (, ) is a bit funny notation, even though it is\nunderstandable.\n\n> message, kept the character data NUL-terminated, etc., much like a\n> strbuf would do. So change it to use a struct strbuf. This makes the\n> code clearer and reduces copying a little bit.\n>\n> A side effect of this change is that the memory for each message is\n> freed after it is used rather than leaked, though that detail is\n> unimportant given that imap-send is a top-level command.\n>\n> --\n\n?\n\n> For some reason, there is a bunch of infrastructure in this file for\n> dealing with IMAP flags, although there is nothing in the code that\n> actually allows any flags to be set. If there is no plan to add\n> support for flags in the future, a bunch of code could be ripped out\n> and \"struct msg_data\" could be completely replaced with strbuf.\n\nYeah, after all these years we have kept the unused flags field\nthere and nobody needed anything out of it. I am OK with a removal\nif it is done at the very end of the series.\n\nThanks.\n"},{"id":"204297","messageId":"7v38zsozn7.fsf@alter.siamese.dyndns.org","threadId":"32192","inReplyTo":"1353841721-16269-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/8] Add function strbuf_addstr_xml_quoted() and more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T21:41:32Z","receivedAt":"2012-11-29T21:41:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> There were two functions doing almost the same XML quoting of\n> character entities, so implement a library function\n> strbuf_addstr_xml_quoted() and use that in both places.\n>\n> Along the way, do a lot of simplification within imap-send.c, which\n> was doing a lot of its own string management instead of using strbuf.\n\nOverall the series looked good to me. Thanks; will queue.\n"},{"id":"204302","messageId":"20121129234340.GA30107@sigill.intra.peff.net","threadId":"32192","inReplyTo":"7vboegp04x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-29T23:43:40Z","receivedAt":"2012-11-29T23:43:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 29, 2012 at 01:30:54PM -0800, Junio C Hamano wrote:\n\n> > For some reason, there is a bunch of infrastructure in this file for\n> > dealing with IMAP flags, although there is nothing in the code that\n> > actually allows any flags to be set. If there is no plan to add\n> > support for flags in the future, a bunch of code could be ripped out\n> > and \"struct msg_data\" could be completely replaced with strbuf.\n> \n> Yeah, after all these years we have kept the unused flags field\n> there and nobody needed anything out of it. I am OK with a removal\n> if it is done at the very end of the series.\n\nThere's a bunch of unused junk in imap-send. The original implementation\ncopied a bunch of code from isync, a much more full-featured imap\nclient, and the result ended up way more complex than it needed to be. I\nhave ripped a few things out over the years when they cause a problem\n(e.g., portability of /dev/urandom, conflict over the name \"struct\nstring_list\"), but have mostly let it be out of a vague sense that we\nmight one day want to pull bugfixes from isync upstream.\n\nThat has not happened once in the last six years, though, and I would\ndoubt that a straightforward merge would work after so many years. So\nripping out and refactoring the code in the name of maintainability is\nprobably a good thing at this point.\n\n-Peff\n"},{"id":"204339","messageId":"50B8B66F.3090300@alum.mit.edu","threadId":"32192","inReplyTo":"7vboegp04x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-30T13:36:47Z","receivedAt":"2012-11-30T13:36:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/29/2012 10:30 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> struct msg_data stored (char *, len) of the data to be included in a\n> \n> That (, ) is a bit funny notation, even though it is\n> understandable.\n\nI understand that it is funny, but it seems like the clearest way to\nexpress what is meant in a way that fits in the summary line. Feel free\nto change it if you like.\n\n>> message, kept the character data NUL-terminated, etc., much like a\n>> strbuf would do. So change it to use a struct strbuf. This makes the\n>> code clearer and reduces copying a little bit.\n>>\n>> A side effect of this change is that the memory for each message is\n>> freed after it is used rather than leaked, though that detail is\n>> unimportant given that imap-send is a top-level command.\n>>\n>> --\n> \n> ?\n\nIf by \"?\" you are wondering where the memory leak was, it was:\n\n* The while loop in main() called split_msg()\n\n * split_msg() cleared the msg_data structure using\n memset(msg, 0, sizeof *msg)\n\n * split_msg() copied the first message out of all_msgs using\n xmemdupz() and stored the result to msg->data\n\n* The msg_data was passed to imap_store_msg(). Its contents were\n copied to cb.data (which will be freed in the imap functions) but\n the original was left unfreed.\n\n* The next time through the loop, split_msg() zeroed the msg_data\n structure again, thus discarding the pointer to the xmemdupz()ed\n memory.\n\nThe leak caused more memory than necessary to be allocated (worst case:\nnearly the total size of all_msgs). But (a) all_msgs is already stored\nin memory, so the wastage is at most a factor of 2; and (b) this all\nhappens in main() shortly before program exit erases all sins.\n\nI didn't bother documenting this in the commit message because the patch\nchanges the code anyway, but feel free to add the above explanation to\nthe commit message if you think it is useful.\n\n>> For some reason, there is a bunch of infrastructure in this file for\n>> dealing with IMAP flags, although there is nothing in the code that\n>> actually allows any flags to be set. If there is no plan to add\n>> support for flags in the future, a bunch of code could be ripped out\n>> and \"struct msg_data\" could be completely replaced with strbuf.\n> \n> Yeah, after all these years we have kept the unused flags field\n> there and nobody needed anything out of it. I am OK with a removal\n> if it is done at the very end of the series.\n\nI don't think the removal of flags needs to be part of the same series.\n I suggest a separate patch series dedicated to deleting *all* the extra\nimap infrastructure at once. That being said, I'm not committing to do\nso. (We could add it to an \"straightforward projects for aspiring git\ndevelopers\" list, if we had such a thing.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"204340","messageId":"50B8B73A.4060801@alum.mit.edu","threadId":"32192","inReplyTo":"7v7gp4p00u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/8] wrap_in_html(): process message in bulk rather than line-by-line","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-30T13:40:10Z","receivedAt":"2012-11-30T13:40:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/29/2012 10:33 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Now that we can xml-quote an arbitrary string in O(N), there is no\n>> reason to process the message line by line. This change saves lots of\n>> memory allocations and copying.\n>>\n>> The old code would have created invalid output for a malformed input\n>> message (one that does not contain a blank line separating the header\n>> from the body). The new code die()s in this situation.\n> \n> Given that imap-send is about sending a patch the distinction would\n> not matter in practice, but isn't the difference between the two\n> that the new version would not allow sending a header-only message\n> without a body, while the old one allowed it?\n\nI was thinking that the end-of-header line is a required part of an\nRFC2282 email message, but I was wrong. If you squash the attached\npatch onto this commit, it will handle emails without bodies correctly.\n\nNevertheless, the old code was even *more* broken because it added a\n\"\" regardless of whether the separator line had been seen, and\ntherefore a message without an end-of-header line would come out like\n\n Header1: foo\n Header2: bar\n \n\nwith no content_type line, no pre_open, and appended to the\nheader without a blank line in between. This is the \"invalid output\"\nthat I was referring to.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n\n\ndiff --git a/imap-send.c b/imap-send.c\nindex eec9e35..e521e2f 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1348,7 +1348,7 @@ static void wrap_in_html(struct strbuf *msg)\n \tconst char *body = strstr(msg->buf, \"\\n\\n\");\n \n \tif (!body)\n-\t\tdie(\"malformed message\");\n+\t\treturn; /* Headers but no body; no wrapping needed */\n \n \tbody += 2;\n \n"},{"id":"204400","messageId":"7v624lns00.fsf@alter.siamese.dyndns.org","threadId":"32192","inReplyTo":"50B8B66F.3090300@alum.mit.edu","subject":"Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-02T01:48:47Z","receivedAt":"2012-12-02T01:48:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 11/29/2012 10:30 PM, Junio C Hamano wrote:\n> \n>>> A side effect of this change is that the memory for each message is\n>>> freed after it is used rather than leaked, though that detail is\n>>> unimportant given that imap-send is a top-level command.\n>>>\n>>> --\n>> \n>> ?\n>\n> If by \"?\" you are wondering where the memory leak was, it was:\n\nNo, I was wondering if you meant to say \"---\" to mark te remainder\nof what you wrote does not exactly belong to the log message.\n\n>>> For some reason, there is a bunch of infrastructure in this file for\n>>> dealing with IMAP flags, although there is nothing in the code that\n>>> actually allows any flags to be set. If there is no plan to add\n>>> support for flags in the future, a bunch of code could be ripped out\n>>> and \"struct msg_data\" could be completely replaced with strbuf.\n>> \n>> Yeah, after all these years we have kept the unused flags field\n>> there and nobody needed anything out of it. I am OK with a removal\n>> if it is done at the very end of the series.\n>\n> I don't think the removal of flags needs to be part of the same series.\n\nOh, I did not think so, either.\n\n> I suggest a separate patch series dedicated to deleting *all* the extra\n> imap infrastructure at once. That being said, I'm not committing to do\n> so. (We could add it to an \"straightforward projects for aspiring git\n> developers\" list, if we had such a thing.)\n\nA \"low-hanging fruit and/or janitorial work\" stack may be worth\nhaving.\n"},{"id":"204414","messageId":"50BAEF28.6000400@alum.mit.edu","threadId":"32192","inReplyTo":"7v624lns00.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-12-02T06:03:20Z","receivedAt":"2012-12-02T06:03:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/02/2012 02:48 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> On 11/29/2012 10:30 PM, Junio C Hamano wrote:\n>>\n>>>> A side effect of this change is that the memory for each message is\n>>>> freed after it is used rather than leaked, though that detail is\n>>>> unimportant given that imap-send is a top-level command.\n>>>>\n>>>> --\n>>>\n>>> ?\n>>\n>> If by \"?\" you are wondering where the memory leak was, it was:\n> \n> No, I was wondering if you meant to say \"---\" to mark te remainder\n> of what you wrote does not exactly belong to the log message.\n\nOh. Yes, that was my intention.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"204418","messageId":"7vpq2slsb4.fsf@alter.siamese.dyndns.org","threadId":"32192","inReplyTo":"50B8B73A.4060801@alum.mit.edu","subject":"Re: [PATCH 8/8] wrap_in_html(): process message in bulk rather than line-by-line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-02T09:25:03Z","receivedAt":"2012-12-02T09:25:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Nevertheless, the old code was even *more* broken because it added a\n> \"\" regardless of whether the separator line had been seen,...\n\nOK. I'll rewrite the tail-end of the original log message to read:\n\n The old code would have created invalid output when there was no\n body, emitting a closing without a blank line nor an opening\n 
 after the header.  The new code simply returns in this\n    situation without doing harm (even though either would not make much\n    sense in the context of imap-send that is meant to send out patches).\n\nand squash this in.\n\nThanks.\n"},{"id":"204421","messageId":"50BB2EF1.5020003@alum.mit.edu","threadId":"32192","inReplyTo":"7vpq2slsb4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/8] wrap_in_html(): process message in bulk rather than line-by-line","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-12-02T10:35:29Z","receivedAt":"2012-12-02T10:35:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/02/2012 10:25 AM, Junio C Hamano wrote:\n> Michael Haggerty  writes:\n> \n>> Nevertheless, the old code was even *more* broken because it added a\n>> \"
\" regardless of whether the separator line had been seen,...\n> \n> OK. I'll rewrite the tail-end of the original log message to read:\n> \n> The old code would have created invalid output when there was no\n> body, emitting a closing without a blank line nor an opening\n> 
 after the header.  The new code simply returns in this\n>     situation without doing harm (even though either would not make much\n>     sense in the context of imap-send that is meant to send out patches).\n> \n> and squash this in.\n\nACK.  Thanks.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"204449","messageId":"CACnwZYdUN+8iubjFsLMbQvEdYjvZFj_XM+oAeTsUe0EUoCwm_g@mail.gmail.com","threadId":"32192","inReplyTo":"7v624lns00.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2012-12-03T15:06:32Z","receivedAt":"2012-12-03T15:06:32Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Sat, Dec 1, 2012 at 11:48 PM, Junio C Hamano  wrote:\n>> I suggest a separate patch series dedicated to deleting *all* the extra\n>> imap infrastructure at once.  That being said, I'm not committing to do\n>> so.  (We could add it to an \"straightforward projects for aspiring git\n>> developers\" list, if we had such a thing.)\n>\n> A \"low-hanging fruit and/or janitorial work\" stack may be worth\n> having.\n\nThat would be good for not so versed developers, I think. Do we have a\nplace for listing janitor projects?\n"}]}