{"thread":{"id":"54656","subject":"[PATCH] hooks: allow input from stdin","startedAt":"2020-11-17T15:02:58Z","lastAt":"2020-12-09T22:39:16Z","messageCount":22,"participants":["Orgad Shaneh via GitGitGadget","Junio C Hamano","Orgad Shaneh","Eric Sunshine","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"410083","messageId":"pull.790.git.1605625363309.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":null,"subject":"[PATCH] hooks: allow input from stdin","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-17T15:02:43Z","receivedAt":"2020-11-17T15:02:58Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nLet hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C,\n2007). Looks like the original shell implementation did have\nstdin open. Not clear why the author chose to close it on\nthe C port (maybe copy&paste).\n\nThe only hook that passes internal information to the hook\nvia stdin is pre-push, which has its own logic.\n\nSome references of users requesting this feature. Some of\nthem use acrobatics to gain access to stdin:\n[1] https://stackoverflow.com/q/1067874/764870\n[2] https://stackoverflow.com/q/47477766/764870\n[3] https://stackoverflow.com/q/3417896/764870\n[4] https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165\n[5] https://github.com/typicode/husky/issues/442\n\nSigned-off-by: Orgad Shaneh \n---\n hooks: allow input from stdin\n \n Let hooks receive user input if applicable.\n \n Closing stdin originates in f5bbc3225 (Port git commit to C, 2007).\n Looks like the original shell implementation did have stdin open. Not\n clear why the author chose to close it on the C port (maybe copy&paste).\n \n The only hook that passes internal information to the hook via stdin is\n pre-push, which has its own logic.\n \n Some references of users requesting this feature. Some of them use\n acrobatics to gain access to stdin: [1] \n https://stackoverflow.com/q/1067874/764870[2] \n https://stackoverflow.com/q/47477766/764870[3] \n https://stackoverflow.com/q/3417896/764870[4] \n https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165[5] \n https://github.com/typicode/husky/issues/442\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/790\n\n run-command.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 2ee59acdc8..a17b613216 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1356,7 +1356,6 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \twhile ((p = va_arg(args, const char *)))\n \t\tstrvec_push(&hook.args, p);\n \thook.env = env;\n-\thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n \n\nbase-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\n-- \ngitgitgadget\n"},{"id":"410123","messageId":"xmqqh7pn69gd.fsf@gitster.c.googlers.com","threadId":"54656","inReplyTo":"pull.790.git.1605625363309.gitgitgadget@gmail.com","subject":"Re: [PATCH] hooks: allow input from stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-17T19:59:30Z","receivedAt":"2020-11-17T19:59:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Orgad Shaneh via GitGitGadget\" writes:\n\n> From: Orgad Shaneh \n>\n> Let hooks receive user input if applicable.\n>\n> Closing stdin originates in f5bbc3225 (Port git commit to C,\n> 2007). Looks like the original shell implementation did have\n> stdin open. Not clear why the author chose to close it on\n> the C port (maybe copy&paste).\n\nFor \"git commit\" leaving the standard input open and let hook\ninteract with the end-user sitting at the terminal might be OK, but\ndoing this for any and all hooks probably breaks callers of hooks\nthat deal with transport protocols where their standard input is\n*not* supposed to be molested (think: the main Git process is about\nto read a packstream over the network and spawns a hook using the\nstandard run_hook*() interface---if the hook reads standard input,\nthe main process would lose the initial part of the pack stream).\n\n> The only hook that passes internal information to the hook\n> via stdin is pre-push, which has its own logic.\n\nHmph, doesn't \"pre-receive\" hook gets information from its standard\ninput? In any case, it is natural that such hooks can and have to\nbe able to read from their standard input---they are spawned with\ntheir input connected to Git process that feeds them input that are\nnecessary for them to make decision, so comparing them with random\nother hooks that do not use information from Git to do their thing\nis comparing apples and oranges.\n\nSo I think the patch we see as-is is probably not a good idea,\nprimarily because it lets run_hook_ve() to pretend that it still is\na generic interface to run any hooks by sitting in run_command.c,\nbut now its behaviour has been tweaked to fit only needs by \"git\ncommit\" and the like. You probably need to add a new \"options\"\nparameter to run_hook_ve() that allows the callers to say \"this hook\nis allowed to read from the standard input\" etc., if we want to keep\nit in run_command.c and let it pretend to be generally useful\ninterface.\n\nAnother possibility is to remove run_hook_ve(), and open code its\nbody in commit.c::run_commit_hook(), but that is less than ideal.\n\n> Some references of users requesting this feature. Some of\n> them use acrobatics to gain access to stdin:\n> [1] https://stackoverflow.com/q/1067874/764870\n> [2] https://stackoverflow.com/q/47477766/764870\n> [3] https://stackoverflow.com/q/3417896/764870\n> [4] https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165\n> [5] https://github.com/typicode/husky/issues/442\n\nInstead of wasting 7 lines here, it would be a much better approach\nto just add a test or two that demonstrate sample usages and that\nprotect this new feature from accidentally getting broken at the\nsame time.\n\n> The only hook that passes internal information to the hook via stdin is\n> pre-push, which has its own logic.\n\n> \n> Some references of users requesting this feature. Some of them use\n> acrobatics to gain access to stdin: [1] \n> https://stackoverflow.com/q/1067874/764870[2] \n> https://stackoverflow.com/q/47477766/764870[3] \n> https://stackoverflow.com/q/3417896/764870[4] \n> https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165[5] \n> https://github.com/typicode/husky/issues/442\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/790\n>\n> run-command.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 2ee59acdc8..a17b613216 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1356,7 +1356,6 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n> \twhile ((p = va_arg(args, const char *)))\n> \t\tstrvec_push(&hook.args, p);\n> \thook.env = env;\n> -\thook.no_stdin = 1;\n> \thook.stdout_to_stderr = 1;\n> \thook.trace2_hook_name = name;\n> \n>\n> base-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\n"},{"id":"410358","messageId":"pull.790.v2.git.1605801043899.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.git.1605625363309.gitgitgadget@gmail.com","subject":"[PATCH v2] hooks: allow input from stdin","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-19T15:50:43Z","receivedAt":"2020-11-19T15:50:49Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nLet hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C,\n2007). Looks like the original shell implementation did have\nstdin open. Not clear why the author chose to close it on\nthe C port (maybe copy&paste).\n\nAllow stdin only for commit-related hooks. Some of the other\nhooks pass their own input to the hook, so don't change them.\n\nSigned-off-by: Orgad Shaneh \n---\n hooks: allow input from stdin\n \n Let hooks receive user input if applicable.\n \n Closing stdin originates in f5bbc3225 (Port git commit to C, 2007).\n Looks like the original shell implementation did have stdin open. Not\n clear why the author chose to close it on the C port (maybe copy&paste).\n \n The only hook that passes internal information to the hook via stdin is\n pre-push, which has its own logic.\n \n Some references of users requesting this feature. Some of them use\n acrobatics to gain access to stdin: [1] \n https://stackoverflow.com/q/1067874/764870[2] \n https://stackoverflow.com/q/47477766/764870[3] \n https://stackoverflow.com/q/3417896/764870[4] \n https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165[5] \n https://github.com/typicode/husky/issues/442\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/790\n\nRange-diff vs v1:\n\n 1: 4aa6b4c507 < -: ---------- hooks: allow input from stdin\n -: ---------- > 1: 6a9bcf9d8b hooks: allow input from stdin\n\n\n builtin/am.c | 6 +--\n builtin/checkout.c | 2 +-\n builtin/clone.c | 2 +-\n builtin/gc.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/rebase.c | 2 +-\n builtin/receive-pack.c | 2 +-\n commit.c | 2 +-\n read-cache.c | 2 +-\n reset.c | 2 +-\n run-command.c | 9 +++--\n run-command.h | 15 +++++---\n ...3-pre-commit-and-pre-merge-commit-hooks.sh | 37 ++++++++++++++++++-\n t/t7504-commit-msg-hook.sh | 15 ++++++++\n t/t7505-prepare-commit-msg-hook.sh | 14 +++++++\n 15 files changed, 92 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f22c73a05b..1946569d5b 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -428,7 +428,7 @@ static int run_applypatch_msg_hook(struct am_state *state)\n \tint ret;\n \n \tassert(state->msg);\n-\tret = run_hook_le(NULL, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n+\tret = run_hook_le(NULL, 0, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n \n \tif (!ret) {\n \t\tFREE_AND_NULL(state->msg);\n@@ -1559,7 +1559,7 @@ static void do_commit(const struct am_state *state)\n \tconst char *reflog_msg, *author, *committer = NULL;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tif (run_hook_le(NULL, \"pre-applypatch\", NULL))\n+\tif (run_hook_le(NULL, 0, \"pre-applypatch\", NULL))\n \t\texit(1);\n \n \tif (write_cache_as_tree(&tree, 0, NULL))\n@@ -1611,7 +1611,7 @@ static void do_commit(const struct am_state *state)\n \t\tfclose(fp);\n \t}\n \n-\trun_hook_le(NULL, \"post-applypatch\", NULL);\n+\trun_hook_le(NULL, 0, \"post-applypatch\", NULL);\n \n \tstrbuf_release(&sb);\n }\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b82119129..293e8ebd76 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -104,7 +104,7 @@ struct branch_info {\n static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,\n \t\t\t int changed)\n {\n-\treturn run_hook_le(NULL, \"post-checkout\",\n+\treturn run_hook_le(NULL, 0, \"post-checkout\",\n \t\t\t oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),\n \t\t\t oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),\n \t\t\t changed ? \"1\" : \"0\", NULL);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0841923cf..6cfd2f23be 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -816,7 +816,7 @@ static int checkout(int submodule_progress)\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\terr |= run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n+\terr |= run_hook_le(NULL, 0, \"post-checkout\", oid_to_hex(&null_oid),\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \n \tif (!err && (option_recurse_submodules.nr > 0)) {\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 5cd2a43f9f..c790a362df 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -385,7 +385,7 @@ static int need_to_gc(void)\n \telse\n \t\treturn 0;\n \n-\tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n+\tif (run_hook_le(NULL, 0, \"pre-auto-gc\", NULL))\n \t\treturn 0;\n \treturn 1;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4c133402a6..1f1d234879 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -474,7 +474,7 @@ static void finish(struct commit *head_commit,\n \t}\n \n \t/* Run a post-merge hook */\n-\trun_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n+\trun_hook_le(NULL, 0, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n \tapply_autostash(git_path_merge_autostash(the_repository));\n \tstrbuf_release(&reflog_message);\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 7b65525301..c17f7a628b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -2014,7 +2014,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \t/* If a hook exists, give it a chance to interrupt*/\n \tif (!ok_to_skip_pre_rebase &&\n-\t run_hook_le(NULL, \"pre-rebase\", options.upstream_arg,\n+\t run_hook_le(NULL, 0, \"pre-rebase\", options.upstream_arg,\n \t\t\targc ? argv[0] : NULL, NULL))\n \t\tdie(_(\"The pre-rebase hook refused to rebase.\"));\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..c1398af755 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1384,7 +1384,7 @@ static const char *push_to_checkout(unsigned char *hash,\n \t\t\t\t const char *work_tree)\n {\n \tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n-\tif (run_hook_le(env->v, push_to_checkout_hook,\n+\tif (run_hook_le(env->v, 0, push_to_checkout_hook,\n \t\t\thash_to_hex(hash), NULL))\n \t\treturn \"push-to-checkout hook declined\";\n \telse\ndiff --git a/commit.c b/commit.c\nindex fe1fa3dc41..775019ec9d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env.v, name, args);\n+\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n \tva_end(args);\n \tstrvec_clear(&hook_env);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex ecf6f68994..a83beac63e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3070,7 +3070,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \telse\n \t\tret = close_lock_file_gently(lock);\n \n-\trun_hook_le(NULL, \"post-index-change\",\n+\trun_hook_le(NULL, 0, \"post-index-change\",\n \t\t\tistate->updated_workdir ? \"1\" : \"0\",\n \t\t\tistate->updated_skipworktree ? \"1\" : \"0\", NULL);\n \tistate->updated_workdir = 0;\ndiff --git a/reset.c b/reset.c\nindex 2f4fbd07c5..33687b0b5b 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -127,7 +127,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \t\t\t\t\t reflog_head);\n \t}\n \tif (run_hook)\n-\t\trun_hook_le(NULL, \"post-checkout\",\n+\t\trun_hook_le(NULL, 0, \"post-checkout\",\n \t\t\t oid_to_hex(orig ? orig : &null_oid),\n \t\t\t oid_to_hex(oid), \"1\", NULL);\n \ndiff --git a/run-command.c b/run-command.c\nindex 2ee59acdc8..21b1f0a5e9 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1343,7 +1343,7 @@ const char *find_hook(const char *name)\n \treturn path.buf;\n }\n \n-int run_hook_ve(const char *const *env, const char *name, va_list args)\n+int run_hook_ve(const char *const *env, int opt, const char *name, va_list args)\n {\n \tstruct child_process hook = CHILD_PROCESS_INIT;\n \tconst char *p;\n@@ -1356,20 +1356,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \twhile ((p = va_arg(args, const char *)))\n \t\tstrvec_push(&hook.args, p);\n \thook.env = env;\n-\thook.no_stdin = 1;\n+\tif (!(opt & RUN_HOOK_ALLOW_STDIN))\n+\t\thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n \n \treturn run_command(&hook);\n }\n \n-int run_hook_le(const char *const *env, const char *name, ...)\n+int run_hook_le(const char *const *env, int opt, const char *name, ...)\n {\n \tva_list args;\n \tint ret;\n \n \tva_start(args, name);\n-\tret = run_hook_ve(env, name, args);\n+\tret = run_hook_ve(env, opt, name, args);\n \tva_end(args);\n \n \treturn ret;\ndiff --git a/run-command.h b/run-command.h\nindex 6472b38bde..e6a850c6fe 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -201,11 +201,16 @@ int run_command(struct child_process *);\n */\n const char *find_hook(const char *name);\n \n+#define RUN_HOOK_ALLOW_STDIN 1\n+\n /**\n * Run a hook.\n- * The first argument is a pathname to an index file, or NULL\n- * if the hook uses the default index file or no index is needed.\n- * The second argument is the name of the hook.\n+ * The first argument is an array of environment variables, or NULL\n+ * if the hook uses the default environment and doesn't require\n+ * additional variables.\n+ * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables\n+ * stdin for the child process (the default is no_stdin).\n+ * The third argument is the name of the hook.\n * The further arguments correspond to the hook arguments.\n * The last argument has to be NULL to terminate the arguments list.\n * If the hook does not exist or is not executable, the return\n@@ -215,8 +220,8 @@ const char *find_hook(const char *name);\n * On execution, .stdout_to_stderr and .no_stdin will be set.\n */\n LAST_ARG_MUST_BE_NULL\n-int run_hook_le(const char *const *env, const char *name, ...);\n-int run_hook_ve(const char *const *env, const char *name, va_list args);\n+int run_hook_le(const char *const *env, int opt, const char *name, ...);\n+int run_hook_ve(const char *const *env, int opt, const char *name, va_list args);\n \n /*\n * Trigger an auto-gc\ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex b3485450a2..e915ffe546 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n PRECOMMIT=\"$HOOKDIR/pre-commit\"\n PREMERGE=\"$HOOKDIR/pre-merge-commit\"\n+POSTCOMMIT=\"$HOOKDIR/post-commit\"\n \n # Prepare sample scripts that write their $0 to actual_hooks\n test_expect_success 'sample script setup' '\n@@ -28,11 +29,15 @@ test_expect_success 'sample script setup' '\n \techo $0 >>actual_hooks\n \ttest $GIT_PREFIX = \"success/\"\n \tEOF\n-\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF\n+\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF &&\n \techo $0 >>actual_hooks\n \ttest \"$GIT_AUTHOR_NAME\" = \"New Author\" &&\n \ttest \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n \tEOF\n+\twrite_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n+\t! read -r line || echo \"$line\" > hook_input\n+\texit 0\n+\tEOF\n '\n \n test_expect_success 'root commit' '\n@@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' '\n \ttest_cmp expected_hooks actual_hooks\n '\n \n+test_expect_success 'with user input' '\n+\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n+\techo \"user input\" > user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" < user_input &&\n+\ttest_cmp user_input hook_input\n+'\n+\n+test_expect_success 'post-commit with user input' '\n+\ttest_when_finished \"rm -f \\\"$POSTCOMMIT\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$POSTCOMMIT\" &&\n+\techo \"user input\" > user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" < user_input &&\n+\ttest_cmp user_input hook_input\n+'\n+\n+test_expect_success 'with user input (merge)' '\n+\ttest_when_finished \"rm -f \\\"$PREMERGE\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$PREMERGE\" &&\n+\techo \"user input\" > user_input &&\n+\tgit checkout side &&\n+\tgit merge -m \"merge master\" master < user_input &&\n+\tgit checkout master &&\n+\ttest_cmp user_input hook_input\n+'\n+\n test_done\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 31b9c6a2c1..ad467aad86 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -294,5 +294,20 @@ test_expect_success 'hook is called for reword during `rebase -i`' '\n \n '\n \n+# now a hook that accepts input and writes it as the commit message\n+cat > \"$HOOK\" <<'EOF'\n+#!/bin/sh\n+! read -r line || echo \"$line\" > \"$1\"\n+EOF\n+chmod +x \"$HOOK\"\n+\n+test_expect_success 'hook with user input' '\n+\n+\techo \"additional\" >> file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"additional\" &&\n+\tcommit_msg_is \"user input\"\n+\n+'\n \n test_done\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 94f85cdf83..16a161f129 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -91,6 +91,11 @@ else\n fi\n test \"$GIT_EDITOR\" = : && source=\"$source (no editor)\"\n \n+if read -r line\n+then\n+\tsource=\"$source $line\"\n+fi\n+\n if test $rebasing = 1\n then\n \techo \"$source $(get_last_cmd)\" >\"$1\"\n@@ -113,6 +118,15 @@ test_expect_success 'with hook (-m)' '\n \n '\n \n+test_expect_success 'with hook (-m and input)' '\n+\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"more\" &&\n+\ttest \"$(git log -1 --pretty=format:%s)\" = \"message (no editor) user input\"\n+\n+'\n+\n test_expect_success 'with hook (-m editor)' '\n \n \techo \"more\" >> file &&\n\nbase-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\n-- \ngitgitgadget\n"},{"id":"410368","messageId":"pull.790.v3.git.1605801376577.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v2.git.1605801043899.gitgitgadget@gmail.com","subject":"[PATCH v3] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-19T15:56:16Z","receivedAt":"2020-11-19T15:56:46Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nLet hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C,\n2007). Looks like the original shell implementation did have\nstdin open. Not clear why the author chose to close it on\nthe C port (maybe copy&paste).\n\nAllow stdin only for commit-related hooks. Some of the other\nhooks pass their own input to the hook, so don't change them.\n\nSigned-off-by: Orgad Shaneh \n---\n hooks: allow input from stdin for commit-related hooks\n \n Let hooks receive user input if applicable.\n \n Closing stdin originates in f5bbc3225 (Port git commit to C, 2007).\n Looks like the original shell implementation did have stdin open. Not\n clear why the author chose to close it on the C port (maybe copy&paste).\n \n The only hook that passes internal information to the hook via stdin is\n pre-push, which has its own logic.\n \n Some references of users requesting this feature. Some of them use\n acrobatics to gain access to stdin: [1] \n https://stackoverflow.com/q/1067874/764870[2] \n https://stackoverflow.com/q/47477766/764870[3] \n https://stackoverflow.com/q/3417896/764870[4] \n https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165[5] \n https://github.com/typicode/husky/issues/442\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/790\n\nRange-diff vs v2:\n\n 1: 6a9bcf9d8b ! 1: 2f7a45c828 hooks: allow input from stdin\n @@ Metadata\n Author: Orgad Shaneh \n \n ## Commit message ##\n - hooks: allow input from stdin\n + hooks: allow input from stdin for commit-related hooks\n \n Let hooks receive user input if applicable.\n \n\n\n builtin/am.c | 6 +--\n builtin/checkout.c | 2 +-\n builtin/clone.c | 2 +-\n builtin/gc.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/rebase.c | 2 +-\n builtin/receive-pack.c | 2 +-\n commit.c | 2 +-\n read-cache.c | 2 +-\n reset.c | 2 +-\n run-command.c | 9 +++--\n run-command.h | 15 +++++---\n ...3-pre-commit-and-pre-merge-commit-hooks.sh | 37 ++++++++++++++++++-\n t/t7504-commit-msg-hook.sh | 15 ++++++++\n t/t7505-prepare-commit-msg-hook.sh | 14 +++++++\n 15 files changed, 92 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f22c73a05b..1946569d5b 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -428,7 +428,7 @@ static int run_applypatch_msg_hook(struct am_state *state)\n \tint ret;\n \n \tassert(state->msg);\n-\tret = run_hook_le(NULL, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n+\tret = run_hook_le(NULL, 0, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n \n \tif (!ret) {\n \t\tFREE_AND_NULL(state->msg);\n@@ -1559,7 +1559,7 @@ static void do_commit(const struct am_state *state)\n \tconst char *reflog_msg, *author, *committer = NULL;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tif (run_hook_le(NULL, \"pre-applypatch\", NULL))\n+\tif (run_hook_le(NULL, 0, \"pre-applypatch\", NULL))\n \t\texit(1);\n \n \tif (write_cache_as_tree(&tree, 0, NULL))\n@@ -1611,7 +1611,7 @@ static void do_commit(const struct am_state *state)\n \t\tfclose(fp);\n \t}\n \n-\trun_hook_le(NULL, \"post-applypatch\", NULL);\n+\trun_hook_le(NULL, 0, \"post-applypatch\", NULL);\n \n \tstrbuf_release(&sb);\n }\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b82119129..293e8ebd76 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -104,7 +104,7 @@ struct branch_info {\n static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,\n \t\t\t int changed)\n {\n-\treturn run_hook_le(NULL, \"post-checkout\",\n+\treturn run_hook_le(NULL, 0, \"post-checkout\",\n \t\t\t oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),\n \t\t\t oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),\n \t\t\t changed ? \"1\" : \"0\", NULL);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0841923cf..6cfd2f23be 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -816,7 +816,7 @@ static int checkout(int submodule_progress)\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\terr |= run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n+\terr |= run_hook_le(NULL, 0, \"post-checkout\", oid_to_hex(&null_oid),\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \n \tif (!err && (option_recurse_submodules.nr > 0)) {\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 5cd2a43f9f..c790a362df 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -385,7 +385,7 @@ static int need_to_gc(void)\n \telse\n \t\treturn 0;\n \n-\tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n+\tif (run_hook_le(NULL, 0, \"pre-auto-gc\", NULL))\n \t\treturn 0;\n \treturn 1;\n }\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4c133402a6..1f1d234879 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -474,7 +474,7 @@ static void finish(struct commit *head_commit,\n \t}\n \n \t/* Run a post-merge hook */\n-\trun_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n+\trun_hook_le(NULL, 0, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n \tapply_autostash(git_path_merge_autostash(the_repository));\n \tstrbuf_release(&reflog_message);\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 7b65525301..c17f7a628b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -2014,7 +2014,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \t/* If a hook exists, give it a chance to interrupt*/\n \tif (!ok_to_skip_pre_rebase &&\n-\t run_hook_le(NULL, \"pre-rebase\", options.upstream_arg,\n+\t run_hook_le(NULL, 0, \"pre-rebase\", options.upstream_arg,\n \t\t\targc ? argv[0] : NULL, NULL))\n \t\tdie(_(\"The pre-rebase hook refused to rebase.\"));\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..c1398af755 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1384,7 +1384,7 @@ static const char *push_to_checkout(unsigned char *hash,\n \t\t\t\t const char *work_tree)\n {\n \tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n-\tif (run_hook_le(env->v, push_to_checkout_hook,\n+\tif (run_hook_le(env->v, 0, push_to_checkout_hook,\n \t\t\thash_to_hex(hash), NULL))\n \t\treturn \"push-to-checkout hook declined\";\n \telse\ndiff --git a/commit.c b/commit.c\nindex fe1fa3dc41..775019ec9d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env.v, name, args);\n+\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n \tva_end(args);\n \tstrvec_clear(&hook_env);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex ecf6f68994..a83beac63e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -3070,7 +3070,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \telse\n \t\tret = close_lock_file_gently(lock);\n \n-\trun_hook_le(NULL, \"post-index-change\",\n+\trun_hook_le(NULL, 0, \"post-index-change\",\n \t\t\tistate->updated_workdir ? \"1\" : \"0\",\n \t\t\tistate->updated_skipworktree ? \"1\" : \"0\", NULL);\n \tistate->updated_workdir = 0;\ndiff --git a/reset.c b/reset.c\nindex 2f4fbd07c5..33687b0b5b 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -127,7 +127,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,\n \t\t\t\t\t reflog_head);\n \t}\n \tif (run_hook)\n-\t\trun_hook_le(NULL, \"post-checkout\",\n+\t\trun_hook_le(NULL, 0, \"post-checkout\",\n \t\t\t oid_to_hex(orig ? orig : &null_oid),\n \t\t\t oid_to_hex(oid), \"1\", NULL);\n \ndiff --git a/run-command.c b/run-command.c\nindex 2ee59acdc8..21b1f0a5e9 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1343,7 +1343,7 @@ const char *find_hook(const char *name)\n \treturn path.buf;\n }\n \n-int run_hook_ve(const char *const *env, const char *name, va_list args)\n+int run_hook_ve(const char *const *env, int opt, const char *name, va_list args)\n {\n \tstruct child_process hook = CHILD_PROCESS_INIT;\n \tconst char *p;\n@@ -1356,20 +1356,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \twhile ((p = va_arg(args, const char *)))\n \t\tstrvec_push(&hook.args, p);\n \thook.env = env;\n-\thook.no_stdin = 1;\n+\tif (!(opt & RUN_HOOK_ALLOW_STDIN))\n+\t\thook.no_stdin = 1;\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n \n \treturn run_command(&hook);\n }\n \n-int run_hook_le(const char *const *env, const char *name, ...)\n+int run_hook_le(const char *const *env, int opt, const char *name, ...)\n {\n \tva_list args;\n \tint ret;\n \n \tva_start(args, name);\n-\tret = run_hook_ve(env, name, args);\n+\tret = run_hook_ve(env, opt, name, args);\n \tva_end(args);\n \n \treturn ret;\ndiff --git a/run-command.h b/run-command.h\nindex 6472b38bde..e6a850c6fe 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -201,11 +201,16 @@ int run_command(struct child_process *);\n */\n const char *find_hook(const char *name);\n \n+#define RUN_HOOK_ALLOW_STDIN 1\n+\n /**\n * Run a hook.\n- * The first argument is a pathname to an index file, or NULL\n- * if the hook uses the default index file or no index is needed.\n- * The second argument is the name of the hook.\n+ * The first argument is an array of environment variables, or NULL\n+ * if the hook uses the default environment and doesn't require\n+ * additional variables.\n+ * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables\n+ * stdin for the child process (the default is no_stdin).\n+ * The third argument is the name of the hook.\n * The further arguments correspond to the hook arguments.\n * The last argument has to be NULL to terminate the arguments list.\n * If the hook does not exist or is not executable, the return\n@@ -215,8 +220,8 @@ const char *find_hook(const char *name);\n * On execution, .stdout_to_stderr and .no_stdin will be set.\n */\n LAST_ARG_MUST_BE_NULL\n-int run_hook_le(const char *const *env, const char *name, ...);\n-int run_hook_ve(const char *const *env, const char *name, va_list args);\n+int run_hook_le(const char *const *env, int opt, const char *name, ...);\n+int run_hook_ve(const char *const *env, int opt, const char *name, va_list args);\n \n /*\n * Trigger an auto-gc\ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex b3485450a2..e915ffe546 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n PRECOMMIT=\"$HOOKDIR/pre-commit\"\n PREMERGE=\"$HOOKDIR/pre-merge-commit\"\n+POSTCOMMIT=\"$HOOKDIR/post-commit\"\n \n # Prepare sample scripts that write their $0 to actual_hooks\n test_expect_success 'sample script setup' '\n@@ -28,11 +29,15 @@ test_expect_success 'sample script setup' '\n \techo $0 >>actual_hooks\n \ttest $GIT_PREFIX = \"success/\"\n \tEOF\n-\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF\n+\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF &&\n \techo $0 >>actual_hooks\n \ttest \"$GIT_AUTHOR_NAME\" = \"New Author\" &&\n \ttest \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n \tEOF\n+\twrite_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n+\t! read -r line || echo \"$line\" > hook_input\n+\texit 0\n+\tEOF\n '\n \n test_expect_success 'root commit' '\n@@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' '\n \ttest_cmp expected_hooks actual_hooks\n '\n \n+test_expect_success 'with user input' '\n+\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n+\techo \"user input\" > user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" < user_input &&\n+\ttest_cmp user_input hook_input\n+'\n+\n+test_expect_success 'post-commit with user input' '\n+\ttest_when_finished \"rm -f \\\"$POSTCOMMIT\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$POSTCOMMIT\" &&\n+\techo \"user input\" > user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" < user_input &&\n+\ttest_cmp user_input hook_input\n+'\n+\n+test_expect_success 'with user input (merge)' '\n+\ttest_when_finished \"rm -f \\\"$PREMERGE\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$PREMERGE\" &&\n+\techo \"user input\" > user_input &&\n+\tgit checkout side &&\n+\tgit merge -m \"merge master\" master < user_input &&\n+\tgit checkout master &&\n+\ttest_cmp user_input hook_input\n+'\n+\n test_done\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 31b9c6a2c1..ad467aad86 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -294,5 +294,20 @@ test_expect_success 'hook is called for reword during `rebase -i`' '\n \n '\n \n+# now a hook that accepts input and writes it as the commit message\n+cat > \"$HOOK\" <<'EOF'\n+#!/bin/sh\n+! read -r line || echo \"$line\" > \"$1\"\n+EOF\n+chmod +x \"$HOOK\"\n+\n+test_expect_success 'hook with user input' '\n+\n+\techo \"additional\" >> file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"additional\" &&\n+\tcommit_msg_is \"user input\"\n+\n+'\n \n test_done\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 94f85cdf83..16a161f129 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -91,6 +91,11 @@ else\n fi\n test \"$GIT_EDITOR\" = : && source=\"$source (no editor)\"\n \n+if read -r line\n+then\n+\tsource=\"$source $line\"\n+fi\n+\n if test $rebasing = 1\n then\n \techo \"$source $(get_last_cmd)\" >\"$1\"\n@@ -113,6 +118,15 @@ test_expect_success 'with hook (-m)' '\n \n '\n \n+test_expect_success 'with hook (-m and input)' '\n+\n+\techo \"more\" >> file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"more\" &&\n+\ttest \"$(git log -1 --pretty=format:%s)\" = \"message (no editor) user input\"\n+\n+'\n+\n test_expect_success 'with hook (-m editor)' '\n \n \techo \"more\" >> file &&\n\nbase-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\n-- \ngitgitgadget\n"},{"id":"410374","messageId":"xmqqwnyhxilp.fsf@gitster.c.googlers.com","threadId":"54656","inReplyTo":"pull.790.v3.git.1605801376577.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] hooks: allow input from stdin for commit-related hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-19T19:16:34Z","receivedAt":"2020-11-19T19:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Orgad Shaneh via GitGitGadget\" writes:\n\n> From: Orgad Shaneh \n>\n> Let hooks receive user input if applicable.\n>\n> Closing stdin originates in f5bbc3225 (Port git commit to C,\n> 2007). Looks like the original shell implementation did have\n> stdin open. Not clear why the author chose to close it on\n> the C port (maybe copy&paste).\n\nPlease drop unsubstanciated guess in the parentheses. If anything,\nwe've learned during the discussion thread that it is a bad idea to\nleave the standard input open when spawning hooks in general, and to\nme it looks like a lot more plausible reason why we tightened the\ninterface when it was made from \"only to run hooks for 'git commit'\"\nto an interface that more widely usable to run any hook. If we are\nnot going to record that finding in the log message to help people\nto find out what we knew at the time when the commit was created,\nthen we shouldn't mislead them with \"maybe copy&paste\" that is not\nbacked by anything other than a hunch.\n\n> Allow stdin only for commit-related hooks. Some of the other\n> hooks pass their own input to the hook, so don't change them.\n\nBefore this paragraph that gives orders to the code to \"be like so\",\nthe log message needs to explain why it is a good idea to make such\na change. Which hook benefits by being able to read the standard\ninput? Describe what becomes possible in terms of end-user visible\neffects (i.e. \"now reading standard input becomes possible for\npre-commit hook\" is *not* an answer. What new things a pre-commit\nhook that now can read from the standard input do for the end user?)\nto justify why such a change is a good thing to have, before this\nparagraph to justify why leaving the standard input open for hooks\nrun by \"git commit\" is a good idea and is a safe thing to do.\n\nNote that even \"git commit\" may compete for its standard input with\nhooks. \"git commit -F - diff --git a/run-command.h b/run-command.h\n> index 6472b38bde..e6a850c6fe 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -201,11 +201,16 @@ int run_command(struct child_process *);\n> */\n> const char *find_hook(const char *name);\n> \n> +#define RUN_HOOK_ALLOW_STDIN 1\n> +\n> /**\n> * Run a hook.\n> - * The first argument is a pathname to an index file, or NULL\n> - * if the hook uses the default index file or no index is needed.\n> - * The second argument is the name of the hook.\n> + * The first argument is an array of environment variables, or NULL\n> + * if the hook uses the default environment and doesn't require\n> + * additional variables.\n> + * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables\n> + * stdin for the child process (the default is no_stdin).\n> + * The third argument is the name of the hook.\n> * The further arguments correspond to the hook arguments.\n> * The last argument has to be NULL to terminate the arguments list.\n> * If the hook does not exist or is not executable, the return\n> @@ -215,8 +220,8 @@ const char *find_hook(const char *name);\n> * On execution, .stdout_to_stderr and .no_stdin will be set.\n> */\n> LAST_ARG_MUST_BE_NULL\n> -int run_hook_le(const char *const *env, const char *name, ...);\n> -int run_hook_ve(const char *const *env, const char *name, va_list args);\n> +int run_hook_le(const char *const *env, int opt, const char *name, ...);\n> +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args);\n\nIs this new parameter meant to be used as an enum? When the\nrun_hook interface gets extended the next time and we want a new\noption, is the option expected to be mutually incompatible with\nallow-stdin? \n\nI suspect that it would make this a more useful API if this new\nparameter is not used as an enum but as a collection of flag bits.\n\nIf so, a few things must change in the above:\n\n - The description of the second parameter in the comment shouldn't\n say \"zero or RUN_HOOK_ALLOW_STDIN\"; it should rather say \"an\n OR'ed collection of feature bits like RUN_HOOK_ALLOW_STDIN\n defined above\"\n\n - The second parameter should be 'unsigned flags', not 'int opt'.\n\nIt is my understanding that \"git commit\" only needs run_hook_ve() to\ndrive its hook scripts. Isn't it premature to touch run_hook_le(),\nin which nobody wants to leave the standard input open while running\nhooks? It _might_ be a better idea to allow users of _le() to do\nthe same eventually, but then perhaps it is a good idea to do so in\na separate step at the end, as \"only to be complete\" patch. That\nis, the structure of the topic ought to be something like:\n\n - [PATCH 1/2] add the \"unsigned flags\" word to _ve(), assign the\n RUN_HOOK_ALLOW_STDIN bit, and update commit.c::run_commit_hook()\n to pass RUN_HOOK_ALLOW_STDIN to it.\n\n - [PATCH 2/2] after surveying the options \"git commit\" takes, find\n out the condition where \"git commit\" itself would want to consume\n the standard input (e.g. \"commit -F -\", there may be others), and\n tell run_commit_hook() *not* to pass RUN_HOOK_ALLOW_STDIN when we\n use the standard input ourselves (i.e. forbid hooks to read from\n it).\n\n - [PATCH 3/2] add the same \"unsigned flags\" word to _le(), and\n teach all callers to pass 0, as a \"just for completeness\" step.\n\nPersonally, I think we should stop at [2/2], and do not do [3/2], as\nthere is no real demonstrated use of the standard input for hooks.\nEspecially because users of the _le() interface includes programs\nlike receive-pack whose standard input should not be molested, I'd\nfeel safer not to see [3/2] done at all (for that matter, I'm not\nhappy with [1/2] unless it comes with [2/2], either).\n\n> diff --git a/run-command.c b/run-command.c\n> index 2ee59acdc8..21b1f0a5e9 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1343,7 +1343,7 @@ const char *find_hook(const char *name)\n> \treturn path.buf;\n> }\n> \n> -int run_hook_ve(const char *const *env, const char *name, va_list args)\n> +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args)\n> {\n> \tstruct child_process hook = CHILD_PROCESS_INIT;\n> \tconst char *p;\n> @@ -1356,20 +1356,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n> \twhile ((p = va_arg(args, const char *)))\n> \t\tstrvec_push(&hook.args, p);\n> \thook.env = env;\n> -\thook.no_stdin = 1;\n> +\tif (!(opt & RUN_HOOK_ALLOW_STDIN))\n> +\t\thook.no_stdin = 1;\n\nOK, so you are using the parameter as a flag word after all. Then\n\"int opt\" should definitely be \"unsigned flags\". And these two\nlines would be more readable, when written like so:\n\n\thook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n\nI would think.\n\n> \thook.stdout_to_stderr = 1;\n> \thook.trace2_hook_name = name;\n> \n> \treturn run_command(&hook);\n> }\n> \n> -int run_hook_le(const char *const *env, const char *name, ...)\n> +int run_hook_le(const char *const *env, int opt, const char *name, ...)\n> {\n> \tva_list args;\n> \tint ret;\n> \n> \tva_start(args, name);\n> -\tret = run_hook_ve(env, name, args);\n> +\tret = run_hook_ve(env, opt, name, args);\n\nI'd rather not to see the function signature of _le() changed in\npatches [1/2] and [2/2]; instead we can just pass hardcoded 0 from\nhere to the underlying _ve().\n\nNow, what is left is individual commands that use the run_hook\ninterface.\n\n> diff --git a/commit.c b/commit.c\n> index fe1fa3dc41..775019ec9d 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n> \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n> \n> \tva_start(args, name);\n> -\tret = run_hook_ve(hook_env.v, name, args);\n> +\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n> \tva_end(args);\n> \tstrvec_clear(&hook_env);\n> \n\nThis is good for [1/2]. We should avoid \"git commit\" from competing\nwith hooks for its standard input by conditionally passing\nALLOW_STDIN from here---only when the program itself does not use\nthe standard input in [2/2].\n\n> diff --git a/builtin/am.c b/builtin/am.c\n\n\"git am diff --git a/builtin/checkout.c b/builtin/checkout.c\n\nI do not offhand think of a reason why \"git checkout\" would compete\nwith its hooks for the standard input. If we were to allow hooks to\nread from the standard input, that should come as an independent\npatch for each program after patch [3/2], I think. The ones I don't\nmention below should never leave the standard input open for hooks.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> diff --git a/reset.c b/reset.c\n\nDitto.\n\n\n> diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n> index b3485450a2..e915ffe546 100755\n> --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n> +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n> @@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n> HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n> PRECOMMIT=\"$HOOKDIR/pre-commit\"\n> PREMERGE=\"$HOOKDIR/pre-merge-commit\"\n> +POSTCOMMIT=\"$HOOKDIR/post-commit\"\n> \n> # Prepare sample scripts that write their $0 to actual_hooks\n> test_expect_success 'sample script setup' '\n> @@ -28,11 +29,15 @@ test_expect_success 'sample script setup' '\n> \techo $0 >>actual_hooks\n> \ttest $GIT_PREFIX = \"success/\"\n> \tEOF\n> -\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF\n> +\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF &&\n> \techo $0 >>actual_hooks\n> \ttest \"$GIT_AUTHOR_NAME\" = \"New Author\" &&\n> \ttest \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n> \tEOF\n> +\twrite_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n> +\t! read -r line || echo \"$line\" > hook_input\n> +\texit 0\n\nStyle (Documentation/CodingGuidelines)\n\n - Redirection operators should be written with space before, but no\n space after them. In other words, write 'echo test >\"$file\"'\n instead of 'echo test> $file' or 'echo test > $file'.\n\nSo, when our \"read\" immediately hits EOF (or I/O error, but let's\nnot worry about that case for now), we leave hook_input file alone,\nbut otherwise we write the single line we read to hook_input, and\nregardless of an error, we report success to the invoking \"git\ncommit\". Which makes sense.\n\nWe report success even when we had trouble writing into hook_input\nfile, though. Perhaps you should lose the \"exit 0\" at the end?\n\n> +\tEOF\n> '\n> \n> test_expect_success 'root commit' '\n> @@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' '\n> \ttest_cmp expected_hooks actual_hooks\n> '\n> \n> +test_expect_success 'with user input' '\n> +\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n> +\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n> +\techo \"user input\" > user_input &&\n\nStyle (I won't repeat from here on).\n\n> +\techo \"more\" >>file &&\n> +\tgit add file &&\n> +\tgit commit -m \"more\" < user_input &&\n> +\ttest_cmp user_input hook_input\n> +'\n\nThis is probably a good place to also test\n\n\tgit commit -F - wrote:\n>\n> \"Orgad Shaneh via GitGitGadget\" writes:\n>\n> > From: Orgad Shaneh \n> >\n> > Let hooks receive user input if applicable.\n> >\n> > Closing stdin originates in f5bbc3225 (Port git commit to C,\n> > 2007). Looks like the original shell implementation did have\n> > stdin open. Not clear why the author chose to close it on\n> > the C port (maybe copy&paste).\n>\n> Please drop unsubstanciated guess in the parentheses. If anything,\n> we've learned during the discussion thread that it is a bad idea to\n> leave the standard input open when spawning hooks in general, and to\n> me it looks like a lot more plausible reason why we tightened the\n> interface when it was made from \"only to run hooks for 'git commit'\"\n> to an interface that more widely usable to run any hook. If we are\n> not going to record that finding in the log message to help people\n> to find out what we knew at the time when the commit was created,\n> then we shouldn't mislead them with \"maybe copy&paste\" that is not\n> backed by anything other than a hunch.\n\nDone\n\n> > Allow stdin only for commit-related hooks. Some of the other\n> > hooks pass their own input to the hook, so don't change them.\n>\n> Before this paragraph that gives orders to the code to \"be like so\",\n> the log message needs to explain why it is a good idea to make such\n> a change. Which hook benefits by being able to read the standard\n> input? Describe what becomes possible in terms of end-user visible\n> effects (i.e. \"now reading standard input becomes possible for\n> pre-commit hook\" is *not* an answer. What new things a pre-commit\n> hook that now can read from the standard input do for the end user?)\n> to justify why such a change is a good thing to have, before this\n> paragraph to justify why leaving the standard input open for hooks\n> run by \"git commit\" is a good idea and is a safe thing to do.\n\nAdded justification.\n\n> Note that even \"git commit\" may compete for its standard input with\n> hooks. \"git commit -F - EOF before doing anything interesting like spawning a hook, but it\n> is not implausible that the reading of the message may want to\n> happen much later in a future codebase, at which point the hook may\n> end up stealing the beginning of the message by reading from the\n> standard input. So ideally, if we can find a way to selectively\n> close the standard input for the hooks if \"git commit\" itself uses\n> the standard input, that would be better than unconditionally\n> leaving it open.\n\nGood catch! I found that pre-commit is called before reading stdin,\nso -F - is broken if pre-commit reads the input. Not sure what's the\nbest way to solve this. Should I pass the flag to run_commit_hook\neverywhere? Or maybe add a new opt-out flag for run_commit_hook,\nand pass 0 on most calls?\n\n> Let's reorder the patch hunks to see the bottom layer first, as the\n> callers are mostly the same.\n>\n> > diff --git a/run-command.h b/run-command.h\n> > index 6472b38bde..e6a850c6fe 100644\n> > --- a/run-command.h\n> > +++ b/run-command.h\n> > @@ -201,11 +201,16 @@ int run_command(struct child_process *);\n> > */\n> > const char *find_hook(const char *name);\n> >\n> > +#define RUN_HOOK_ALLOW_STDIN 1\n> > +\n> > /**\n> > * Run a hook.\n> > - * The first argument is a pathname to an index file, or NULL\n> > - * if the hook uses the default index file or no index is needed.\n> > - * The second argument is the name of the hook.\n> > + * The first argument is an array of environment variables, or NULL\n> > + * if the hook uses the default environment and doesn't require\n> > + * additional variables.\n> > + * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables\n> > + * stdin for the child process (the default is no_stdin).\n> > + * The third argument is the name of the hook.\n> > * The further arguments correspond to the hook arguments.\n> > * The last argument has to be NULL to terminate the arguments list.\n> > * If the hook does not exist or is not executable, the return\n> > @@ -215,8 +220,8 @@ const char *find_hook(const char *name);\n> > * On execution, .stdout_to_stderr and .no_stdin will be set.\n> > */\n> > LAST_ARG_MUST_BE_NULL\n> > -int run_hook_le(const char *const *env, const char *name, ...);\n> > -int run_hook_ve(const char *const *env, const char *name, va_list args);\n> > +int run_hook_le(const char *const *env, int opt, const char *name, ...);\n> > +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args);\n>\n> Is this new parameter meant to be used as an enum? When the\n> run_hook interface gets extended the next time and we want a new\n> option, is the option expected to be mutually incompatible with\n> allow-stdin?\n\nYou found it :)\n\n> I suspect that it would make this a more useful API if this new\n> parameter is not used as an enum but as a collection of flag bits.\n>\n> If so, a few things must change in the above:\n>\n> - The description of the second parameter in the comment shouldn't\n> say \"zero or RUN_HOOK_ALLOW_STDIN\"; it should rather say \"an\n> OR'ed collection of feature bits like RUN_HOOK_ALLOW_STDIN\n> defined above\"\n\nDone, thanks.\n\n> - The second parameter should be 'unsigned flags', not 'int opt'.\n\nI copied from run_command_v_opt*, which have int opt for flags. Changed anyway.\n\n> It is my understanding that \"git commit\" only needs run_hook_ve() to\n> drive its hook scripts. Isn't it premature to touch run_hook_le(),\n> in which nobody wants to leave the standard input open while running\n> hooks? It _might_ be a better idea to allow users of _le() to do\n> the same eventually, but then perhaps it is a good idea to do so in\n> a separate step at the end, as \"only to be complete\" patch. That\n> is, the structure of the topic ought to be something like:\n>\n> - [PATCH 1/2] add the \"unsigned flags\" word to _ve(), assign the\n> RUN_HOOK_ALLOW_STDIN bit, and update commit.c::run_commit_hook()\n> to pass RUN_HOOK_ALLOW_STDIN to it.\n>\n> - [PATCH 2/2] after surveying the options \"git commit\" takes, find\n> out the condition where \"git commit\" itself would want to consume\n> the standard input (e.g. \"commit -F -\", there may be others), and\n> tell run_commit_hook() *not* to pass RUN_HOOK_ALLOW_STDIN when we\n> use the standard input ourselves (i.e. forbid hooks to read from\n> it).\n\nAccepted. Waiting for your feedback to implement this part.\n\n> - [PATCH 3/2] add the same \"unsigned flags\" word to _le(), and\n> teach all callers to pass 0, as a \"just for completeness\" step.\n>\n> Personally, I think we should stop at [2/2], and do not do [3/2], as\n> there is no real demonstrated use of the standard input for hooks.\n> Especially because users of the _le() interface includes programs\n> like receive-pack whose standard input should not be molested, I'd\n> feel safer not to see [3/2] done at all (for that matter, I'm not\n> happy with [1/2] unless it comes with [2/2], either).\n\nAgreed.\n\n> > + if (!(opt & RUN_HOOK_ALLOW_STDIN))\n> > + hook.no_stdin = 1;\n>\n> OK, so you are using the parameter as a flag word after all. Then\n> \"int opt\" should definitely be \"unsigned flags\". And these two\n> lines would be more readable, when written like so:\n>\n> hook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n>\n> I would think.\n\nDone.\n\n> > hook.stdout_to_stderr = 1;\n> > hook.trace2_hook_name = name;\n> >\n> > return run_command(&hook);\n> > }\n> >\n> > -int run_hook_le(const char *const *env, const char *name, ...)\n> > +int run_hook_le(const char *const *env, int opt, const char *name, ...)\n> > {\n> > va_list args;\n> > int ret;\n> >\n> > va_start(args, name);\n> > - ret = run_hook_ve(env, name, args);\n> > + ret = run_hook_ve(env, opt, name, args);\n>\n> I'd rather not to see the function signature of _le() changed in\n> patches [1/2] and [2/2]; instead we can just pass hardcoded 0 from\n> here to the underlying _ve().\n>\n> Now, what is left is individual commands that use the run_hook\n> interface.\n>\n> > diff --git a/commit.c b/commit.c\n> > index fe1fa3dc41..775019ec9d 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n> > strvec_push(&hook_env, \"GIT_EDITOR=:\");\n> >\n> > va_start(args, name);\n> > - ret = run_hook_ve(hook_env.v, name, args);\n> > + ret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n> > va_end(args);\n> > strvec_clear(&hook_env);\n> >\n>\n> This is good for [1/2]. We should avoid \"git commit\" from competing\n> with hooks for its standard input by conditionally passing\n> ALLOW_STDIN from here---only when the program itself does not use\n> the standard input in [2/2].\n>\n> > diff --git a/builtin/am.c b/builtin/am.c\n>\n> \"git am the end user via its stdin/stdout. There may be other situations\n> where the hooks should not touch the standard input.\n>\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n>\n> I do not offhand think of a reason why \"git checkout\" would compete\n> with its hooks for the standard input. If we were to allow hooks to\n> read from the standard input, that should come as an independent\n> patch for each program after patch [3/2], I think. The ones I don't\n> mention below should never leave the standard input open for hooks.\n>\n> > diff --git a/builtin/clone.c b/builtin/clone.c\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > diff --git a/builtin/merge.c b/builtin/merge.c\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > diff --git a/reset.c b/reset.c\n>\n> Ditto.\n>\n>\n> > diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n> > index b3485450a2..e915ffe546 100755\n> > --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n> > +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n> > @@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n> > HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n> > PRECOMMIT=\"$HOOKDIR/pre-commit\"\n> > PREMERGE=\"$HOOKDIR/pre-merge-commit\"\n> > +POSTCOMMIT=\"$HOOKDIR/post-commit\"\n> >\n> > # Prepare sample scripts that write their $0 to actual_hooks\n> > test_expect_success 'sample script setup' '\n> > @@ -28,11 +29,15 @@ test_expect_success 'sample script setup' '\n> > echo $0 >>actual_hooks\n> > test $GIT_PREFIX = \"success/\"\n> > EOF\n> > - write_script \"$HOOKDIR/check-author.sample\" <<-\\EOF\n> > + write_script \"$HOOKDIR/check-author.sample\" <<-\\EOF &&\n> > echo $0 >>actual_hooks\n> > test \"$GIT_AUTHOR_NAME\" = \"New Author\" &&\n> > test \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n> > EOF\n> > + write_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n> > + ! read -r line || echo \"$line\" > hook_input\n> > + exit 0\n>\n> Style (Documentation/CodingGuidelines)\n\nFixed.\n\n> - Redirection operators should be written with space before, but no\n> space after them. In other words, write 'echo test >\"$file\"'\n> instead of 'echo test> $file' or 'echo test > $file'.\n>\n> So, when our \"read\" immediately hits EOF (or I/O error, but let's\n> not worry about that case for now), we leave hook_input file alone,\n> but otherwise we write the single line we read to hook_input, and\n> regardless of an error, we report success to the invoking \"git\n> commit\". Which makes sense.\n>\n> We report success even when we had trouble writing into hook_input\n> file, though. Perhaps you should lose the \"exit 0\" at the end?\n\nRemoved.\n\n> > + EOF\n> > '\n> >\n> > test_expect_success 'root commit' '\n> > @@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' '\n> > test_cmp expected_hooks actual_hooks\n> > '\n> >\n> > +test_expect_success 'with user input' '\n> > + test_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n> > + cp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n> > + echo \"user input\" > user_input &&\n>\n> Style (I won't repeat from here on).\n\nFixed.\n\n> > + echo \"more\" >>file &&\n> > + git add file &&\n> > + git commit -m \"more\" < user_input &&\n> > + test_cmp user_input hook_input\n> > +'\n>\n> This is probably a good place to also test\n>\n> git commit -F - \n> and see what happens.\n\nAdded a test (currently failing).\n\n> Thanks.\n\nThank you! Your feedback is thorough and helpful.\n\n- Orgad\n"},{"id":"410380","messageId":"e048a9db62ccd7dd3a0e7a4475d2f8b307785de8.1605819390.git.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v4.git.1605819390.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] commit: fix stdin conflict between message and hook","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-19T20:56:30Z","receivedAt":"2020-11-19T20:56:56Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nIf git commit is executed with -F - (meaning read the commit message\nfrom stdin), and pre-commit hook is also reading from stdin, the\nmessage itself was consumed by the hook before reaching the point\nwhere it is read for the commit message.\n\nFix this by detecting this case, and passing this information to\nrun_commit_hook.\n\nSigned-off-by: Orgad Shaneh \n---\n builtin/commit.c | 14 +++++++++-----\n builtin/merge.c | 12 ++++++++----\n commit.c | 4 ++--\n commit.h | 3 ++-\n sequencer.c | 6 +++---\n t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 2 +-\n 6 files changed, 25 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 505fe60956..074a57937f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -695,11 +695,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n \tint old_display_comment_prefix;\n \tint merge_contains_scissors = 0;\n+\tint message_from_stdin = logfile && !strcmp(logfile, \"-\");\n+\tconst unsigned hook_flags = message_from_stdin ? 0 : RUN_HOOK_ALLOW_STDIN;\n \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\tif (!no_verify &&\n+\t run_commit_hook(use_editor, index_file, hook_flags, \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -724,7 +727,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (have_option_m && !fixup_message) {\n \t\tstrbuf_addbuf(&sb, &message);\n \t\thook_arg1 = \"message\";\n-\t} else if (logfile && !strcmp(logfile, \"-\")) {\n+\t} else if (message_from_stdin) {\n \t\tif (isatty(0))\n \t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\n \t\tif (strbuf_read(&sb, 0, 0) < 0)\n@@ -998,7 +1001,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n+\tif (run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"prepare-commit-msg\",\n \t\t\t git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n@@ -1015,7 +1018,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n+\t run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n+\t\t\t git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1701,7 +1705,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n-\trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n+\trun_commit_hook(use_editor, get_index_file(), RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4c133402a6..550b38cd20 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -822,8 +822,11 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n+\tif (!no_verify &&\n+\t run_commit_hook(0 < option_edit, index_file, RUN_HOOK_ALLOW_STDIN,\n+\t\t\t \"pre-merge-commit\", NULL)) {\n \t\tabort_commit(remoteheads, NULL);\n+\t}\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n \t * and write it out as a tree. We must do this before we invoke\n@@ -850,8 +853,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n \twrite_merge_heads(remoteheads);\n \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n-\tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n-\t\t\t git_path_merge_msg(the_repository), \"merge\", NULL))\n+\tif (run_commit_hook(0 < option_edit, get_index_file(), RUN_HOOK_ALLOW_STDIN,\n+\t\t\t \"prepare-commit-msg\", git_path_merge_msg(the_repository),\n+\t\t\t \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n@@ -859,7 +863,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t \"commit-msg\",\n+\t\t\t\t\t RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n \t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \ndiff --git a/commit.c b/commit.c\nindex 775019ec9d..3f5a50164e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,7 +1631,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)\n }\n \n int run_commit_hook(int editor_is_used, const char *index_file,\n-\t\t const char *name, ...)\n+\t\t unsigned flags, const char *name, ...)\n {\n \tstruct strvec hook_env = STRVEC_INIT;\n \tva_list args;\n@@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n+\tret = run_hook_ve(hook_env.v, flags, name, args);\n \tva_end(args);\n \tstrvec_clear(&hook_env);\n \ndiff --git a/commit.h b/commit.h\nindex 5467786c7b..72215d57fb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -352,6 +352,7 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)\n int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);\n \n LAST_ARG_MUST_BE_NULL\n-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n+int run_commit_hook(int editor_is_used, const char *index_file, unsigned flags,\n+\t\t const char *name, ...);\n \n #endif /* COMMIT_H */\ndiff --git a/sequencer.c b/sequencer.c\nindex 684ea9d5ce..505101c29c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1171,8 +1171,8 @@ static int run_prepare_commit_msg_hook(struct repository *r,\n \t} else {\n \t\targ1 = \"message\";\n \t}\n-\tif (run_commit_hook(0, r->index_file, \"prepare-commit-msg\", name,\n-\t\t\t arg1, arg2, NULL))\n+\tif (run_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN,\n+\t\t\t \"prepare-commit-msg\", name, arg1, arg2, NULL))\n \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n \n \treturn ret;\n@@ -1496,7 +1496,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n+\trun_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n \t\tcommit_post_rewrite(r, current_head, oid);\n \ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex 7bfb7435c6..a243b7efa1 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -292,7 +292,7 @@ test_expect_success 'with user input' '\n \ttest_cmp user_input hook_input\n '\n \n-test_expect_failure 'with user input combined with -F -' '\n+test_expect_success 'with user input combined with -F -' '\n \ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n \tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n \techo \"user input\" >user_input &&\n-- \ngitgitgadget\n"},{"id":"410381","messageId":"pull.790.v4.git.1605819390.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v3.git.1605801376577.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-19T20:56:28Z","receivedAt":"2020-11-19T20:56:56Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Let hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C, 2007). Looks\nlike the original shell implementation did have stdin open. Not clear why\nthe author chose to close it on the C port (maybe copy&paste).\n\nThe only hook that passes internal information to the hook via stdin is\npre-push, which has its own logic.\n\nSome references of users requesting this feature. Some of them use\nacrobatics to gain access to stdin: [1] \nhttps://stackoverflow.com/q/1067874/764870[2] \nhttps://stackoverflow.com/q/47477766/764870[3] \nhttps://stackoverflow.com/q/3417896/764870[4] \nhttps://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165[5] \nhttps://github.com/typicode/husky/issues/442\n\nOrgad Shaneh (2):\n hooks: allow input from stdin for commit-related hooks\n commit: fix stdin conflict between message and hook\n\n builtin/commit.c | 14 ++++--\n builtin/merge.c | 12 +++--\n commit.c | 4 +-\n commit.h | 3 +-\n run-command.c | 6 +--\n run-command.h | 17 +++++--\n sequencer.c | 6 +--\n ...3-pre-commit-and-pre-merge-commit-hooks.sh | 46 ++++++++++++++++++-\n t/t7504-commit-msg-hook.sh | 15 ++++++\n t/t7505-prepare-commit-msg-hook.sh | 14 ++++++\n 10 files changed, 113 insertions(+), 24 deletions(-)\n\n\nbase-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/790\n\nRange-diff vs v3:\n\n 1: 2f7a45c828 ! 1: 3bd6024a23 hooks: allow input from stdin for commit-related hooks\n @@ Commit message\n \n Closing stdin originates in f5bbc3225 (Port git commit to C,\n 2007). Looks like the original shell implementation did have\n - stdin open. Not clear why the author chose to close it on\n - the C port (maybe copy&paste).\n + stdin open.\n \n - Allow stdin only for commit-related hooks. Some of the other\n - hooks pass their own input to the hook, so don't change them.\n + This allows for example prompting the user to choose an issue\n + in prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n \n - Signed-off-by: Orgad Shaneh \n + Another possible use-case is running sanity test on pre-commit,\n + and having a prompt like \"This and that issue were found in your\n + changes. Are you sure you want to commit? [Y/N]\".\n \n - ## builtin/am.c ##\n -@@ builtin/am.c: static int run_applypatch_msg_hook(struct am_state *state)\n - \tint ret;\n - \n - \tassert(state->msg);\n --\tret = run_hook_le(NULL, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n -+\tret = run_hook_le(NULL, 0, \"applypatch-msg\", am_path(state, \"final-commit\"), NULL);\n - \n - \tif (!ret) {\n - \t\tFREE_AND_NULL(state->msg);\n -@@ builtin/am.c: static void do_commit(const struct am_state *state)\n - \tconst char *reflog_msg, *author, *committer = NULL;\n - \tstruct strbuf sb = STRBUF_INIT;\n - \n --\tif (run_hook_le(NULL, \"pre-applypatch\", NULL))\n -+\tif (run_hook_le(NULL, 0, \"pre-applypatch\", NULL))\n - \t\texit(1);\n - \n - \tif (write_cache_as_tree(&tree, 0, NULL))\n -@@ builtin/am.c: static void do_commit(const struct am_state *state)\n - \t\tfclose(fp);\n - \t}\n - \n --\trun_hook_le(NULL, \"post-applypatch\", NULL);\n -+\trun_hook_le(NULL, 0, \"post-applypatch\", NULL);\n - \n - \tstrbuf_release(&sb);\n - }\n -\n - ## builtin/checkout.c ##\n -@@ builtin/checkout.c: struct branch_info {\n - static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,\n - \t\t\t int changed)\n - {\n --\treturn run_hook_le(NULL, \"post-checkout\",\n -+\treturn run_hook_le(NULL, 0, \"post-checkout\",\n - \t\t\t oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),\n - \t\t\t oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),\n - \t\t\t changed ? \"1\" : \"0\", NULL);\n -\n - ## builtin/clone.c ##\n -@@ builtin/clone.c: static int checkout(int submodule_progress)\n - \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n - \t\tdie(_(\"unable to write new index file\"));\n - \n --\terr |= run_hook_le(NULL, \"post-checkout\", oid_to_hex(&null_oid),\n -+\terr |= run_hook_le(NULL, 0, \"post-checkout\", oid_to_hex(&null_oid),\n - \t\t\t oid_to_hex(&oid), \"1\", NULL);\n - \n - \tif (!err && (option_recurse_submodules.nr > 0)) {\n -\n - ## builtin/gc.c ##\n -@@ builtin/gc.c: static int need_to_gc(void)\n - \telse\n - \t\treturn 0;\n - \n --\tif (run_hook_le(NULL, \"pre-auto-gc\", NULL))\n -+\tif (run_hook_le(NULL, 0, \"pre-auto-gc\", NULL))\n - \t\treturn 0;\n - \treturn 1;\n - }\n -\n - ## builtin/merge.c ##\n -@@ builtin/merge.c: static void finish(struct commit *head_commit,\n - \t}\n - \n - \t/* Run a post-merge hook */\n --\trun_hook_le(NULL, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n -+\trun_hook_le(NULL, 0, \"post-merge\", squash ? \"1\" : \"0\", NULL);\n - \n - \tapply_autostash(git_path_merge_autostash(the_repository));\n - \tstrbuf_release(&reflog_message);\n + Allow stdin only for commit-related hooks. Some of the other\n + hooks pass their own input to the hook, so don't change them.\n \n - ## builtin/rebase.c ##\n -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n - \n - \t/* If a hook exists, give it a chance to interrupt*/\n - \tif (!ok_to_skip_pre_rebase &&\n --\t run_hook_le(NULL, \"pre-rebase\", options.upstream_arg,\n -+\t run_hook_le(NULL, 0, \"pre-rebase\", options.upstream_arg,\n - \t\t\targc ? argv[0] : NULL, NULL))\n - \t\tdie(_(\"The pre-rebase hook refused to rebase.\"));\n - \n + Note: If pre-commit reads from stdin, and git commit is executed\n + with -F - (read message from stdin), the message is not read\n + correctly. This is fixed in the follow-up commit.\n \n - ## builtin/receive-pack.c ##\n -@@ builtin/receive-pack.c: static const char *push_to_checkout(unsigned char *hash,\n - \t\t\t\t const char *work_tree)\n - {\n - \tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n --\tif (run_hook_le(env->v, push_to_checkout_hook,\n -+\tif (run_hook_le(env->v, 0, push_to_checkout_hook,\n - \t\t\thash_to_hex(hash), NULL))\n - \t\treturn \"push-to-checkout hook declined\";\n - \telse\n + Signed-off-by: Orgad Shaneh \n \n ## commit.c ##\n @@ commit.c: int run_commit_hook(int editor_is_used, const char *index_file,\n @@ commit.c: int run_commit_hook(int editor_is_used, const char *index_file,\n \tstrvec_clear(&hook_env);\n \n \n - ## read-cache.c ##\n -@@ read-cache.c: static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n - \telse\n - \t\tret = close_lock_file_gently(lock);\n - \n --\trun_hook_le(NULL, \"post-index-change\",\n -+\trun_hook_le(NULL, 0, \"post-index-change\",\n - \t\t\tistate->updated_workdir ? \"1\" : \"0\",\n - \t\t\tistate->updated_skipworktree ? \"1\" : \"0\", NULL);\n - \tistate->updated_workdir = 0;\n -\n - ## reset.c ##\n -@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char *action,\n - \t\t\t\t\t reflog_head);\n - \t}\n - \tif (run_hook)\n --\t\trun_hook_le(NULL, \"post-checkout\",\n -+\t\trun_hook_le(NULL, 0, \"post-checkout\",\n - \t\t\t oid_to_hex(orig ? orig : &null_oid),\n - \t\t\t oid_to_hex(oid), \"1\", NULL);\n - \n -\n ## run-command.c ##\n @@ run-command.c: const char *find_hook(const char *name)\n \treturn path.buf;\n }\n \n -int run_hook_ve(const char *const *env, const char *name, va_list args)\n -+int run_hook_ve(const char *const *env, int opt, const char *name, va_list args)\n ++int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args)\n {\n \tstruct child_process hook = CHILD_PROCESS_INIT;\n \tconst char *p;\n @@ run-command.c: int run_hook_ve(const char *const *env, const char *name, va_list\n \t\tstrvec_push(&hook.args, p);\n \thook.env = env;\n -\thook.no_stdin = 1;\n -+\tif (!(opt & RUN_HOOK_ALLOW_STDIN))\n -+\t\thook.no_stdin = 1;\n ++\thook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n \n - \treturn run_command(&hook);\n - }\n - \n --int run_hook_le(const char *const *env, const char *name, ...)\n -+int run_hook_le(const char *const *env, int opt, const char *name, ...)\n - {\n - \tva_list args;\n +@@ run-command.c: int run_hook_le(const char *const *env, const char *name, ...)\n \tint ret;\n \n \tva_start(args, name);\n -\tret = run_hook_ve(env, name, args);\n -+\tret = run_hook_ve(env, opt, name, args);\n ++\tret = run_hook_ve(env, 0, name, args);\n \tva_end(args);\n \n \treturn ret;\n @@ run-command.h: int run_command(struct child_process *);\n - * The first argument is a pathname to an index file, or NULL\n - * if the hook uses the default index file or no index is needed.\n - * The second argument is the name of the hook.\n -+ * The first argument is an array of environment variables, or NULL\n ++ * The env argument is an array of environment variables, or NULL\n + * if the hook uses the default environment and doesn't require\n + * additional variables.\n -+ * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables\n ++ * The flags argument is an OR'ed collection of feature bits like\n ++ * RUN_HOOK_ALLOW_STDIN defined above, which enables\n + * stdin for the child process (the default is no_stdin).\n -+ * The third argument is the name of the hook.\n ++ * The name argument is the name of the hook.\n * The further arguments correspond to the hook arguments.\n * The last argument has to be NULL to terminate the arguments list.\n * If the hook does not exist or is not executable, the return\n -@@ run-command.h: const char *find_hook(const char *name);\n - * On execution, .stdout_to_stderr and .no_stdin will be set.\n + * value will be zero.\n + * If it is executable, the hook will be executed and the exit\n + * status of the hook is returned.\n +- * On execution, .stdout_to_stderr and .no_stdin will be set.\n ++ * On execution, .stdout_to_stderr will be set, and .no_stdin will be\n ++ * set unless RUN_HOOK_ALLOW_STDIN flag is requested.\n */\n LAST_ARG_MUST_BE_NULL\n --int run_hook_le(const char *const *env, const char *name, ...);\n + int run_hook_le(const char *const *env, const char *name, ...);\n -int run_hook_ve(const char *const *env, const char *name, va_list args);\n -+int run_hook_le(const char *const *env, int opt, const char *name, ...);\n -+int run_hook_ve(const char *const *env, int opt, const char *name, va_list args);\n ++int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args);\n \n /*\n * Trigger an auto-gc\n @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'sample sc\n \ttest \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n \tEOF\n +\twrite_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n -+\t! read -r line || echo \"$line\" > hook_input\n -+\texit 0\n ++\t! read -r line || echo \"$line\" >hook_input\n +\tEOF\n '\n \n @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'check the\n +test_expect_success 'with user input' '\n +\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n +\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n -+\techo \"user input\" > user_input &&\n ++\techo \"user input\" >user_input &&\n +\techo \"more\" >>file &&\n +\tgit add file &&\n -+\tgit commit -m \"more\" < user_input &&\n ++\tgit commit -m \"more\" user_input &&\n ++\techo \"more\" >>file &&\n ++\tgit add file &&\n ++\tgit commit -F - user_input &&\n ++\techo \"user input\" >user_input &&\n +\techo \"more\" >>file &&\n +\tgit add file &&\n -+\tgit commit -m \"more\" < user_input &&\n ++\tgit commit -m \"more\" user_input &&\n ++\techo \"user input\" >user_input &&\n +\tgit checkout side &&\n -+\tgit merge -m \"merge master\" master < user_input &&\n ++\tgit merge -m \"merge master\" master \"$HOOK\" <<'EOF'\n ++cat >\"$HOOK\" <<'EOF'\n +#!/bin/sh\n -+! read -r line || echo \"$line\" > \"$1\"\n ++! read -r line || echo \"$line\" >\"$1\"\n +EOF\n +chmod +x \"$HOOK\"\n +\n +test_expect_success 'hook with user input' '\n +\n -+\techo \"additional\" >> file &&\n ++\techo \"additional\" >>file &&\n +\tgit add file &&\n +\techo \"user input\" | git commit -m \"additional\" &&\n +\tcommit_msg_is \"user input\"\n @@ t/t7505-prepare-commit-msg-hook.sh: test_expect_success 'with hook (-m)' '\n \n +test_expect_success 'with hook (-m and input)' '\n +\n -+\techo \"more\" >> file &&\n ++\techo \"more\" >>file &&\n +\tgit add file &&\n +\techo \"user input\" | git commit -m \"more\" &&\n +\ttest \"$(git log -1 --pretty=format:%s)\" = \"message (no editor) user input\"\n -: ---------- > 2: e048a9db62 commit: fix stdin conflict between message and hook\n\n-- \ngitgitgadget\n"},{"id":"410382","messageId":"3bd6024a236b061c89bb6b60daf3dc15ef1e32ca.1605819390.git.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v4.git.1605819390.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-19T20:56:29Z","receivedAt":"2020-11-19T20:56:57Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nLet hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C,\n2007). Looks like the original shell implementation did have\nstdin open.\n\nThis allows for example prompting the user to choose an issue\nin prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n\nAnother possible use-case is running sanity test on pre-commit,\nand having a prompt like \"This and that issue were found in your\nchanges. Are you sure you want to commit? [Y/N]\".\n\nAllow stdin only for commit-related hooks. Some of the other\nhooks pass their own input to the hook, so don't change them.\n\nNote: If pre-commit reads from stdin, and git commit is executed\nwith -F - (read message from stdin), the message is not read\ncorrectly. This is fixed in the follow-up commit.\n\nSigned-off-by: Orgad Shaneh \n---\n commit.c | 2 +-\n run-command.c | 6 +--\n run-command.h | 17 +++++--\n ...3-pre-commit-and-pre-merge-commit-hooks.sh | 46 ++++++++++++++++++-\n t/t7504-commit-msg-hook.sh | 15 ++++++\n t/t7505-prepare-commit-msg-hook.sh | 14 ++++++\n 6 files changed, 90 insertions(+), 10 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex fe1fa3dc41..775019ec9d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env.v, name, args);\n+\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n \tva_end(args);\n \tstrvec_clear(&hook_env);\n \ndiff --git a/run-command.c b/run-command.c\nindex 2ee59acdc8..38ce53bee5 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1343,7 +1343,7 @@ const char *find_hook(const char *name)\n \treturn path.buf;\n }\n \n-int run_hook_ve(const char *const *env, const char *name, va_list args)\n+int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args)\n {\n \tstruct child_process hook = CHILD_PROCESS_INIT;\n \tconst char *p;\n@@ -1356,7 +1356,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \twhile ((p = va_arg(args, const char *)))\n \t\tstrvec_push(&hook.args, p);\n \thook.env = env;\n-\thook.no_stdin = 1;\n+\thook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n \n@@ -1369,7 +1369,7 @@ int run_hook_le(const char *const *env, const char *name, ...)\n \tint ret;\n \n \tva_start(args, name);\n-\tret = run_hook_ve(env, name, args);\n+\tret = run_hook_ve(env, 0, name, args);\n \tva_end(args);\n \n \treturn ret;\ndiff --git a/run-command.h b/run-command.h\nindex 6472b38bde..e613e5e3f9 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -201,22 +201,29 @@ int run_command(struct child_process *);\n */\n const char *find_hook(const char *name);\n \n+#define RUN_HOOK_ALLOW_STDIN 1\n+\n /**\n * Run a hook.\n- * The first argument is a pathname to an index file, or NULL\n- * if the hook uses the default index file or no index is needed.\n- * The second argument is the name of the hook.\n+ * The env argument is an array of environment variables, or NULL\n+ * if the hook uses the default environment and doesn't require\n+ * additional variables.\n+ * The flags argument is an OR'ed collection of feature bits like\n+ * RUN_HOOK_ALLOW_STDIN defined above, which enables\n+ * stdin for the child process (the default is no_stdin).\n+ * The name argument is the name of the hook.\n * The further arguments correspond to the hook arguments.\n * The last argument has to be NULL to terminate the arguments list.\n * If the hook does not exist or is not executable, the return\n * value will be zero.\n * If it is executable, the hook will be executed and the exit\n * status of the hook is returned.\n- * On execution, .stdout_to_stderr and .no_stdin will be set.\n+ * On execution, .stdout_to_stderr will be set, and .no_stdin will be\n+ * set unless RUN_HOOK_ALLOW_STDIN flag is requested.\n */\n LAST_ARG_MUST_BE_NULL\n int run_hook_le(const char *const *env, const char *name, ...);\n-int run_hook_ve(const char *const *env, const char *name, va_list args);\n+int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args);\n \n /*\n * Trigger an auto-gc\ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex b3485450a2..7bfb7435c6 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n PRECOMMIT=\"$HOOKDIR/pre-commit\"\n PREMERGE=\"$HOOKDIR/pre-merge-commit\"\n+POSTCOMMIT=\"$HOOKDIR/post-commit\"\n \n # Prepare sample scripts that write their $0 to actual_hooks\n test_expect_success 'sample script setup' '\n@@ -28,11 +29,14 @@ test_expect_success 'sample script setup' '\n \techo $0 >>actual_hooks\n \ttest $GIT_PREFIX = \"success/\"\n \tEOF\n-\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF\n+\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF &&\n \techo $0 >>actual_hooks\n \ttest \"$GIT_AUTHOR_NAME\" = \"New Author\" &&\n \ttest \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n \tEOF\n+\twrite_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n+\t! read -r line || echo \"$line\" >hook_input\n+\tEOF\n '\n \n test_expect_success 'root commit' '\n@@ -278,4 +282,44 @@ test_expect_success 'check the author in hook' '\n \ttest_cmp expected_hooks actual_hooks\n '\n \n+test_expect_success 'with user input' '\n+\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n+\techo \"user input\" >user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -F - user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" user_input &&\n+\tgit checkout side &&\n+\tgit merge -m \"merge master\" master \"$HOOK\" <<'EOF'\n+#!/bin/sh\n+! read -r line || echo \"$line\" >\"$1\"\n+EOF\n+chmod +x \"$HOOK\"\n+\n+test_expect_success 'hook with user input' '\n+\n+\techo \"additional\" >>file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"additional\" &&\n+\tcommit_msg_is \"user input\"\n+\n+'\n \n test_done\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 94f85cdf83..aa9c9375e6 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -91,6 +91,11 @@ else\n fi\n test \"$GIT_EDITOR\" = : && source=\"$source (no editor)\"\n \n+if read -r line\n+then\n+\tsource=\"$source $line\"\n+fi\n+\n if test $rebasing = 1\n then\n \techo \"$source $(get_last_cmd)\" >\"$1\"\n@@ -113,6 +118,15 @@ test_expect_success 'with hook (-m)' '\n \n '\n \n+test_expect_success 'with hook (-m and input)' '\n+\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"more\" &&\n+\ttest \"$(git log -1 --pretty=format:%s)\" = \"message (no editor) user input\"\n+\n+'\n+\n test_expect_success 'with hook (-m editor)' '\n \n \techo \"more\" >> file &&\n-- \ngitgitgadget\n\n"},{"id":"410383","messageId":"CAPig+cSN=-7KWgDcXM8po44PEKi27U6mJEEL0mj_wrTJBUf=WA@mail.gmail.com","threadId":"54656","inReplyTo":"3bd6024a236b061c89bb6b60daf3dc15ef1e32ca.1605819390.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-19T21:23:11Z","receivedAt":"2020-11-19T21:23:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget\n wrote:\n> Let hooks receive user input if applicable.\n> [...]\n> This allows for example prompting the user to choose an issue\n> in prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n>\n> Another possible use-case is running sanity test on pre-commit,\n> and having a prompt like \"This and that issue were found in your\n> changes. Are you sure you want to commit? [Y/N]\".\n\nThese use-cases really help readers understand the motivation for this\nchange. Good.\n\n> Allow stdin only for commit-related hooks. Some of the other\n> hooks pass their own input to the hook, so don't change them.\n>\n> Note: If pre-commit reads from stdin, and git commit is executed\n> with -F - (read message from stdin), the message is not read\n> correctly. This is fixed in the follow-up commit.\n\nRather than making such a fundamental change and having to deal with\nthe fallout by introducing complexity to handle various special-cases\nwhich pop up now and in the future, I wonder if it makes more sense to\ninstead just update documentation to tell hook authors to read\nexplicitly from the console rather than expecting stdin to be\navailable (since stdin may already be consumed for other purposes when\ndealing with hooks or commands which invoke the hooks).\n"},{"id":"410386","messageId":"xmqqwnyhvxra.fsf@gitster.c.googlers.com","threadId":"54656","inReplyTo":"CAPig+cSN=-7KWgDcXM8po44PEKi27U6mJEEL0mj_wrTJBUf=WA@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-19T21:32:09Z","receivedAt":"2020-11-19T21:32:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Rather than making such a fundamental change and having to deal with\n> the fallout by introducing complexity to handle various special-cases\n> which pop up now and in the future, I wonder if it makes more sense to\n> instead just update documentation to tell hook authors to read\n> explicitly from the console rather than expecting stdin to be\n> available (since stdin may already be consumed for other purposes when\n> dealing with hooks or commands which invoke the hooks).\n\n;-)\n\nThanks for saying this.\n"},{"id":"410405","messageId":"CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com","threadId":"54656","inReplyTo":"CAPig+cSN=-7KWgDcXM8po44PEKi27U6mJEEL0mj_wrTJBUf=WA@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2020-11-20T05:23:12Z","receivedAt":"2020-11-20T05:23:47Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine wrote:\n>\n> On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget\n> wrote:\n> > Let hooks receive user input if applicable.\n> > [...]\n> > This allows for example prompting the user to choose an issue\n> > in prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n> >\n> > Another possible use-case is running sanity test on pre-commit,\n> > and having a prompt like \"This and that issue were found in your\n> > changes. Are you sure you want to commit? [Y/N]\".\n>\n> These use-cases really help readers understand the motivation for this\n> change. Good.\n>\n> > Allow stdin only for commit-related hooks. Some of the other\n> > hooks pass their own input to the hook, so don't change them.\n> >\n> > Note: If pre-commit reads from stdin, and git commit is executed\n> > with -F - (read message from stdin), the message is not read\n> > correctly. This is fixed in the follow-up commit.\n>\n> Rather than making such a fundamental change and having to deal with\n> the fallout by introducing complexity to handle various special-cases\n> which pop up now and in the future, I wonder if it makes more sense to\n> instead just update documentation to tell hook authors to read\n> explicitly from the console rather than expecting stdin to be\n> available (since stdin may already be consumed for other purposes when\n> dealing with hooks or commands which invoke the hooks).\n\nOn the first revision I had several links in the commit message to\nusers who solved it this way. This solution however is not optimal.\nI have a prepare-commit-msg hook that requires user interaction for\nchoosing an issue. This hook must work from the terminal and also\nfrom GUI applications like IDE.\n\nCurrently the hook always pops a GUI window, but when using it\nfrom the terminal this is inconvenient (and when running over\nremote SSH without X forwarding it can't work), so I'd like it to be\nusable also from the terminal.\n\nTo achieve that, I created 2 classes - one for terminal and one\nfor GUI, and trying to choose the correct class by checking if\nstdin is a tty. The condition looks like this (Ruby):\nclient = STDIN.tty? ? Terminal.new : GUI.new\n\nAt this point I was surprised to discover that Git closes stdin,\nso the condition is never satisfied, and I always end up with GUI.\n\nAs I mentioned, I need it to work also when executed from\nGUI applications, so just reading from the console will not work\nin my case. I tried other ways to detect \"running from terminal\"\nwithout the tty condition, but couldn't. The environment variables\nare identical when running in a GUI terminal and in the IDE.\n\nCan you suggest an alternative way to determine if I can accept user\ninput from the console or not?\n\n- Orgad\n"},{"id":"410408","messageId":"CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com","threadId":"54656","inReplyTo":"CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-20T06:38:58Z","receivedAt":"2020-11-20T06:39:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 20, 2020 at 12:23 AM Orgad Shaneh wrote:\n> On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine wrote:\n> > [...] I wonder if it makes more sense to\n> > instead just update documentation to tell hook authors to read\n> > explicitly from the console rather than expecting stdin to be\n> > available [...]\n>\n> I have a prepare-commit-msg hook that requires user interaction for\n> choosing an issue. This hook must work from the terminal and also\n> from GUI applications like IDE.\n> [...]\n> As I mentioned, I need it to work also when executed from\n> GUI applications, so just reading from the console will not work\n> in my case. I tried other ways to detect \"running from terminal\"\n> without the tty condition, but couldn't. The environment variables\n> are identical when running in a GUI terminal and in the IDE.\n>\n> Can you suggest an alternative way to determine if I can accept user\n> input from the console or not?\n\nNot at present, and I expect that the answer is that any such\nmechanism for determining this would be IDE-dependent. (That is,\nalthough your IDE doesn't distinguish itself in any way which your\nhook can detect, other IDE's might, but that doesn't help in the\ngeneral case.)\n\nWhat I can say, though, is that the additional information you\nsupplied in your response should be part of the commit message to help\nreviewers and future readers better understand why this change is\nwanted. The use-cases presented in the v4 commit message, although\nhelpful, didn't provide sufficient explanation considering that the\nfirst question which popped into this reviewer's mind was \"why not\nhave the hook read from the console explicitly?\". (It is an\nunfortunate fact that reviewer time is a limited resource, so many\nreviewers on this list don't bother chasing down links like those you\nincluded in the commit message of v1 -- which would have helped\njustify the change -- but instead base their reviews only on the\ninformation presented in the commit message itself. In my case, I was\nonly lightly skimming this series, thus didn't even bother chasing\ndown those links -- but have done so now for this reply.)\n\nI do find it quite concerning that the way this series handles the\nstdin conflict between the hook and `-F -` can break the hook silently\nand mysteriously. How confusing for a user to write a hook which works\nwith `git commit -m msg` and `git commit -F file` but breaks silently\nwith `git commit -F -`. What is worse is that this breakage may be\noutside the user's control. For instance, it is easy to imagine some\nIDE passing the commit message to git-commit via stdin (using `-F -`)\nrather than via a file (using `-F file`).\n\nAt the very least, this change deserves a documentation update, both\nto explain that the prepare-commit-msg hook has a valid stdin, and\n(importantly) that it won't be able to rely upon stdin in conjunction\nwith `-F -`. (This also makes me wonder if it would be possible to\nsignal to the hook whether or not stdin is available. Perhaps this\ncould be done by passing an additional argument to the hook.)\n\nFinally, I realize that you followed Junio's suggestion for organizing\nthe series, however, it feels undesirable for patch [1/2] to leave the\ncommand in a somewhat broken state, by which I'm referring to the\nindeterminate outcome of the hook and `-F -` competing for stdin; a\nsituation which is only resolved in [2/2]. To me, a cleaner\norganization would be for [1/2] to introduce the underlying mechanism\nand support by adding `flags` to run_hook_ve() (and perhaps to\nrun_commit_hook()) but not to turn on RUN_HOOK_ALLOW_STDIN, and then\nhave patch [2/2] actually enable RUN_HOOK_ALLOW_STDIN where\nappropriate _and_ deal with the `-F -` conflict all at the same time.\n(And the commit message should mention the conflict and how it is\nhandled.)\n"},{"id":"410409","messageId":"CAPig+cTaV-L_m3OFw=WAUKaiLqSVvhP7fjjFbE13QStibVmRjw@mail.gmail.com","threadId":"54656","inReplyTo":"CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-11-20T06:48:57Z","receivedAt":"2020-11-20T06:49:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 20, 2020 at 1:38 AM Eric Sunshine wrote:\n> I do find it quite concerning that the way this series handles the\n> stdin conflict between the hook and `-F -` can break the hook silently\n> and mysteriously. How confusing for a user to write a hook which works\n> with `git commit -m msg` and `git commit -F file` but breaks silently\n> with `git commit -F -`. What is worse is that this breakage may be\n> outside the user's control. For instance, it is easy to imagine some\n> IDE passing the commit message to git-commit via stdin (using `-F -`)\n> rather than via a file (using `-F file`).\n>\n> At the very least, this change deserves a documentation update, both\n> to explain that the prepare-commit-msg hook has a valid stdin, and\n> (importantly) that it won't be able to rely upon stdin in conjunction\n> with `-F -`.\n\nWhat I forgot to say here was that this patch series doesn't help\nusers at all if their IDE passes the commit message to git-commit via\nstdin using `-F -`. In such a case, their hook will _never_ see a\nvalid stdin coming from Git, no matter what their script does. So, the\nchange made by this patch series may help some users but not others,\nand this is a limitation that should be stated in the commit message\n(and perhaps mentioned in the documentation, though that may be\ndifficult to do in a general way).\n"},{"id":"410410","messageId":"CAGHpTB+2SV_GZT9QBSOQLYCO4znBCToCRJ5-guWMxxuQD+aA-w@mail.gmail.com","threadId":"54656","inReplyTo":"CAPig+cTaV-L_m3OFw=WAUKaiLqSVvhP7fjjFbE13QStibVmRjw@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2020-11-20T07:16:27Z","receivedAt":"2020-11-20T07:17:01Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Fri, Nov 20, 2020 at 8:49 AM Eric Sunshine wrote:\n>\n> On Fri, Nov 20, 2020 at 1:38 AM Eric Sunshine wrote:\n> > I do find it quite concerning that the way this series handles the\n> > stdin conflict between the hook and `-F -` can break the hook silently\n> > and mysteriously. How confusing for a user to write a hook which works\n> > with `git commit -m msg` and `git commit -F file` but breaks silently\n> > with `git commit -F -`. What is worse is that this breakage may be\n> > outside the user's control. For instance, it is easy to imagine some\n> > IDE passing the commit message to git-commit via stdin (using `-F -`)\n> > rather than via a file (using `-F file`).\n> >\n> > At the very least, this change deserves a documentation update, both\n> > to explain that the prepare-commit-msg hook has a valid stdin, and\n> > (importantly) that it won't be able to rely upon stdin in conjunction\n> > with `-F -`.\n>\n> What I forgot to say here was that this patch series doesn't help\n> users at all if their IDE passes the commit message to git-commit via\n> stdin using `-F -`. In such a case, their hook will _never_ see a\n> valid stdin coming from Git, no matter what their script does. So, the\n> change made by this patch series may help some users but not others,\n> and this is a limitation that should be stated in the commit message\n> (and perhaps mentioned in the documentation, though that may be\n> difficult to do in a general way).\n\nAt least in my case, I never expect stdin to be available when running\nin the IDE, so my hook is expected to use GUI anyway. I only need\nstdin when the user is running git from the terminal. So all I need from\nthe IDE is that it doesn't pretend to be a tty while running Git.\n\nAnd regarding the hook itself - the hook author should be aware that\nstdin is not always a tty, and sometimes can be closed or pipe, and\nwrite the hook in a way that handles special cases.\n\n- Orgad\n"},{"id":"410412","messageId":"87sg94pa45.fsf@evledraar.gmail.com","threadId":"54656","inReplyTo":"CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-20T10:59:22Z","receivedAt":"2020-11-20T10:59:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 20 2020, Orgad Shaneh wrote:\n\n> On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine wrote:\n>>\n>> On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget\n>> wrote:\n>> > Let hooks receive user input if applicable.\n>> > [...]\n>> > This allows for example prompting the user to choose an issue\n>> > in prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n>> >\n>> > Another possible use-case is running sanity test on pre-commit,\n>> > and having a prompt like \"This and that issue were found in your\n>> > changes. Are you sure you want to commit? [Y/N]\".\n>>\n>> These use-cases really help readers understand the motivation for this\n>> change. Good.\n>>\n>> > Allow stdin only for commit-related hooks. Some of the other\n>> > hooks pass their own input to the hook, so don't change them.\n>> >\n>> > Note: If pre-commit reads from stdin, and git commit is executed\n>> > with -F - (read message from stdin), the message is not read\n>> > correctly. This is fixed in the follow-up commit.\n>>\n>> Rather than making such a fundamental change and having to deal with\n>> the fallout by introducing complexity to handle various special-cases\n>> which pop up now and in the future, I wonder if it makes more sense to\n>> instead just update documentation to tell hook authors to read\n>> explicitly from the console rather than expecting stdin to be\n>> available (since stdin may already be consumed for other purposes when\n>> dealing with hooks or commands which invoke the hooks).\n>\n> On the first revision I had several links in the commit message to\n> users who solved it this way. This solution however is not optimal.\n> I have a prepare-commit-msg hook that requires user interaction for\n> choosing an issue. This hook must work from the terminal and also\n> from GUI applications like IDE.\n>\n> Currently the hook always pops a GUI window, but when using it\n> from the terminal this is inconvenient (and when running over\n> remote SSH without X forwarding it can't work), so I'd like it to be\n> usable also from the terminal.\n>\n> To achieve that, I created 2 classes - one for terminal and one\n> for GUI, and trying to choose the correct class by checking if\n> stdin is a tty. The condition looks like this (Ruby):\n> client = STDIN.tty? ? Terminal.new : GUI.new\n>\n> At this point I was surprised to discover that Git closes stdin,\n> so the condition is never satisfied, and I always end up with GUI.\n>\n> As I mentioned, I need it to work also when executed from\n> GUI applications, so just reading from the console will not work\n> in my case. I tried other ways to detect \"running from terminal\"\n> without the tty condition, but couldn't. The environment variables\n> are identical when running in a GUI terminal and in the IDE.\n>\n> Can you suggest an alternative way to determine if I can accept user\n> input from the console or not?\n\nLike Eric noted in his reply I can't think of a way to do that\nparticular thing reliably either, and agree with his comments that if\nsuch a way is found / some aspect of this change is kept having this\nexplanation in the patch/commit message is really helpful.\n\nI think what you're trying to do here isn't a good fit for most git\nworkflows. Instead of trying to interactively compose a commit message\nwhy not change the commit template to start with e.g.:\n\n # You must replace XXX with an issue number here!:\n Issue #XXX:\n\nThat gives the user the same thing to fill out, but in their editor\ninstead of via some terminal/GUI prompt. They need to write the rest of\nthe commit message anyway in the editor, so even if you could why open\nup two UIs?\n\nProjects that have these conventions also typically settle on just not\ntrying to solve this problem on the client-side, but e.g. having a\npre-receive hook that does the validation, or do it via CI / before a\nmerge to master happens etc.\n"},{"id":"410417","messageId":"CAGHpTB+LzXTNp3UGia6bdEDqV=mjAY+JQkO3aeUmddhYa1xajw@mail.gmail.com","threadId":"54656","inReplyTo":"87sg94pa45.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2020-11-20T12:34:19Z","receivedAt":"2020-11-20T12:34:52Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Fri, Nov 20, 2020 at 12:59 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, Nov 20 2020, Orgad Shaneh wrote:\n>\n> > Can you suggest an alternative way to determine if I can accept user\n> > input from the console or not?\n>\n> Like Eric noted in his reply I can't think of a way to do that\n> particular thing reliably either, and agree with his comments that if\n> such a way is found / some aspect of this change is kept having this\n> explanation in the patch/commit message is really helpful.\n\nI'll reword.\n\n> I think what you're trying to do here isn't a good fit for most git\n> workflows. Instead of trying to interactively compose a commit message\n> why not change the commit template to start with e.g.:\n>\n> # You must replace XXX with an issue number here!:\n> Issue #XXX:\n>\n> That gives the user the same thing to fill out, but in their editor\n> instead of via some terminal/GUI prompt. They need to write the rest of\n> the commit message anyway in the editor, so even if you could why open\n> up two UIs?\n\nWe do have a template. The hook pops a listbox with all the open issues\nassigned to the user, which he/she can easily pick from, instead of\nsearching for them in the browser and copying the issue id. This is only\ndone if the user doesn't write an issue in the commit message.\n\n> Projects that have these conventions also typically settle on just not\n> trying to solve this problem on the client-side, but e.g. having a\n> pre-receive hook that does the validation, or do it via CI / before a\n> merge to master happens etc.\n\nWe have validation on the server too. The hook is there for convenience.\n\n- Orgad\n"},{"id":"410433","messageId":"xmqqr1onuc9w.fsf@gitster.c.googlers.com","threadId":"54656","inReplyTo":"CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com","subject":"Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-20T18:13:47Z","receivedAt":"2020-11-20T18:14:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> At the very least, this change deserves a documentation update, both\n> to explain that the prepare-commit-msg hook has a valid stdin, and\n> (importantly) that it won't be able to rely upon stdin in conjunction\n> with `-F -`. (This also makes me wonder if it would be possible to\n> signal to the hook whether or not stdin is available. Perhaps this\n> could be done by passing an additional argument to the hook.)\n>\n> Finally, I realize that you followed Junio's suggestion for organizing\n> the series, however, it feels undesirable for patch [1/2] to leave the\n> command in a somewhat broken state, ...\n\nTrue. The split you suggest sounds saner, if we were to still move\nforward with this change. I originally threw \"commit -F -\" in the\n\"don't do it if it hurts\" category, but I agree with you that it is\nquite plausible that IDE would want to use the feature to feed the\nlog message to the command (that way they do not need to worry\nabout a temporary file at all), so it can become a real issue.\n\nThanks.\n"},{"id":"411928","messageId":"27e66d43c833c2e5b8b612e91b6b513076d7fcb7.1607544408.git.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v5.git.1607544408.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] hooks: lay foundations for passing stdin to hooks","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-09T20:06:47Z","receivedAt":"2020-12-09T20:07:39Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nUsed in the follow-up commit for enabling stdin for commit-related\nhooks.\n\nSigned-off-by: Orgad Shaneh \n---\n builtin/commit.c | 8 ++++----\n builtin/merge.c | 6 +++---\n commit.c | 4 ++--\n commit.h | 3 ++-\n run-command.c | 6 +++---\n run-command.h | 17 ++++++++++++-----\n sequencer.c | 4 ++--\n 7 files changed, 28 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 505fe60956d..70a7842e224 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -699,7 +699,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\tif (!no_verify && run_commit_hook(use_editor, index_file, 0, \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -998,7 +998,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n+\tif (run_commit_hook(use_editor, index_file, 0, \"prepare-commit-msg\",\n \t\t\t git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n@@ -1015,7 +1015,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n+\t run_commit_hook(use_editor, index_file, 0, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1701,7 +1701,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n-\trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n+\trun_commit_hook(use_editor, get_index_file(), 0, \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 1cff7307153..26e6ae15993 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -836,7 +836,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n+\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, 0, \"pre-merge-commit\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n@@ -864,7 +864,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n \twrite_merge_heads(remoteheads);\n \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n-\tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n+\tif (run_commit_hook(0 < option_edit, get_index_file(), 0, \"prepare-commit-msg\",\n \t\t\t git_path_merge_msg(the_repository), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n@@ -873,7 +873,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t \"commit-msg\",\n+\t\t\t\t\t 0, \"commit-msg\",\n \t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \ndiff --git a/commit.c b/commit.c\nindex fe1fa3dc41f..3f5a50164eb 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,7 +1631,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)\n }\n \n int run_commit_hook(int editor_is_used, const char *index_file,\n-\t\t const char *name, ...)\n+\t\t unsigned flags, const char *name, ...)\n {\n \tstruct strvec hook_env = STRVEC_INIT;\n \tva_list args;\n@@ -1646,7 +1646,7 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env.v, name, args);\n+\tret = run_hook_ve(hook_env.v, flags, name, args);\n \tva_end(args);\n \tstrvec_clear(&hook_env);\n \ndiff --git a/commit.h b/commit.h\nindex 5467786c7be..72215d57fb2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -352,6 +352,7 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)\n int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);\n \n LAST_ARG_MUST_BE_NULL\n-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n+int run_commit_hook(int editor_is_used, const char *index_file, unsigned flags,\n+\t\t const char *name, ...);\n \n #endif /* COMMIT_H */\ndiff --git a/run-command.c b/run-command.c\nindex ea4d0fb4b15..30d69562f43 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1344,7 +1344,7 @@ const char *find_hook(const char *name)\n \treturn path.buf;\n }\n \n-int run_hook_ve(const char *const *env, const char *name, va_list args)\n+int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args)\n {\n \tstruct child_process hook = CHILD_PROCESS_INIT;\n \tconst char *p;\n@@ -1357,7 +1357,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)\n \twhile ((p = va_arg(args, const char *)))\n \t\tstrvec_push(&hook.args, p);\n \thook.env = env;\n-\thook.no_stdin = 1;\n+\thook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n \thook.stdout_to_stderr = 1;\n \thook.trace2_hook_name = name;\n \n@@ -1370,7 +1370,7 @@ int run_hook_le(const char *const *env, const char *name, ...)\n \tint ret;\n \n \tva_start(args, name);\n-\tret = run_hook_ve(env, name, args);\n+\tret = run_hook_ve(env, 0, name, args);\n \tva_end(args);\n \n \treturn ret;\ndiff --git a/run-command.h b/run-command.h\nindex 6472b38bde4..e613e5e3f92 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -201,22 +201,29 @@ int run_command(struct child_process *);\n */\n const char *find_hook(const char *name);\n \n+#define RUN_HOOK_ALLOW_STDIN 1\n+\n /**\n * Run a hook.\n- * The first argument is a pathname to an index file, or NULL\n- * if the hook uses the default index file or no index is needed.\n- * The second argument is the name of the hook.\n+ * The env argument is an array of environment variables, or NULL\n+ * if the hook uses the default environment and doesn't require\n+ * additional variables.\n+ * The flags argument is an OR'ed collection of feature bits like\n+ * RUN_HOOK_ALLOW_STDIN defined above, which enables\n+ * stdin for the child process (the default is no_stdin).\n+ * The name argument is the name of the hook.\n * The further arguments correspond to the hook arguments.\n * The last argument has to be NULL to terminate the arguments list.\n * If the hook does not exist or is not executable, the return\n * value will be zero.\n * If it is executable, the hook will be executed and the exit\n * status of the hook is returned.\n- * On execution, .stdout_to_stderr and .no_stdin will be set.\n+ * On execution, .stdout_to_stderr will be set, and .no_stdin will be\n+ * set unless RUN_HOOK_ALLOW_STDIN flag is requested.\n */\n LAST_ARG_MUST_BE_NULL\n int run_hook_le(const char *const *env, const char *name, ...);\n-int run_hook_ve(const char *const *env, const char *name, va_list args);\n+int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args);\n \n /*\n * Trigger an auto-gc\ndiff --git a/sequencer.c b/sequencer.c\nindex 8909a467700..5f48d32e2fa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1203,7 +1203,7 @@ static int run_prepare_commit_msg_hook(struct repository *r,\n \t} else {\n \t\targ1 = \"message\";\n \t}\n-\tif (run_commit_hook(0, r->index_file, \"prepare-commit-msg\", name,\n+\tif (run_commit_hook(0, r->index_file, 0, \"prepare-commit-msg\", name,\n \t\t\t arg1, arg2, NULL))\n \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n \n@@ -1528,7 +1528,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n+\trun_commit_hook(0, r->index_file, 0, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n \t\tcommit_post_rewrite(r, current_head, oid);\n \n-- \ngitgitgadget\n\n"},{"id":"411929","messageId":"25db4da3cd5fc7e81141078261086c392541c5d1.1607544408.git.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v5.git.1607544408.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-09T20:06:48Z","receivedAt":"2020-12-09T20:08:01Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh \n\nLet hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C, 2007).\nLooks like the original shell implementation did have stdin open.\n\nDue to stdin being closed, hooks that require user input have to either\nread the input directly from the console (which can't work when running\nfrom GUI applications), or popup a GUI dialog (which is inconvenient\nwhen running from the terminal).\n\nThis allows for example prompting the user to choose an issue in\nprepare-commit-msg, and add \"Fixes #123\" to the commit message.\n\nAnother possible use-case is running sanity test on pre-commit, and\nhaving a prompt like \"This and that issue were found in your changes.\nAre you sure you want to commit? [Y/N]\".\n\nIt's important to note that the hook author should be aware that stdin\nis not always applicable. For example, when running from IDE. This can\nbe checked by isatty on stdin. The hooks should handle cases of closed\ninput, and possibly fall-back to GUI input, or have sane defaults with\na message to the user on this case.\n\nAllow stdin only for commit-related hooks. Some of the other hooks pass\ntheir own input to the hook, so don't change them.\n\nNote: If pre-commit reads from stdin, and git commit is executed with\n-F - (read message from stdin), stdin cannot be passed to the hook,\nsince it will consume it before reaching the point where it is read for\nthe commit message.\n\nSigned-off-by: Orgad Shaneh \n---\n builtin/commit.c | 14 ++++--\n builtin/merge.c | 12 +++--\n sequencer.c | 6 +--\n ...3-pre-commit-and-pre-merge-commit-hooks.sh | 46 ++++++++++++++++++-\n t/t7504-commit-msg-hook.sh | 15 ++++++\n t/t7505-prepare-commit-msg-hook.sh | 14 ++++++\n 6 files changed, 94 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 70a7842e224..074a57937f1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -695,11 +695,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n \tint old_display_comment_prefix;\n \tint merge_contains_scissors = 0;\n+\tint message_from_stdin = logfile && !strcmp(logfile, \"-\");\n+\tconst unsigned hook_flags = message_from_stdin ? 0 : RUN_HOOK_ALLOW_STDIN;\n \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, 0, \"pre-commit\", NULL))\n+\tif (!no_verify &&\n+\t run_commit_hook(use_editor, index_file, hook_flags, \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -724,7 +727,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (have_option_m && !fixup_message) {\n \t\tstrbuf_addbuf(&sb, &message);\n \t\thook_arg1 = \"message\";\n-\t} else if (logfile && !strcmp(logfile, \"-\")) {\n+\t} else if (message_from_stdin) {\n \t\tif (isatty(0))\n \t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\n \t\tif (strbuf_read(&sb, 0, 0) < 0)\n@@ -998,7 +1001,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_commit_hook(use_editor, index_file, 0, \"prepare-commit-msg\",\n+\tif (run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"prepare-commit-msg\",\n \t\t\t git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n@@ -1015,7 +1018,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t run_commit_hook(use_editor, index_file, 0, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n+\t run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n+\t\t\t git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1701,7 +1705,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n-\trun_commit_hook(use_editor, get_index_file(), 0, \"post-commit\", NULL);\n+\trun_commit_hook(use_editor, get_index_file(), RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 26e6ae15993..d6faca59258 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -836,8 +836,11 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, 0, \"pre-merge-commit\", NULL))\n+\tif (!no_verify &&\n+\t run_commit_hook(0 < option_edit, index_file, RUN_HOOK_ALLOW_STDIN,\n+\t\t\t \"pre-merge-commit\", NULL)) {\n \t\tabort_commit(remoteheads, NULL);\n+\t}\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n \t * and write it out as a tree. We must do this before we invoke\n@@ -864,8 +867,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n \twrite_merge_heads(remoteheads);\n \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n-\tif (run_commit_hook(0 < option_edit, get_index_file(), 0, \"prepare-commit-msg\",\n-\t\t\t git_path_merge_msg(the_repository), \"merge\", NULL))\n+\tif (run_commit_hook(0 < option_edit, get_index_file(), RUN_HOOK_ALLOW_STDIN,\n+\t\t\t \"prepare-commit-msg\", git_path_merge_msg(the_repository),\n+\t\t\t \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n \t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n@@ -873,7 +877,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t 0, \"commit-msg\",\n+\t\t\t\t\t RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n \t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 5f48d32e2fa..5190879695a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1203,8 +1203,8 @@ static int run_prepare_commit_msg_hook(struct repository *r,\n \t} else {\n \t\targ1 = \"message\";\n \t}\n-\tif (run_commit_hook(0, r->index_file, 0, \"prepare-commit-msg\", name,\n-\t\t\t arg1, arg2, NULL))\n+\tif (run_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN,\n+\t\t\t \"prepare-commit-msg\", name, arg1, arg2, NULL))\n \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n \n \treturn ret;\n@@ -1528,7 +1528,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\trun_commit_hook(0, r->index_file, 0, \"post-commit\", NULL);\n+\trun_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n \t\tcommit_post_rewrite(r, current_head, oid);\n \ndiff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\nindex b3485450a20..a243b7efa19 100755\n--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh\n@@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n PRECOMMIT=\"$HOOKDIR/pre-commit\"\n PREMERGE=\"$HOOKDIR/pre-merge-commit\"\n+POSTCOMMIT=\"$HOOKDIR/post-commit\"\n \n # Prepare sample scripts that write their $0 to actual_hooks\n test_expect_success 'sample script setup' '\n@@ -28,11 +29,14 @@ test_expect_success 'sample script setup' '\n \techo $0 >>actual_hooks\n \ttest $GIT_PREFIX = \"success/\"\n \tEOF\n-\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF\n+\twrite_script \"$HOOKDIR/check-author.sample\" <<-\\EOF &&\n \techo $0 >>actual_hooks\n \ttest \"$GIT_AUTHOR_NAME\" = \"New Author\" &&\n \ttest \"$GIT_AUTHOR_EMAIL\" = \"newauthor@example.com\"\n \tEOF\n+\twrite_script \"$HOOKDIR/user-input.sample\" <<-\\EOF\n+\t! read -r line || echo \"$line\" >hook_input\n+\tEOF\n '\n \n test_expect_success 'root commit' '\n@@ -278,4 +282,44 @@ test_expect_success 'check the author in hook' '\n \ttest_cmp expected_hooks actual_hooks\n '\n \n+test_expect_success 'with user input' '\n+\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n+\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n+\techo \"user input\" >user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -F - user_input &&\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\tgit commit -m \"more\" user_input &&\n+\tgit checkout side &&\n+\tgit merge -m \"merge master\" master \"$HOOK\" <<'EOF'\n+#!/bin/sh\n+! read -r line || echo \"$line\" >\"$1\"\n+EOF\n+chmod +x \"$HOOK\"\n+\n+test_expect_success 'hook with user input' '\n+\n+\techo \"additional\" >>file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"additional\" &&\n+\tcommit_msg_is \"user input\"\n+\n+'\n \n test_done\ndiff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh\nindex 94f85cdf831..aa9c9375e63 100755\n--- a/t/t7505-prepare-commit-msg-hook.sh\n+++ b/t/t7505-prepare-commit-msg-hook.sh\n@@ -91,6 +91,11 @@ else\n fi\n test \"$GIT_EDITOR\" = : && source=\"$source (no editor)\"\n \n+if read -r line\n+then\n+\tsource=\"$source $line\"\n+fi\n+\n if test $rebasing = 1\n then\n \techo \"$source $(get_last_cmd)\" >\"$1\"\n@@ -113,6 +118,15 @@ test_expect_success 'with hook (-m)' '\n \n '\n \n+test_expect_success 'with hook (-m and input)' '\n+\n+\techo \"more\" >>file &&\n+\tgit add file &&\n+\techo \"user input\" | git commit -m \"more\" &&\n+\ttest \"$(git log -1 --pretty=format:%s)\" = \"message (no editor) user input\"\n+\n+'\n+\n test_expect_success 'with hook (-m editor)' '\n \n \techo \"more\" >> file &&\n-- \ngitgitgadget\n"},{"id":"411930","messageId":"pull.790.v5.git.1607544408.gitgitgadget@gmail.com","threadId":"54656","inReplyTo":"pull.790.v4.git.1605819390.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] hooks: allow input from stdin for commit-related hooks","fromName":"Orgad Shaneh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-09T20:06:46Z","receivedAt":"2020-12-09T20:08:01Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Let hooks receive user input if applicable.\n\nClosing stdin originates in f5bbc3225 (Port git commit to C, 2007). Looks\nlike the original shell implementation did have stdin open. Not clear why\nthe author chose to close it on the C port (maybe copy&paste).\n\nThe only hook that passes internal information to the hook via stdin is\npre-push, which has its own logic.\n\nSome references of users requesting this feature. Some of them use\nacrobatics to gain access to stdin: [1]\nhttps://stackoverflow.com/q/1067874/764870 [2]\nhttps://stackoverflow.com/q/47477766/764870 [3]\nhttps://stackoverflow.com/q/3417896/764870 [4]\nhttps://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3165 [5]\nhttps://github.com/typicode/husky/issues/442\n\nOrgad Shaneh (2):\n hooks: lay foundations for passing stdin to hooks\n hooks: allow input from stdin for commit-related hooks\n\n builtin/commit.c | 14 ++++--\n builtin/merge.c | 12 +++--\n commit.c | 4 +-\n commit.h | 3 +-\n run-command.c | 6 +--\n run-command.h | 17 +++++--\n sequencer.c | 6 +--\n ...3-pre-commit-and-pre-merge-commit-hooks.sh | 46 ++++++++++++++++++-\n t/t7504-commit-msg-hook.sh | 15 ++++++\n t/t7505-prepare-commit-msg-hook.sh | 14 ++++++\n 10 files changed, 113 insertions(+), 24 deletions(-)\n\n\nbase-commit: 3cf59784d42c4152a0b3de7bb7a75d0071e5f878\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-790%2Forgads%2Fhooks-stdin-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-790/orgads/hooks-stdin-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/790\n\nRange-diff vs v4:\n\n 2: e048a9db62c ! 1: 27e66d43c83 commit: fix stdin conflict between message and hook\n @@ Metadata\n Author: Orgad Shaneh \n \n ## Commit message ##\n - commit: fix stdin conflict between message and hook\n + hooks: lay foundations for passing stdin to hooks\n \n - If git commit is executed with -F - (meaning read the commit message\n - from stdin), and pre-commit hook is also reading from stdin, the\n - message itself was consumed by the hook before reaching the point\n - where it is read for the commit message.\n -\n - Fix this by detecting this case, and passing this information to\n - run_commit_hook.\n + Used in the follow-up commit for enabling stdin for commit-related\n + hooks.\n \n Signed-off-by: Orgad Shaneh \n \n ## builtin/commit.c ##\n @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n - \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n - \tint old_display_comment_prefix;\n - \tint merge_contains_scissors = 0;\n -+\tint message_from_stdin = logfile && !strcmp(logfile, \"-\");\n -+\tconst unsigned hook_flags = message_from_stdin ? 0 : RUN_HOOK_ALLOW_STDIN;\n - \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n -+\tif (!no_verify &&\n -+\t run_commit_hook(use_editor, index_file, hook_flags, \"pre-commit\", NULL))\n ++\tif (!no_verify && run_commit_hook(use_editor, index_file, 0, \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n -@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n - \tif (have_option_m && !fixup_message) {\n - \t\tstrbuf_addbuf(&sb, &message);\n - \t\thook_arg1 = \"message\";\n --\t} else if (logfile && !strcmp(logfile, \"-\")) {\n -+\t} else if (message_from_stdin) {\n - \t\tif (isatty(0))\n - \t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\n - \t\tif (strbuf_read(&sb, 0, 0) < 0)\n @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n -\tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n -+\tif (run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"prepare-commit-msg\",\n ++\tif (run_commit_hook(use_editor, index_file, 0, \"prepare-commit-msg\",\n \t\t\t git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha\n \n \tif (!no_verify &&\n -\t run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n -+\t run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n -+\t\t\t git_path_commit_editmsg(), NULL)) {\n ++\t run_commit_hook(use_editor, index_file, 0, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n @@ builtin/commit.c: int cmd_commit(int argc, const char **argv, const char *prefix\n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n -\trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n -+\trun_commit_hook(use_editor, get_index_file(), RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n ++\trun_commit_hook(use_editor, get_index_file(), 0, \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\n @@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n \tconst char *index_file = get_index_file();\n \n -\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n -+\tif (!no_verify &&\n -+\t run_commit_hook(0 < option_edit, index_file, RUN_HOOK_ALLOW_STDIN,\n -+\t\t\t \"pre-merge-commit\", NULL)) {\n ++\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, 0, \"pre-merge-commit\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n -+\t}\n \t/*\n \t * Re-read the index as pre-merge-commit hook could have updated it,\n - \t * and write it out as a tree. We must do this before we invoke\n @@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n \twrite_merge_heads(remoteheads);\n \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n -\tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n --\t\t\t git_path_merge_msg(the_repository), \"merge\", NULL))\n -+\tif (run_commit_hook(0 < option_edit, get_index_file(), RUN_HOOK_ALLOW_STDIN,\n -+\t\t\t \"prepare-commit-msg\", git_path_merge_msg(the_repository),\n -+\t\t\t \"merge\", NULL))\n ++\tif (run_commit_hook(0 < option_edit, get_index_file(), 0, \"prepare-commit-msg\",\n + \t\t\t git_path_merge_msg(the_repository), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n - \t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n @@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n -\t\t\t\t\t \"commit-msg\",\n -+\t\t\t\t\t RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n ++\t\t\t\t\t 0, \"commit-msg\",\n \t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \n @@ commit.c: int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n --\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n +-\tret = run_hook_ve(hook_env.v, name, args);\n +\tret = run_hook_ve(hook_env.v, flags, name, args);\n \tva_end(args);\n \tstrvec_clear(&hook_env);\n @@ commit.h: int compare_commits_by_commit_date(const void *a_, const void *b_, voi\n \n #endif /* COMMIT_H */\n \n + ## run-command.c ##\n +@@ run-command.c: const char *find_hook(const char *name)\n + \treturn path.buf;\n + }\n + \n +-int run_hook_ve(const char *const *env, const char *name, va_list args)\n ++int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args)\n + {\n + \tstruct child_process hook = CHILD_PROCESS_INIT;\n + \tconst char *p;\n +@@ run-command.c: int run_hook_ve(const char *const *env, const char *name, va_list args)\n + \twhile ((p = va_arg(args, const char *)))\n + \t\tstrvec_push(&hook.args, p);\n + \thook.env = env;\n +-\thook.no_stdin = 1;\n ++\thook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n + \thook.stdout_to_stderr = 1;\n + \thook.trace2_hook_name = name;\n + \n +@@ run-command.c: int run_hook_le(const char *const *env, const char *name, ...)\n + \tint ret;\n + \n + \tva_start(args, name);\n +-\tret = run_hook_ve(env, name, args);\n ++\tret = run_hook_ve(env, 0, name, args);\n + \tva_end(args);\n + \n + \treturn ret;\n +\n + ## run-command.h ##\n +@@ run-command.h: int run_command(struct child_process *);\n + */\n + const char *find_hook(const char *name);\n + \n ++#define RUN_HOOK_ALLOW_STDIN 1\n ++\n + /**\n + * Run a hook.\n +- * The first argument is a pathname to an index file, or NULL\n +- * if the hook uses the default index file or no index is needed.\n +- * The second argument is the name of the hook.\n ++ * The env argument is an array of environment variables, or NULL\n ++ * if the hook uses the default environment and doesn't require\n ++ * additional variables.\n ++ * The flags argument is an OR'ed collection of feature bits like\n ++ * RUN_HOOK_ALLOW_STDIN defined above, which enables\n ++ * stdin for the child process (the default is no_stdin).\n ++ * The name argument is the name of the hook.\n + * The further arguments correspond to the hook arguments.\n + * The last argument has to be NULL to terminate the arguments list.\n + * If the hook does not exist or is not executable, the return\n + * value will be zero.\n + * If it is executable, the hook will be executed and the exit\n + * status of the hook is returned.\n +- * On execution, .stdout_to_stderr and .no_stdin will be set.\n ++ * On execution, .stdout_to_stderr will be set, and .no_stdin will be\n ++ * set unless RUN_HOOK_ALLOW_STDIN flag is requested.\n + */\n + LAST_ARG_MUST_BE_NULL\n + int run_hook_le(const char *const *env, const char *name, ...);\n +-int run_hook_ve(const char *const *env, const char *name, va_list args);\n ++int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args);\n + \n + /*\n + * Trigger an auto-gc\n +\n ## sequencer.c ##\n @@ sequencer.c: static int run_prepare_commit_msg_hook(struct repository *r,\n \t} else {\n \t\targ1 = \"message\";\n \t}\n -\tif (run_commit_hook(0, r->index_file, \"prepare-commit-msg\", name,\n --\t\t\t arg1, arg2, NULL))\n -+\tif (run_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN,\n -+\t\t\t \"prepare-commit-msg\", name, arg1, arg2, NULL))\n ++\tif (run_commit_hook(0, r->index_file, 0, \"prepare-commit-msg\", name,\n + \t\t\t arg1, arg2, NULL))\n \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n \n - \treturn ret;\n @@ sequencer.c: static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n -\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n -+\trun_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n ++\trun_commit_hook(0, r->index_file, 0, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n \t\tcommit_post_rewrite(r, current_head, oid);\n \n -\n - ## t/t7503-pre-commit-and-pre-merge-commit-hooks.sh ##\n -@@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'with user input' '\n - \ttest_cmp user_input hook_input\n - '\n - \n --test_expect_failure 'with user input combined with -F -' '\n -+test_expect_success 'with user input combined with -F -' '\n - \ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n - \tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n - \techo \"user input\" >user_input &&\n 1: 3bd6024a236 ! 2: 25db4da3cd5 hooks: allow input from stdin for commit-related hooks\n @@ Commit message\n \n Let hooks receive user input if applicable.\n \n - Closing stdin originates in f5bbc3225 (Port git commit to C,\n - 2007). Looks like the original shell implementation did have\n - stdin open.\n + Closing stdin originates in f5bbc3225 (Port git commit to C, 2007).\n + Looks like the original shell implementation did have stdin open.\n \n - This allows for example prompting the user to choose an issue\n - in prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n + Due to stdin being closed, hooks that require user input have to either\n + read the input directly from the console (which can't work when running\n + from GUI applications), or popup a GUI dialog (which is inconvenient\n + when running from the terminal).\n \n - Another possible use-case is running sanity test on pre-commit,\n - and having a prompt like \"This and that issue were found in your\n - changes. Are you sure you want to commit? [Y/N]\".\n + This allows for example prompting the user to choose an issue in\n + prepare-commit-msg, and add \"Fixes #123\" to the commit message.\n \n - Allow stdin only for commit-related hooks. Some of the other\n - hooks pass their own input to the hook, so don't change them.\n + Another possible use-case is running sanity test on pre-commit, and\n + having a prompt like \"This and that issue were found in your changes.\n + Are you sure you want to commit? [Y/N]\".\n \n - Note: If pre-commit reads from stdin, and git commit is executed\n - with -F - (read message from stdin), the message is not read\n - correctly. This is fixed in the follow-up commit.\n + It's important to note that the hook author should be aware that stdin\n + is not always applicable. For example, when running from IDE. This can\n + be checked by isatty on stdin. The hooks should handle cases of closed\n + input, and possibly fall-back to GUI input, or have sane defaults with\n + a message to the user on this case.\n +\n + Allow stdin only for commit-related hooks. Some of the other hooks pass\n + their own input to the hook, so don't change them.\n +\n + Note: If pre-commit reads from stdin, and git commit is executed with\n + -F - (read message from stdin), stdin cannot be passed to the hook,\n + since it will consume it before reaching the point where it is read for\n + the commit message.\n \n Signed-off-by: Orgad Shaneh \n \n - ## commit.c ##\n -@@ commit.c: int run_commit_hook(int editor_is_used, const char *index_file,\n - \t\tstrvec_push(&hook_env, \"GIT_EDITOR=:\");\n - \n - \tva_start(args, name);\n --\tret = run_hook_ve(hook_env.v, name, args);\n -+\tret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args);\n - \tva_end(args);\n - \tstrvec_clear(&hook_env);\n - \n -\n - ## run-command.c ##\n -@@ run-command.c: const char *find_hook(const char *name)\n - \treturn path.buf;\n - }\n - \n --int run_hook_ve(const char *const *env, const char *name, va_list args)\n -+int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args)\n - {\n - \tstruct child_process hook = CHILD_PROCESS_INIT;\n - \tconst char *p;\n -@@ run-command.c: int run_hook_ve(const char *const *env, const char *name, va_list args)\n - \twhile ((p = va_arg(args, const char *)))\n - \t\tstrvec_push(&hook.args, p);\n - \thook.env = env;\n --\thook.no_stdin = 1;\n -+\thook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN);\n - \thook.stdout_to_stderr = 1;\n - \thook.trace2_hook_name = name;\n - \n -@@ run-command.c: int run_hook_le(const char *const *env, const char *name, ...)\n - \tint ret;\n - \n - \tva_start(args, name);\n --\tret = run_hook_ve(env, name, args);\n -+\tret = run_hook_ve(env, 0, name, args);\n - \tva_end(args);\n + ## builtin/commit.c ##\n +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n + \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n + \tint old_display_comment_prefix;\n + \tint merge_contains_scissors = 0;\n ++\tint message_from_stdin = logfile && !strcmp(logfile, \"-\");\n ++\tconst unsigned hook_flags = message_from_stdin ? 0 : RUN_HOOK_ALLOW_STDIN;\n \n - \treturn ret;\n + \t/* This checks and barfs if author is badly specified */\n + \tdetermine_author_info(author_ident);\n + \n +-\tif (!no_verify && run_commit_hook(use_editor, index_file, 0, \"pre-commit\", NULL))\n ++\tif (!no_verify &&\n ++\t run_commit_hook(use_editor, index_file, hook_flags, \"pre-commit\", NULL))\n + \t\treturn 0;\n + \n + \tif (squash_message) {\n +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n + \tif (have_option_m && !fixup_message) {\n + \t\tstrbuf_addbuf(&sb, &message);\n + \t\thook_arg1 = \"message\";\n +-\t} else if (logfile && !strcmp(logfile, \"-\")) {\n ++\t} else if (message_from_stdin) {\n + \t\tif (isatty(0))\n + \t\t\tfprintf(stderr, _(\"(reading log message from standard input)\\n\"));\n + \t\tif (strbuf_read(&sb, 0, 0) < 0)\n +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n + \t\treturn 0;\n + \t}\n + \n +-\tif (run_commit_hook(use_editor, index_file, 0, \"prepare-commit-msg\",\n ++\tif (run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"prepare-commit-msg\",\n + \t\t\t git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n + \t\treturn 0;\n + \n +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,\n + \t}\n + \n + \tif (!no_verify &&\n +-\t run_commit_hook(use_editor, index_file, 0, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n ++\t run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n ++\t\t\t git_path_commit_editmsg(), NULL)) {\n + \t\treturn 0;\n + \t}\n + \n +@@ builtin/commit.c: int cmd_commit(int argc, const char **argv, const char *prefix)\n + \n + \trepo_rerere(the_repository, 0);\n + \trun_auto_maintenance(quiet);\n +-\trun_commit_hook(use_editor, get_index_file(), 0, \"post-commit\", NULL);\n ++\trun_commit_hook(use_editor, get_index_file(), RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n + \tif (amend && !no_post_rewrite) {\n + \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n + \t}\n \n - ## run-command.h ##\n -@@ run-command.h: int run_command(struct child_process *);\n - */\n - const char *find_hook(const char *name);\n + ## builtin/merge.c ##\n +@@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n + \tstruct strbuf msg = STRBUF_INIT;\n + \tconst char *index_file = get_index_file();\n + \n +-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, 0, \"pre-merge-commit\", NULL))\n ++\tif (!no_verify &&\n ++\t run_commit_hook(0 < option_edit, index_file, RUN_HOOK_ALLOW_STDIN,\n ++\t\t\t \"pre-merge-commit\", NULL)) {\n + \t\tabort_commit(remoteheads, NULL);\n ++\t}\n + \t/*\n + \t * Re-read the index as pre-merge-commit hook could have updated it,\n + \t * and write it out as a tree. We must do this before we invoke\n +@@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n + \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n + \twrite_merge_heads(remoteheads);\n + \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n +-\tif (run_commit_hook(0 < option_edit, get_index_file(), 0, \"prepare-commit-msg\",\n +-\t\t\t git_path_merge_msg(the_repository), \"merge\", NULL))\n ++\tif (run_commit_hook(0 < option_edit, get_index_file(), RUN_HOOK_ALLOW_STDIN,\n ++\t\t\t \"prepare-commit-msg\", git_path_merge_msg(the_repository),\n ++\t\t\t \"merge\", NULL))\n + \t\tabort_commit(remoteheads, NULL);\n + \tif (0 < option_edit) {\n + \t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n +@@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n + \t}\n + \n + \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n +-\t\t\t\t\t 0, \"commit-msg\",\n ++\t\t\t\t\t RUN_HOOK_ALLOW_STDIN, \"commit-msg\",\n + \t\t\t\t\t git_path_merge_msg(the_repository), NULL))\n + \t\tabort_commit(remoteheads, NULL);\n + \n +\n + ## sequencer.c ##\n +@@ sequencer.c: static int run_prepare_commit_msg_hook(struct repository *r,\n + \t} else {\n + \t\targ1 = \"message\";\n + \t}\n +-\tif (run_commit_hook(0, r->index_file, 0, \"prepare-commit-msg\", name,\n +-\t\t\t arg1, arg2, NULL))\n ++\tif (run_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN,\n ++\t\t\t \"prepare-commit-msg\", name, arg1, arg2, NULL))\n + \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n + \n + \treturn ret;\n +@@ sequencer.c: static int try_to_commit(struct repository *r,\n + \t\tgoto out;\n + \t}\n + \n +-\trun_commit_hook(0, r->index_file, 0, \"post-commit\", NULL);\n ++\trun_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN, \"post-commit\", NULL);\n + \tif (flags & AMEND_MSG)\n + \t\tcommit_post_rewrite(r, current_head, oid);\n \n -+#define RUN_HOOK_ALLOW_STDIN 1\n -+\n - /**\n - * Run a hook.\n -- * The first argument is a pathname to an index file, or NULL\n -- * if the hook uses the default index file or no index is needed.\n -- * The second argument is the name of the hook.\n -+ * The env argument is an array of environment variables, or NULL\n -+ * if the hook uses the default environment and doesn't require\n -+ * additional variables.\n -+ * The flags argument is an OR'ed collection of feature bits like\n -+ * RUN_HOOK_ALLOW_STDIN defined above, which enables\n -+ * stdin for the child process (the default is no_stdin).\n -+ * The name argument is the name of the hook.\n - * The further arguments correspond to the hook arguments.\n - * The last argument has to be NULL to terminate the arguments list.\n - * If the hook does not exist or is not executable, the return\n - * value will be zero.\n - * If it is executable, the hook will be executed and the exit\n - * status of the hook is returned.\n -- * On execution, .stdout_to_stderr and .no_stdin will be set.\n -+ * On execution, .stdout_to_stderr will be set, and .no_stdin will be\n -+ * set unless RUN_HOOK_ALLOW_STDIN flag is requested.\n - */\n - LAST_ARG_MUST_BE_NULL\n - int run_hook_le(const char *const *env, const char *name, ...);\n --int run_hook_ve(const char *const *env, const char *name, va_list args);\n -+int run_hook_ve(const char *const *env, unsigned flags, const char *name, va_list args);\n - \n - /*\n - * Trigger an auto-gc\n \n ## t/t7503-pre-commit-and-pre-merge-commit-hooks.sh ##\n @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_description='pre-commit and pre-merge-commit hooks'\n @@ t/t7503-pre-commit-and-pre-merge-commit-hooks.sh: test_expect_success 'check the\n +\ttest_cmp user_input hook_input\n +'\n +\n -+test_expect_failure 'with user input combined with -F -' '\n ++test_expect_success 'with user input combined with -F -' '\n +\ttest_when_finished \"rm -f \\\"$PRECOMMIT\\\" user_input hook_input\" &&\n +\tcp \"$HOOKDIR/user-input.sample\" \"$PRECOMMIT\" &&\n +\techo \"user input\" >user_input &&\n\n-- \ngitgitgadget\n"},{"id":"411943","messageId":"xmqq360e1u9a.fsf@gitster.c.googlers.com","threadId":"54656","inReplyTo":"25db4da3cd5fc7e81141078261086c392541c5d1.1607544408.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/2] hooks: allow input from stdin for commit-related hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-09T22:37:05Z","receivedAt":"2020-12-09T22:39:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Orgad Shaneh via GitGitGadget\" writes:\n\n> It's important to note that the hook author should be aware that stdin\n> is not always applicable. For example, when running from IDE. This can\n> be checked by isatty on stdin. The hooks should handle cases of closed\n> input, and possibly fall-back to GUI input, or have sane defaults with\n> a message to the user on this case.\n\nI think this point was already brought up in the review on previous\nrounds, but when the hook needs to check the standard input anyway,\nit probably is a better design to close and have the hook open tty\nif needed, isn't it? I do not recall I saw a satisfactory answer to\nthat question.\n\n> Allow stdin only for commit-related hooks. Some of the other hooks pass\n> their own input to the hook, so don't change them.\n>\n> Note: If pre-commit reads from stdin, and git commit is executed with\n> -F - (read message from stdin), stdin cannot be passed to the hook,\n> since it will consume it before reaching the point where it is read for\n> the commit message.\n\nIt is unclear what that Note is trying to achieve. Is it describing\na known-bug in this implementation (if so, we'd probably need to\nupdate the documentation to mention this known regression)? Is it\ndescribing a reason why certain part of patch was done in a certain\nway that is not described in this message (e.g. when -F option is in\neffect the standard input stream is closed when invoking a hook)?\n\nThanks.\n"}]}