{"thread":{"id":"29144","subject":"[PATCH resend] Do not create commits whose message contains NUL","startedAt":"2011-12-13T11:56:08Z","lastAt":"2012-01-03T20:03:31Z","messageCount":23,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Miles Bader","Junio C Hamano","Drew Northup"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181030","messageId":"1323777368-19697-1-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":null,"subject":"[PATCH resend] Do not create commits whose message contains NUL","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-13T11:56:08Z","receivedAt":"2011-12-13T11:56:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We assume that the commit log messages are uninterpreted sequences of\nnon-NUL bytes (see Documentation/i18n.txt). However the assumption\ndoes not really stand out and it's quite easy to set an editor to save\nin a NUL-included encoding. Currently we silently cut at the first NUL\nwe see.\n\nMake it more obvious that NUL is not welcome by refusing to create\nsuch commits. Those who deliberately want to create them can still do\nwith hash-object.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n This is from UTF-16 in commit message discussion [1] a few months\n ago. I don't want to resurrect the discussion again. However I think\n it's a good idea to stop users from shooting themselves in this case,\n especially at porcelain level.\n \n There were no comments on this patch previously. So, any comments\n this time ? Should I drop it?\n\n [1] http://thread.gmane.org/gmane.comp.version-control.git/184123/focus=184335\n commit.c               |    3 +++\n t/t3900-i18n-commit.sh |    6 ++++++\n t/t3900/UTF-16.txt     |  Bin 0 -> 32 bytes\n 3 files changed, 9 insertions(+), 0 deletions(-)\n create mode 100644 t/t3900/UTF-16.txt\n\ndiff --git a/commit.c b/commit.c\nindex d67b8c7..0775eec 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -855,6 +855,9 @@ int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,\n \n \tassert_sha1_type(tree, OBJ_TREE);\n \n+\tif (strlen(msg) < msg_len)\n+\t\tdie(_(\"cannot commit with NUL in commit message\"));\n+\n \t/* Not having i18n.commitencoding is the same as having utf-8 */\n \tencoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n \ndiff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\nindex 1f62c15..d48a7c0 100755\n--- a/t/t3900-i18n-commit.sh\n+++ b/t/t3900-i18n-commit.sh\n@@ -34,6 +34,12 @@ test_expect_success 'no encoding header for base case' '\n \ttest z = \"z$E\"\n '\n \n+test_expect_failure 'UTF-16 refused because of NULs' '\n+\techo UTF-16 >F &&\n+\tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt\n+'\n+\n+\n for H in ISO8859-1 eucJP ISO-2022-JP\n do\n \ttest_expect_success \"$H setup\" '\ndiff --git a/t/t3900/UTF-16.txt b/t/t3900/UTF-16.txt\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..53296be684253f40964c0604be7fa7ff12e200cb\nGIT binary patch\nliteral 32\nmcmezOpWz6@X@-jo=NYasZ~@^#h9rjP3@HpR7}6Nh8Mpw;r3yp<\n\nliteral 0\nHcmV?d00001\n\n-- \n1.7.8.36.g69ee2\n"},{"id":"181052","messageId":"20111213175932.GA1663@sigill.intra.peff.net","threadId":"29144","inReplyTo":"1323777368-19697-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH resend] Do not create commits whose message contains NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T17:59:32Z","receivedAt":"2011-12-13T17:59:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 06:56:08PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> We assume that the commit log messages are uninterpreted sequences of\n> non-NUL bytes (see Documentation/i18n.txt). However the assumption\n> does not really stand out and it's quite easy to set an editor to save\n> in a NUL-included encoding. Currently we silently cut at the first NUL\n> we see.\n> \n> Make it more obvious that NUL is not welcome by refusing to create\n> such commits. Those who deliberately want to create them can still do\n> with hash-object.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  This is from UTF-16 in commit message discussion [1] a few months\n>  ago. I don't want to resurrect the discussion again. However I think\n>  it's a good idea to stop users from shooting themselves in this case,\n>  especially at porcelain level.\n>\n>  There were no comments on this patch previously. So, any comments\n>  this time ? Should I drop it?\n\nI think this is a sane thing to do. Having thought about and\nexperimented a little with utf-16 in the past few months, I really don't\nsee how you could be disrupting anybody's workflow. utf-16 messages get\nbutchered so badly already; we are much better off letting the user know\nof the problem as soon as possible.\n\nIt looks like we already have a check for is_utf8, and this is not\nfailing that check. I guess because is_utf8 takes a NUL-terminated\nbuffer, so it simply sees the truncated result (i.e., depending on\nendianness, \"foo\" in utf16 is something like \"f\\0o\\0o\\0\", so we check\nonly \"f\"). We could make is_utf8 take a length parameter to be more\naccurate, and then it would catch this.\n\nHowever, I think that's not quite what we want. We only check is_utf8 if\nthe encoding field is not set. And really, we want to reject NULs no\nmatter _which_ encoding they've set, because git simply doesn't handle\nthem properly.\n\n> diff --git a/commit.c b/commit.c\n> index d67b8c7..0775eec 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -855,6 +855,9 @@ int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,\n\nHmm. My version of commit_tree does not have a \"msg_len\" parameter, nor\ndo I have d67b8c7. Is there some refactoring patch this is based on that\nI missed?\n\n> +\tif (strlen(msg) < msg_len)\n> +\t\tdie(_(\"cannot commit with NUL in commit message\"));\n> +\n\nTwo nits:\n\n  1. For some reason, checking strlen(msg) seems a subtle way of looking\n     for NULs in a buffer. I would have found:\n\n         if (memchr(msg, '\\0', msglen))\n\n     much more obvious. But perhaps it is just me. Certainly not a big\n     deal either way.\n\n  2. The error message could be a little friendlier. The likely reason\n     for NULs is a bogus encoding setting in the user's editor. We\n     already have a nice \"your message isn't utf-8\" message. Though it\n     does suggest setting i18n.commitencoding, which probably _isn't_\n     the solution here (since their encoding clearly isn't supported).\n     But maybe it would be nicer to say something like:\n\n       error: your commit message contains NUL characters.\n       hint: This is often caused by using multibyte encodings such as\n       hint: UTF-16. Please check your editor settings.\n\n     We could even go further and detect some common NUL-containing\n     encodings, but I don't think it's worth the effort.\n\n> diff --git a/t/t3900/UTF-16.txt b/t/t3900/UTF-16.txt\n> new file mode 100644\n> index 0000000000000000000000000000000000000000..53296be684253f40964c0604be7fa7ff12e200cb\n> GIT binary patch\n> literal 32\n> mcmezOpWz6@X@-jo=NYasZ~@^#h9rjP3@HpR7}6Nh8Mpw;r3yp<\n\nI was disappointed not to find a secret message. :)\n\n-Peff\n"},{"id":"181113","messageId":"buomxavwwtq.fsf@dhlpc061.dev.necel.com","threadId":"29144","inReplyTo":"20111213175932.GA1663@sigill.intra.peff.net","subject":"Re: [PATCH resend] Do not create commits whose message contains NUL","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2011-12-14T05:23:29Z","receivedAt":"2011-12-14T05:23:29Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Jeff King <peff@peff.net> writes:\n>      But maybe it would be nicer to say something like:\n>\n>        error: your commit message contains NUL characters.\n>        hint: This is often caused by using multibyte encodings such as\n>        hint: UTF-16. Please check your editor settings.\n\nI think the error message with the hint is much better for users, but\nisn't the term \"multibyte\" a little misleading here?  It seems like\nit's really _wide_ encodings that are generally the culprit.\n\n[UTF-16 of course is particularly nasty in that it uses both units which\nare wider than a byte (\"wide\"), _and_ multiple units per code-point....]\n\nThanks,\n\n-Miles\n\n-- \nQuack, n. A murderer without a license.\n"},{"id":"181115","messageId":"20111214071755.GA19945@sigill.intra.peff.net","threadId":"29144","inReplyTo":"buomxavwwtq.fsf@dhlpc061.dev.necel.com","subject":"Re: [PATCH resend] Do not create commits whose message contains NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T07:17:55Z","receivedAt":"2011-12-14T07:17:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 02:23:29PM +0900, Miles Bader wrote:\n\n> Jeff King <peff@peff.net> writes:\n> >      But maybe it would be nicer to say something like:\n> >\n> >        error: your commit message contains NUL characters.\n> >        hint: This is often caused by using multibyte encodings such as\n> >        hint: UTF-16. Please check your editor settings.\n> \n> I think the error message with the hint is much better for users, but\n> isn't the term \"multibyte\" a little misleading here?  It seems like\n> it's really _wide_ encodings that are generally the culprit.\n\nYeah, wide is probably a better term. I'm not sure it is rigorously\ndefined anywhere, but in general I think it refers to the set of\nencodings that do not care about the embedding of 8-bit ascii bytes as\nsubsets.\n\n-Peff\n"},{"id":"181134","messageId":"1323871699-8839-1-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323777368-19697-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 0/3] git-commit rejects messages with NULs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-14T14:08:16Z","receivedAt":"2011-12-14T14:08:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Hi,\n\nI'm sorry I forgot the patch that makes commit_tree() take message\nlength. This version rewords the error message and use memchr()\ninstead.\n\nNguyễn Thái Ngọc Duy (3):\n  Make commit_tree() take message length in addition to the commit\n    message\n  merge: abort if fails to commit\n  Do not create commits whose message contains NUL\n\n Documentation/config.txt |    4 ++++\n advice.c                 |    2 ++\n advice.h                 |    1 +\n builtin/commit-tree.c    |    2 +-\n builtin/commit.c         |    2 +-\n builtin/merge.c          |    8 ++++++--\n builtin/notes.c          |    2 +-\n commit.c                 |   13 +++++++++++--\n commit.h                 |    2 +-\n notes-cache.c            |    2 +-\n notes-merge.c            |    8 ++++----\n notes-merge.h            |    2 +-\n t/t3900-i18n-commit.sh   |    6 ++++++\n t/t3900/UTF-16.txt       |  Bin 0 -> 32 bytes\n 14 files changed, 40 insertions(+), 14 deletions(-)\n create mode 100644 t/t3900/UTF-16.txt\n\n-- \n1.7.8.36.g69ee2\n"},{"id":"181136","messageId":"1323871699-8839-2-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323871699-8839-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/3] Make commit_tree() take message length in addition to the commit message","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-14T14:08:17Z","receivedAt":"2011-12-14T14:08:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/commit-tree.c |    2 +-\n builtin/commit.c      |    2 +-\n builtin/merge.c       |    4 ++--\n builtin/notes.c       |    2 +-\n commit.c              |    4 ++--\n commit.h              |    2 +-\n notes-cache.c         |    2 +-\n notes-merge.c         |    8 ++++----\n notes-merge.h         |    2 +-\n 9 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex d083795..8fa384f 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -56,7 +56,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \tif (strbuf_read(&buffer, 0, 0) < 0)\n \t\tdie_errno(\"git commit-tree: failed to read\");\n \n-\tif (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {\n+\tif (commit_tree(buffer.buf, buffer.len, tree_sha1, parents, commit_sha1, NULL)) {\n \t\tstrbuf_release(&buffer);\n \t\treturn 1;\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8f2bebe..ce0e47f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,7 +1483,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\texit(1);\n \t}\n \n-\tif (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,\n+\tif (commit_tree(sb.buf, sb.len, active_cache_tree->sha1, parents, sha1,\n \t\t\tauthor_ident.buf)) {\n \t\trollback_index_files();\n \t\tdie(_(\"failed to write commit object\"));\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2870a6a..df4548a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -913,7 +913,7 @@ static int merge_trivial(struct commit *head)\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n \tprepare_to_commit();\n-\tcommit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);\n+\tcommit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);\n \tfinish(head, result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\n@@ -944,7 +944,7 @@ static int finish_automerge(struct commit *head,\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit();\n \tfree_commit_list(remoteheads);\n-\tcommit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);\n+\tcommit_tree(merge_msg.buf, merge_msg.len, result_tree, parents, result_commit, NULL);\n \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n \tfinish(head, result_commit, buf.buf);\n \tstrbuf_release(&buf);\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f8e437d..d665459 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -306,7 +306,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \tif (buf.buf[buf.len - 1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n'); /* Make sure msg ends with newline */\n \n-\tcreate_notes_commit(t, NULL, buf.buf + 7, commit_sha1);\n+\tcreate_notes_commit(t, NULL, buf.buf + 7, buf.len - 7, commit_sha1);\n \tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\ndiff --git a/commit.c b/commit.c\nindex 73b7e00..d67b8c7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -845,7 +845,7 @@ static const char commit_utf8_warn[] =\n \"You may want to amend it after fixing the message, or set the config\\n\"\n \"variable i18n.commitencoding to the encoding your project uses.\\n\";\n \n-int commit_tree(const char *msg, unsigned char *tree,\n+int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,\n \t\tstruct commit_list *parents, unsigned char *ret,\n \t\tconst char *author)\n {\n@@ -884,7 +884,7 @@ int commit_tree(const char *msg, unsigned char *tree,\n \tstrbuf_addch(&buffer, '\\n');\n \n \t/* And add the comment */\n-\tstrbuf_addstr(&buffer, msg);\n+\tstrbuf_add(&buffer, msg, msg_len);\n \n \t/* And check the encoding */\n \tif (encoding_is_utf8 && !is_utf8(buffer.buf))\ndiff --git a/commit.h b/commit.h\nindex 009b113..1acaf53 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -181,7 +181,7 @@ static inline int single_parent(struct commit *commit)\n \n struct commit_list *reduce_heads(struct commit_list *heads);\n \n-extern int commit_tree(const char *msg, unsigned char *tree,\n+extern int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,\n \t\tstruct commit_list *parents, unsigned char *ret,\n \t\tconst char *author);\n \ndiff --git a/notes-cache.c b/notes-cache.c\nindex 4c8984e..04a5698 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -56,7 +56,7 @@ int notes_cache_write(struct notes_cache *c)\n \n \tif (write_notes_tree(&c->tree, tree_sha1))\n \t\treturn -1;\n-\tif (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL) < 0)\n+\tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL, commit_sha1, NULL) < 0)\n \t\treturn -1;\n \tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n \t\t       0, QUIET_ON_ERR) < 0)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex ce10aac..b3baaf4 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -530,7 +530,7 @@ static int merge_from_diffs(struct notes_merge_options *o,\n }\n \n void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n-\t\t\t const char *msg, unsigned char *result_sha1)\n+\t\t\t const char *msg, size_t msg_len, unsigned char *result_sha1)\n {\n \tunsigned char tree_sha1[20];\n \n@@ -551,7 +551,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n \t\t/* else: t->ref points to nothing, assume root/orphan commit */\n \t}\n \n-\tif (commit_tree(msg, tree_sha1, parents, result_sha1, NULL))\n+\tif (commit_tree(msg, msg_len, tree_sha1, parents, result_sha1, NULL))\n \t\tdie(\"Failed to commit notes tree to database\");\n }\n \n@@ -669,7 +669,7 @@ int notes_merge(struct notes_merge_options *o,\n \t\tcommit_list_insert(remote, &parents); /* LIFO order */\n \t\tcommit_list_insert(local, &parents);\n \t\tcreate_notes_commit(local_tree, parents, o->commit_msg.buf,\n-\t\t\t\t    result_sha1);\n+\t\t\t\t    o->commit_msg.len, result_sha1);\n \t}\n \n found_result:\n@@ -734,7 +734,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t}\n \n \tcreate_notes_commit(partial_tree, partial_commit->parents, msg,\n-\t\t\t    result_sha1);\n+\t\t\t    strlen(msg), result_sha1);\n \tif (o->verbosity >= 4)\n \t\tprintf(\"Finalized notes merge commit: %s\\n\",\n \t\t\tsha1_to_hex(result_sha1));\ndiff --git a/notes-merge.h b/notes-merge.h\nindex 168a672..fd52988 100644\n--- a/notes-merge.h\n+++ b/notes-merge.h\n@@ -37,7 +37,7 @@ void init_notes_merge_options(struct notes_merge_options *o);\n  * The resulting commit SHA1 is stored in result_sha1.\n  */\n void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n-\t\t\t const char *msg, unsigned char *result_sha1);\n+\t\t\t const char *msg, size_t msg_len, unsigned char *result_sha1);\n \n /*\n  * Merge notes from o->remote_ref into o->local_ref\n-- \n1.7.8.36.g69ee2\n"},{"id":"181135","messageId":"1323871699-8839-3-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323871699-8839-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] merge: abort if fails to commit","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-14T14:08:18Z","receivedAt":"2011-12-14T14:08:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/merge.c |    8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex df4548a..e57eefa 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -913,7 +913,9 @@ static int merge_trivial(struct commit *head)\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n \tprepare_to_commit();\n-\tcommit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);\n+\tif (commit_tree(merge_msg.buf, merge_msg.len,\n+\t\t\tresult_tree, parent, result_commit, NULL))\n+\t\tdie(_(\"failed to write commit object\"));\n \tfinish(head, result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\n@@ -944,7 +946,9 @@ static int finish_automerge(struct commit *head,\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit();\n \tfree_commit_list(remoteheads);\n-\tcommit_tree(merge_msg.buf, merge_msg.len, result_tree, parents, result_commit, NULL);\n+\tif (commit_tree(merge_msg.buf, merge_msg.len,\n+\t\t\tresult_tree, parents, result_commit, NULL))\n+\t\tdie(_(\"failed to write commit object\"));\n \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n \tfinish(head, result_commit, buf.buf);\n \tstrbuf_release(&buf);\n-- \n1.7.8.36.g69ee2\n"},{"id":"181137","messageId":"1323871699-8839-4-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323871699-8839-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-14T14:08:19Z","receivedAt":"2011-12-14T14:08:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We assume that the commit log messages are uninterpreted sequences of\nnon-NUL bytes (see Documentation/i18n.txt). However the assumption\ndoes not really stand out and it's quite easy to set an editor to save\nin a NUL-included encoding. Currently we silently cut at the first NUL\nwe see.\n\nMake it more obvious that NUL is not welcome by refusing to create\nsuch commits. Those who deliberately want to create them can still do\nwith hash-object.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/config.txt |    4 ++++\n advice.c                 |    2 ++\n advice.h                 |    1 +\n commit.c                 |    9 +++++++++\n t/t3900-i18n-commit.sh   |    6 ++++++\n t/t3900/UTF-16.txt       |  Bin 0 -> 32 bytes\n 6 files changed, 22 insertions(+), 0 deletions(-)\n create mode 100644 t/t3900/UTF-16.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5a841da..daf57c2 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -144,6 +144,10 @@ advice.*::\n \t\tAdvice shown when you used linkgit::git-checkout[1] to\n \t\tmove to the detach HEAD state, to instruct how to create\n \t\ta local branch after the fact.  Default: true.\n+\tcommitWideEncoding::\n+\t\tAdvice shown when linkgit::git-commit[1] refuses to\n+\t\tproceed because there are NULs in commit message.\n+\t\tDefault: true.\n --\n \n core.fileMode::\ndiff --git a/advice.c b/advice.c\nindex e02e632..130949e 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -6,6 +6,7 @@ int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n int advice_implicit_identity = 1;\n int advice_detached_head = 1;\n+int advice_commmit_wide_encoding = 1;\n \n static struct {\n \tconst char *name;\n@@ -17,6 +18,7 @@ static struct {\n \t{ \"resolveconflict\", &advice_resolve_conflict },\n \t{ \"implicitidentity\", &advice_implicit_identity },\n \t{ \"detachedhead\", &advice_detached_head },\n+\t{ \"commitwideencoding\", &advice_commmit_wide_encoding },\n };\n \n void advise(const char *advice, ...)\ndiff --git a/advice.h b/advice.h\nindex e5d0af7..d913bdb 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -9,6 +9,7 @@ extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\n extern int advice_implicit_identity;\n extern int advice_detached_head;\n+extern int advice_commmit_wide_encoding;\n \n int git_default_advice_config(const char *var, const char *value);\n void advise(const char *advice, ...);\ndiff --git a/commit.c b/commit.c\nindex d67b8c7..59e5bce 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -855,6 +855,15 @@ int commit_tree(const char *msg, size_t msg_len, unsigned char *tree,\n \n \tassert_sha1_type(tree, OBJ_TREE);\n \n+\tif (memchr(msg, '\\0', msg_len)) {\n+\t\terror(_(\"your commit message contains NUL characters.\"));\n+\t\tif (advice_commmit_wide_encoding) {\n+\t\t\tadvise(_(\"This is often caused by using wide encodings such as\"));\n+\t\t\tadvise(_(\"UTF-16. Please check your editor settings.\"));\n+\t\t}\n+\t\treturn -1;\n+\t}\n+\n \t/* Not having i18n.commitencoding is the same as having utf-8 */\n \tencoding_is_utf8 = is_encoding_utf8(git_commit_encoding);\n \ndiff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\nindex 1f62c15..d48a7c0 100755\n--- a/t/t3900-i18n-commit.sh\n+++ b/t/t3900-i18n-commit.sh\n@@ -34,6 +34,12 @@ test_expect_success 'no encoding header for base case' '\n \ttest z = \"z$E\"\n '\n \n+test_expect_failure 'UTF-16 refused because of NULs' '\n+\techo UTF-16 >F &&\n+\tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt\n+'\n+\n+\n for H in ISO8859-1 eucJP ISO-2022-JP\n do\n \ttest_expect_success \"$H setup\" '\ndiff --git a/t/t3900/UTF-16.txt b/t/t3900/UTF-16.txt\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..53296be684253f40964c0604be7fa7ff12e200cb\nGIT binary patch\nliteral 32\nmcmezOpWz6@X@-jo=NYasZ~@^#h9rjP3@HpR7}6Nh8Mpw;r3yp<\n\nliteral 0\nHcmV?d00001\n\n-- \n1.7.8.36.g69ee2\n"},{"id":"181177","messageId":"7v8vmfqayx.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"1323871699-8839-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/3] Make commit_tree() take message length in addition to the commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-14T18:12:06Z","receivedAt":"2011-12-14T18:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nJustification?\n\nAs all 3 primary users of this API feed strbuf.buf to the function, it\nwould make more sense to change the first parameter to a pointer to a\nstrbuf, no?\n"},{"id":"181178","messageId":"7v4nx3qawc.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"1323871699-8839-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/3] merge: abort if fails to commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-14T18:13:39Z","receivedAt":"2011-12-14T18:13:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/merge.c |    8 ++++++--\n>  1 files changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index df4548a..e57eefa 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -913,7 +913,9 @@ static int merge_trivial(struct commit *head)\n>  \tparent->next->item = remoteheads->item;\n>  \tparent->next->next = NULL;\n>  \tprepare_to_commit();\n> -\tcommit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);\n> +\tif (commit_tree(merge_msg.buf, merge_msg.len,\n> +\t\t\tresult_tree, parent, result_commit, NULL))\n> +\t\tdie(_(\"failed to write commit object\"));\n>  \tfinish(head, result_commit, \"In-index merge\");\n>  \tdrop_save();\n>  \treturn 0;\n\nShould we die immediately, or should we do some clean-ups after ourselves\nbefore doing so?\n\nIn any case, this is a good change that shouldn't be taken hostage to the\nunrelated change in patch [1/3].\n\nThanks.\n\n> @@ -944,7 +946,9 @@ static int finish_automerge(struct commit *head,\n>  \tstrbuf_addch(&merge_msg, '\\n');\n>  \tprepare_to_commit();\n>  \tfree_commit_list(remoteheads);\n> -\tcommit_tree(merge_msg.buf, merge_msg.len, result_tree, parents, result_commit, NULL);\n> +\tif (commit_tree(merge_msg.buf, merge_msg.len,\n> +\t\t\tresult_tree, parents, result_commit, NULL))\n> +\t\tdie(_(\"failed to write commit object\"));\n>  \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n>  \tfinish(head, result_commit, buf.buf);\n>  \tstrbuf_release(&buf);\n"},{"id":"181180","messageId":"7vzkevow2j.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"1323871699-8839-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-14T18:19:16Z","receivedAt":"2011-12-14T18:19:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> We assume that the commit log messages are uninterpreted sequences of\n> non-NUL bytes (see Documentation/i18n.txt). However the assumption\n> does not really stand out and it's quite easy to set an editor to save\n> in a NUL-included encoding. Currently we silently cut at the first NUL\n> we see.\n>\n> Make it more obvious that NUL is not welcome by refusing to create\n> such commits. Those who deliberately want to create them can still do\n> with hash-object.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nLimiting the Porcelain layer to deal only with reasonable text encodings\n(yes, I am declaring that utf16 is not among them) is perfectly fine, but\nI was somehow hoping that you would allow the option for the low-level\nfunction commit_tree() to create a commit object with binary blob in the\nbody part, especially after seeing the patch 1/3 to do so.\n\nCertainly that kind of usage would not give the binary blob literally in\n\"git log\" output, but it is with or without the issue around NUL byte. A\ncustom program linked with commit.c to call commit_tree() may not be using\nthe data structure to store anything that is meant to be read by \"git log\"\nto begin with.\n\nNot a strong veto at all, just throwing out something to think about.\n"},{"id":"181181","messageId":"20111214182953.GA6469@sigill.intra.peff.net","threadId":"29144","inReplyTo":"7vzkevow2j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T18:29:53Z","receivedAt":"2011-12-14T18:29:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 10:19:16AM -0800, Junio C Hamano wrote:\n\n> Limiting the Porcelain layer to deal only with reasonable text encodings\n> (yes, I am declaring that utf16 is not among them) is perfectly fine, but\n> I was somehow hoping that you would allow the option for the low-level\n> function commit_tree() to create a commit object with binary blob in the\n> body part, especially after seeing the patch 1/3 to do so.\n> \n> Certainly that kind of usage would not give the binary blob literally in\n> \"git log\" output, but it is with or without the issue around NUL byte. A\n> custom program linked with commit.c to call commit_tree() may not be using\n> the data structure to store anything that is meant to be read by \"git log\"\n> to begin with.\n\nI'm happy to ignore custom programs linking against internal git code,\nbut what should \"git commit-tree\" do?\n\nMy gut feeling is that it should store the literal binary contents.\nHowever, I don't think this has ever been the case. Even in the initial\nversion of commit-tree.c, we read the input line-by-line and sprintf it\ninto place.\n\n-Peff\n"},{"id":"181199","messageId":"CADCnXoaqEXJV+Mb1=nQge_bjA3H6R7=BPt213CKLX55zyTHEtg@mail.gmail.com","threadId":"29144","inReplyTo":"1323871699-8839-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2011-12-15T01:04:06Z","receivedAt":"2011-12-15T01:04:06Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"> +       commitWideEncoding::\n> +               Advice shown when linkgit::git-commit[1] refuses to\n> +               proceed because there are NULs in commit message.\n> +               Default: true.\n\nAlthough \"wide encoding\" is a reasonable guess at cause of embedded\nzero characters (and so a useful term for diagnostic messages, as it\ncan help users identify the problem in their environment which is\ncausing such zero bytes), it's really only a guess in most cases...\n\nShouldn't the variable be named based on what it actually does, which\nis allow zero-bytes in commit messages...?\n\n-Miles\n\n-- \nCat is power.  Cat is peace.\n"},{"id":"181200","messageId":"20111215011855.GA24568@sigill.intra.peff.net","threadId":"29144","inReplyTo":"CADCnXoaqEXJV+Mb1=nQge_bjA3H6R7=BPt213CKLX55zyTHEtg@mail.gmail.com","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-15T01:18:55Z","receivedAt":"2011-12-15T01:18:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 15, 2011 at 10:04:06AM +0900, Miles Bader wrote:\n\n> > +       commitWideEncoding::\n> > +               Advice shown when linkgit::git-commit[1] refuses to\n> > +               proceed because there are NULs in commit message.\n> > +               Default: true.\n> \n> Although \"wide encoding\" is a reasonable guess at cause of embedded\n> zero characters (and so a useful term for diagnostic messages, as it\n> can help users identify the problem in their environment which is\n> causing such zero bytes), it's really only a guess in most cases...\n> \n> Shouldn't the variable be named based on what it actually does, which\n> is allow zero-bytes in commit messages...?\n\nI agree, but...\n\nReally this variable is overkill. The advice.* subsystem is for\nsilencing hints and warnings from git that you see repeatedly because\nyou are smarter than git, and want to ignore its advice.\n\nBut in this case, I don't see a user saying \"stupid git, of _course_ I\nwant to commit NULs. Stop nagging me\". Especially because it is not a\nwarning, but a fatal error. :)\n\nSo yes, it's verbose, but no, it's not something somebody is going to be\nso bothered by that they will find the config option to turn it off.\nInstead, they will stop doing the bad thing and never see it again.  At\nbest this config option is useless, and at worst it clutters the\nadvice.* namespace, making it harder for people to find the advice\noption they _do_ want to turn off).\n\nPerhaps it should just be dropped.\n\n-Peff\n"},{"id":"181203","messageId":"7vobvapm3l.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"20111215011855.GA24568@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T03:09:18Z","receivedAt":"2011-12-15T03:09:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Perhaps it should just be dropped.\n\nThanks. I have nothing to add---you said everything that needs to be said.\n"},{"id":"181225","messageId":"1323956843-5326-1-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323871699-8839-2-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/3] merge: abort if fails to commit","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-15T13:47:21Z","receivedAt":"2011-12-15T13:47:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n 2011/12/15 Junio C Hamano <gitster@pobox.com>:\n >> -     commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);\n >> +     if (commit_tree(merge_msg.buf, merge_msg.len,\n >> +                     result_tree, parent, result_commit, NULL))\n >> +             die(_(\"failed to write commit object\"));\n >>       finish(head, result_commit, \"In-index merge\");\n >>       drop_save();\n >>       return 0;\n >\n > Should we die immediately, or should we do some clean-ups after ourselves\n > before doing so?\n\n I'm not sure. I had a quick look over the command and it seems we do\n not need to do any clean-ups. But I'm not familiar with the command\n anyway..\n\n > In any case, this is a good change that shouldn't be taken hostage to the\n > unrelated change in patch [1/3].\n\n Moved it up so it you can cherry-pick it independently.\n\n builtin/merge.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2870a6a..27576c0 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -913,7 +913,8 @@ static int merge_trivial(struct commit *head)\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n \tprepare_to_commit();\n-\tcommit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);\n+\tif (commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL))\n+\t\tdie(_(\"failed to write commit object\"));\n \tfinish(head, result_commit, \"In-index merge\");\n \tdrop_save();\n \treturn 0;\n@@ -944,7 +945,8 @@ static int finish_automerge(struct commit *head,\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit();\n \tfree_commit_list(remoteheads);\n-\tcommit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);\n+\tif (commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL))\n+\t\tdie(_(\"failed to write commit object\"));\n \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n \tfinish(head, result_commit, buf.buf);\n \tstrbuf_release(&buf);\n-- \n1.7.8.36.g69ee2\n"},{"id":"181226","messageId":"1323956843-5326-2-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323956843-5326-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] Convert commit_tree() to take strbuf as message","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-15T13:47:22Z","receivedAt":"2011-12-15T13:47:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Because strbuf provides message length, we can create commits that\ninclude NULs.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/commit-tree.c |    2 +-\n builtin/commit.c      |    2 +-\n builtin/merge.c       |    4 ++--\n builtin/notes.c       |    4 ++--\n commit.c              |    4 ++--\n commit.h              |    2 +-\n notes-cache.c         |    5 ++++-\n notes-merge.c         |   10 ++++++----\n notes-merge.h         |    2 +-\n 9 files changed, 20 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex d083795..0895861 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -56,7 +56,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \tif (strbuf_read(&buffer, 0, 0) < 0)\n \t\tdie_errno(\"git commit-tree: failed to read\");\n \n-\tif (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {\n+\tif (commit_tree(&buffer, tree_sha1, parents, commit_sha1, NULL)) {\n \t\tstrbuf_release(&buffer);\n \t\treturn 1;\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8f2bebe..849151e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,7 +1483,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\texit(1);\n \t}\n \n-\tif (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,\n+\tif (commit_tree(&sb, active_cache_tree->sha1, parents, sha1,\n \t\t\tauthor_ident.buf)) {\n \t\trollback_index_files();\n \t\tdie(_(\"failed to write commit object\"));\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 27576c0..e066bf1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -913,7 +913,7 @@ static int merge_trivial(struct commit *head)\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n \tprepare_to_commit();\n-\tif (commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL))\n+\tif (commit_tree(&merge_msg, result_tree, parent, result_commit, NULL))\n \t\tdie(_(\"failed to write commit object\"));\n \tfinish(head, result_commit, \"In-index merge\");\n \tdrop_save();\n@@ -945,7 +945,7 @@ static int finish_automerge(struct commit *head,\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit();\n \tfree_commit_list(remoteheads);\n-\tif (commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL))\n+\tif (commit_tree(&merge_msg, result_tree, parents, result_commit, NULL))\n \t\tdie(_(\"failed to write commit object\"));\n \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n \tfinish(head, result_commit, buf.buf);\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f8e437d..5e32548 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -301,12 +301,12 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \t\treturn; /* don't have to commit an unchanged tree */\n \n \t/* Prepare commit message and reflog message */\n-\tstrbuf_addstr(&buf, \"notes: \"); /* commit message starts at index 7 */\n \tstrbuf_addstr(&buf, msg);\n \tif (buf.buf[buf.len - 1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n'); /* Make sure msg ends with newline */\n \n-\tcreate_notes_commit(t, NULL, buf.buf + 7, commit_sha1);\n+\tcreate_notes_commit(t, NULL, &buf, commit_sha1);\n+\tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n \tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\ndiff --git a/commit.c b/commit.c\nindex 73b7e00..0a214a6 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -845,7 +845,7 @@ static const char commit_utf8_warn[] =\n \"You may want to amend it after fixing the message, or set the config\\n\"\n \"variable i18n.commitencoding to the encoding your project uses.\\n\";\n \n-int commit_tree(const char *msg, unsigned char *tree,\n+int commit_tree(const struct strbuf *msg, unsigned char *tree,\n \t\tstruct commit_list *parents, unsigned char *ret,\n \t\tconst char *author)\n {\n@@ -884,7 +884,7 @@ int commit_tree(const char *msg, unsigned char *tree,\n \tstrbuf_addch(&buffer, '\\n');\n \n \t/* And add the comment */\n-\tstrbuf_addstr(&buffer, msg);\n+\tstrbuf_addbuf(&buffer, msg);\n \n \t/* And check the encoding */\n \tif (encoding_is_utf8 && !is_utf8(buffer.buf))\ndiff --git a/commit.h b/commit.h\nindex 009b113..5cf46b2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -181,7 +181,7 @@ static inline int single_parent(struct commit *commit)\n \n struct commit_list *reduce_heads(struct commit_list *heads);\n \n-extern int commit_tree(const char *msg, unsigned char *tree,\n+extern int commit_tree(const struct strbuf *msg, unsigned char *tree,\n \t\tstruct commit_list *parents, unsigned char *ret,\n \t\tconst char *author);\n \ndiff --git a/notes-cache.c b/notes-cache.c\nindex 4c8984e..bea013e 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -48,6 +48,7 @@ int notes_cache_write(struct notes_cache *c)\n {\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n+\tstruct strbuf msg = STRBUF_INIT;\n \n \tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n \t\treturn -1;\n@@ -56,7 +57,9 @@ int notes_cache_write(struct notes_cache *c)\n \n \tif (write_notes_tree(&c->tree, tree_sha1))\n \t\treturn -1;\n-\tif (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL) < 0)\n+\tstrbuf_attach(&msg, c->validity,\n+\t\t      strlen(c->validity), strlen(c->validity) + 1);\n+\tif (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL) < 0)\n \t\treturn -1;\n \tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n \t\t       0, QUIET_ON_ERR) < 0)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex ce10aac..b5a36ac 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -530,7 +530,7 @@ static int merge_from_diffs(struct notes_merge_options *o,\n }\n \n void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n-\t\t\t const char *msg, unsigned char *result_sha1)\n+\t\t\t const struct strbuf *msg, unsigned char *result_sha1)\n {\n \tunsigned char tree_sha1[20];\n \n@@ -668,7 +668,7 @@ int notes_merge(struct notes_merge_options *o,\n \t\tstruct commit_list *parents = NULL;\n \t\tcommit_list_insert(remote, &parents); /* LIFO order */\n \t\tcommit_list_insert(local, &parents);\n-\t\tcreate_notes_commit(local_tree, parents, o->commit_msg.buf,\n+\t\tcreate_notes_commit(local_tree, parents, &o->commit_msg,\n \t\t\t\t    result_sha1);\n \t}\n \n@@ -695,7 +695,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tstruct dir_struct dir;\n \tchar *path = xstrdup(git_path(NOTES_MERGE_WORKTREE \"/\"));\n \tint path_len = strlen(path), i;\n-\tconst char *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n+\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n+\tstruct strbuf sb_msg = STRBUF_INIT;\n \n \tif (o->verbosity >= 3)\n \t\tprintf(\"Committing notes in notes merge worktree at %.*s\\n\",\n@@ -733,7 +734,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t\t\t\tsha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));\n \t}\n \n-\tcreate_notes_commit(partial_tree, partial_commit->parents, msg,\n+\tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n+\tcreate_notes_commit(partial_tree, partial_commit->parents, &sb_msg,\n \t\t\t    result_sha1);\n \tif (o->verbosity >= 4)\n \t\tprintf(\"Finalized notes merge commit: %s\\n\",\ndiff --git a/notes-merge.h b/notes-merge.h\nindex 168a672..0c11b17 100644\n--- a/notes-merge.h\n+++ b/notes-merge.h\n@@ -37,7 +37,7 @@ void init_notes_merge_options(struct notes_merge_options *o);\n  * The resulting commit SHA1 is stored in result_sha1.\n  */\n void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n-\t\t\t const char *msg, unsigned char *result_sha1);\n+\t\t\t const struct strbuf *msg, unsigned char *result_sha1);\n \n /*\n  * Merge notes from o->remote_ref into o->local_ref\n-- \n1.7.8.36.g69ee2\n"},{"id":"181227","messageId":"1323956843-5326-3-git-send-email-pclouds@gmail.com","threadId":"29144","inReplyTo":"1323956843-5326-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] commit: refuse commit messages that contain NULs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-15T13:47:23Z","receivedAt":"2011-12-15T13:47:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current implementation sees NUL as terminator. If users give a\nNUL-included message (e.g. editor accidentally set to save as UTF-16),\nthe new commit message will have NULs. However following operations\n(displaying or amending a commit for example) will not show anything\nafter the first NUL.\n\nStop user right when they do this. If NUL is added by mistake, they\nhave their chance to fix. If they know that they are doing,\ncommit-tree will gladly commit whatever is given.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Advice stuff dropped. I realized quite late that commit_tree() is\n also used for plumbing commands (also thanks to Junio's comments),\n while I wanted to check at porcelain level only. So I moved the check\n up to builtin/commit.c. If we need the same check for other commands,\n which I doubt, similar checks can be added.\n\n builtin/commit.c       |    7 +++++++\n t/t3900-i18n-commit.sh |    6 ++++++\n 2 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 849151e..5db7673 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1483,6 +1483,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\texit(1);\n \t}\n \n+\tif (memchr(sb.buf, '\\0', sb.len)) {\n+\t\trollback_index_files();\n+\t\tdie(_(\"your commit message contains NUL characters.\\n\"\n+\t\t      \"hint: This is often caused by using wide encodings such as\\n\"\n+\t\t      \"hint: UTF-16. Please check your editor settings.\"));\n+\t}\n+\n \tif (commit_tree(&sb, active_cache_tree->sha1, parents, sha1,\n \t\t\tauthor_ident.buf)) {\n \t\trollback_index_files();\ndiff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\nindex 1f62c15..d48a7c0 100755\n--- a/t/t3900-i18n-commit.sh\n+++ b/t/t3900-i18n-commit.sh\n@@ -34,6 +34,12 @@ test_expect_success 'no encoding header for base case' '\n \ttest z = \"z$E\"\n '\n \n+test_expect_failure 'UTF-16 refused because of NULs' '\n+\techo UTF-16 >F &&\n+\tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt\n+'\n+\n+\n for H in ISO8859-1 eucJP ISO-2022-JP\n do\n \ttest_expect_success \"$H setup\" '\n-- \n1.7.8.36.g69ee2\n"},{"id":"181238","messageId":"7viplhofsq.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"1323956843-5326-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 1/3] merge: abort if fails to commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T18:23:01Z","receivedAt":"2011-12-15T18:23:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"181240","messageId":"7vehw5oepj.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"20111214182953.GA6469@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T18:46:32Z","receivedAt":"2011-12-15T18:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm happy to ignore custom programs linking against internal git code,\n> but what should \"git commit-tree\" do?\n>\n> My gut feeling is that it should store the literal binary contents.\n> However, I don't think this has ever been the case. Even in the initial\n> version of commit-tree.c, we read the input line-by-line and sprintf it\n> into place.\n\nYeah, you are right. Perhaps we should tweak updated 3/3 to check at the\nlower level commit_tree() then.\n\nI've rewrote the log message for 2/3 as follows so we can go either way\n;-)\n\n    Convert commit_tree() to take strbuf as message\n    \n    There wan't a way for commit_tree() to notice if the message the caller\n    prepared contained a NUL byte, as it did not take the length of the\n    message as a parameter. Use a pointer to a strbuf instead, so that we can\n    either choose to allow low-level plumbing commands to make commits\n    that contain NUL byte in its message, or forbid NUL everywhere by\n    adding the check in commit_tree(), in later patches.\n    \n    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"181242","messageId":"7v62hhocgm.fsf@alter.siamese.dyndns.org","threadId":"29144","inReplyTo":"7vehw5oepj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] Do not create commits whose message contains NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T19:35:05Z","receivedAt":"2011-12-15T19:35:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> My gut feeling is that it should store the literal binary contents.\n>> However, I don't think this has ever been the case. Even in the initial\n>> version of commit-tree.c, we read the input line-by-line and sprintf it\n>> into place.\n>\n> Yeah, you are right. Perhaps we should tweak updated 3/3 to check at the\n> lower level commit_tree() then.\n>\n> I've rewrote the log message for 2/3 as follows so we can go either way\n> ;-)\n\ns/rewrote/rewritten/ obviously...\n\n>     Convert commit_tree() to take strbuf as message\n>     \n>     There wan't a way for commit_tree() to notice if the message the caller\n>     prepared contained a NUL byte, as it did not take the length of the\n>     message as a parameter. Use a pointer to a strbuf instead, so that we can\n>     either choose to allow low-level plumbing commands to make commits\n>     that contain NUL byte in its message, or forbid NUL everywhere by\n>     adding the check in commit_tree(), in later patches.\n>     \n>     Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>     Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nAnd 3/3 looks like this:\n\n    commit_tree(): refuse commit messages that contain NULs\n    \n    Current implementation sees NUL as terminator. If users give a message\n    with NUL byte in it (e.g. editor set to save as UTF-16), the new commit\n    message will have NULs. However following operations (displaying or\n    amending a commit for example) will not keep anything after the first NUL.\n    \n    Stop user right when they do this. If NUL is added by mistake, they have\n    their chance to fix. Otherwise, log messages will no longer be text \"git\n    log\" and friends would grok.\n    \n    Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"181826","messageId":"1325435251.4752.104.camel@drew-northup.unet.maine.edu","threadId":"29144","inReplyTo":"20111213175932.GA1663@sigill.intra.peff.net","subject":"Re: [PATCH resend] Do not create commits whose message contains NUL","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2012-01-01T16:27:31Z","receivedAt":"2012-01-01T16:27:31Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"On Tue, 2011-12-13 at 12:59 -0500, Jeff King wrote:\n> It looks like we already have a check for is_utf8, and this is not\n> failing that check. I guess because is_utf8 takes a NUL-terminated\n> buffer, so it simply sees the truncated result (i.e., depending on\n> endianness, \"foo\" in utf16 is something like \"f\\0o\\0o\\0\", so we check\n> only \"f\"). We could make is_utf8 take a length parameter to be more\n> accurate, and then it would catch this.\n> \n> However, I think that's not quite what we want. We only check is_utf8 if\n> the encoding field is not set. And really, we want to reject NULs no\n> matter _which_ encoding they've set, because git simply doesn't handle\n> them properly.\n\nI had already started experimenting with automatically detecting decent\nUTF-16 a long while back so that compatible platforms could handle it\nappropriately in terms of creating diffs and dealing with newline\nmunging between platforms. There is no 100% sure-fire check for UTF-16\nif you don't already suspect it is possibly UTF-16. If we really want to\ncheck for possible UTF-16 specifically I can scrape out the check I\nwrote up and send it along.\nThe is_utf8 check was not written to detect 100% valid UTF-8 per-se. It\nseems to me that it was written as part of the \"is this a binary or not\"\ncheck in the add/commit path. I have thought for some time that\nspecifying buffer length in that whole code path would be a good idea\n(but I thought that somebody else had taken up that battle while I was\nbusy dealing with other problems elsewhere), if for no other reason it\nwould force it to deal with NULs more intelligently.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"181885","messageId":"20120103200331.GG20926@sigill.intra.peff.net","threadId":"29144","inReplyTo":"1325435251.4752.104.camel@drew-northup.unet.maine.edu","subject":"Re: [PATCH resend] Do not create commits whose message contains NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-03T20:03:31Z","receivedAt":"2012-01-03T20:03:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 01, 2012 at 11:27:31AM -0500, Drew Northup wrote:\n\n> I had already started experimenting with automatically detecting decent\n> UTF-16 a long while back so that compatible platforms could handle it\n> appropriately in terms of creating diffs and dealing with newline\n> munging between platforms. There is no 100% sure-fire check for UTF-16\n> if you don't already suspect it is possibly UTF-16. If we really want to\n> check for possible UTF-16 specifically I can scrape out the check I\n> wrote up and send it along.\n\nI also looked into this recently. You can generally detect UTF-16 by the\nBOM at the beginning of the file (which will also tell you the\nendian-ness). I did a simple test by integrating it into the check for\nbinary-ness during diffs. However, as I recall, the result wasn't\nparticularly useful. Some of the diff code wasn't happy with the\nembedded NUL bytes (i.e., there is code that assumes that NUL is the end\nof a string). Not to mention that ascii newline (0x0a) can appear as\npart of other characters in a wide encoding like utf-16. And since git\noutputs straight ascii for all of the diff boilerplate, you end up with\na mish-mash of utf-16 and ascii (this is OK with utf-8, of course,\nbecause utf-8 is a superset of ascii).\n\nIf anything, I think you would want to do something like \"textconv\" to\nconvert the utf-16 into utf-8, then diff that. Git won't do it\nautomatically based on encoding, but if you know the filenames of the\nutf-16 files in your repository, you can do something like:\n\n  echo 'foo.txt diff=utf16' >.gitattributes\n  git config diff.utf16.textconv 'iconv -f utf16 -t utf8'\n\nand get readable diffs. Of course you couldn't use that diff to apply a\npatch, though.\n\nI strongly suspect that not many people are really using git for utf-16\nfiles. Git treats them as binary, which makes them unpleasant for\nanything except simple storage.\n\n> The is_utf8 check was not written to detect 100% valid UTF-8 per-se. It\n> seems to me that it was written as part of the \"is this a binary or not\"\n> check in the add/commit path.\n\nWe shouldn't care about binary file content at all in the add or commit\ncode paths. I would guess we do only if you are using auto-crlf (but\nthen, I don't think we care about utf8 in that cases, only whether line\nendings should be converted or not).\n\nWe do check that the commit message itself is utf8, but only to generate\na warning that you should set i81n.commitencoding.\n\n-Peff\n"}]}