COMMIT_EDITMSG not updated after pre-commit hook

COMMIT_EDITMSG not updated after pre-commit hook

[Hinrik Örn Sigurðsson]

I have a pre-commit hook which extracts documentation from file $foo
if it has pending changes to be committed. The hook creates/updates
the documentation file and calls "git add" on it.

When I do "git commit", the COMMIT_EDITMSG delivered to my editor
notes that this documentation file has been created/updated, but not
that its changes have been added to the index. However, if I go ahead
with the commit, I can see that the doc file changes were indeed
committed.

Here is a simplified test case:

$ cat .git/hooks/pre-commit
#!/usr/bin/env perl
use strict;
use warnings;

my $old = qx"git rev-parse HEAD:foo 2>/dev/null";
my $new = qx"git rev-parse :foo 2>/dev/null";

if (($? >> 8) != 0 || $old ne $new) {
    system "cat source > dest";
    system "git add dest";
}

$ ls
source

$ echo 123 >> source

$ git status
# On branch master
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   source
#
no changes added to commit (use "git add" and/or "git commit -a")

$ git commit -a
Test commit
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       modified:   source
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       dest
[master 535ed7f] Test commit
 2 files changed, 3 insertions(+), 0 deletions(-)
 create mode 100644 dest

$ git status
# On branch master
nothing to commit (working directory clean)

[RFC/PATCH] commit: update the index after running the pre-commit hook

[Ævar Arnfjörð Bjarmason]

Change git-commit to update the index after running the pre-commit
hook, but before it constructs the comments that'll accompany the
commit message displayed in the $EDITOR.

The use case for this is e.g. a pre-commit hook that looks like this:

    #!/bin/sh
    echo unf >>hlagh
    git add hlagh

In that case the $EDITOR will display "hlagh" under "Untracked files",
but it should be under "Changes to be committed:". Refreshing before
we construct the message fixes this bug.

Reported-by: Hinrik Örn Sigurðsson <hinrik.sig@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

2009/5/3 Hinrik Örn Sigurðsson <hinrik.sig@gmail.com>:
> I have a pre-commit hook which extracts documentation from file $foo
> if it has pending changes to be committed. The hook creates/updates
> the documentation file and calls "git add" on it.
>
> When I do "git commit", the COMMIT_EDITMSG delivered to my editor
> notes that this documentation file has been created/updated, but not
> that its changes have been added to the index. However, if I go ahead
> with the commit, I can see that the doc file changes were indeed
> committed.
>
> Here is a simplified test case:
>
> $ cat .git/hooks/pre-commit
> #!/usr/bin/env perl
> use strict;
> use warnings;
>
> my $old = qx"git rev-parse HEAD:foo 2>/dev/null";
> my $new = qx"git rev-parse :foo 2>/dev/null";
>
> if (($? >> 8) != 0 || $old ne $new) {
>    system "cat source > dest";
>    system "git add dest";
> }
>
> $ ls
> source
>
> $ echo 123 >> source
>
> $ git status
> # On branch master
> # Changed but not updated:
> #   (use "git add <file>..." to update what will be committed)
> #   (use "git checkout -- <file>..." to discard changes in working directory)
> #
> #       modified:   source
> #
> no changes added to commit (use "git add" and/or "git commit -a")
>
> $ git commit -a
> Test commit
> # Please enter the commit message for your changes. Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
> # On branch master
> # Changes to be committed:
> #   (use "git reset HEAD <file>..." to unstage)
> #
> #       modified:   source
> #
> # Untracked files:
> #   (use "git add <file>..." to include in what will be committed)
> #
> #       dest
> [master 535ed7f] Test commit
>  2 files changed, 3 insertions(+), 0 deletions(-)
>  create mode 100644 dest
>
> $ git status
> # On branch master
> nothing to commit (working directory clean)

Thanks for the report. Here's a proposed fix for this. I'm not
familiar with the commit code so this may be the wrong way to go, but
it fixes the bug and passes all tests.

The reporter has promised offlist to write a test for this.

 builtin/commit.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..dbb4ff5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -550,6 +550,20 @@ static int ends_rfc2822_footer(struct strbuf *sb)
 	return 1;
 }
 
+static int update_index(const char *index_file) {
+	discard_cache();
+	read_cache_from(index_file);
+	if (!active_cache_tree)
+		active_cache_tree = cache_tree();
+	if (cache_tree_update(active_cache_tree,
+			      active_cache, active_nr, 0, 0) < 0) {
+		error("Error building trees");
+		return 0;
+	}
+
+	return 1;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct wt_status *s)
 {
@@ -565,6 +579,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
 
+	/* Update the index after we run the pre-commit hook, but before
+	 * we construct the message we're sending to the editor. The
+	 * pre-commit hook may e.g. create a new file and add it to the
+	 * index.
+	 *
+	 * Having that file show up as modified but not staged is confusing.
+	 */
+	if (!update_index(index_file))
+		return 0;
+
 	if (message.len) {
 		strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
@@ -728,15 +752,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * 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 (!active_cache_tree)
-		active_cache_tree = cache_tree();
-	if (cache_tree_update(active_cache_tree,
-			      active_cache, active_nr, 0, 0) < 0) {
-		error("Error building trees");
+	if (!update_index(index_file))
 		return 0;
-	}
 
 	if (run_hook(index_file, "prepare-commit-msg",
 		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
-- 
1.7.2.1.414.g9bf49

Re: [RFC/PATCH] commit: update the index after running the pre-commit hook

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change git-commit to update the index after running the pre-commit
> hook, but before it constructs the comments that'll accompany the
> commit message displayed in the $EDITOR.

Hmm.

I have always thought that "pre-commit" is about verification, just like
other pre-anything hooks.  We have never sanctioned it to modify the tree,
and the document does not say the hook is allowed to.

Although I am not opposed to this patch, it is a change to the defined
semantics that is somewhat worrysome.  We probably should document what it
is and is not allowed to a bit better.

Re: [RFC/PATCH] commit: update the index after running the pre-commit hook

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> +++ b/builtin/commit.c
> @@ -565,6 +579,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
>  		return 0;
>  
> +	/* Update the index after we run the pre-commit hook, but before
> +	 * we construct the message we're sending to the editor. The
> +	 * pre-commit hook may e.g. create a new file and add it to the
> +	 * index.
> +	 *
> +	 * Having that file show up as modified but not staged is confusing.
> +	 */
> +	if (!update_index(index_file))
> +		return 0;
> +
>  	if (message.len) {
>  		strbuf_addbuf(&sb, &message);
>  		hook_arg1 = "message";
> @@ -728,15 +752,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	 * 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 (!active_cache_tree)
> -		active_cache_tree = cache_tree();
> -	if (cache_tree_update(active_cache_tree,
> -			      active_cache, active_nr, 0, 0) < 0) {
> -		error("Error building trees");
> +	if (!update_index(index_file))
>  		return 0;
> -	}
>  
>  	if (run_hook(index_file, "prepare-commit-msg",

Before, "commit" updated the index once, and after, twice.  How does
this affect the running time on, say, the linux-2.6 tree?  Could one
of the update_index() calls be suppressed when there is no hook to
run?