{"thread":{"id":"59187","subject":"[PATCH v2 0/5] hook API: support stdin, convert post-rewrite","startedAt":"2023-02-03T12:16:27Z","lastAt":"2023-02-03T12:16:45Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"471420","messageId":"cover-v2-0.5-00000000000-20230203T104319Z-avarab@gmail.com","threadId":"59187","inReplyTo":null,"subject":"[PATCH v2 0/5] hook API: support stdin, convert post-rewrite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-03T12:15:30Z","receivedAt":"2023-02-03T12:16:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"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\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\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.1397.g8c8c074958d\n\n"},{"id":"471421","messageId":"patch-v2-1.5-488b24e1c98-20230203T104319Z-avarab@gmail.com","threadId":"59187","inReplyTo":"cover-v2-0.5-00000000000-20230203T104319Z-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-03T12:15:31Z","receivedAt":"2023-02-03T12:16:37Z","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.1397.g8c8c074958d\n\n"},{"id":"471422","messageId":"patch-v2-2.5-9a178577dcc-20230203T104319Z-avarab@gmail.com","threadId":"59187","inReplyTo":"cover-v2-0.5-00000000000-20230203T104319Z-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-03T12:15:32Z","receivedAt":"2023-02-03T12:16:39Z","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.1397.g8c8c074958d\n\n"},{"id":"471423","messageId":"patch-v2-4.5-b96522d593f-20230203T104319Z-avarab@gmail.com","threadId":"59187","inReplyTo":"cover-v2-0.5-00000000000-20230203T104319Z-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-03T12:15:34Z","receivedAt":"2023-02-03T12:16:40Z","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.1397.g8c8c074958d\n\n"},{"id":"471424","messageId":"patch-v2-3.5-3d3dd6b900a-20230203T104319Z-avarab@gmail.com","threadId":"59187","inReplyTo":"cover-v2-0.5-00000000000-20230203T104319Z-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-03T12:15:33Z","receivedAt":"2023-02-03T12:16:44Z","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.1397.g8c8c074958d\n\n"},{"id":"471425","messageId":"patch-v2-5.5-b4e02f41194-20230203T104319Z-avarab@gmail.com","threadId":"59187","inReplyTo":"cover-v2-0.5-00000000000-20230203T104319Z-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-03T12:15:35Z","receivedAt":"2023-02-03T12:16:45Z","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.1397.g8c8c074958d\n\n"}]}