{"thread":{"id":"59143","subject":"[PATCH 1/5] run-command.c: remove dead assignment in while-loop","startedAt":"2023-01-23T17:15:25Z","lastAt":"2023-02-09T02:10:39Z","messageCount":27,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Michael Strawbridge","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"470933","messageId":"patch-1.5-351c6a55a41-20230123T170551Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","subject":"[PATCH 1/5] run-command.c: remove dead assignment in while-loop","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T17:15:05Z","receivedAt":"2023-01-23T17:15:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove code that's been unused since it was added in\nc553c72eed6 (run-command: add an asynchronous parallel child\nprocessor, 2015-12-15), the next use of \"i\" in this function is:\n\n\tfor (i = 0; ...\n\nSo we'll always clobber the \"i\" that's set here. Presumably the \"i\"\nassignment is an artifact of WIP code that made it into our tree.\n\nA subsequent commit will need to adjust the type of the \"i\" variable\nin the otherwise unrelated for-loop, which is why this is being\nremoved now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 50cc011654e..b439c7974ca 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1632,9 +1632,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n \t\t\t const struct run_process_parallel_opts *opts,\n \t\t\t int output_timeout)\n {\n-\tint i;\n-\n-\twhile ((i = poll(pp->pfd, opts->processes, output_timeout) < 0)) {\n+\twhile (poll(pp->pfd, opts->processes, output_timeout) < 0) {\n \t\tif (errno == EINTR)\n \t\t\tcontinue;\n \t\tpp_cleanup(pp, opts);\n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"470934","messageId":"patch-2.5-81eef2f60a0-20230123T170551Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","subject":"[PATCH 2/5] run-command: allow stdin for run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T17:15:06Z","receivedAt":"2023-01-23T17:15:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nWhile it makes sense not to inherit stdin from the parent process to\navoid deadlocking, it's not necessary to completely ban stdin to\nchildren. An informed user should be able to configure stdin safely. By\nsetting `some_child.process.no_stdin=1` before calling `get_next_task()`\nwe provide a reasonable default behavior but enable users to set up\nstdin streaming for themselves during the callback.\n\n`some_child.process.stdout_to_stderr`, however, remains unmodifiable by\n`get_next_task()` - the rest of the run_processes_parallel() API depends\non child output in stderr.\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex b439c7974ca..6bd16acb060 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1586,6 +1586,14 @@ static int pp_start_one(struct parallel_processes *pp,\n \tif (i == opts->processes)\n \t\tBUG(\"bookkeeping is hard\");\n \n+\t/*\n+\t * By default, do not inherit stdin from the parent process - otherwise,\n+\t * all children would share stdin! Users may overwrite this to provide\n+\t * something to the child's stdin by having their 'get_next_task'\n+\t * callback assign 0 to .no_stdin and an appropriate integer to .in.\n+\t */\n+\tpp->children[i].process.no_stdin = 1;\n+\n \tcode = opts->get_next_task(&pp->children[i].process,\n \t\t\t\t opts->ungroup ? NULL : &pp->children[i].err,\n \t\t\t\t opts->data,\n@@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,\n \t\tpp->children[i].process.err = -1;\n \t\tpp->children[i].process.stdout_to_stderr = 1;\n \t}\n-\tpp->children[i].process.no_stdin = 1;\n \n \tif (start_command(&pp->children[i].process)) {\n \t\tif (opts->start_failure)\n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"470935","messageId":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","threadId":"59143","inReplyTo":null,"subject":"[PATCH 0/5] hook API: support stdin, convert post-rewrite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T17:15:04Z","receivedAt":"2023-01-23T17:15:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The greater \"config-based-hooks\" topic has been stalled for a while.\n\nIn the last couple of cycles it was held up by run-command.c API\nchanges (which will make the rest of this much simpler), and lack of\ntime on my part.\n\nBut let's get the ball rolling again. The last time this was\non-list[1] it was part of a 36 patch series, these 5 patches only\nconvert one hook (implemented by both sequencer & git-am) to the hook\nAPI, but it's an important step: To do so we need so support reading\nfrom stdin, and passing that to the hooks.\n\nThe (passing) CI & topic branch for this is at[2].\n\nThe immediate motivation for this is to supply a \"stdin\" interface\nthat git-send-email.perl can use, see [3].\n\nChanges since the [1] v5:\n\n * A new 1/5 here, picked from\n https://lore.kernel.org/git/patch-v2-07.22-b90961ae76d-20221012T084850Z-avarab@gmail.com/;\n A small while-at-it cleanup of a related API.\n\n * Updates for the aforementioned run-command API changes.\n\n * Commit message updates & clarifications.\n\n * The previous version of the \"sequencer.c\" change changed a variable\n name while at it, now it doesn't, making the diff much easier to\n read.\n\n * Updates for the *.txt and -h usage, so that we'll pass the\n since-added t0450 test.\n\n1. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/\n2. https://github.com/avar/git/tree/es-avar/config-based-hooks-the-beginning\n3. https://lore.kernel.org/git/230123.86wn5ds602.gmgdl@evledraar.gmail.com/\n\nEmily Shaffer (4):\n run-command: allow stdin for run_processes_parallel\n hook API: support passing stdin to hooks, convert am's 'post-rewrite'\n sequencer: use the new hook API for the simpler \"post-rewrite\" call\n hook: support a --to-stdin= option for testing\n\nÆvar Arnfjörð Bjarmason (1):\n run-command.c: remove dead assignment in while-loop\n\n Documentation/git-hook.txt | 7 ++++++-\n builtin/am.c | 20 ++++----------------\n builtin/hook.c | 4 +++-\n hook.c | 8 +++++++-\n hook.h | 5 +++++\n run-command.c | 13 +++++++++----\n sequencer.c | 18 ++++--------------\n t/t1800-hook.sh | 18 ++++++++++++++++++\n 8 files changed, 56 insertions(+), 37 deletions(-)\n\nRange-diff:\n 1: ac419613fdc < -: ----------- Makefile: mark \"check\" target as .PHONY\n 2: a161b7f0a5c < -: ----------- Makefile: stop hardcoding {command,config}-list.h\n 3: ffef1d3257e < -: ----------- Makefile: remove an out-of-date comment\n 4: 545e16c6f04 < -: ----------- hook.[ch]: move find_hook() from run-command.c to hook.c\n 5: a9bc4519e9a < -: ----------- hook.c: add a hook_exists() wrapper and use it in bugreport.c\n 6: e99ec2e6f8f < -: ----------- hook.c users: use \"hook_exists()\" instead of \"find_hook()\"\n 7: 2ffb2332c8a < -: ----------- hook-list.h: add a generated list of hooks, like config-list.h\n 8: 72dd1010f5b < -: ----------- hook: add 'run' subcommand\n 9: 821cc9bf11e < -: ----------- gc: use hook library for pre-auto-gc hook\n10: d71c90254ea < -: ----------- rebase: convert pre-rebase to use hook.h\n11: ea3af2ccc4d < -: ----------- am: convert applypatch to use hook.h\n12: fed0b52f88f < -: ----------- hooks: convert 'post-checkout' hook to hook library\n13: 53d8721a0e3 < -: ----------- merge: convert post-merge to use hook.h\n14: d60827a2856 < -: ----------- git hook run: add an --ignore-missing flag\n15: d4976a0821f < -: ----------- send-email: use 'git hook run' for 'sendemail-validate'\n16: 99f3dcd1945 < -: ----------- git-p4: use 'git hook' to run hooks\n17: 509761454e6 < -: ----------- commit: convert {pre-commit,prepare-commit-msg} hook to hook.h\n18: e2c94d95427 < -: ----------- read-cache: convert post-index-change to use hook.h\n19: fa7d0d24ea2 < -: ----------- receive-pack: convert push-to-checkout hook to hook.h\n20: 428bb5a6792 < -: ----------- run-command: remove old run_hook_{le,ve}() hook API\n -: ----------- > 1: 351c6a55a41 run-command.c: remove dead assignment in while-loop\n21: 994f6ad8602 ! 2: 81eef2f60a0 run-command: allow stdin for run_processes_parallel\n @@ Commit message\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## run-command.c ##\n -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp)\n - \tif (i == pp->max_processes)\n +@@ run-command.c: static int pp_start_one(struct parallel_processes *pp,\n + \tif (i == opts->processes)\n \t\tBUG(\"bookkeeping is hard\");\n \n +\t/*\n @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)\n +\t */\n +\tpp->children[i].process.no_stdin = 1;\n +\n - \tcode = pp->get_next_task(&pp->children[i].process,\n - \t\t\t\t &pp->children[i].err,\n - \t\t\t\t pp->data,\n -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp)\n + \tcode = opts->get_next_task(&pp->children[i].process,\n + \t\t\t\t opts->ungroup ? NULL : &pp->children[i].err,\n + \t\t\t\t opts->data,\n +@@ run-command.c: static int pp_start_one(struct parallel_processes *pp,\n + \t\tpp->children[i].process.err = -1;\n + \t\tpp->children[i].process.stdout_to_stderr = 1;\n \t}\n - \tpp->children[i].process.err = -1;\n - \tpp->children[i].process.stdout_to_stderr = 1;\n -\tpp->children[i].process.no_stdin = 1;\n \n \tif (start_command(&pp->children[i].process)) {\n - \t\tcode = pp->start_failure(&pp->children[i].err,\n + \t\tif (opts->start_failure)\n23: f548e3d15e7 ! 3: c6b9b69c516 am: convert 'post-rewrite' hook to hook.h\n @@ Metadata\n Author: Emily Shaffer \n \n ## Commit message ##\n - am: convert 'post-rewrite' hook to hook.h\n + hook API: support passing stdin to hooks, convert am's 'post-rewrite'\n +\n + Convert the invocation of the 'post-rewrite' hook run by 'git am' to\n + use the hook.h library. To do this we need to add a \"path_to_stdin\"\n + member to \"struct run_hooks_opt\".\n +\n + In our API this is supported by asking for a file path, rather\n + than by reading stdin. Reading directly from stdin would involve caching\n + the entire stdin (to memory or to disk) once the hook API is made to\n + support \"jobs\" larger than 1, along with support for executing N hooks\n + at a time (i.e. the upcoming config-based hooks).\n \n Signed-off-by: Emily Shaffer \n Signed-off-by: Ævar Arnfjörð Bjarmason \n @@ builtin/am.c: static int run_applypatch_msg_hook(struct am_state *state)\n -\n -\tif (!hook)\n -\t\treturn 0;\n --\n ++\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n + \n -\tstrvec_push(&cp.args, hook);\n -\tstrvec_push(&cp.args, \"rebase\");\n --\n ++\tstrvec_push(&opt.args, \"rebase\");\n ++\topt.path_to_stdin = am_path(state, \"rewritten\");\n + \n -\tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n -\tcp.stdout_to_stderr = 1;\n -\tcp.trace2_hook_name = \"post-rewrite\";\n -+\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n - \n +-\n -\tret = run_command(&cp);\n -+\tstrvec_push(&opt.args, \"rebase\");\n -+\topt.path_to_stdin = am_path(state, \"rewritten\");\n - \n +-\n -\tclose(cp.in);\n -\treturn ret;\n -+\treturn run_hooks_oneshot(\"post-rewrite\", &opt);\n ++\treturn run_hooks_opt(\"post-rewrite\", &opt);\n }\n \n /**\n +\n + ## hook.c ##\n +@@ hook.c: static int pick_next_hook(struct child_process *cp,\n + \tif (!hook_path)\n + \t\treturn 0;\n + \n +-\tcp->no_stdin = 1;\n + \tstrvec_pushv(&cp->env, hook_cb->options->env.v);\n ++\t/* reopen the file for stdin; run_command closes it. */\n ++\tif (hook_cb->options->path_to_stdin) {\n ++\t\tcp->no_stdin = 0;\n ++\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n ++\t} else {\n ++\t\tcp->no_stdin = 1;\n ++\t}\n + \tcp->stdout_to_stderr = 1;\n + \tcp->trace2_hook_name = hook_cb->hook_name;\n + \tcp->dir = hook_cb->options->dir;\n +\n + ## hook.h ##\n +@@ hook.h: struct run_hooks_opt\n + \t * was invoked.\n + \t */\n + \tint *invoked_hook;\n ++\n ++\t/**\n ++\t * Path to file which should be piped to stdin for each hook.\n ++\t */\n ++\tconst char *path_to_stdin;\n + };\n + \n + #define RUN_HOOKS_OPT_INIT { \\\n -: ----------- > 4: 7a55c95f60f sequencer: use the new hook API for the simpler \"post-rewrite\" call\n22: 3ccc654a664 ! 5: cb9ef7a89c4 hook: support passing stdin to hooks\n @@ Metadata\n Author: Emily Shaffer \n \n ## Commit message ##\n - hook: support passing stdin to hooks\n + hook: support a --to-stdin= option for testing\n \n - Some hooks (such as post-rewrite) need to take input via stdin.\n - Previously, callers provided stdin to hooks by setting\n - run-command.h:child_process.in, which takes a FD. Callers would open the\n - file in question themselves before calling run-command(). However, since\n - we will now need to seek to the front of the file and read it again for\n - every hook which runs, hook.h:run_command() takes a path and handles FD\n - management itself. Since this file is opened for read only, it should\n - not prevent later parallel execution support.\n -\n - On the frontend, this is supported by asking for a file path, rather\n - than by reading stdin. Reading directly from stdin would involve caching\n - the entire stdin (to memory or to disk) and reading it back from the\n - beginning to each hook. We'd want to support cases like insufficient\n - memory or storage for the file. While this may prove useful later, for\n - now the path of least resistance is to just ask the user to make this\n - interim file themselves.\n + Expose the \"path_to_stdin\" API added in the preceding commit in the\n + \"git hook run\" command. For now we won't be using this command\n + interface outside of the tests, but exposing this functionality makes\n + it easier to test the hook API.\n \n Signed-off-by: Emily Shaffer \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## Documentation/git-hook.txt ##\n -@@ Documentation/git-hook.txt: git-hook - run git hooks\n +@@ Documentation/git-hook.txt: git-hook - Run git hooks\n SYNOPSIS\n --------\n [verse]\n -'git hook' run [--ignore-missing] [-- ]\n -+'git hook' run [--to-stdin=] [--ignore-missing] [-- ]\n ++'git hook' run [--ignore-missing] [--to-stdin=] [-- ]\n \n DESCRIPTION\n -----------\n -@@ Documentation/git-hook.txt: what those are.\n +@@ Documentation/git-hook.txt: linkgit:githooks[5] for arguments hooks might expect (if any).\n OPTIONS\n -------\n \n @@ builtin/hook.c\n @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix)\n \tstruct option run_options[] = {\n \t\tOPT_BOOL(0, \"ignore-missing\", &ignore_missing,\n - \t\t\t N_(\"exit quietly with a zero exit code if the requested hook cannot be found\")),\n + \t\t\t N_(\"silently ignore missing requested \")),\n +\t\tOPT_STRING(0, \"to-stdin\", &opt.path_to_stdin, N_(\"path\"),\n +\t\t\t N_(\"file to read into hooks' stdin\")),\n \t\tOPT_END(),\n \t};\n \tint ret;\n \n - ## hook.c ##\n -@@ hook.c: static int pick_next_hook(struct child_process *cp,\n - \tif (!run_me)\n - \t\treturn 0;\n - \n --\tcp->no_stdin = 1;\n -+\t/* reopen the file for stdin; run_command closes it. */\n -+\tif (hook_cb->options->path_to_stdin) {\n -+\t\tcp->no_stdin = 0;\n -+\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n -+\t} else {\n -+\t\tcp->no_stdin = 1;\n -+\t}\n - \tcp->env = hook_cb->options->env.v;\n - \tcp->stdout_to_stderr = 1;\n - \tcp->trace2_hook_name = hook_cb->hook_name;\n -\n - ## hook.h ##\n -@@ hook.h: struct run_hooks_opt\n - \n - \t/* Path to initial working directory for subprocess */\n - \tconst char *dir;\n -+\n -+\t/* Path to file which should be piped to stdin for each hook */\n -+\tconst char *path_to_stdin;\n - };\n - \n - #define RUN_HOOKS_OPT_INIT { \\\n -\n ## t/t1800-hook.sh ##\n -@@ t/t1800-hook.sh: test_expect_success 'git -c core.hooksPath= hook run' '\n +@@ t/t1800-hook.sh: test_expect_success 'git hook run a hook with a bad shebang' '\n \ttest_cmp expect actual\n '\n \n24: bb119fa7cc0 < -: ----------- run-command: add stdin callback for parallelization\n25: 2439f7752b8 < -: ----------- hook: provide stdin by string_list or callback\n26: 48a380b3a91 < -: ----------- hook: convert 'post-rewrite' hook in sequencer.c to hook.h\n27: af6b9292aaa < -: ----------- transport: convert pre-push hook to hook.h\n28: 957691f0b6d < -: ----------- hook tests: test for exact \"pre-push\" hook input\n29: 88fe2621549 < -: ----------- hook tests: use a modern style for \"pre-push\" tests\n30: 1d905e81779 < -: ----------- reference-transaction: use hook.h to run hooks\n31: fac56a9d8af < -: ----------- run-command: allow capturing of collated output\n32: 7d185cdf9d1 < -: ----------- hooks: allow callers to capture output\n33: c8150e1239f < -: ----------- receive-pack: convert 'update' hook to hook.h\n34: a20ad847c14 < -: ----------- post-update: use hook.h library\n35: 79c380be6ed < -: ----------- receive-pack: convert receive hooks to hook.h\n36: fe056098534 < -: ----------- hooks: fix a TOCTOU in \"did we run a hook?\" heuristic\n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"470936","messageId":"patch-4.5-7a55c95f60f-20230123T170551Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","subject":"[PATCH 4/5] sequencer: use the new hook API for the simpler \"post-rewrite\" call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T17:15:08Z","receivedAt":"2023-01-23T17:15:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nChange the invocation of the \"post-rewrite\" hook added in\n795160457db (sequencer (rebase -i): run the post-rewrite hook, if\nneeded, 2017-01-02) to use the new hook API.\n\nThis leaves the more complex \"post-rewrite\" invocation added in\na87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)\nhere in sequencer.c unconverted. That'll be done in a subsequent\ncommit.\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sequencer.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e4a1972897..d8d59d05dd4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,\n \t\tif (!stat(rebase_path_rewritten_list(), &st) &&\n \t\t\t\tst.st_size > 0) {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n-\t\t\tconst char *post_rewrite_hook =\n-\t\t\t\tfind_hook(\"post-rewrite\");\n+\t\t\tstruct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;\n \n \t\t\tchild.in = open(rebase_path_rewritten_list(), O_RDONLY);\n \t\t\tchild.git_cmd = 1;\n@@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,\n \t\t\t/* we don't care if this copying failed */\n \t\t\trun_command(&child);\n \n-\t\t\tif (post_rewrite_hook) {\n-\t\t\t\tstruct child_process hook = CHILD_PROCESS_INIT;\n-\n-\t\t\t\thook.in = open(rebase_path_rewritten_list(),\n-\t\t\t\t\tO_RDONLY);\n-\t\t\t\thook.stdout_to_stderr = 1;\n-\t\t\t\thook.trace2_hook_name = \"post-rewrite\";\n-\t\t\t\tstrvec_push(&hook.args, post_rewrite_hook);\n-\t\t\t\tstrvec_push(&hook.args, \"rebase\");\n-\t\t\t\t/* we don't care if this hook failed */\n-\t\t\t\trun_command(&hook);\n-\t\t\t}\n+\t\t\thook_opt.path_to_stdin = rebase_path_rewritten_list();\n+\t\t\tstrvec_push(&hook_opt.args, \"rebase\");\n+\t\t\trun_hooks_opt(\"post-rewrite\", &hook_opt);\n \t\t}\n \t\tapply_autostash(rebase_path_autostash());\n \n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"470937","messageId":"patch-3.5-c6b9b69c516-20230123T170551Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","subject":"[PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T17:15:07Z","receivedAt":"2023-01-23T17:15:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nConvert the invocation of the 'post-rewrite' hook run by 'git am' to\nuse the hook.h library. To do this we need to add a \"path_to_stdin\"\nmember to \"struct run_hooks_opt\".\n\nIn our API this is supported by asking for a file path, rather\nthan by reading stdin. Reading directly from stdin would involve caching\nthe entire stdin (to memory or to disk) once the hook API is made to\nsupport \"jobs\" larger than 1, along with support for executing N hooks\nat a time (i.e. the upcoming config-based hooks).\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 20 ++++----------------\n hook.c | 8 +++++++-\n hook.h | 5 +++++\n 3 files changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 82a41cbfc4e..8be91617fef 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -495,24 +495,12 @@ static int run_applypatch_msg_hook(struct am_state *state)\n */\n static int run_post_rewrite_hook(const struct am_state *state)\n {\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *hook = find_hook(\"post-rewrite\");\n-\tint ret;\n-\n-\tif (!hook)\n-\t\treturn 0;\n+\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \n-\tstrvec_push(&cp.args, hook);\n-\tstrvec_push(&cp.args, \"rebase\");\n+\tstrvec_push(&opt.args, \"rebase\");\n+\topt.path_to_stdin = am_path(state, \"rewritten\");\n \n-\tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n-\tcp.stdout_to_stderr = 1;\n-\tcp.trace2_hook_name = \"post-rewrite\";\n-\n-\tret = run_command(&cp);\n-\n-\tclose(cp.in);\n-\treturn ret;\n+\treturn run_hooks_opt(\"post-rewrite\", &opt);\n }\n \n /**\ndiff --git a/hook.c b/hook.c\nindex a4fa1031f28..86c6dc1fe70 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -53,8 +53,14 @@ static int pick_next_hook(struct child_process *cp,\n \tif (!hook_path)\n \t\treturn 0;\n \n-\tcp->no_stdin = 1;\n \tstrvec_pushv(&cp->env, hook_cb->options->env.v);\n+\t/* reopen the file for stdin; run_command closes it. */\n+\tif (hook_cb->options->path_to_stdin) {\n+\t\tcp->no_stdin = 0;\n+\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n+\t} else {\n+\t\tcp->no_stdin = 1;\n+\t}\n \tcp->stdout_to_stderr = 1;\n \tcp->trace2_hook_name = hook_cb->hook_name;\n \tcp->dir = hook_cb->options->dir;\ndiff --git a/hook.h b/hook.h\nindex 4258b13da0d..19ab9a5806e 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -30,6 +30,11 @@ struct run_hooks_opt\n \t * was invoked.\n \t */\n \tint *invoked_hook;\n+\n+\t/**\n+\t * Path to file which should be piped to stdin for each hook.\n+\t */\n+\tconst char *path_to_stdin;\n };\n \n #define RUN_HOOKS_OPT_INIT { \\\n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"470938","messageId":"patch-5.5-cb9ef7a89c4-20230123T170551Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","subject":"[PATCH 5/5] hook: support a --to-stdin= option for testing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-23T17:15:09Z","receivedAt":"2023-01-23T17:16:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nExpose the \"path_to_stdin\" API added in the preceding commit in the\n\"git hook run\" command. For now we won't be using this command\ninterface outside of the tests, but exposing this functionality makes\nit easier to test the hook API.\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-hook.txt | 7 ++++++-\n builtin/hook.c | 4 +++-\n t/t1800-hook.sh | 18 ++++++++++++++++++\n 3 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt\nindex 77c3a8ad909..3407f3c2c07 100644\n--- a/Documentation/git-hook.txt\n+++ b/Documentation/git-hook.txt\n@@ -8,7 +8,7 @@ git-hook - Run git hooks\n SYNOPSIS\n --------\n [verse]\n-'git hook' run [--ignore-missing] [-- ]\n+'git hook' run [--ignore-missing] [--to-stdin=] [-- ]\n \n DESCRIPTION\n -----------\n@@ -31,6 +31,11 @@ linkgit:githooks[5] for arguments hooks might expect (if any).\n OPTIONS\n -------\n \n+--to-stdin::\n+\tFor \"run\"; Specify a file which will be streamed into the\n+\thook's stdin. The hook will receive the entire file from\n+\tbeginning to EOF.\n+\n --ignore-missing::\n \tIgnore any missing hook by quietly returning zero. Used for\n \ttools that want to do a blind one-shot run of a hook that may\ndiff --git a/builtin/hook.c b/builtin/hook.c\nindex b6530d189ad..f95b7965c58 100644\n--- a/builtin/hook.c\n+++ b/builtin/hook.c\n@@ -7,7 +7,7 @@\n #include \"strvec.h\"\n \n #define BUILTIN_HOOK_RUN_USAGE \\\n-\tN_(\"git hook run [--ignore-missing] [-- ]\")\n+\tN_(\"git hook run [--ignore-missing] [--to-stdin=] [-- ]\")\n \n static const char * const builtin_hook_usage[] = {\n \tBUILTIN_HOOK_RUN_USAGE,\n@@ -28,6 +28,8 @@ static int run(int argc, const char **argv, const char *prefix)\n \tstruct option run_options[] = {\n \t\tOPT_BOOL(0, \"ignore-missing\", &ignore_missing,\n \t\t\t N_(\"silently ignore missing requested \")),\n+\t\tOPT_STRING(0, \"to-stdin\", &opt.path_to_stdin, N_(\"path\"),\n+\t\t\t N_(\"file to read into hooks' stdin\")),\n \t\tOPT_END(),\n \t};\n \tint ret;\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 2ef3579fa7c..3506f627b6c 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -177,4 +177,22 @@ test_expect_success 'git hook run a hook with a bad shebang' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stdin to hooks' '\n+\twrite_script .git/hooks/test-hook <<-\\EOF &&\n+\techo BEGIN stdin\n+\tcat\n+\techo END stdin\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tBEGIN stdin\n+\thello\n+\tEND stdin\n+\tEOF\n+\n+\techo hello >input &&\n+\tgit hook run --to-stdin=input test-hook 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.39.1.1301.gffb37c08dee\n\n"},{"id":"470954","messageId":"xmqqcz74lvf6.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-1.5-351c6a55a41-20230123T170551Z-avarab@gmail.com","subject":"Re: [PATCH 1/5] run-command.c: remove dead assignment in while-loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-23T22:48:13Z","receivedAt":"2023-01-23T22:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Remove code that's been unused since it was added in\n> c553c72eed6 (run-command: add an asynchronous parallel child\n> processor, 2015-12-15), the next use of \"i\" in this function is:\n>\n> \tfor (i = 0; ...\n\nAnd it has been updated to a different type, i.e.\n\n\tfor (size_t i = 0; ...\n\nso it doubly makes sense to kill that unused variable.\n\nMakes sense.\n\n> So we'll always clobber the \"i\" that's set here. Presumably the \"i\"\n> assignment is an artifact of WIP code that made it into our tree.\n>\n> A subsequent commit will need to adjust the type of the \"i\" variable\n> in the otherwise unrelated for-loop, which is why this is being\n> removed now.\n\nThat, together with the earlier mention of the other i (which I\nthink came from the same source---perhaps the original topic this\nwas taken from had int->size_t change in it) are both stale.\n\nPlease proofread what you send out before submitting.\n\nThanks.\n\n"},{"id":"470955","messageId":"xmqq8rhslv8m.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-2.5-81eef2f60a0-20230123T170551Z-avarab@gmail.com","subject":"Re: [PATCH 2/5] run-command: allow stdin for run_processes_parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-23T22:52:09Z","receivedAt":"2023-01-23T22:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +\t/*\n> +\t * By default, do not inherit stdin from the parent process - otherwise,\n> +\t * all children would share stdin! Users may overwrite this to provide\n> +\t * something to the child's stdin by having their 'get_next_task'\n> +\t * callback assign 0 to .no_stdin and an appropriate integer to .in.\n> +\t */\n> +\tpp->children[i].process.no_stdin = 1;\n> +\n> \tcode = opts->get_next_task(&pp->children[i].process,\n> \t\t\t\t opts->ungroup ? NULL : &pp->children[i].err,\n> \t\t\t\t opts->data,\n> @@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,\n> \t\tpp->children[i].process.err = -1;\n> \t\tpp->children[i].process.stdout_to_stderr = 1;\n> \t}\n> -\tpp->children[i].process.no_stdin = 1;\n\nFor this single process, by default .no_stdin is set before it is\npassed to start_command(), so the default behaviour does not change.\n\nThis needs a new safety to ensure that processes that have .no_stdin\nturned off do not share the same value in their .in member, doesn't\nit? Hopefully that will be added in a later step in the series?\n\nProvided that such a safety will appear in the end result, this\nconversion does make sense to me. Let's read on.\n\n> \tif (start_command(&pp->children[i].process)) {\n> \t\tif (opts->start_failure)\n"},{"id":"470956","messageId":"xmqq3580lue9.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-3.5-c6b9b69c516-20230123T170551Z-avarab@gmail.com","subject":"Re: [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-23T23:10:22Z","receivedAt":"2023-01-23T23:10:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/hook.c b/hook.c\n> index a4fa1031f28..86c6dc1fe70 100644\n> --- a/hook.c\n> +++ b/hook.c\n> @@ -53,8 +53,14 @@ static int pick_next_hook(struct child_process *cp,\n> \tif (!hook_path)\n> \t\treturn 0;\n> \n> -\tcp->no_stdin = 1;\n> \tstrvec_pushv(&cp->env, hook_cb->options->env.v);\n> +\t/* reopen the file for stdin; run_command closes it. */\n> +\tif (hook_cb->options->path_to_stdin) {\n> +\t\tcp->no_stdin = 0;\n> +\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n> +\t} else {\n> +\t\tcp->no_stdin = 1;\n> +\t}\n\nDo we need this else clause? I thought that we've made sure\nno_stdin is the default. Is it just being explicit?\n\n> diff --git a/hook.h b/hook.h\n> index 4258b13da0d..19ab9a5806e 100644\n> --- a/hook.h\n> +++ b/hook.h\n> @@ -30,6 +30,11 @@ struct run_hooks_opt\n> \t * was invoked.\n> \t */\n> \tint *invoked_hook;\n> +\n> +\t/**\n> +\t * Path to file which should be piped to stdin for each hook.\n> +\t */\n> +\tconst char *path_to_stdin;\n> };\n> \n> #define RUN_HOOKS_OPT_INIT { \\\n"},{"id":"470957","messageId":"xmqqy1pskfo6.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-3.5-c6b9b69c516-20230123T170551Z-avarab@gmail.com","subject":"Re: [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-23T23:13:45Z","receivedAt":"2023-01-23T23:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> @@ -53,8 +53,14 @@ static int pick_next_hook(struct child_process *cp,\n> \tif (!hook_path)\n> \t\treturn 0;\n> \n> -\tcp->no_stdin = 1;\n> \tstrvec_pushv(&cp->env, hook_cb->options->env.v);\n> +\t/* reopen the file for stdin; run_command closes it. */\n> +\tif (hook_cb->options->path_to_stdin) {\n> +\t\tcp->no_stdin = 0;\n> +\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n> +\t} else {\n> +\t\tcp->no_stdin = 1;\n> +\t}\n\nBy the way, using the path_to_stdin as the customization machinery\nfor the API users, and keeping it to the API implementation to\nactually open the file and stuff .in member with it, is a good way\nto make sure that multiple processes do not compete for the same\nstandard input stream. IOW, what I was worried about in my review\nof [2/5] is addressed by this mechanism.\n\nThanks.\n"},{"id":"470958","messageId":"xmqqtu0gkaye.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-5.5-cb9ef7a89c4-20230123T170551Z-avarab@gmail.com","subject":"Re: [PATCH 5/5] hook: support a --to-stdin= option for testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-24T00:55:37Z","receivedAt":"2023-01-24T00:55:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> From: Emily Shaffer \n>\n> Expose the \"path_to_stdin\" API added in the preceding commit in the\n> \"git hook run\" command. For now we won't be using this command\n> interface outside of the tests, but exposing this functionality makes\n> it easier to test the hook API.\n\nPresumably, the send-email validation topic would be using this\nimmediately once it becomes available, no?\n\nWhen \"git hook\" finds and runs more than one hook script, do they\nget the same input? What I am wondering is if \"to-stdin\" should be\nexposed like this interface (which may be sufficient for testing\npurposes). I imagine that scripters (e.g. send-email developers)\nwould find it more convenient if they do not have to come up with a\ntemporary file and they can just run \"git hook\" and feed whatever\nthey want to give to the hook from its standard input. \"git hook\"\ncommand, upon startup, should do the reading of its standard input\nand spooling it to a temporary file it uses to pass the contents to\nthe hook scripts, in other words.\n\nOther than that, it surely looks not just handy for tests, but has\nimmediate uses.\n\nThanks.\n"},{"id":"470959","messageId":"ad152e25-4061-9955-d3e6-a2c8b1bd24e7@amd.com","threadId":"59143","inReplyTo":"xmqqtu0gkaye.fsf@gitster.g","subject":"Re: [PATCH 5/5] hook: support a --to-stdin= option for testing","fromName":"Michael Strawbridge","fromEmail":"michael.strawbridge@amd.com","sentAt":"2023-01-24T00:59:49Z","receivedAt":"2023-01-24T00:59:58Z","isPatch":true,"sender":{"key":"michael.strawbridge@amd.com","avatar":null},"body":"\nOn 2023-01-23 19:55, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> From: Emily Shaffer \n>>\n>> Expose the \"path_to_stdin\" API added in the preceding commit in the\n>> \"git hook run\" command. For now we won't be using this command\n>> interface outside of the tests, but exposing this functionality makes\n>> it easier to test the hook API.\n> Presumably, the send-email validation topic would be using this\n> immediately once it becomes available, no?\n\nYes I would be trying to use it for my send-email header patch right away.\n\n>\n> When \"git hook\" finds and runs more than one hook script, do they\n> get the same input? What I am wondering is if \"to-stdin\" should be\n> exposed like this interface (which may be sufficient for testing\n> purposes). I imagine that scripters (e.g. send-email developers)\n> would find it more convenient if they do not have to come up with a\n> temporary file and they can just run \"git hook\" and feed whatever\n> they want to give to the hook from its standard input. \"git hook\"\n> command, upon startup, should do the reading of its standard input\n> and spooling it to a temporary file it uses to pass the contents to\n> the hook scripts, in other words.\n>\n> Other than that, it surely looks not just handy for tests, but has\n> immediate uses.\n>\n> Thanks.\n"},{"id":"470976","messageId":"a2810f20-c093-ba73-0fed-5d179e3e954b@dunelm.org.uk","threadId":"59143","inReplyTo":"patch-4.5-7a55c95f60f-20230123T170551Z-avarab@gmail.com","subject":"Re: [PATCH 4/5] sequencer: use the new hook API for the simpler \"post-rewrite\" call","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-24T14:46:41Z","receivedAt":"2023-01-24T14:46:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote:\n> From: Emily Shaffer \n> \n> Change the invocation of the \"post-rewrite\" hook added in\n> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if\n> needed, 2017-01-02) to use the new hook API.\n> \n> This leaves the more complex \"post-rewrite\" invocation added in\n> a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)\n> here in sequencer.c unconverted. That'll be done in a subsequent\n> commit.\n\nAs a reader I'd find it more helpful to explain why the conversion isn't \ndone here rather than leaving be to run \"git show\" to figure it out. If \nyou re-roll perhaps we could replace the commit citation with something like\n\nsequencer.c also contains an invocation of the \"post-rewrite\" hook in \nrun_rewrite_hook() that is not converted as the hook API does not allow \nus to pass the hook input as a string yet.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Emily Shaffer \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> sequencer.c | 18 ++++--------------\n> 1 file changed, 4 insertions(+), 14 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 3e4a1972897..d8d59d05dd4 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,\n> \t\tif (!stat(rebase_path_rewritten_list(), &st) &&\n> \t\t\t\tst.st_size > 0) {\n> \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n> -\t\t\tconst char *post_rewrite_hook =\n> -\t\t\t\tfind_hook(\"post-rewrite\");\n> +\t\t\tstruct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;\n> \n> \t\t\tchild.in = open(rebase_path_rewritten_list(), O_RDONLY);\n> \t\t\tchild.git_cmd = 1;\n> @@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,\n> \t\t\t/* we don't care if this copying failed */\n> \t\t\trun_command(&child);\n> \n> -\t\t\tif (post_rewrite_hook) {\n> -\t\t\t\tstruct child_process hook = CHILD_PROCESS_INIT;\n> -\n> -\t\t\t\thook.in = open(rebase_path_rewritten_list(),\n> -\t\t\t\t\tO_RDONLY);\n> -\t\t\t\thook.stdout_to_stderr = 1;\n> -\t\t\t\thook.trace2_hook_name = \"post-rewrite\";\n> -\t\t\t\tstrvec_push(&hook.args, post_rewrite_hook);\n> -\t\t\t\tstrvec_push(&hook.args, \"rebase\");\n> -\t\t\t\t/* we don't care if this hook failed */\n> -\t\t\t\trun_command(&hook);\n> -\t\t\t}\n> +\t\t\thook_opt.path_to_stdin = rebase_path_rewritten_list();\n> +\t\t\tstrvec_push(&hook_opt.args, \"rebase\");\n> +\t\t\trun_hooks_opt(\"post-rewrite\", &hook_opt);\n> \t\t}\n> \t\tapply_autostash(rebase_path_autostash());\n> \n"},{"id":"471088","messageId":"5a905a5e-1c1e-2072-a7cd-e39b85df41c9@dunelm.org.uk","threadId":"59143","inReplyTo":"a2810f20-c093-ba73-0fed-5d179e3e954b@dunelm.org.uk","subject":"Re: [PATCH 4/5] sequencer: use the new hook API for the simpler \"post-rewrite\" call","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-27T15:08:28Z","receivedAt":"2023-01-27T15:08:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 24/01/2023 14:46, Phillip Wood wrote:\n> Hi Ævar\n> \n> On 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote:\n>> From: Emily Shaffer \n>>\n>> Change the invocation of the \"post-rewrite\" hook added in\n>> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if\n>> needed, 2017-01-02) to use the new hook API.\n>>\n>> This leaves the more complex \"post-rewrite\" invocation added in\n>> a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)\n>> here in sequencer.c unconverted. That'll be done in a subsequent\n>> commit.\n> \n> As a reader I'd find it more helpful to explain why the conversion isn't \n> done here rather than leaving be to run \"git show\" to figure it out. If \n> you re-roll perhaps we could replace the commit citation with something \n> like\n> \n> sequencer.c also contains an invocation of the \"post-rewrite\" hook in \n> run_rewrite_hook() that is not converted as the hook API does not allow \n> us to pass the hook input as a string yet.\n\nSorry, I forgot to say in my previous reply that I like the code change \nhere - it is a nice simplification for callers. builtin/am.c has a \nsimilar function to the one that is converted here.\n\nBest Wishes\n\nPhillip\n\n> Best Wishes\n> \n> Phillip\n> \n>> Signed-off-by: Emily Shaffer \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>>   sequencer.c | 18 ++++--------------\n>>   1 file changed, 4 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 3e4a1972897..d8d59d05dd4 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,\n>>           if (!stat(rebase_path_rewritten_list(), &st) &&\n>>                   st.st_size > 0) {\n>>               struct child_process child = CHILD_PROCESS_INIT;\n>> -            const char *post_rewrite_hook =\n>> -                find_hook(\"post-rewrite\");\n>> +            struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;\n>>               child.in = open(rebase_path_rewritten_list(), O_RDONLY);\n>>               child.git_cmd = 1;\n>> @@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,\n>>               /* we don't care if this copying failed */\n>>               run_command(&child);\n>> -            if (post_rewrite_hook) {\n>> -                struct child_process hook = CHILD_PROCESS_INIT;\n>> -\n>> -                hook.in = open(rebase_path_rewritten_list(),\n>> -                    O_RDONLY);\n>> -                hook.stdout_to_stderr = 1;\n>> -                hook.trace2_hook_name = \"post-rewrite\";\n>> -                strvec_push(&hook.args, post_rewrite_hook);\n>> -                strvec_push(&hook.args, \"rebase\");\n>> -                /* we don't care if this hook failed */\n>> -                run_command(&hook);\n>> -            }\n>> +            hook_opt.path_to_stdin = rebase_path_rewritten_list();\n>> +            strvec_push(&hook_opt.args, \"rebase\");\n>> +            run_hooks_opt(\"post-rewrite\", &hook_opt);\n>>           }\n>>           apply_autostash(rebase_path_autostash());\n"},{"id":"471787","messageId":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com","subject":"[PATCH v2 0/5] hook API: support stdin, convert post-rewrite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T19:21:10Z","receivedAt":"2023-02-08T19:21:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"BEGIN UPDATE\n\nI sent this v2 in already as [0]; but didn't fill in the\nIn-Reply-Header, consequently it was disconnected from the v1 thread,\nand per https://lore.kernel.org/git/xmqqr0v7o0pp.fsf@gitster.g/ (and\nin \"seen\") hasn't been picked up. Sorry about that.\n\nEND UPDATE\n\nAs noted in the v1[1] this is the initial part of the greater\n\"config-based hooks\" topic. I believe this iteration addresses all\ncomments on v1. Changes since then:\n\n* Remove a couple of paragraphs in 1/4 that aren't relevant anymore,\n an already-landed topic addressed those.\n\n* Don't needlessly change \"cp->no_stdin = 1\" and introduce an\n \"else\". This refactoring was there because that code eventually\n changes in the full \"config-based hooks\" topic, but going through\n those future changes I found that it wasn't for a good reason there\n either. We can just keep the \"no_stdin = 1\" by default, and have\n specific cases override that.\n\n* Elaborate on why we're not converting the last \"post-rewrite\" hook\n here.\n\n* Mention the future expected use for sendemail-validate in 5/5\n\nThe (passing) CI & topic branch for this is at[2].\n\n0. https://lore.kernel.org/git/cover-v2-0.5-00000000000-20230203T104319Z-avarab@gmail.com/\n1. https://lore.kernel.org/git/cover-0.5-00000000000-20230123T170550Z-avarab@gmail.com/\n2. https://github.com/avar/git/tree/es-avar/config-based-hooks-the-beginning-2\n\nEmily Shaffer (4):\n run-command: allow stdin for run_processes_parallel\n hook API: support passing stdin to hooks, convert am's 'post-rewrite'\n sequencer: use the new hook API for the simpler \"post-rewrite\" call\n hook: support a --to-stdin= option\n\nÆvar Arnfjörð Bjarmason (1):\n run-command.c: remove dead assignment in while-loop\n\n Documentation/git-hook.txt | 7 ++++++-\n builtin/am.c | 20 ++++----------------\n builtin/hook.c | 4 +++-\n hook.c | 5 +++++\n hook.h | 5 +++++\n run-command.c | 13 +++++++++----\n sequencer.c | 18 ++++--------------\n t/t1800-hook.sh | 18 ++++++++++++++++++\n 8 files changed, 54 insertions(+), 36 deletions(-)\n\nRange-diff against v1:\n1: 351c6a55a41 ! 1: 488b24e1c98 run-command.c: remove dead assignment in while-loop\n @@ Commit message\n \n Remove code that's been unused since it was added in\n c553c72eed6 (run-command: add an asynchronous parallel child\n - processor, 2015-12-15), the next use of \"i\" in this function is:\n -\n - for (i = 0; ...\n -\n - So we'll always clobber the \"i\" that's set here. Presumably the \"i\"\n - assignment is an artifact of WIP code that made it into our tree.\n -\n - A subsequent commit will need to adjust the type of the \"i\" variable\n - in the otherwise unrelated for-loop, which is why this is being\n - removed now.\n + processor, 2015-12-15).\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n2: 81eef2f60a0 = 2: 9a178577dcc run-command: allow stdin for run_processes_parallel\n3: c6b9b69c516 ! 3: 3d3dd6b900a hook API: support passing stdin to hooks, convert am's 'post-rewrite'\n @@ builtin/am.c: static int run_applypatch_msg_hook(struct am_state *state)\n \n ## hook.c ##\n @@ hook.c: static int pick_next_hook(struct child_process *cp,\n - \tif (!hook_path)\n - \t\treturn 0;\n \n --\tcp->no_stdin = 1;\n + \tcp->no_stdin = 1;\n \tstrvec_pushv(&cp->env, hook_cb->options->env.v);\n +\t/* reopen the file for stdin; run_command closes it. */\n +\tif (hook_cb->options->path_to_stdin) {\n +\t\tcp->no_stdin = 0;\n +\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n -+\t} else {\n -+\t\tcp->no_stdin = 1;\n +\t}\n \tcp->stdout_to_stderr = 1;\n \tcp->trace2_hook_name = hook_cb->hook_name;\n4: 7a55c95f60f ! 4: b96522d593f sequencer: use the new hook API for the simpler \"post-rewrite\" call\n @@ Commit message\n \n This leaves the more complex \"post-rewrite\" invocation added in\n a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)\n - here in sequencer.c unconverted. That'll be done in a subsequent\n - commit.\n + here in sequencer.c unconverted.\n +\n + Here we can pass in a file's via the \"in\" file descriptor, in that\n + case we don't have a file, but will need to write_in_full() to an \"in\"\n + provide by the API. Support for that will be added to the hook API in\n + the future, but we're not there yet.\n \n Signed-off-by: Emily Shaffer \n Signed-off-by: Ævar Arnfjörð Bjarmason \n5: cb9ef7a89c4 ! 5: b4e02f41194 hook: support a --to-stdin= option for testing\n @@ Metadata\n Author: Emily Shaffer \n \n ## Commit message ##\n - hook: support a --to-stdin= option for testing\n + hook: support a --to-stdin= option\n \n Expose the \"path_to_stdin\" API added in the preceding commit in the\n - \"git hook run\" command. For now we won't be using this command\n - interface outside of the tests, but exposing this functionality makes\n - it easier to test the hook API.\n + \"git hook run\" command.\n +\n + For now we won't be using this command interface outside of the tests,\n + but exposing this functionality makes it easier to test the hook\n + API. The plan is to use this to extend the \"sendemail-validate\"\n + hook[1][2].\n +\n + 1. https://lore.kernel.org/git/ad152e25-4061-9955-d3e6-a2c8b1bd24e7@amd.com\n + 2. https://lore.kernel.org/git/20230120012459.920932-1-michael.strawbridge@amd.com\n \n Signed-off-by: Emily Shaffer \n Signed-off-by: Ævar Arnfjörð Bjarmason \n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471788","messageId":"patch-v2-1.5-488b24e1c98-20230208T191924Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","subject":"[PATCH v2 1/5] run-command.c: remove dead assignment in while-loop","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T19:21:11Z","receivedAt":"2023-02-08T19:21:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove code that's been unused since it was added in\nc553c72eed6 (run-command: add an asynchronous parallel child\nprocessor, 2015-12-15).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 50cc011654e..b439c7974ca 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1632,9 +1632,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n \t\t\t const struct run_process_parallel_opts *opts,\n \t\t\t int output_timeout)\n {\n-\tint i;\n-\n-\twhile ((i = poll(pp->pfd, opts->processes, output_timeout) < 0)) {\n+\twhile (poll(pp->pfd, opts->processes, output_timeout) < 0) {\n \t\tif (errno == EINTR)\n \t\t\tcontinue;\n \t\tpp_cleanup(pp, opts);\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471789","messageId":"patch-v2-2.5-9a178577dcc-20230208T191924Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","subject":"[PATCH v2 2/5] run-command: allow stdin for run_processes_parallel","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T19:21:12Z","receivedAt":"2023-02-08T19:21:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nWhile it makes sense not to inherit stdin from the parent process to\navoid deadlocking, it's not necessary to completely ban stdin to\nchildren. An informed user should be able to configure stdin safely. By\nsetting `some_child.process.no_stdin=1` before calling `get_next_task()`\nwe provide a reasonable default behavior but enable users to set up\nstdin streaming for themselves during the callback.\n\n`some_child.process.stdout_to_stderr`, however, remains unmodifiable by\n`get_next_task()` - the rest of the run_processes_parallel() API depends\non child output in stderr.\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex b439c7974ca..6bd16acb060 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1586,6 +1586,14 @@ static int pp_start_one(struct parallel_processes *pp,\n \tif (i == opts->processes)\n \t\tBUG(\"bookkeeping is hard\");\n \n+\t/*\n+\t * By default, do not inherit stdin from the parent process - otherwise,\n+\t * all children would share stdin! Users may overwrite this to provide\n+\t * something to the child's stdin by having their 'get_next_task'\n+\t * callback assign 0 to .no_stdin and an appropriate integer to .in.\n+\t */\n+\tpp->children[i].process.no_stdin = 1;\n+\n \tcode = opts->get_next_task(&pp->children[i].process,\n \t\t\t\t opts->ungroup ? NULL : &pp->children[i].err,\n \t\t\t\t opts->data,\n@@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,\n \t\tpp->children[i].process.err = -1;\n \t\tpp->children[i].process.stdout_to_stderr = 1;\n \t}\n-\tpp->children[i].process.no_stdin = 1;\n \n \tif (start_command(&pp->children[i].process)) {\n \t\tif (opts->start_failure)\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471790","messageId":"patch-v2-3.5-3d3dd6b900a-20230208T191924Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","subject":"[PATCH v2 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T19:21:13Z","receivedAt":"2023-02-08T19:21:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nConvert the invocation of the 'post-rewrite' hook run by 'git am' to\nuse the hook.h library. To do this we need to add a \"path_to_stdin\"\nmember to \"struct run_hooks_opt\".\n\nIn our API this is supported by asking for a file path, rather\nthan by reading stdin. Reading directly from stdin would involve caching\nthe entire stdin (to memory or to disk) once the hook API is made to\nsupport \"jobs\" larger than 1, along with support for executing N hooks\nat a time (i.e. the upcoming config-based hooks).\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 20 ++++----------------\n hook.c | 5 +++++\n hook.h | 5 +++++\n 3 files changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 82a41cbfc4e..8be91617fef 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -495,24 +495,12 @@ static int run_applypatch_msg_hook(struct am_state *state)\n */\n static int run_post_rewrite_hook(const struct am_state *state)\n {\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *hook = find_hook(\"post-rewrite\");\n-\tint ret;\n-\n-\tif (!hook)\n-\t\treturn 0;\n+\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \n-\tstrvec_push(&cp.args, hook);\n-\tstrvec_push(&cp.args, \"rebase\");\n+\tstrvec_push(&opt.args, \"rebase\");\n+\topt.path_to_stdin = am_path(state, \"rewritten\");\n \n-\tcp.in = xopen(am_path(state, \"rewritten\"), O_RDONLY);\n-\tcp.stdout_to_stderr = 1;\n-\tcp.trace2_hook_name = \"post-rewrite\";\n-\n-\tret = run_command(&cp);\n-\n-\tclose(cp.in);\n-\treturn ret;\n+\treturn run_hooks_opt(\"post-rewrite\", &opt);\n }\n \n /**\ndiff --git a/hook.c b/hook.c\nindex a4fa1031f28..1a848318634 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -55,6 +55,11 @@ static int pick_next_hook(struct child_process *cp,\n \n \tcp->no_stdin = 1;\n \tstrvec_pushv(&cp->env, hook_cb->options->env.v);\n+\t/* reopen the file for stdin; run_command closes it. */\n+\tif (hook_cb->options->path_to_stdin) {\n+\t\tcp->no_stdin = 0;\n+\t\tcp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);\n+\t}\n \tcp->stdout_to_stderr = 1;\n \tcp->trace2_hook_name = hook_cb->hook_name;\n \tcp->dir = hook_cb->options->dir;\ndiff --git a/hook.h b/hook.h\nindex 4258b13da0d..19ab9a5806e 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -30,6 +30,11 @@ struct run_hooks_opt\n \t * was invoked.\n \t */\n \tint *invoked_hook;\n+\n+\t/**\n+\t * Path to file which should be piped to stdin for each hook.\n+\t */\n+\tconst char *path_to_stdin;\n };\n \n #define RUN_HOOKS_OPT_INIT { \\\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471792","messageId":"patch-v2-5.5-b4e02f41194-20230208T191924Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","subject":"[PATCH v2 5/5] hook: support a --to-stdin= option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T19:21:15Z","receivedAt":"2023-02-08T19:21:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nExpose the \"path_to_stdin\" API added in the preceding commit in the\n\"git hook run\" command.\n\nFor now we won't be using this command interface outside of the tests,\nbut exposing this functionality makes it easier to test the hook\nAPI. The plan is to use this to extend the \"sendemail-validate\"\nhook[1][2].\n\n1. https://lore.kernel.org/git/ad152e25-4061-9955-d3e6-a2c8b1bd24e7@amd.com\n2. https://lore.kernel.org/git/20230120012459.920932-1-michael.strawbridge@amd.com\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-hook.txt | 7 ++++++-\n builtin/hook.c | 4 +++-\n t/t1800-hook.sh | 18 ++++++++++++++++++\n 3 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt\nindex 77c3a8ad909..3407f3c2c07 100644\n--- a/Documentation/git-hook.txt\n+++ b/Documentation/git-hook.txt\n@@ -8,7 +8,7 @@ git-hook - Run git hooks\n SYNOPSIS\n --------\n [verse]\n-'git hook' run [--ignore-missing] [-- ]\n+'git hook' run [--ignore-missing] [--to-stdin=] [-- ]\n \n DESCRIPTION\n -----------\n@@ -31,6 +31,11 @@ linkgit:githooks[5] for arguments hooks might expect (if any).\n OPTIONS\n -------\n \n+--to-stdin::\n+\tFor \"run\"; Specify a file which will be streamed into the\n+\thook's stdin. The hook will receive the entire file from\n+\tbeginning to EOF.\n+\n --ignore-missing::\n \tIgnore any missing hook by quietly returning zero. Used for\n \ttools that want to do a blind one-shot run of a hook that may\ndiff --git a/builtin/hook.c b/builtin/hook.c\nindex b6530d189ad..f95b7965c58 100644\n--- a/builtin/hook.c\n+++ b/builtin/hook.c\n@@ -7,7 +7,7 @@\n #include \"strvec.h\"\n \n #define BUILTIN_HOOK_RUN_USAGE \\\n-\tN_(\"git hook run [--ignore-missing] [-- ]\")\n+\tN_(\"git hook run [--ignore-missing] [--to-stdin=] [-- ]\")\n \n static const char * const builtin_hook_usage[] = {\n \tBUILTIN_HOOK_RUN_USAGE,\n@@ -28,6 +28,8 @@ static int run(int argc, const char **argv, const char *prefix)\n \tstruct option run_options[] = {\n \t\tOPT_BOOL(0, \"ignore-missing\", &ignore_missing,\n \t\t\t N_(\"silently ignore missing requested \")),\n+\t\tOPT_STRING(0, \"to-stdin\", &opt.path_to_stdin, N_(\"path\"),\n+\t\t\t N_(\"file to read into hooks' stdin\")),\n \t\tOPT_END(),\n \t};\n \tint ret;\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 2ef3579fa7c..3506f627b6c 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -177,4 +177,22 @@ test_expect_success 'git hook run a hook with a bad shebang' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stdin to hooks' '\n+\twrite_script .git/hooks/test-hook <<-\\EOF &&\n+\techo BEGIN stdin\n+\tcat\n+\techo END stdin\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tBEGIN stdin\n+\thello\n+\tEND stdin\n+\tEOF\n+\n+\techo hello >input &&\n+\tgit hook run --to-stdin=input test-hook 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471791","messageId":"patch-v2-4.5-b96522d593f-20230208T191924Z-avarab@gmail.com","threadId":"59143","inReplyTo":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","subject":"[PATCH v2 4/5] sequencer: use the new hook API for the simpler \"post-rewrite\" call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-08T19:21:14Z","receivedAt":"2023-02-08T19:22:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Emily Shaffer \n\nChange the invocation of the \"post-rewrite\" hook added in\n795160457db (sequencer (rebase -i): run the post-rewrite hook, if\nneeded, 2017-01-02) to use the new hook API.\n\nThis leaves the more complex \"post-rewrite\" invocation added in\na87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)\nhere in sequencer.c unconverted.\n\nHere we can pass in a file's via the \"in\" file descriptor, in that\ncase we don't have a file, but will need to write_in_full() to an \"in\"\nprovide by the API. Support for that will be added to the hook API in\nthe future, but we're not there yet.\n\nSigned-off-by: Emily Shaffer \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n sequencer.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e4a1972897..d8d59d05dd4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,\n \t\tif (!stat(rebase_path_rewritten_list(), &st) &&\n \t\t\t\tst.st_size > 0) {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n-\t\t\tconst char *post_rewrite_hook =\n-\t\t\t\tfind_hook(\"post-rewrite\");\n+\t\t\tstruct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;\n \n \t\t\tchild.in = open(rebase_path_rewritten_list(), O_RDONLY);\n \t\t\tchild.git_cmd = 1;\n@@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,\n \t\t\t/* we don't care if this copying failed */\n \t\t\trun_command(&child);\n \n-\t\t\tif (post_rewrite_hook) {\n-\t\t\t\tstruct child_process hook = CHILD_PROCESS_INIT;\n-\n-\t\t\t\thook.in = open(rebase_path_rewritten_list(),\n-\t\t\t\t\tO_RDONLY);\n-\t\t\t\thook.stdout_to_stderr = 1;\n-\t\t\t\thook.trace2_hook_name = \"post-rewrite\";\n-\t\t\t\tstrvec_push(&hook.args, post_rewrite_hook);\n-\t\t\t\tstrvec_push(&hook.args, \"rebase\");\n-\t\t\t\t/* we don't care if this hook failed */\n-\t\t\t\trun_command(&hook);\n-\t\t\t}\n+\t\t\thook_opt.path_to_stdin = rebase_path_rewritten_list();\n+\t\t\tstrvec_push(&hook_opt.args, \"rebase\");\n+\t\t\trun_hooks_opt(\"post-rewrite\", &hook_opt);\n \t\t}\n \t\tapply_autostash(rebase_path_autostash());\n \n-- \n2.39.1.1475.gc2542cdc5ef\n\n"},{"id":"471798","messageId":"xmqqa61nrhtk.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-v2-1.5-488b24e1c98-20230208T191924Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/5] run-command.c: remove dead assignment in while-loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T21:03:03Z","receivedAt":"2023-02-08T21:03:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Remove code that's been unused since it was added in\n> c553c72eed6 (run-command: add an asynchronous parallel child\n> processor, 2015-12-15).\n\nObviously correct. Thanks, will queue.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> run-command.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 50cc011654e..b439c7974ca 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1632,9 +1632,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp,\n> \t\t\t const struct run_process_parallel_opts *opts,\n> \t\t\t int output_timeout)\n> {\n> -\tint i;\n> -\n> -\twhile ((i = poll(pp->pfd, opts->processes, output_timeout) < 0)) {\n> +\twhile (poll(pp->pfd, opts->processes, output_timeout) < 0) {\n> \t\tif (errno == EINTR)\n> \t\t\tcontinue;\n> \t\tpp_cleanup(pp, opts);\n"},{"id":"471800","messageId":"xmqq357frhix.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-v2-2.5-9a178577dcc-20230208T191924Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] run-command: allow stdin for run_processes_parallel","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T21:09:26Z","receivedAt":"2023-02-08T21:09:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> From: Emily Shaffer \n>\n> While it makes sense not to inherit stdin from the parent process to\n> avoid deadlocking, it's not necessary to completely ban stdin to\n> children.\n\nI do not think deadlock avoidance is an issue. Unpredictable\nfeeding of pieces of input into multiple children is. One possible\nsemantics is to grab the input and dup/tee into all the children, so \nthat each child gets its own copy to process. A hook like \"pre-receive\",\nwhen it has more than one scripts listening to the event, may want\nsuch a semantics. Another possible semantics is to give priority\namong the children running simultaneously and feed only one child,\nwhile starving others.\n\n> An informed user should be able to configure stdin safely. By\n> setting `some_child.process.no_stdin=1` before calling `get_next_task()`\n> we provide a reasonable default behavior but enable users to set up\n> stdin streaming for themselves during the callback.\n\nI _think_ this alludes to the latter, e.g. \"only one child is\nallowed, and the one that controls what children are spawned sets\nno_stdin for everybody but the chosen one\". We may want to be a bit\nmore explicit in the proposed log message and definitely in the\ndocumentation.\n\nThe implementation is \"nice\".\n\n> +\t/*\n> +\t * By default, do not inherit stdin from the parent process - otherwise,\n> +\t * all children would share stdin! Users may overwrite this to provide\n> +\t * something to the child's stdin by having their 'get_next_task'\n> +\t * callback assign 0 to .no_stdin and an appropriate integer to .in.\n> +\t */\n> +\tpp->children[i].process.no_stdin = 1;\n> +\n> \tcode = opts->get_next_task(&pp->children[i].process,\n> \t\t\t\t opts->ungroup ? NULL : &pp->children[i].err,\n> \t\t\t\t opts->data,\n> @@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,\n> \t\tpp->children[i].process.err = -1;\n> \t\tpp->children[i].process.stdout_to_stderr = 1;\n> \t}\n> -\tpp->children[i].process.no_stdin = 1;\n> \n> \tif (start_command(&pp->children[i].process)) {\n> \t\tif (opts->start_failure)\n"},{"id":"471801","messageId":"xmqqy1p7q2t8.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-v2-3.5-3d3dd6b900a-20230208T191924Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T21:12:35Z","receivedAt":"2023-02-08T21:12:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> From: Emily Shaffer \n>\n> Convert the invocation of the 'post-rewrite' hook run by 'git am' to\n> use the hook.h library. To do this we need to add a \"path_to_stdin\"\n> member to \"struct run_hooks_opt\".\n>\n> In our API this is supported by asking for a file path, rather\n> than by reading stdin. Reading directly from stdin would involve caching\n> the entire stdin (to memory or to disk) once the hook API is made to\n> support \"jobs\" larger than 1, along with support for executing N hooks\n> at a time (i.e. the upcoming config-based hooks).\n\nOK, that is a sensible plan to spool and dup/tee the input to\nchildren. It may not be necessary yet at this step, but it is very\ngood to be thinking ahead.\n\nLooking good.\n"},{"id":"471802","messageId":"xmqqttzvq2ks.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-v2-4.5-b96522d593f-20230208T191924Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] sequencer: use the new hook API for the simpler \"post-rewrite\" call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T21:17:39Z","receivedAt":"2023-02-08T21:17:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> From: Emily Shaffer \n>\n> Change the invocation of the \"post-rewrite\" hook added in\n> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if\n> needed, 2017-01-02) to use the new hook API.\n\nVery straight-forward, thanks to the previous step (i.e. addition of\nthe \"path_to_stdin\" support). Nicely done.\n"},{"id":"471803","messageId":"xmqqpmajq2cx.fsf@gitster.g","threadId":"59143","inReplyTo":"patch-v2-5.5-b4e02f41194-20230208T191924Z-avarab@gmail.com","subject":"Re: [PATCH v2 5/5] hook: support a --to-stdin= option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T21:22:22Z","receivedAt":"2023-02-08T21:22:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> From: Emily Shaffer \n>\n> Expose the \"path_to_stdin\" API added in the preceding commit in the\n> \"git hook run\" command.\n>\n> For now we won't be using this command interface outside of the tests,\n> but exposing this functionality makes it easier to test the hook\n> API. The plan is to use this to extend the \"sendemail-validate\"\n> hook[1][2].\n\nOK.\n\nWhat does it take to tackle the obvious leftover bits of [4/5]? Use\ntempfile API to allocate a temporary file, slurp the input and close\nit, and then use the \"path_to_stdin\" feature to spawn the hook?\n"},{"id":"471804","messageId":"xmqqlel7q2bj.fsf@gitster.g","threadId":"59143","inReplyTo":"cover-v2-0.5-00000000000-20230208T191924Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/5] hook API: support stdin, convert post-rewrite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T21:23:12Z","receivedAt":"2023-02-08T21:23:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> As noted in the v1[1] this is the initial part of the greater\n> \"config-based hooks\" topic. I believe this iteration addresses all\n> comments on v1. Changes since then:\n>\n> * Remove a couple of paragraphs in 1/4 that aren't relevant anymore,\n> an already-landed topic addressed those.\n>\n> * Don't needlessly change \"cp->no_stdin = 1\" and introduce an\n> \"else\". This refactoring was there because that code eventually\n> changes in the full \"config-based hooks\" topic, but going through\n> those future changes I found that it wasn't for a good reason there\n> either. We can just keep the \"no_stdin = 1\" by default, and have\n> specific cases override that.\n>\n> * Elaborate on why we're not converting the last \"post-rewrite\" hook\n> here.\n>\n> * Mention the future expected use for sendemail-validate in 5/5\n\nAll read well. Will queue. Thanks.\n"},{"id":"471832","messageId":"230209.86y1p7y4fa.gmgdl@evledraar.gmail.com","threadId":"59143","inReplyTo":"xmqqpmajq2cx.fsf@gitster.g","subject":"Re: [PATCH v2 5/5] hook: support a --to-stdin= option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-09T01:56:24Z","receivedAt":"2023-02-09T02:10:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 08 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> From: Emily Shaffer \n>>\n>> Expose the \"path_to_stdin\" API added in the preceding commit in the\n>> \"git hook run\" command.\n>>\n>> For now we won't be using this command interface outside of the tests,\n>> but exposing this functionality makes it easier to test the hook\n>> API. The plan is to use this to extend the \"sendemail-validate\"\n>> hook[1][2].\n>\n> OK.\n>\n> What does it take to tackle the obvious leftover bits of [4/5]? Use\n> tempfile API to allocate a temporary file, slurp the input and close\n> it, and then use the \"path_to_stdin\" feature to spawn the hook?\n\nYou did ask for it :)\n\nThe below is something I wrote for the end of the initial v2 CL, but\nthen decided it was way too long and dropped it.\n\nThe tl;dr is that no, that would be the shortest way forward, and\narguably what we should do now.\n\nBut those hooks currently print straight to the pipe, so having the API\nforce them to use a tempfile would suck.\n\nBut since this is all going for supporting N hooks in parallel the next\nstep requires an API that's future-proofing the feeding of the same\ncontent to multiple hooks.\n\nSo, without further adieu, that dropped part of the v2 CL: \n\nThe rest of this is a large digression about the future API design of\nthis topic, please don't read ahead unless you're very curious about\nthat (mainly I wanted to brain-dump this somewhere).\n\nI considered expanding this series to include the rest of the\nremaining \"post-rewrite\" hook in sequencer.c, but as that needs at\nleast a couple of prep patches to expand the API I've left it out for\nnow.\n\nThe main reason I didn't include (aside from the \"let's start small\"\nof this topic) it is that I still don't like the API we'll eventually\nneed for the parallel hooks that take \"stdin\", and would like to mull\nit over a bit.\n\nWhat comes after this series eventually wants to convert these hooks\nfrom (pseudocode):\n\n\tstruct child_process proc = CHILD_PROCESS_INIT;\n\t[...]\n\tstart_command(&proc);\n\t[...]\n\tfor item in transaction:\n\t\twrite_in_full(proc.in, item, strlen(item));\n\nTo e.g.:\n\n\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_PARALLEL;\n\tstruct string_list to_stdin = STRING_LIST_INIT_DUP;\n\n\t[...]\n\n opt.feed_pipe = pipe_from_string_list;\n opt.feed_pipe_ctx = &to_stdin;\n\n\t[...]\n\n\tfor item in transaction:\n\t\tstring_list_append(&to_stdin, item);\n\n\trun_hooks_opt(\"some-hook\", &opt);\n\tstring_list_clear(&to_stdin, 0);\n\nThe reason is that if we're producing data for stdin we'll need to\ngive it to N hooks. For this topic we neatly side-step that with a\n\"to-stdin\", as Junio notes in [3] and [4].\n\nI think even that is arguably a bit ugly, but it's all internally\nchangable uglyness, i.e. for the \"sendemail-validate\" whether we feed\n\"git hook\" with content on stdin or are forced to create a file and\nfeed it with \"--to-stdin\" is something we can change later.\n\nBut the answer to the question raised in [4] is that we'll eventually\nneed something like the above. But I don't like it, because:\n\nA) The currently proposed API[5] wants to represent lines to the hooks\n are a \"struct string_list\", so you add \"\\n\"-less items to it, and\n we'll always print \"%s\\n\" for each item.\n\n This is an arbitrary limitation over the write_in_full() we do now,\n and will be a hassle e.g. if you have already prepared content,\n you'll first need to line-split it.\n\n Maybe I'm missing some reason for why the hook interface needs to\n intrinsically promise that it'll be doing write()'s to the hooks\n ending in \\n's, but right now I don't see that, we could leave that\n up to each hook, and if they're spewing content larger than that\n the hook itself should just be line-buffering if they care about\n the distinction.\n\nB) Even if we internally line-split a \"struct string_list *\" is just a\n bad fit as we lose the string size, we should pass something that\n give us a \"size_t len\" (which we could stuff in the \"util\" field,\n but...)\n\nC) We need to buffer up the stdin in full before we feed the first\n line to the hook, which seems to me to be an API design that\n creates the problem the second paragraph of [5] claims to be trying\n to avoid. I.e. \"simply taking a string_list or strbuf is not as\n scalable as using a callback\".\n\n That would be true if the result of the callback were streamed to\n the N hooks we have, but it's not the case. It's a callback\n mechanism that amounts to just handing off a big \"struct\n string_list\", which we need to fully populate before we start the\n hook.\n\nD) Most importantly, the API seems to be structured around a problem\n we don't actually have.\n\n The more general problem *could be* that you'd want to feed N hooks\n with the same content, we ourselves get that content streamed into\n us on \"stdin\", and therefore need to either buffer it in full\n up-front, or as we read it re-spew it into the N hooks we're\n executing.\n\n But e.g. for the \"reference-transaction\" hook all we need to\n support N hooks is to allocate a single \"size_t pos\" for them, as\n when we're executing them we have a \"struct ref_transaction\n *transaction\" that doesn't change for the duration of the hook. The\n same goes for the \"pre-push.\n\n Actually, the only eventual API users that really could have used\n buffering to ensure consistent results won't use the buffering\n API. E.g. for the \"post-rewrite\" and \"rebase\" hooks we consume a\n file in \".git/\" to give to the hooks on stdin. Currently (and still\n with this topic) we'll only have one hook, so the file's content\n will always be the same.\n\n But if we were being paranoid we'd buffer it up, so that we could\n ensure that our N hooks all get the same input, but for the API\n users that could get a benefit from that we don't use it, but only\n for those that are guaranteed not to need it.\n\nSo before the next iteration I'll try to find some time to play with\nthat. I.e. I think we can rip out the whole \"struct string_list\" feeder,\nand just have an eventual mechanism for each of the N hooks to\ninit/release their \"feed stdin\" state with callbacks, and to save away\ntheir state in their own \"void *\".\n\nThen e.g. for the reference-transaction we'd just allocate a \"size_t\npos\" per hook, and then just spew content at them on the fly from the\ntransaction struct, no pre-generation necessary.\n\nSuch an interface will nicely support streaming without pre-buffering\ndelay, and could even run lock-less while multi-threaded (the source\ndata being const, and (almost) all state per-thread.\n\nThe interface would then be general enough to support\npre-slurping/buffering content at the start, and then streaming to N\nhooks, e.g. for the \"read a file, but guarantee that everyone gets the\nsame version\". We won't need a string list for that, at most a strbuf,\nbut maybe we can just mmap() those...\n\n3. https://lore.kernel.org/git/xmqqy1pskfo6.fsf@gitster.g/\n4. https://lore.kernel.org/git/xmqqtu0gkaye.fsf@gitster.g/\n5. https://lore.kernel.org/git/patch-v5-24.36-bb119fa7cc0-20210902T125110Z-avarab@gmail.com/\n"}]}