Re: Issue when changing staged files in a pre-commit hook

From

Duy Nguyen <pclouds@gmail.com>

Date

Jan 22, 2016, 08:58 UTC

Message-ID

<20160122085847.GA31700@lanh>

In-Reply-To

<xmqqk2n56n49.fsf@gitster.mtv.corp.google.com>

On Tue, Jan 19, 2016 at 05:44:22PM -0800, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > On Wed, Jan 20, 2016 at 6:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> >> The only sensible thing we can do at that point in the code after
> >> re-reading the index is to make sure that hasn't been changed by the
> >> pre-commit hook and complain loudly to die if we find it modified, I
> >> think.
> >
> > OK. Two more points
> >
> >  - do we catch index changes for partial commit case only? Because
> > when $GIT_DIR/index is used, we do not have this problem.
> 
> I do not think you are correct.  As-is commit with un-added changes
> in the working tree would have the same problem.  .....

-- 8< --

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..f9f4957 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -656,6 +656,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix,
+			     const unsigned char *tree_sha1,
 			     struct commit *current_head,
 			     struct wt_status *s,
 			     struct strbuf *author_ident)
@@ -928,9 +929,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	/*
-	 * Re-read the index as pre-commit hook could have updated it,
-	 * and write it out as a tree.  We must do this before we invoke
-	 * the editor and after we invoke run_status above.
+	 * Re-read the index and check if pre-commit hook has updated it,
+	 * We must do this before we invoke the editor and after we
+	 * invoke run_status above.
 	 */
 	discard_cache();
 	read_cache_from(index_file);
@@ -938,6 +939,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		error(_("Error building trees"));
 		return 0;
 	}
+	if (hashcmp(active_cache_tree->sha1, tree_sha1)) {
+		error(_("pre-commit hook has changed commit content"));
+		return 0;
+	}
 
 	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
 			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
@@ -1636,6 +1641,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct commit_extra_header *extra = NULL;
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	unsigned char tree_sha1[GIT_SHA1_RAWSZ];
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1657,10 +1663,16 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);
+	if (update_main_cache_tree(0)) {
+		error(_("Error building trees"));
+		rollback_index_files();
+		return 1;
+	}
+	hashcpy(tree_sha1, active_cache_tree->sha1);
 
 	/* Set up everything for writing the commit object.  This includes
 	   running hooks, writing the trees, and interacting with the user.  */
-	if (!prepare_to_commit(index_file, prefix,
+	if (!prepare_to_commit(index_file, prefix, tree_sha1,
 			       current_head, &s, &author_ident)) {
 		rollback_index_files();
 		return 1;
@@ -1749,7 +1761,7 @@ 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->sha1,
+	if (commit_tree_extended(sb.buf, sb.len, tree_sha1,
 			 parents, sha1, author_ident.buf, sign_commit, extra)) {
 		rollback_index_files();
 		die(_("failed to write commit object"));
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..e6b5979 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -136,4 +136,12 @@ test_expect_success 'check the author in hook' '
 	git show -s
 '
 
+test_expect_success 'catch pre-commit hooks that modify index' '
+	echo modified >>file &&
+	write_script "$HOOK" <<-\EOF &&
+	git rm -f file
+	EOF
+	test_must_fail git commit -m m file
+'
+
 test_done
-- 8< --