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

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