| Message ID | patch-v2-5.5-b4e02f41194-20230208T191924Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0414b3891cd3adb80b879c7be49d9b727e2b23f5 |
| Headers | show |
| Series | [v2,1/5] run-command.c: remove dead assignment in while-loop | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > From: Emily Shaffer <emilyshaffer@google.com> > > Expose the "path_to_stdin" API added in the preceding commit in the > "git hook run" command. > > For now we won't be using this command interface outside of the tests, > but exposing this functionality makes it easier to test the hook > API. The plan is to use this to extend the "sendemail-validate" > hook[1][2]. OK. What does it take to tackle the obvious leftover bits of [4/5]? Use tempfile API to allocate a temporary file, slurp the input and close it, and then use the "path_to_stdin" feature to spawn the hook?
On Wed, Feb 08 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> From: Emily Shaffer <emilyshaffer@google.com> >> >> Expose the "path_to_stdin" API added in the preceding commit in the >> "git hook run" command. >> >> For now we won't be using this command interface outside of the tests, >> but exposing this functionality makes it easier to test the hook >> API. The plan is to use this to extend the "sendemail-validate" >> hook[1][2]. > > OK. > > What does it take to tackle the obvious leftover bits of [4/5]? Use > tempfile API to allocate a temporary file, slurp the input and close > it, and then use the "path_to_stdin" feature to spawn the hook? You did ask for it :) The below is something I wrote for the end of the initial v2 CL, but then decided it was way too long and dropped it. The tl;dr is that no, that would be the shortest way forward, and arguably what we should do now. But those hooks currently print straight to the pipe, so having the API force them to use a tempfile would suck. But since this is all going for supporting N hooks in parallel the next step requires an API that's future-proofing the feeding of the same content to multiple hooks. So, without further adieu, that dropped part of the v2 CL: The rest of this is a large digression about the future API design of this topic, please don't read ahead unless you're very curious about that (mainly I wanted to brain-dump this somewhere). I considered expanding this series to include the rest of the remaining "post-rewrite" hook in sequencer.c, but as that needs at least a couple of prep patches to expand the API I've left it out for now. The main reason I didn't include (aside from the "let's start small" of this topic) it is that I still don't like the API we'll eventually need for the parallel hooks that take "stdin", and would like to mull it over a bit. What comes after this series eventually wants to convert these hooks from (pseudocode): struct child_process proc = CHILD_PROCESS_INIT; [...] start_command(&proc); [...] for item in transaction: write_in_full(proc.in, item, strlen(item)); To e.g.: struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_PARALLEL; struct string_list to_stdin = STRING_LIST_INIT_DUP; [...] opt.feed_pipe = pipe_from_string_list; opt.feed_pipe_ctx = &to_stdin; [...] for item in transaction: string_list_append(&to_stdin, item); run_hooks_opt("some-hook", &opt); string_list_clear(&to_stdin, 0); The reason is that if we're producing data for stdin we'll need to give it to N hooks. For this topic we neatly side-step that with a "to-stdin", as Junio notes in [3] and [4]. I think even that is arguably a bit ugly, but it's all internally changable uglyness, i.e. for the "sendemail-validate" whether we feed "git hook" with content on stdin or are forced to create a file and feed it with "--to-stdin" is something we can change later. But the answer to the question raised in [4] is that we'll eventually need something like the above. But I don't like it, because: A) The currently proposed API[5] wants to represent lines to the hooks are a "struct string_list", so you add "\n"-less items to it, and we'll always print "%s\n" for each item. This is an arbitrary limitation over the write_in_full() we do now, and will be a hassle e.g. if you have already prepared content, you'll first need to line-split it. Maybe I'm missing some reason for why the hook interface needs to intrinsically promise that it'll be doing write()'s to the hooks ending in \n's, but right now I don't see that, we could leave that up to each hook, and if they're spewing content larger than that the hook itself should just be line-buffering if they care about the distinction. B) Even if we internally line-split a "struct string_list *" is just a bad fit as we lose the string size, we should pass something that give us a "size_t len" (which we could stuff in the "util" field, but...) C) We need to buffer up the stdin in full before we feed the first line to the hook, which seems to me to be an API design that creates the problem the second paragraph of [5] claims to be trying to avoid. I.e. "simply taking a string_list or strbuf is not as scalable as using a callback". That would be true if the result of the callback were streamed to the N hooks we have, but it's not the case. It's a callback mechanism that amounts to just handing off a big "struct string_list", which we need to fully populate before we start the hook. D) Most importantly, the API seems to be structured around a problem we don't actually have. The more general problem *could be* that you'd want to feed N hooks with the same content, we ourselves get that content streamed into us on "stdin", and therefore need to either buffer it in full up-front, or as we read it re-spew it into the N hooks we're executing. But e.g. for the "reference-transaction" hook all we need to support N hooks is to allocate a single "size_t pos" for them, as when we're executing them we have a "struct ref_transaction *transaction" that doesn't change for the duration of the hook. The same goes for the "pre-push. Actually, the only eventual API users that really could have used buffering to ensure consistent results won't use the buffering API. E.g. for the "post-rewrite" and "rebase" hooks we consume a file in ".git/" to give to the hooks on stdin. Currently (and still with this topic) we'll only have one hook, so the file's content will always be the same. But if we were being paranoid we'd buffer it up, so that we could ensure that our N hooks all get the same input, but for the API users that could get a benefit from that we don't use it, but only for those that are guaranteed not to need it. So before the next iteration I'll try to find some time to play with that. I.e. I think we can rip out the whole "struct string_list" feeder, and just have an eventual mechanism for each of the N hooks to init/release their "feed stdin" state with callbacks, and to save away their state in their own "void *". Then e.g. for the reference-transaction we'd just allocate a "size_t pos" per hook, and then just spew content at them on the fly from the transaction struct, no pre-generation necessary. Such an interface will nicely support streaming without pre-buffering delay, and could even run lock-less while multi-threaded (the source data being const, and (almost) all state per-thread. The interface would then be general enough to support pre-slurping/buffering content at the start, and then streaming to N hooks, e.g. for the "read a file, but guarantee that everyone gets the same version". We won't need a string list for that, at most a strbuf, but maybe we can just mmap() those... 3. https://lore.kernel.org/git/xmqqy1pskfo6.fsf@gitster.g/ 4. https://lore.kernel.org/git/xmqqtu0gkaye.fsf@gitster.g/ 5. https://lore.kernel.org/git/patch-v5-24.36-bb119fa7cc0-20210902T125110Z-avarab@gmail.com/
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index 77c3a8ad909..3407f3c2c07 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -8,7 +8,7 @@ git-hook - Run git hooks SYNOPSIS -------- [verse] -'git hook' run [--ignore-missing] <hook-name> [-- <hook-args>] +'git hook' run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>] DESCRIPTION ----------- @@ -31,6 +31,11 @@ linkgit:githooks[5] for arguments hooks might expect (if any). OPTIONS ------- +--to-stdin:: + For "run"; Specify a file which will be streamed into the + hook's stdin. The hook will receive the entire file from + beginning to EOF. + --ignore-missing:: Ignore any missing hook by quietly returning zero. Used for tools that want to do a blind one-shot run of a hook that may diff --git a/builtin/hook.c b/builtin/hook.c index b6530d189ad..f95b7965c58 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -7,7 +7,7 @@ #include "strvec.h" #define BUILTIN_HOOK_RUN_USAGE \ - N_("git hook run [--ignore-missing] <hook-name> [-- <hook-args>]") + N_("git hook run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]") static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, @@ -28,6 +28,8 @@ static int run(int argc, const char **argv, const char *prefix) struct option run_options[] = { OPT_BOOL(0, "ignore-missing", &ignore_missing, N_("silently ignore missing requested <hook-name>")), + OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"), + N_("file to read into hooks' stdin")), OPT_END(), }; int ret; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 2ef3579fa7c..3506f627b6c 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,4 +177,22 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' +test_expect_success 'stdin to hooks' ' + write_script .git/hooks/test-hook <<-\EOF && + echo BEGIN stdin + cat + echo END stdin + EOF + + cat >expect <<-EOF && + BEGIN stdin + hello + END stdin + EOF + + echo hello >input && + git hook run --to-stdin=input test-hook 2>actual && + test_cmp expect actual +' + test_done