{"thread":{"id":"19166","subject":"COMMIT_EDITMSG not updated after pre-commit hook","startedAt":"2009-05-03T23:20:04Z","lastAt":"2010-08-18T22:51:30Z","messageCount":4,"participants":["Hinrik Örn Sigurðsson","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"112938","messageId":"6e4c323b0905031620m48769dbdp52684d6b470ebea3@mail.gmail.com","threadId":"19166","inReplyTo":null,"subject":"COMMIT_EDITMSG not updated after pre-commit hook","fromName":"Hinrik Örn Sigurðsson","fromEmail":"hinrik.sig@gmail.com","sentAt":"2009-05-03T23:20:04Z","receivedAt":"2009-05-03T23:20:04Z","isPatch":false,"sender":{"key":"hinrik.sig@gmail.com","avatar":null},"body":"I have a pre-commit hook which extracts documentation from file $foo\nif it has pending changes to be committed. The hook creates/updates\nthe documentation file and calls \"git add\" on it.\n\nWhen I do \"git commit\", the COMMIT_EDITMSG delivered to my editor\nnotes that this documentation file has been created/updated, but not\nthat its changes have been added to the index. However, if I go ahead\nwith the commit, I can see that the doc file changes were indeed\ncommitted.\n\nHere is a simplified test case:\n\n$ cat .git/hooks/pre-commit\n#!/usr/bin/env perl\nuse strict;\nuse warnings;\n\nmy $old = qx\"git rev-parse HEAD:foo 2>/dev/null\";\nmy $new = qx\"git rev-parse :foo 2>/dev/null\";\n\nif (($? >> 8) != 0 || $old ne $new) {\n system \"cat source > dest\";\n system \"git add dest\";\n}\n\n$ ls\nsource\n\n$ echo 123 >> source\n\n$ git status\n# On branch master\n# Changed but not updated:\n# (use \"git add ...\" to update what will be committed)\n# (use \"git checkout -- ...\" to discard changes in working directory)\n#\n#\tmodified: source\n#\nno changes added to commit (use \"git add\" and/or \"git commit -a\")\n\n$ git commit -a\nTest commit\n# Please enter the commit message for your changes. Lines starting\n# with '#' will be ignored, and an empty message aborts the commit.\n# On branch master\n# Changes to be committed:\n# (use \"git reset HEAD ...\" to unstage)\n#\n# modified: source\n#\n# Untracked files:\n# (use \"git add ...\" to include in what will be committed)\n#\n# dest\n[master 535ed7f] Test commit\n 2 files changed, 3 insertions(+), 0 deletions(-)\n create mode 100644 dest\n\n$ git status\n# On branch master\nnothing to commit (working directory clean)\n"},{"id":"148383","messageId":"1282139649-21097-1-git-send-email-avarab@gmail.com","threadId":"19166","inReplyTo":"6e4c323b0905031620m48769dbdp52684d6b470ebea3@mail.gmail.com","subject":"[RFC/PATCH] commit: update the index after running the pre-commit hook","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-08-18T13:54:09Z","receivedAt":"2010-08-18T13:54:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change git-commit to update the index after running the pre-commit\nhook, but before it constructs the comments that'll accompany the\ncommit message displayed in the $EDITOR.\n\nThe use case for this is e.g. a pre-commit hook that looks like this:\n\n #!/bin/sh\n echo unf >>hlagh\n git add hlagh\n\nIn that case the $EDITOR will display \"hlagh\" under \"Untracked files\",\nbut it should be under \"Changes to be committed:\". Refreshing before\nwe construct the message fixes this bug.\n\nReported-by: Hinrik Örn Sigurðsson \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\n2009/5/3 Hinrik Örn Sigurðsson :\n> I have a pre-commit hook which extracts documentation from file $foo\n> if it has pending changes to be committed. The hook creates/updates\n> the documentation file and calls \"git add\" on it.\n>\n> When I do \"git commit\", the COMMIT_EDITMSG delivered to my editor\n> notes that this documentation file has been created/updated, but not\n> that its changes have been added to the index. However, if I go ahead\n> with the commit, I can see that the doc file changes were indeed\n> committed.\n>\n> Here is a simplified test case:\n>\n> $ cat .git/hooks/pre-commit\n> #!/usr/bin/env perl\n> use strict;\n> use warnings;\n>\n> my $old = qx\"git rev-parse HEAD:foo 2>/dev/null\";\n> my $new = qx\"git rev-parse :foo 2>/dev/null\";\n>\n> if (($? >> 8) != 0 || $old ne $new) {\n> system \"cat source > dest\";\n> system \"git add dest\";\n> }\n>\n> $ ls\n> source\n>\n> $ echo 123 >> source\n>\n> $ git status\n> # On branch master\n> # Changed but not updated:\n> # (use \"git add ...\" to update what will be committed)\n> # (use \"git checkout -- ...\" to discard changes in working directory)\n> #\n> # modified: source\n> #\n> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>\n> $ git commit -a\n> Test commit\n> # Please enter the commit message for your changes. Lines starting\n> # with '#' will be ignored, and an empty message aborts the commit.\n> # On branch master\n> # Changes to be committed:\n> # (use \"git reset HEAD ...\" to unstage)\n> #\n> # modified: source\n> #\n> # Untracked files:\n> # (use \"git add ...\" to include in what will be committed)\n> #\n> # dest\n> [master 535ed7f] Test commit\n> 2 files changed, 3 insertions(+), 0 deletions(-)\n> create mode 100644 dest\n>\n> $ git status\n> # On branch master\n> nothing to commit (working directory clean)\n\nThanks for the report. Here's a proposed fix for this. I'm not\nfamiliar with the commit code so this may be the wrong way to go, but\nit fixes the bug and passes all tests.\n\nThe reporter has promised offlist to write a test for this.\n\n builtin/commit.c | 33 +++++++++++++++++++++++++--------\n 1 files changed, 25 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66fdd22..dbb4ff5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -550,6 +550,20 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n \treturn 1;\n }\n \n+static int update_index(const char *index_file) {\n+\tdiscard_cache();\n+\tread_cache_from(index_file);\n+\tif (!active_cache_tree)\n+\t\tactive_cache_tree = cache_tree();\n+\tif (cache_tree_update(active_cache_tree,\n+\t\t\t active_cache, active_nr, 0, 0) < 0) {\n+\t\terror(\"Error building trees\");\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t struct wt_status *s)\n {\n@@ -565,6 +579,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (!no_verify && run_hook(index_file, \"pre-commit\", NULL))\n \t\treturn 0;\n \n+\t/* Update the index after we run the pre-commit hook, but before\n+\t * we construct the message we're sending to the editor. The\n+\t * pre-commit hook may e.g. create a new file and add it to the\n+\t * index.\n+\t *\n+\t * Having that file show up as modified but not staged is confusing.\n+\t */\n+\tif (!update_index(index_file))\n+\t\treturn 0;\n+\n \tif (message.len) {\n \t\tstrbuf_addbuf(&sb, &message);\n \t\thook_arg1 = \"message\";\n@@ -728,15 +752,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t * and write it out as a tree. We must do this before we invoke\n \t * the editor and after we invoke run_status above.\n \t */\n-\tdiscard_cache();\n-\tread_cache_from(index_file);\n-\tif (!active_cache_tree)\n-\t\tactive_cache_tree = cache_tree();\n-\tif (cache_tree_update(active_cache_tree,\n-\t\t\t active_cache, active_nr, 0, 0) < 0) {\n-\t\terror(\"Error building trees\");\n+\tif (!update_index(index_file))\n \t\treturn 0;\n-\t}\n \n \tif (run_hook(index_file, \"prepare-commit-msg\",\n \t\t git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n-- \n1.7.2.1.414.g9bf49\n"},{"id":"148411","messageId":"7vwrrn8xz5.fsf@alter.siamese.dyndns.org","threadId":"19166","inReplyTo":"1282139649-21097-1-git-send-email-avarab@gmail.com","subject":"Re: [RFC/PATCH] commit: update the index after running the pre-commit hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-18T20:30:54Z","receivedAt":"2010-08-18T20:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change git-commit to update the index after running the pre-commit\n> hook, but before it constructs the comments that'll accompany the\n> commit message displayed in the $EDITOR.\n\nHmm.\n\nI have always thought that \"pre-commit\" is about verification, just like\nother pre-anything hooks. We have never sanctioned it to modify the tree,\nand the document does not say the hook is allowed to.\n\nAlthough I am not opposed to this patch, it is a change to the defined\nsemantics that is somewhat worrysome. We probably should document what it\nis and is not allowed to a bit better.\n"},{"id":"148418","messageId":"20100818225130.GC2346@burratino","threadId":"19166","inReplyTo":"1282139649-21097-1-git-send-email-avarab@gmail.com","subject":"Re: [RFC/PATCH] commit: update the index after running the pre-commit hook","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-18T22:51:30Z","receivedAt":"2010-08-18T22:51:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> +++ b/builtin/commit.c\n> @@ -565,6 +579,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \tif (!no_verify && run_hook(index_file, \"pre-commit\", NULL))\n> \t\treturn 0;\n> \n> +\t/* Update the index after we run the pre-commit hook, but before\n> +\t * we construct the message we're sending to the editor. The\n> +\t * pre-commit hook may e.g. create a new file and add it to the\n> +\t * index.\n> +\t *\n> +\t * Having that file show up as modified but not staged is confusing.\n> +\t */\n> +\tif (!update_index(index_file))\n> +\t\treturn 0;\n> +\n> \tif (message.len) {\n> \t\tstrbuf_addbuf(&sb, &message);\n> \t\thook_arg1 = \"message\";\n> @@ -728,15 +752,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t * and write it out as a tree. We must do this before we invoke\n> \t * the editor and after we invoke run_status above.\n> \t */\n> -\tdiscard_cache();\n> -\tread_cache_from(index_file);\n> -\tif (!active_cache_tree)\n> -\t\tactive_cache_tree = cache_tree();\n> -\tif (cache_tree_update(active_cache_tree,\n> -\t\t\t active_cache, active_nr, 0, 0) < 0) {\n> -\t\terror(\"Error building trees\");\n> +\tif (!update_index(index_file))\n> \t\treturn 0;\n> -\t}\n> \n> \tif (run_hook(index_file, \"prepare-commit-msg\",\n\nBefore, \"commit\" updated the index once, and after, twice. How does\nthis affect the running time on, say, the linux-2.6 tree? Could one\nof the update_index() calls be suppressed when there is no hook to\nrun?\n"}]}