threads / patch / 48193

patchbuiltin/commit.c: prevent bad commits

Subject: [PATCH] builtin/commit.c: prevent bad commits

## tl;dr

3 messages between Apr 1, 2018 and Apr 1, 2018. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Dan Aloni· Apr 1, 2018, 14:06 UTC · lore

These commits which have hashes starting with the hex string 'bad', always give me the chills. Why should a perfectly good commit be jinxed?

Statistically, one of 4096 commits may be 'bad'. This change adds a '--prevent-bad' switch to the commit command in order to prevent such commit hashes from being generated. Internally, the commit is retried with a slight commit meta-data modification - a newline is added to the end of the commit message. The meta-data change results in a different hash, that if we are lucky enough (4095/4096 chance) may not be 'bad'.

Note that this change does not affect actual software quality maintained using Git. Thus, it is recommended keep testing all generated versions regardless of commit hash jinxes.

Signed-off-by: Dan Aloni <alonid@gmail.com>
---
 builtin/commit.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)
Show changes to builtin/commit.c +29 −5
diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab0a0..afaa7cefaedf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -89,6 +89,7 @@ static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
+static int prevent_bad;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit;
 
@@ -1449,6 +1450,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('z', "null", &s.null_termination,
 			 N_("terminate entries with NUL")),
 		OPT_BOOL(0, "amend", &amend, N_("amend previous commit")),
+		OPT_BOOL(0, "prevent-bad", &prevent_bad, N_("prevent a bad commit")),
 		OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")),
 		{ 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" },
 		/* end commit contents options */
@@ -1583,12 +1585,34 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		append_merge_tag_headers(parents, &tail);
 	}
 
-	if (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,
-				 parents, &oid, author_ident.buf, sign_commit,
-				 extra)) {
-		rollback_index_files();
-		die(_("failed to write commit object"));
+	for (;;) {
+		char *oid_hex;
+		struct commit_list *copy_parents;
+
+		copy_parents = copy_commit_list(parents);
+
+		if (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,
+					 parents, &oid, author_ident.buf, sign_commit,
+					 extra)) {
+			rollback_index_files();
+			die(_("failed to write commit object"));
+		}
+
+		oid_hex = oid_to_hex(&oid);
+		if (prevent_bad &&
+		    oid_hex[0] == 'b' &&
+		    oid_hex[1] == 'a' &&
+		    oid_hex[2] == 'd' )
+		{
+			parents = copy_parents;
+			strbuf_add(&sb, "\n", 1);
+			continue;
+		}
+
+		free_commit_list(copy_parents);
+		break;
 	}
+
 	strbuf_release(&author_ident);
 	free_commit_extra_headers(extra);
 
-- 
2.14.3
Junio C Hamano· Apr 1, 2018, 16:37 UTC · re: Dan Aloni · lore

Re: [PATCH] builtin/commit.c: prevent bad commits

Dan Aloni <alonid@gmail.com> writes:
Show 10 quoted lines
> These commits which have hashes starting with the hex string 'bad',
> always give me the chills. Why should a perfectly good commit be
> jinxed?
> ...
> Note that this change does not affect actual software quality maintained
> using Git. Thus, it is recommended keep testing all generated versions
> regardless of commit hash jinxes.
>
> Signed-off-by: Dan Aloni <alonid@gmail.com>
> ---
Ah, I forgot that it's that time of the year again.
Show 10 quoted lines
> + ...
> +		oid_hex = oid_to_hex(&oid);
> +		if (prevent_bad &&
> +		    oid_hex[0] == 'b' &&
> +		    oid_hex[1] == 'a' &&
> +		    oid_hex[2] == 'd' )
> +		{
> +			parents = copy_parents;
> +			strbuf_add(&sb, "\n", 1);
> +			continue;

We used to allow a variant of this that lets you append invisible cruft at the end of the log message by hiding it after a NUL. You also could loop here to turn the abbreviated commit object name to an actively good one, not just "not bad" name ;-).

Show 8 quoted lines
> +		}
> +
> +		free_commit_list(copy_parents);
> +		break;
>  	}
> +
>  	strbuf_release(&author_ident);
>  	free_commit_extra_headers(extra);
Jeff King· Apr 1, 2018, 22:50 UTC · re: Dan Aloni · lore

Re: [PATCH] builtin/commit.c: prevent bad commits

On Sun, Apr 01, 2018 at 05:06:50PM +0300, Dan Aloni wrote:
Show 14 quoted lines
> These commits which have hashes starting with the hex string 'bad',
> always give me the chills. Why should a perfectly good commit be
> jinxed?
> 
> Statistically, one of 4096 commits may be 'bad'. This change adds a
> '--prevent-bad' switch to the commit command in order to prevent such
> commit hashes from being generated. Internally, the commit is retried
> with a slight commit meta-data modification - a newline is added to the
> end of the commit message. The meta-data change results in a different
> hash, that if we are lucky enough (4095/4096 chance) may not be 'bad'.
> 
> Note that this change does not affect actual software quality maintained
> using Git. Thus, it is recommended keep testing all generated versions
> regardless of commit hash jinxes.
Cute.

A while back we had patches to generate "desirable" commit prefixes, and they were focused on making the brute-forcing as fast as possible. I don't think I've seen this reverse case, but it's much easier: we can be fairly slow since the probability of hitting the bad case repeatedly is low.

Show 21 quoted lines
> @@ -1583,12 +1585,34 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		append_merge_tag_headers(parents, &tail);
>  	}
>  
> -	if (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,
> -				 parents, &oid, author_ident.buf, sign_commit,
> -				 extra)) {
> -		rollback_index_files();
> -		die(_("failed to write commit object"));
> +	for (;;) {
> +		char *oid_hex;
> +		struct commit_list *copy_parents;
> +
> +		copy_parents = copy_commit_list(parents);
> +
> +		if (commit_tree_extended(sb.buf, sb.len, &active_cache_tree->oid,
> +					 parents, &oid, author_ident.buf, sign_commit,
> +					 extra)) {
> +			rollback_index_files();
> +			die(_("failed to write commit object"));
> +		}
You could still have a "bad" tree. :)
-Peff

← back to recent threads