{"thread":{"id":"48193","subject":"[PATCH] builtin/commit.c: prevent bad commits","startedAt":"2018-04-01T14:07:10Z","lastAt":"2018-04-01T22:50:42Z","messageCount":3,"participants":["Dan Aloni","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343554","messageId":"20180401140650.18928-1-alonid@gmail.com","threadId":"48193","inReplyTo":null,"subject":"[PATCH] builtin/commit.c: prevent bad commits","fromName":"Dan Aloni","fromEmail":"alonid@gmail.com","sentAt":"2018-04-01T14:06:50Z","receivedAt":"2018-04-01T14:07:10Z","isPatch":true,"sender":{"key":"alonid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/321273?v=4"},"body":"These commits which have hashes starting with the hex string 'bad',\nalways give me the chills. Why should a perfectly good commit be\njinxed?\n\nStatistically, one of 4096 commits may be 'bad'. This change adds a\n'--prevent-bad' switch to the commit command in order to prevent such\ncommit hashes from being generated. Internally, the commit is retried\nwith a slight commit meta-data modification - a newline is added to the\nend of the commit message. The meta-data change results in a different\nhash, that if we are lucky enough (4095/4096 chance) may not be 'bad'.\n\nNote that this change does not affect actual software quality maintained\nusing Git. Thus, it is recommended keep testing all generated versions\nregardless of commit hash jinxes.\n\nSigned-off-by: Dan Aloni <alonid@gmail.com>\n---\n builtin/commit.c | 34 +++++++++++++++++++++++++++++-----\n 1 file changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 37fcb55ab0a0..afaa7cefaedf 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -89,6 +89,7 @@ static int edit_flag = -1; /* unspecified */\n static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n static int config_commit_verbose = -1; /* unspecified */\n static int no_post_rewrite, allow_empty_message;\n+static int prevent_bad;\n static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;\n static char *sign_commit;\n \n@@ -1449,6 +1450,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('z', \"null\", &s.null_termination,\n \t\t\t N_(\"terminate entries with NUL\")),\n \t\tOPT_BOOL(0, \"amend\", &amend, N_(\"amend previous commit\")),\n+\t\tOPT_BOOL(0, \"prevent-bad\", &prevent_bad, N_(\"prevent a bad commit\")),\n \t\tOPT_BOOL(0, \"no-post-rewrite\", &no_post_rewrite, N_(\"bypass post-rewrite hook\")),\n \t\t{ OPTION_STRING, 'u', \"untracked-files\", &untracked_files_arg, N_(\"mode\"), N_(\"show untracked files, optional modes: all, normal, no. (Default: all)\"), PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n \t\t/* end commit contents options */\n@@ -1583,12 +1585,34 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tappend_merge_tag_headers(parents, &tail);\n \t}\n \n-\tif (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,\n-\t\t\t\t parents, &oid, author_ident.buf, sign_commit,\n-\t\t\t\t extra)) {\n-\t\trollback_index_files();\n-\t\tdie(_(\"failed to write commit object\"));\n+\tfor (;;) {\n+\t\tchar *oid_hex;\n+\t\tstruct commit_list *copy_parents;\n+\n+\t\tcopy_parents = copy_commit_list(parents);\n+\n+\t\tif (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,\n+\t\t\t\t\t parents, &oid, author_ident.buf, sign_commit,\n+\t\t\t\t\t extra)) {\n+\t\t\trollback_index_files();\n+\t\t\tdie(_(\"failed to write commit object\"));\n+\t\t}\n+\n+\t\toid_hex = oid_to_hex(&oid);\n+\t\tif (prevent_bad &&\n+\t\t    oid_hex[0] == 'b' &&\n+\t\t    oid_hex[1] == 'a' &&\n+\t\t    oid_hex[2] == 'd' )\n+\t\t{\n+\t\t\tparents = copy_parents;\n+\t\t\tstrbuf_add(&sb, \"\\n\", 1);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tfree_commit_list(copy_parents);\n+\t\tbreak;\n \t}\n+\n \tstrbuf_release(&author_ident);\n \tfree_commit_extra_headers(extra);\n \n-- \n2.14.3\n\n"},{"id":"343557","messageId":"xmqq4lku2880.fsf@gitster-ct.c.googlers.com","threadId":"48193","inReplyTo":"20180401140650.18928-1-alonid@gmail.com","subject":"Re: [PATCH] builtin/commit.c: prevent bad commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-01T16:37:51Z","receivedAt":"2018-04-01T16:37:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Aloni <alonid@gmail.com> writes:\n\n> These commits which have hashes starting with the hex string 'bad',\n> always give me the chills. Why should a perfectly good commit be\n> jinxed?\n> ...\n> Note that this change does not affect actual software quality maintained\n> using Git. Thus, it is recommended keep testing all generated versions\n> regardless of commit hash jinxes.\n>\n> Signed-off-by: Dan Aloni <alonid@gmail.com>\n> ---\n\nAh, I forgot that it's that time of the year again.\n\n> + ...\n> +\t\toid_hex = oid_to_hex(&oid);\n> +\t\tif (prevent_bad &&\n> +\t\t    oid_hex[0] == 'b' &&\n> +\t\t    oid_hex[1] == 'a' &&\n> +\t\t    oid_hex[2] == 'd' )\n> +\t\t{\n> +\t\t\tparents = copy_parents;\n> +\t\t\tstrbuf_add(&sb, \"\\n\", 1);\n> +\t\t\tcontinue;\n\nWe used to allow a variant of this that lets you append invisible\ncruft at the end of the log message by hiding it after a NUL.  You\nalso could loop here to turn the abbreviated commit object name to\nan actively good one, not just \"not bad\" name ;-).\n\n> +\t\t}\n> +\n> +\t\tfree_commit_list(copy_parents);\n> +\t\tbreak;\n>  \t}\n> +\n>  \tstrbuf_release(&author_ident);\n>  \tfree_commit_extra_headers(extra);\n"},{"id":"343560","messageId":"20180401225016.GA21634@sigill.intra.peff.net","threadId":"48193","inReplyTo":"20180401140650.18928-1-alonid@gmail.com","subject":"Re: [PATCH] builtin/commit.c: prevent bad commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-01T22:50:17Z","receivedAt":"2018-04-01T22:50:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 01, 2018 at 05:06:50PM +0300, Dan Aloni wrote:\n\n> These commits which have hashes starting with the hex string 'bad',\n> always give me the chills. Why should a perfectly good commit be\n> jinxed?\n> \n> Statistically, one of 4096 commits may be 'bad'. This change adds a\n> '--prevent-bad' switch to the commit command in order to prevent such\n> commit hashes from being generated. Internally, the commit is retried\n> with a slight commit meta-data modification - a newline is added to the\n> end of the commit message. The meta-data change results in a different\n> hash, that if we are lucky enough (4095/4096 chance) may not be 'bad'.\n> \n> Note that this change does not affect actual software quality maintained\n> using Git. Thus, it is recommended keep testing all generated versions\n> regardless of commit hash jinxes.\n\nCute.\n\nA while back we had patches to generate \"desirable\" commit prefixes, and\nthey were focused on making the brute-forcing as fast as possible. I\ndon't think I've seen this reverse case, but it's much easier: we can be\nfairly slow since the probability of hitting the bad case repeatedly is\nlow.\n\n> @@ -1583,12 +1585,34 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\tappend_merge_tag_headers(parents, &tail);\n>  \t}\n>  \n> -\tif (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,\n> -\t\t\t\t parents, &oid, author_ident.buf, sign_commit,\n> -\t\t\t\t extra)) {\n> -\t\trollback_index_files();\n> -\t\tdie(_(\"failed to write commit object\"));\n> +\tfor (;;) {\n> +\t\tchar *oid_hex;\n> +\t\tstruct commit_list *copy_parents;\n> +\n> +\t\tcopy_parents = copy_commit_list(parents);\n> +\n> +\t\tif (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,\n> +\t\t\t\t\t parents, &oid, author_ident.buf, sign_commit,\n> +\t\t\t\t\t extra)) {\n> +\t\t\trollback_index_files();\n> +\t\t\tdie(_(\"failed to write commit object\"));\n> +\t\t}\n\nYou could still have a \"bad\" tree. :)\n\n-Peff\n"}]}