[PATCH] commit: skip discarding the index if there is no pre-commit hook

If there is not a pre-commit hook, there is no reason to discard
the index and reread it.

This change checks to presence of a pre-commit hook and then only
discards the index if there was one.

Signed-off-by: Kevin Willford <redacted>
---
 builtin/commit.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e7a2cb6285..443949d87b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -671,12 +671,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg2 = NULL;
 	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 	int old_display_comment_prefix;
+	const char *precommit_hook = NULL;
 
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
-		return 0;
+
+	if (!no_verify) {
+		/*
+		 * Check to see if there is a pre-commit hook
+		 * If there not one we can skip discarding the index later on
+		 */
+		precommit_hook = find_hook("pre-commit");
+		if (precommit_hook &&
+		    run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+			return 0;
+	}
 
 	if (squash_message) {
 		/*

@@ -940,12 +950,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	/*
-	 * 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.
-	 */
-	discard_cache();
+	if (!no_verify && precommit_hook) {
+		/*
+		 * 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.
+		 */
+		discard_cache();
+	}
+
 	read_cache_from(index_file);
 	if (update_main_cache_tree(0)) {
 		error(_("Error building trees"));

-- 
2.14.0.rc0.286.g44127d70e4

On Thu, Aug 10, 2017 at 02:54:16PM -0400, Kevin Willford wrote:

If there is not a pre-commit hook, there is no reason to discard
the index and reread it.

This change checks to presence of a pre-commit hook and then only
discards the index if there was one.

Sounds like a smart optimization.

The return value from find_hook() points to storage that may later be
overwritten or even freed.  I know your patch doesn't actually look at
the contents of precommit_hook, but it seems like it's setting up a
potential bomb for somebody later.

Can we make this:

  int have_precommit_hook = 0;
  ...
  have_precommit_hook = !!find_hook("pre-commit");

? Though see below.

-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
-		return 0;
+
+	if (!no_verify) {
+		/*
+		 * Check to see if there is a pre-commit hook
+		 * If there not one we can skip discarding the index later on
+		 */
+		precommit_hook = find_hook("pre-commit");
+		if (precommit_hook &&
+		    run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+			return 0;
+	}

We'll find the hook again in run_commit_hook(), but it's not so
expensive that it's worth trying to pass down the hook path we found
(and if we switch to an integer flag we don't have the path anyway ;) ).

But note that we don't even care about precommit_hook here. We could
just call run_commit_hook() regardless. We only care later whether we
ran it or not. So we could drop this hunk and the precommit_hook
variable entirely, and...

Just make this:

  if (!no_verify && find_hook("pre-commit"))
     ... discard cache ...

That is racy if somebody removes the hook while we're running (so is
your patch, but in the opposite direction). What we really want to know
is "did we run the hook" and annoyingly run_hook_ve() doesn't tell us
that.  So I think the most robust solution would be refactoring that to
pass out the information, and then use the flag here. But I'm not sure
it's actually worth worrying about such a race in practice.

-Peff

If there is not a pre-commit hook, there is no reason to discard
the index and reread it.

This change checks to presence of a pre-commit hook and then only
discards the index if there was one.

Signed-off-by: Kevin Willford <redacted>
---
 builtin/commit.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e7a2cb6285..ab71b93518 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	/*
-	 * 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.
-	 */
-	discard_cache();
+	if (!no_verify && find_hook("pre-commit")) {
+		/*
+		 * 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.
+		 */
+		discard_cache();
+	}
+
 	read_cache_from(index_file);
 	if (update_main_cache_tree(0)) {
 		error(_("Error building trees"));

-- 
2.14.1.481.g785c1dc9ae

On Mon, Aug 14, 2017 at 03:54:25PM -0600, Kevin Willford wrote:

If there is not a pre-commit hook, there is no reason to discard
the index and reread it.

This change checks to presence of a pre-commit hook and then only
discards the index if there was one.

Signed-off-by: Kevin Willford <redacted>
---
 builtin/commit.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Thanks, this looks nice and simple.

This read_cache_from() should be a noop, right, because it immediately
sees istate->initialized is set? So it shouldn't matter that it is not
in the conditional with discard_cache(). Though if its only purpose is
to re-read the just-discarded contents, perhaps it makes sense to put it
there for readability.

-Peff

On Mon, Aug 14, 2017 at 03:54:25PM -0600, Kevin Willford wrote:

quoted

If there is not a pre-commit hook, there is no reason to discard
the index and reread it.

This change checks to presence of a pre-commit hook and then only
discards the index if there was one.

Signed-off-by: Kevin Willford <redacted>
---
 builtin/commit.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Thanks, this looks nice and simple.

quoted

diff --git a/builtin/commit.c b/builtin/commit.c
index e7a2cb6285..ab71b93518 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file,

const char *prefix,

quoted

 		return 0;
 	}

-	/*
-	 * 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.
-	 */
-	discard_cache();
+	if (!no_verify && find_hook("pre-commit")) {
+		/*
+		 * 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.
+		 */
+		discard_cache();
+	}
+
 	read_cache_from(index_file);

This read_cache_from() should be a noop, right, because it immediately
sees istate->initialized is set? So it shouldn't matter that it is not
in the conditional with discard_cache(). Though if its only purpose is
to re-read the just-discarded contents, perhaps it makes sense to put it
there for readability.

-Peff

I thought about that and didn't know if there were cases when this would be called
and the cache has not been loaded.  It didn't look like it since it is only called from 
cmd_commit and prepare_index is called before it.  Also if in the future this call would
be made when it had not read the index yet so thought it was safest just to leave
this as always being called since it is basically a noop if the istate->initialized is set.

Also based on this commit
https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48
it looked like the discard_cache was added independent of the read_cache_from call,
which made me think that the two were not tied together.

Kevin

On Tue, Aug 15, 2017 at 04:23:50AM +0000, Kevin Willford wrote:

quoted

This read_cache_from() should be a noop, right, because it immediately
sees istate->initialized is set? So it shouldn't matter that it is not
in the conditional with discard_cache(). Though if its only purpose is
to re-read the just-discarded contents, perhaps it makes sense to put it
there for readability.

I thought about that and didn't know if there were cases when this would be called
and the cache has not been loaded.  It didn't look like it since it is only called from 
cmd_commit and prepare_index is called before it.  Also if in the future this call would
be made when it had not read the index yet so thought it was safest just to leave
this as always being called since it is basically a noop if the istate->initialized is set.

Yeah, I agree it might be safer as you have it. And hopefully the
comment above the discard would point anybody in the right direction.

Also based on this commit
https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48
it looked like the discard_cache was added independent of the read_cache_from call,
which made me think that the two were not tied together.

Yeah, I tried to dig in the history and figure it out, but it was not
nearly as clear as I would have liked. :)

-Peff