[PATCH v2] builtin-commit: re-read file index before run_status

["Samuel Yvon via GitGitGadget" <gitgitgadget@gmail.com>]

From: Samuel Yvon <samuelyvon9@gmail.com>

Changes made within a pre-commit hook are not reflected within
the editor (for instance, with `git commit --verbose`) because
the index is re-read after the status has been written.

This has the consequence of not displaying the actual changes made
by the pre-commit hook, but committing them anyways.

While it is often argued that the hook's purpose is not to automatically
write content to the repository, it is acknowledged that using the
pre-commit to apply mechanical fixes on top of the existing changes
is a supported use case, along with verifying the content.

I think not seeing these mechanical fixes before commiting is incorrect.
A developer might expect the commit to look a certain way but if the
pre-commit goes wrong the invalid changes will go unnoticed until
committed.

The comment suggesting that the cache must be reset after run_status
and before the editor being launched was added in ec84bd00,
(git-commit: Refactor creation of log message., 2008-02-05). It is
unclear why the run_status must be called *after* the cache reset.
However, calling run_status after the cache reset does not update
the status line to state of the current index in the case a
pre-commit hook is ran and changes files in the staging area.

Signed-off-by: Samuel Yvon <samuelyvon9@gmail.com>
---
    builtin-commit: re-read file index before run_status
    
    Changes made within a pre-commit hook are not reflected within the
    editor (for instance, with git commit --verbose) because the index is
    re-read after the editor has been used.
    
    This has the consequence of not displaying the actual changes made by
    the pre-commit hook, but committing them anyways.
    
    While it is often argued that the hook's purpose is not to automatically
    write content to the repository, it is acknowledged that using the
    pre-commit to apply mechanical fixes on top of the existing changes is a
    supported use case, along with verifying the content.
    
    I think not seeing these mechanical fixes before commiting is incorrect.
    A developer might expect the commit to look a certain way but if the
    pre-commit goes wrong the invalid changes will go unnoticed until
    committed.
    
    I had a small exchange in the Google Group before submitting this Pr.
    Here is a link for cross-referencing:
    https://groups.google.com/g/git-mentoring/c/FsP83I9mN6c
    
    As a side note, I do not know who manages the Github Repo but the
    following description threw me off a little bit:
    
    Git Source Code Mirror - This is a publish-only repository and all pull requests are ignored. 
    
    
    since after looking deeper it seems the PRs are not ignored.
    
    Changes since v1:
    
     * Edited the title to more accurately reflect the changes
     * More details in commit messages
    
    Signed-off-by: Samuel Yvon samuelyvon9@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1127%2FSamuelYvon%2Fmaint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1127/SamuelYvon/maint-v2
Pull-Request: https://github.com/git/git/pull/1127

Range-diff vs v1:

 1:  271bcf89d1e ! 1:  f09244a0ba3 builtin-commit: re-read file index before launching editor
     @@ Metadata
      Author: Samuel Yvon <samuelyvon9@gmail.com>
      
       ## Commit message ##
     -    builtin-commit: re-read file index before launching editor
     +    builtin-commit: re-read file index before run_status
      
          Changes made within a pre-commit hook are not reflected within
          the editor (for instance, with `git commit --verbose`) because
     -    the index is re-read after the editor has been used.
     +    the index is re-read after the status has been written.
      
          This has the consequence of not displaying the actual changes made
          by the pre-commit hook, but committing them anyways.
     @@ Commit message
          pre-commit goes wrong the invalid changes will go unnoticed until
          committed.
      
     +    The comment suggesting that the cache must be reset after run_status
     +    and before the editor being launched was added in ec84bd00,
     +    (git-commit: Refactor creation of log message., 2008-02-05). It is
     +    unclear why the run_status must be called *after* the cache reset.
     +    However, calling run_status after the cache reset does not update
     +    the status line to state of the current index in the case a
     +    pre-commit hook is ran and changes files in the staging area.
     +
          Signed-off-by: Samuel Yvon <samuelyvon9@gmail.com>
      
       ## builtin/commit.c ##


 builtin/commit.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 7c9b1e7be3a..e75b11d1c60 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -728,8 +728,17 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* 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 && find_hook("pre-commit")) {
+		if(run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+			return 0;
+
+		/*
+		 * Re-read the index as pre-commit hook could have updated it,
+		 * and write it out as a tree.
+		 */
+		discard_cache();
+		read_cache_from(index_file);
+	}
 
 	if (squash_message) {
 		/*
@@ -1051,14 +1060,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 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);
 
 	if (update_main_cache_tree(0)) {

base-commit: 5fbd2fc5997dfa4d4593a862fe729b1e7a89bcf8
-- 
gitgitgadget