commit: skip discarding the index if there is no pre-commit hook
PATCH 1/1 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.
--- builtin/commit.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)
builtin/commit.c +21 -8
@@ -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" ));
Signature · 1 line
2.14.0.rc0.286.g44127d70e4
Quoted 5 lines · Kevin Willford
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.
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 674
Quoted 9 lines
builtin/commit.c +1
-0@@ -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 ;
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.
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 689
Quoted 13 lines
builtin/commit.c +11
-2-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...
Quoted 8 lines
builtin/commit.c +0
-4@@ -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.
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 960
Quoted 10 lines
builtin/commit.c +8
-2-*/ -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 (); + }
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
Quoted 8 lines · Kevin Willford
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 <[email protected]>---
Peff already has done a good job reviewing the patch text, and I agree that this is a worthwhile optimization.
Could Microsoft folks all make sure that their signed-off-by lines match their From: address (or leave an in-body From: to override the From: address your MUA places in your messages)?
Thanks.
Quoted 54 lines
builtin/commit.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)builtin/commit.c +21
-8@@ -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" ));
PATCH v2 1/1 commit: skip discarding the index if there is no pre-commit hook
In reply to Kevin WillfordIf 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.
--- builtin/commit.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
builtin/commit.c +9 -6
@@ -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" ));
Signature · 1 line
2.14.1.481.g785c1dc9ae
Quoted 10 lines · Kevin Willford
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 <[email protected]>--- builtin/commit.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
Thanks, this looks nice and simple.
Quoted 14 lines
builtin/commit.c +0
-6@@ -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 ();
builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 952
Quoted 10 lines
builtin/commit.c +9
-0+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
Quoted 47 lines · Kevin Willford
Quoted 11 lines
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 <[email protected]>--- builtin/commit.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)Thanks, this looks nice and simple.
builtin/commit.c static int prepare_to_commit(const char *index_file,
Quoted 5 lines
builtin/commit.c +0
-0@@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file,const char *prefix,
Quoted 9 lines
builtin/commit.c +0
-6return 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 ();builtin/commit.c static int prepare_to_commit(const char *index_file, const char *prefix, · line 952
Quoted 10 lines
builtin/commit.c +9
-0+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 seesistate->initializedis set? So it shouldn't matter that it is not in the conditional withdiscard_cache(). Though if its only purpose is to re-read the just-discarded contents, perhaps it makes sense to put it there for readability.
Quoted 1 line
-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
Quoted 11 lines · Kevin Willford
Quoted 5 lines
This
read_cache_from()should be a noop, right, because it immediately seesistate->initializedis set? So it shouldn't matter that it is not in the conditional withdiscard_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_commitandprepare_indexis 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 theistate->initializedis 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.
Quoted 4 lines
Also based on this commit
https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48
it looked like thediscard_cachewas added independent of theread_cache_fromcall, 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
Quoted 8 lines · Jeff King
Quoted 6 lines · Kevin Willford
Quoted 5 lines
This
read_cache_from()should be a noop, right, because it immediately seesistate->initializedis set? So it shouldn't matter that it is not in the conditional withdiscard_cache(). Though if its only purpose is to re-read the just-discarded contents, perhaps it makes sense to put it there for readability.
Quoted 8 lines
Quoted 5 lines
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_commitandprepare_indexis 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 theistate->initializedis 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.
I agree that it would not hurt to have it outside the conditional, because read_cache_from() is a no-op when it is already populated. I however do not think it is sensible to call prepare_to_commit() without having populated the in-core index---in the function, nobody conditionally reads the in-core index, expecting that the caller might not have prepared the in-core index, when we need to do the wt-status thing, for example.
Quoted 4 lines
Quoted 4 lines
Also based on this commit
https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48
it looked like thediscard_cachewas added independent of theread_cache_fromcall, which made me think that the two were not tied together.
That one comes from the first commit of the C reimplementation, f5bbc322 ("Port git commit to C.", 2007-11-08).
If I am reading the old code correctly, read_cache_from() is called for entirely different reasons. Back in the old code, when doing "commit --patch", prepare_index() called interactive_add(), which would return to us after updating the index file in the filesystem. The caller of prepare_index(), which was cmd_commit(), needed to read that off of the filesystem hence the call is there.
When not doing "--patch", prepare_index() called read_cache(), so the read_cache_from() there was a no-op.
2888605c ("builtin-commit: fix partial-commit support", 2007-11-18) was to fix the mistake of not re-reading the index from the file at that point for non "--patch" cases. Having read_cache_from() that is most of the time no-op was not sufficient and needed additional discard_cache() there.