On Thu, Aug 19 2021, Emily Shaffer wrote: I'm submitting a re-roll of this soon, just some notes. > On Tue, Aug 03, 2021 at 09:38:26PM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> This is a v4 re-roll of the "Base for "config-based-hooks" topic. >> * Almost all the callers were just "one-shot" callers, I introduced a >> new run_hooks_oneshot() function for those, which gets rid of the >> verbosity around memory management, see e.g. the "builtin/gc.c" in >> the range-diff below. That run_hooks_oneshot() can also take a NULL >> set of options. > > I discussed this elswhere, but just to make doubly sure that comment > doesn't get lost, here's a link to my mail: > https://lore.kernel.org/git/YR2jLdYQA5CVzX5h%40google.com > >> Range-diff against v3: >> 1: 27c94247f87 = 1: 81fe1ed90d5 Makefile: mark "check" target as .PHONY >> 2: 6e164edb0b0 ! 2: 0f749530777 Makefile: stop hardcoding {command,config}-list.h >> 3: ddae86802e2 ! 3: 644b31fe281 Makefile: remove an out-of-date comment >> 4: 58c37e4f06e = 4: 89c4d44b0c3 hook.[ch]: move find_hook() to this new library >> 5: 0cf7e078ef4 = 5: 3514e0c0251 hook.c: add a hook_exists() wrapper and use it in bugreport.c >> -: ----------- > 6: d5ef40f77dc hook.c users: use "hook_exists()" insted of "find_hook()" >> 6: f343fc7ae66 ! 7: 4cfd72722c1 hook-list.h: add a generated list of hooks, like config-list.h > > Since these 6 were broken out and then brought back into this series, > I'll review them individually downthread. > >> 7: cf4b06bfdf8 ! 8: 7cb4a4cb69e hook: add 'run' subcommand >> @@ builtin/hook.c (new) >> +#include "strbuf.h" >> +#include "strvec.h" >> + >> ++#define BUILTIN_HOOK_RUN_USAGE \ >> ++ N_("git hook run <hook-name> [-- <hook-args>]") >> ++ > > Nice, now we avoid string duplication here... > >> +static const char * const builtin_hook_usage[] = { >> -+ N_("git hook <command> [...]"), > > ...and there is no point including this vague thing that has a more > specific description right after. Ok. > >> -+ N_("git hook run <hook-name> [-- <hook-args>]"), >> ++ BUILTIN_HOOK_RUN_USAGE, >> + NULL >> +}; > [...] >> @@ builtin/hook.c (new) >> +{ >> + int i; >> + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> -+ int rc = 0; >> + const char *hook_name; >> + const char *hook_path; >> -+ >> + struct option run_options[] = { >> + OPT_END(), >> + }; >> ++ int ret; >> + >> + argc = parse_options(argc, argv, prefix, run_options, >> + builtin_hook_run_usage, >> -+ PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH); >> -+ >> -+ if (argc > 1) { >> -+ if (strcmp(argv[1], "--") && >> -+ strcmp(argv[1], "--end-of-options")) >> -+ /* Having a -- for "run" is mandatory */ >> -+ usage_with_options(builtin_hook_usage, run_options); >> -+ /* Add our arguments, start after -- */ >> -+ for (i = 2 ; i < argc; i++) >> -+ strvec_push(&opt.args, argv[i]); >> -+ } >> ++ PARSE_OPT_KEEP_DASHDASH); > > Nice - this code is being broken up and moved later. > >> + >> -+ /* Need to take into account core.hooksPath */ >> -+ git_config(git_default_config, NULL); >> ++ if (!argc) >> ++ goto usage; >> + >> + /* >> -+ * We are not using run_hooks() because we'd like to detect >> -+ * missing hooks. Let's find it ourselves and call >> -+ * run_found_hooks() instead. > > This comment disappears entirely. I am not too terribly upset about it, > since this behavior changes in my series anyway, but it seems like it > might have been an accident? I removed it intentionally, I thought it was clear enough from the control flow, but added it back in. Thanks. >> ++ * Having a -- for "run" when providing <hook-args> is >> ++ * mandatory. >> + */ >> ++ if (argc > 1 && strcmp(argv[1], "--") && >> ++ strcmp(argv[1], "--end-of-options")) >> ++ goto usage; >> ++ >> ++ /* Add our arguments, start after -- */ >> ++ for (i = 2 ; i < argc; i++) >> ++ strvec_push(&opt.args, argv[i]); >> ++ >> ++ /* Need to take into account core.hooksPath */ >> ++ git_config(git_default_config, NULL); >> ++ >> + hook_name = argv[0]; >> + hook_path = find_hook(hook_name); >> + if (!hook_path) { >> + error("cannot find a hook named %s", hook_name); >> + return 1; >> + } >> -+ rc = run_found_hooks(hook_name, hook_path, &opt); >> + >> ++ ret = run_hooks(hook_name, hook_path, &opt); >> + run_hooks_opt_clear(&opt); >> -+ >> -+ return rc; >> ++ return ret; >> ++usage: >> ++ usage_with_options(builtin_hook_run_usage, run_options); >> +} >> + >> +int cmd_hook(int argc, const char **argv, const char *prefix) >> @@ builtin/hook.c (new) >> + struct option builtin_hook_options[] = { >> + OPT_END(), >> + }; >> ++ >> + argc = parse_options(argc, argv, NULL, builtin_hook_options, >> + builtin_hook_usage, PARSE_OPT_STOP_AT_NON_OPTION); >> + if (!argc) >> -+ usage_with_options(builtin_hook_usage, builtin_hook_options); >> ++ goto usage; >> + >> + if (!strcmp(argv[0], "run")) >> + return run(argc, argv, prefix); >> -+ else >> -+ usage_with_options(builtin_hook_usage, builtin_hook_options); >> ++ >> ++usage: >> ++ usage_with_options(builtin_hook_usage, builtin_hook_options); >> +} > > The goto pattern seems readable enough elsewhere, though. > >> ## command-list.txt ## >> @@ hook.c: int hook_exists(const char *name) >> + struct hook_cb_data *hook_cb = pp_cb; >> + struct hook *run_me = hook_cb->run_me; >> + >> ++ if (!run_me) >> ++ return 0; >> ++ > Now we protect ourselves from the repeated calls to pick_next_hook(). > Good. >> + cp->no_stdin = 1; >> + cp->env = hook_cb->options->env.v; >> + cp->stdout_to_stderr = 1; >> @@ hook.c: int hook_exists(const char *name) >> + /* Provide context for errors if necessary */ >> + *pp_task_cb = run_me; >> + >> ++ /* >> ++ * This pick_next_hook() will be called again, we're only >> ++ * running one hook, so indicate that no more work will be >> ++ * done. >> ++ */ >> ++ hook_cb->run_me = NULL; >> ++ > And clearly explaining the caller's behavior here. Sure. >> + return 1; >> +} >> + >> @@ hook.c: int hook_exists(const char *name) >> + >> + hook_cb->rc |= result; >> + >> -+ return 1; >> ++ return 0; > And finally, report "everything is fine" on task finished. Thanks. >> +} >> + >> -+int run_found_hooks(const char *hook_name, const char *hook_path, >> -+ struct run_hooks_opt *options) >> ++int run_hooks(const char *hook_name, const char *hook_path, >> ++ struct run_hooks_opt *options) > > Ok. With the rename, we only have "verbose way to call it" and "speedy > way to call it". No more "run_found_hooks()". Thanks, I think I like > this better - I found "run_found_hooks()" to be a little ambiguous. > >> @@ hook.c: int hook_exists(const char *name) >> + hook_name); >> + >> + return cb_data.rc; >> -+} >> -+ >> -+int run_hooks(const char *hook_name, struct run_hooks_opt *options) >> -+{ >> -+ const char *hook_path; >> -+ int ret; >> -+ if (!options) >> -+ BUG("a struct run_hooks_opt must be provided to run_hooks"); >> -+ >> -+ hook_path = find_hook(hook_name); >> -+ >> -+ /* >> -+ * If you need to act on a missing hook, use run_found_hooks() >> -+ * instead >> -+ */ >> -+ if (!hook_path) >> -+ return 0; >> -+ >> -+ ret = run_found_hooks(hook_name, hook_path, options); >> -+ return ret; > > Ok - so we wait for another patch to introduce run_hooks_oneshot. Fine. > >> +} >> >> ## hook.h ## >> @@ hook.h: const char *find_hook(const char *name); >> + >> + /* Args to be passed to each hook */ >> + struct strvec args; >> -+ >> -+ /* >> -+ * Number of threads to parallelize across, currently a stub, >> -+ * we use the parallel API for future-proofing, but we always >> -+ * have one hook of a given name, so this is always an >> -+ * implicit 1 for now. >> -+ */ >> -+ int jobs; > > Thanks. I think this makes more sense than the weird "futureproofing" > state it was in before. > >> ## t/t1800-hook.sh (new) ## >> @@ t/t1800-hook.sh (new) >> + >> +test_expect_success 'git hook usage' ' >> + test_expect_code 129 git hook && >> -+ test_expect_code 129 git hook -h && >> -+ test_expect_code 129 git hook run -h >> ++ test_expect_code 129 git hook run && >> ++ test_expect_code 129 git hook run -h && >> ++ test_expect_code 129 git hook run --unknown 2>err && >> ++ grep "unknown option" err >> +' > > Nice. I was able to use this test myself when I added 'git hook list', > too. > > The changes to this patch look good to me. > >> 8: 7209f73f281 ! 9: 2b8500aa675 gc: use hook library for pre-auto-gc hook >> @@ Metadata >> ## Commit message ## >> gc: use hook library for pre-auto-gc hook >> >> - Using the hook.h library instead of the run-command.h library to run >> - pre-auto-gc means that those hooks can be set up in config files, as >> - well as in the hookdir. pre-auto-gc is called only from builtin/gc.c. >> + Move the pre-auto-gc hook away from run-command.h to and over to the >> + new hook.h library. >> + >> + To do this introduce a simple run_hooks_oneshot() wrapper, we'll be >> + using it extensively for these simple cases of wanting to run a single >> + hook under a given name, and having it free the memory we allocate for >> + us. > > Cool, so we have an excuse for introducing the oneshot wrapper right > away. Thanks. > >> -@@ builtin/gc.c: static void add_repack_incremental_option(void) >> - >> - static int need_to_gc(void) >> - { >> -+ struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT; >> -+ >> - /* >> - * Setting gc.auto to 0 or negative can disable the >> - * automatic gc. >> @@ builtin/gc.c: static int need_to_gc(void) >> else >> return 0; >> >> - if (run_hook_le(NULL, "pre-auto-gc", NULL)) >> -+ if (run_hooks("pre-auto-gc", &hook_opt)) { >> -+ run_hooks_opt_clear(&hook_opt); >> ++ if (run_hooks_oneshot("pre-auto-gc", NULL)) >> return 0; >> -+ } >> -+ run_hooks_opt_clear(&hook_opt); >> return 1; >> } > > Nice - the callsite looks much tidier now. > >> + >> + ## hook.c ## >> +@@ hook.c: int run_hooks(const char *hook_name, const char *hook_path, >> >> + return cb_data.rc; >> + } >> ++ >> ++int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options) >> ++{ >> ++ const char *hook_path; >> ++ int ret; >> ++ struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT; >> ++ >> ++ if (!options) >> ++ options = &hook_opt_scratch; > > Ah, it still takes an options (optionally, ha ha) because run_hooks() > would require the caller to provide the hook list/hook path. Ok. Seems > fine, and I like that the '_oneshot' naming change makes it less > surprising that run_hooks_opt_clear() will get called for you. > >> ++ >> ++ hook_path = find_hook(hook_name); >> ++ if (!hook_path) { >> ++ ret = 0; >> ++ goto cleanup; >> ++ } >> ++ >> ++ ret = run_hooks(hook_name, hook_path, options); >> ++cleanup: >> ++ run_hooks_opt_clear(options); >> ++ return ret; >> ++} >> + >> + ## hook.h ## >> +@@ hook.h: void run_hooks_opt_clear(struct run_hooks_opt *o); >> + /** >> + * Takes an already resolved hook found via find_hook() and runs >> + * it. Does not call run_hooks_opt_clear() for you. >> ++ * >> ++ * See run_hooks_oneshot() for the simpler one-shot API. >> + */ >> + int run_hooks(const char *hookname, const char *hook_path, >> + struct run_hooks_opt *options); >> ++ >> ++/** >> ++ * Calls find_hook() on your "hook_name" and runs the hooks (if any) >> ++ * with run_hooks(). >> ++ * >> ++ * If "options" is provided calls run_hooks_opt_clear() on it for >> ++ * you. If "options" is NULL a scratch one will be provided for you >> ++ * before calling run_hooks(). > > "A scratch one will be provided for you" doesn't sound quite right - > it's not exposed to the caller at all, but the comment sounds like > you're handing the caller this scratch struct. Maybe it's better to say > "the default options from RUN_HOOKS_OPT_INIT will be used"? > > As a bonus, if you directly reference the initter macro, then it will > automatically become clear to folks reading the documentation what the > expected parallelism is on their hook, as the parallelism (_SERIES or > _PARALLEL) is included on the name of the macro in my change later. *Nod*, did that. >> ++ */ >> ++int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options); >> ++ >> + #endif > >> 9: e9a1e7cf61e ! 10: 3ee55d2c10f rebase: teach pre-rebase to use hook.h >> @@ Metadata >> Author: Emily Shaffer <emilyshaffer@xxxxxxxxxx> >> >> ## Commit message ## >> - rebase: teach pre-rebase to use hook.h >> + rebase: convert pre-rebase to use hook.h >> >> Move the pre-rebase hook away from run-command.h to and over to the >> new hook.h library. >> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix >> if (!ok_to_skip_pre_rebase && >> - run_hook_le(NULL, "pre-rebase", options.upstream_arg, >> - argc ? argv[0] : NULL, NULL)) >> -+ run_hooks("pre-rebase", &hook_opt)) { >> -+ run_hooks_opt_clear(&hook_opt); >> ++ run_hooks_oneshot("pre-rebase", &hook_opt)) > > Ok, it just uses the oneshot call instead. Looks good. >> 10: 1d087269303 ! 11: 050f20d14f0 am: convert applypatch hooks to use config >> 11: 32eec5dc2f0 ! 12: ac875d284da hooks: convert 'post-checkout' hook to hook library >> 12: e9fa3f67593 ! 13: 69763bc2255 merge: use config-based hooks for post-merge hook > > The above all just use the oneshot instead, and have some minor tweaks > to commit messages to lose references to the config. Looks good. > >> 13: 12347d901bb ! 14: 2ca1ca1b8e4 git hook run: add an --ignore-missing flag >> @@ Documentation/git-hook.txt: optional `--` (or `--end-of-options`, see linkgit:gi >> linkgit:githooks[5] >> >> ## builtin/hook.c ## >> +@@ >> + #include "strvec.h" >> + >> + #define BUILTIN_HOOK_RUN_USAGE \ >> +- N_("git hook run <hook-name> [-- <hook-args>]") >> ++ N_("git hook run [--ignore-missing] <hook-name> [-- <hook-args>]") > > Ok. We update the usage in the macro here instead. Nice. > >> @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix) >> - /* >> - * We are not using run_hooks() because we'd like to detect >> - * missing hooks. Let's find it ourselves and call >> -- * run_found_hooks() instead. >> -+ * run_found_hooks() instead... >> - */ >> + git_config(git_default_config, NULL); >> + >> hook_name = argv[0]; >> ++ if (ignore_missing) >> ++ return run_hooks_oneshot(hook_name, &opt); >> hook_path = find_hook(hook_name); >> if (!hook_path) { >> -+ /* ... act like run_hooks() under --ignore-missing */ >> -+ if (ignore_missing) >> -+ return 0; >> error("cannot find a hook named %s", hook_name); >> - return 1; >> - } > > Hm. I actually think this was clearer before. I *think* that is because > you and I have different opinions on the clarity of multiple returns :) > > But I think it is a little confusing to say "ok, we will call an > entirely different entry point in case of this one thing which we will > have to check anyways". Tried to reword this per the above.. >> 14: 71d209b4077 ! 15: 5b66b04bec7 send-email: use 'git hook run' for 'sendemail-validate' >> @@ git-send-email.perl: sub validate_patch { >> if ($repo) { >> + my $hook_name = 'sendemail-validate'; >> my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks'); >> -- my $validate_hook = catfile($hooks_path, >> + require File::Spec; >> +- my $validate_hook = File::Spec->catfile($hooks_path, >> - 'sendemail-validate'); >> -+ my $validate_hook = catfile($hooks_path, $hook_name); >> ++ my $validate_hook = File::Spec->catfile($hooks_path, $hook_name); > > Ok, we are adapting to some other change around File::Spec that happened > to git-send-email.perl elsewhere. The change to this patch looks good to > me. > >> my $hook_error; >> if (-x $validate_hook) { >> - my $target = abs_path($fn); >> + require Cwd; >> @@ git-send-email.perl: sub validate_patch { >> chdir($repo->wc_path() or $repo->repo_path()) >> or die("chdir: $!"); >> 15: 246a82b55b2 = 16: 14a37a43db2 git-p4: use 'git hook' to run hooks >> 16: e3f8482d803 ! 17: ad5d0e0e7de commit: use hook.h to execute hooks >> @@ Metadata >> Author: Emily Shaffer <emilyshaffer@xxxxxxxxxx> >> >> ## Commit message ## >> - commit: use hook.h to execute hooks >> + commit: convert {pre-commit,prepare-commit-msg} hook to hook.h >> >> - Teach run_commit_hook() to call hook.h instead of run-command.h. This >> - covers 'pre-commit', 'commit-msg', and >> - 'prepare-commit-msg'. >> - >> - Additionally, ask the hook library - not run-command - whether any >> - hooks will be run, as it's possible hooks may exist in the config but >> - not the hookdir. >> - >> - Because all but 'post-commit' hooks are expected to make some state >> - change, force all but 'post-commit' hook to run in series. 'post-commit' >> - "is meant primarily for notification, and cannot affect the outcome of >> - `git commit`," so it is fine to run in parallel. I tried to address all of the above. > I am a little bummed that the patch reordering effectively removes the > justification for parallel/serial execution for each hook from 'git > blame'. Oh well. But it'll be there if you add it to your commit to migrate them to the parallel macros, and that'll be easier to find with "blame" than having it tangled up in the previous "do all the hook things" changes. So a note to reword/rebase that into your series... >> + Move these hooks hook away from run-command.h to and over to the new >> + hook.h library. >> >> Signed-off-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> >> ## commit.c ## > > Otherwise the main change here is dealing with other improvements > earlier in the series, like 'hook_exists()' and 'run_hooks_oneshot()'. > Looks fine. > >> 17: 6ed61071c5e ! 18: 3d3a33e2674 read-cache: convert post-index-change hook to use config >> @@ Metadata >> Author: Emily Shaffer <emilyshaffer@xxxxxxxxxx> >> >> ## Commit message ## >> - read-cache: convert post-index-change hook to use config >> + read-cache: convert post-index-change to use hook.h >> >> - By using hook.h instead of run-command.h to run, post-index-change hooks >> - can now be specified in the config in addition to the hookdir. >> - post-index-change is not run anywhere besides in read-cache.c. >> + Move the post-index-change hook away from run-command.h to and over to >> + the new hook.h library. > > I do not think it is necessary to drop the mention about > 'post-index-change' being run anywhere else, but that's just a nit, not > worth fixing :) I removed this in rewording/removing now-false mentions of "this does config". I didn't add the post-index-change mentiond back in, I figured to do it consistently I'd need to start with pre-auto-gc saying it's only handled in builtin/gc.c & all the way down the line... >> >> This removes the last direct user of run_hook_ve(), so we can make the >> function static now. It'll be removed entirely soon. >> @@ read-cache.c: static int do_write_locked_index(struct index_state *istate, struc >> + istate->updated_workdir ? "1" : "0", >> + istate->updated_skipworktree ? "1" : "0", >> + NULL); >> -+ run_hooks("post-index-change", &hook_opt); >> -+ run_hooks_opt_clear(&hook_opt); >> ++ run_hooks_oneshot("post-index-change", &hook_opt); >> + > > Mechanical change to using run_hooks_oneshot(). OK. > >> istate->updated_workdir = 0; >> istate->updated_skipworktree = 0; >> 18: e4ef3f4548a ! 19: 893f8666301 receive-pack: convert push-to-checkout hook to hook.h >> @@ Metadata >> ## Commit message ## >> receive-pack: convert push-to-checkout hook to hook.h >> >> - By using hook.h instead of run-command.h to invoke push-to-checkout, >> - hooks can now be specified in the config as well as in the hookdir. >> - push-to-checkout is not called anywhere but in builtin/receive-pack.c. >> + Move the push-to-checkout hook away from run-command.h to and over to >> + the new hook.h library. > > You made a comment in the commit before about the final instance of > run_hook_ve; do you want to make a similar one here about run_hook_le? Willd.o > The range-diff here is mechanical so looks fine to me. > >> 19: e3dda367ec9 = 20: 070433deba5 run-command: remove old run_hook_{le,ve}() hook API >> 20: 477d75bf579 = 21: 1028e0c1667 run-command: allow stdin for run_processes_parallel >> 21: b7c0ee9719a ! 22: 639e59e9ed0 hook: support passing stdin to hooks >> @@ Documentation/git-hook.txt: what those are. >> >> ## builtin/hook.c ## >> @@ >> + #include "strvec.h" >> >> - static const char * const builtin_hook_usage[] = { >> - N_("git hook <command> [...]"), >> -- N_("git hook run <hook-name> [-- <hook-args>]"), >> -+ N_("git hook run [<args>] <hook-name> [-- <hook-args>]"), >> - NULL >> - }; >> - >> - static const char * const builtin_hook_run_usage[] = { >> - N_("git hook run <hook-name> [-- <hook-args>]"), >> -+ N_("git hook run [--to-stdin=<path>] <hook-name> [-- <hook-args>]"), >> - NULL >> - }; >> + #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>]") >> > Just coping with the macro instead of the duplicated strings. Ok. > > The range-diff looks fine to me. > >> 22: 4035069a98c ! 23: 7d1925cca48 am: convert 'post-rewrite' hook to hook.h >> @@ builtin/am.c: static int run_applypatch_msg_hook(struct am_state *state) >> { >> - struct child_process cp = CHILD_PROCESS_INIT; >> - const char *hook = find_hook("post-rewrite"); >> -+ struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> - int ret; >> - >> +- int ret; >> +- >> - if (!hook) >> - return 0; >> - >> @@ builtin/am.c: static int run_applypatch_msg_hook(struct am_state *state) >> - cp.in = xopen(am_path(state, "rewritten"), O_RDONLY); >> - cp.stdout_to_stderr = 1; >> - cp.trace2_hook_name = "post-rewrite"; >> -+ strvec_push(&opt.args, "rebase"); >> -+ opt.path_to_stdin = am_path(state, "rewritten"); >> ++ struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> >> - ret = run_command(&cp); >> -+ ret = run_hooks("post-rewrite", &opt); >> ++ strvec_push(&opt.args, "rebase"); >> ++ opt.path_to_stdin = am_path(state, "rewritten"); >> >> - close(cp.in); >> -+ run_hooks_opt_clear(&opt); >> - return ret; >> +- return ret; >> ++ return run_hooks_oneshot("post-rewrite", &opt); >> } >> >> + /** > > Man, the range-diff did a terrible job of summarizing this :) > > The patch itself looks great, though, with the switch to using > run_hooks_oneshot. Thanks. > >> 24: da46c859c1c ! 25: 05d1085f7eb hook: provide stdin by string_list or callback >> @@ hook.c: static int pick_next_hook(struct child_process *cp, >> } else { >> cp->no_stdin = 1; >> } >> -@@ hook.c: int run_found_hooks(const char *hook_name, const char *hook_path, >> - run_processes_parallel_tr2(options->jobs, >> +@@ hook.c: int run_hooks(const char *hook_name, const char *hook_path, >> + run_processes_parallel_tr2(jobs, >> pick_next_hook, >> notify_start_failure, >> - NULL, >> @@ hook.c: int run_found_hooks(const char *hook_name, const char *hook_path, >> notify_hook_finished, >> &cb_data, >> "hook", >> -@@ hook.c: int run_hooks(const char *hook_name, struct run_hooks_opt *options) >> +@@ hook.c: int run_hooks(const char *hook_name, const char *hook_path, >> + >> + if (options->absolute_path) >> + strbuf_release(&abs_path); >> ++ free(my_hook.feed_pipe_cb_data); > > Nice catch. Thanks. > >> 25: 7343be28ef4 ! 26: 4b7175af2e5 hook: convert 'post-rewrite' hook in sequencer.c to hook.h >> 26: 85bf13a0835 ! 27: 3f24e056410 transport: convert pre-push hook to use config >> @@ Metadata >> Author: Emily Shaffer <emilyshaffer@xxxxxxxxxx> >> >> ## Commit message ## >> - transport: convert pre-push hook to use config >> + transport: convert pre-push hook to hook.h >> >> - By using the hook.h:run_hooks API, pre-push hooks can be specified in >> - the config as well as in the hookdir. >> + Move the pre-push hook away from run-command.h to and over to the new >> + hook.h library. >> >> Signed-off-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> @@ transport.c: static void die_with_unpushed_submodules(struct string_list *needs_ >> - int ret = 0, x; >> + int ret = 0; >> + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> -+ struct strbuf tmp = STRBUF_INIT; >> struct ref *r; >> - struct child_process proc = CHILD_PROCESS_INIT; >> - struct strbuf buf; >> @@ transport.c: static void die_with_unpushed_submodules(struct string_list *needs_ >> - finish_command(&proc); >> - return -1; >> - } >> -- >> -- sigchain_push(SIGPIPE, SIG_IGN); >> -+ struct string_list to_stdin = STRING_LIST_INIT_DUP; >> ++ struct string_list to_stdin = STRING_LIST_INIT_NODUP; >> >> +- sigchain_push(SIGPIPE, SIG_IGN); >> +- >> - strbuf_init(&buf, 256); >> + strvec_push(&opt.args, transport->remote->name); >> + strvec_push(&opt.args, transport->url); >> >> for (r = remote_refs; r; r = r->next) { >> ++ struct strbuf buf = STRBUF_INIT; > > Ah. It is not being freed because its lifetime is being managed by > to_stdin instead. Ok, cool! > >> ++ >> if (!r->peer_ref) continue; >> -@@ transport.c: static int run_pre_push_hook(struct transport *transport, >> + if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue; >> + if (r->status == REF_STATUS_REJECT_STALE) continue; >> if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) continue; >> if (r->status == REF_STATUS_UPTODATE) continue; >> >> - strbuf_reset(&buf); >> - strbuf_addf( &buf, "%s %s %s %s\n", >> -+ strbuf_reset(&tmp); >> -+ strbuf_addf(&tmp, "%s %s %s %s", >> ++ strbuf_addf(&buf, "%s %s %s %s", >> r->peer_ref->name, oid_to_hex(&r->new_oid), >> r->name, oid_to_hex(&r->old_oid)); >> - >> @@ transport.c: static int run_pre_push_hook(struct transport *transport, >> - ret = -1; >> - break; >> - } >> -+ string_list_append(&to_stdin, tmp.buf); >> ++ string_list_append(&to_stdin, strbuf_detach(&buf, NULL)); >> } >> >> - strbuf_release(&buf); >> @@ transport.c: static int run_pre_push_hook(struct transport *transport, >> - x = finish_command(&proc); >> - if (!ret) >> - ret = x; >> -+ ret = run_hooks("pre-push", &opt); >> -+ run_hooks_opt_clear(&opt); >> -+ strbuf_release(&tmp); >> ++ ret = run_hooks_oneshot("pre-push", &opt); >> ++ to_stdin.strdup_strings = 1; > > And this is a typical change to the oneshot function. Cool. > >> + string_list_clear(&to_stdin, 0); >> >> return ret; >> -: ----------- > 28: ecf75f33233 hook tests: test for exact "pre-push" hook input >> -: ----------- > 29: 2c961be94b4 hook tests: use a modern style for "pre-push" tests > > I'll take a look at these separately. > >> 27: 331014bad17 ! 30: 1ce456f9d9d reference-transaction: use hook.h to run hooks >> @@ refs.c: int ref_update_reject_duplicates(struct string_list *refnames, >> const char *state) >> { >> - struct child_process proc = CHILD_PROCESS_INIT; >> - struct strbuf buf = STRBUF_INIT; >> +- struct strbuf buf = STRBUF_INIT; >> - const char *hook; >> + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> -+ struct string_list to_stdin = STRING_LIST_INIT_DUP; >> ++ struct string_list to_stdin = STRING_LIST_INIT_NODUP; >> int ret = 0, i; >> -+ char o[GIT_MAX_HEXSZ + 1], n[GIT_MAX_HEXSZ + 1]; >> >> - hook = find_hook("reference-transaction"); >> - if (!hook) >> -+ if (!hook_exists("reference-transaction")) >> - return ret; >> - >> +- return ret; >> +- >> - strvec_pushl(&proc.args, hook, state, NULL); >> - proc.in = -1; >> - proc.stdout_to_stderr = 1; >> @@ refs.c: int ref_update_reject_duplicates(struct string_list *refnames, >> - >> - ret = start_command(&proc); >> - if (ret) >> -- return ret; >> -- >> ++ if (!hook_exists("reference-transaction")) >> + return ret; >> + >> - sigchain_push(SIGPIPE, SIG_IGN); >> + strvec_push(&opt.args, state); >> >> for (i = 0; i < transaction->nr; i++) { >> struct ref_update *update = transaction->updates[i]; >> -+ oid_to_hex_r(o, &update->old_oid); >> -+ oid_to_hex_r(n, &update->new_oid); >> ++ struct strbuf buf = STRBUF_INIT; > > Ah, this is doing the same thing as with 'pre-push'. Cool. > >> >> - strbuf_reset(&buf); >> +- strbuf_reset(&buf); >> - strbuf_addf(&buf, "%s %s %s\n", >> -- oid_to_hex(&update->old_oid), >> -- oid_to_hex(&update->new_oid), >> -- update->refname); >> ++ strbuf_addf(&buf, "%s %s %s", >> + oid_to_hex(&update->old_oid), >> + oid_to_hex(&update->new_oid), >> + update->refname); >> - >> - if (write_in_full(proc.in, buf.buf, buf.len) < 0) { >> - if (errno != EPIPE) >> - ret = -1; >> - break; >> - } >> -+ strbuf_addf(&buf, "%s %s %s", o, n, update->refname); >> -+ string_list_append(&to_stdin, buf.buf); >> ++ string_list_append(&to_stdin, strbuf_detach(&buf, NULL)); >> } >> >> - close(proc.in); >> - sigchain_pop(SIGPIPE); >> +- strbuf_release(&buf); >> + opt.feed_pipe = pipe_from_string_list; >> + opt.feed_pipe_ctx = &to_stdin; >> + >> -+ ret = run_hooks("reference-transaction", &opt); >> -+ run_hooks_opt_clear(&opt); >> - strbuf_release(&buf); >> ++ ret = run_hooks_oneshot("reference-transaction", &opt); >> ++ to_stdin.strdup_strings = 1; > > And turning on strdup_strings here means we will free them when we call > string_list_clear. Ok. > >> + string_list_clear(&to_stdin, 0); >> >> - ret |= finish_command(&proc); >> 32: db70b59b3bd ! 35: ceef2f3e804 receive-pack: convert receive hooks to hook.h >> @@ builtin/receive-pack.c: static void hook_output_to_sideband(struct strbuf *outpu >> +{ >> + struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; >> + struct receive_hook_feed_context ctx; >> -+ int rc; >> + struct command *iter = commands; >> + >> + /* if there are no valid commands, don't invoke the hook at all. */ >> @@ builtin/receive-pack.c: static void hook_output_to_sideband(struct strbuf *outpu >> + if (!iter) >> + return 0; >> + >> -+ /* pre-receive hooks should run in series as the hook updates refs */ >> -+ if (!strcmp(hook_name, "pre-receive")) >> -+ opt.jobs = 1; >> -+ > > Hm, interesting. I'll note this as a case to watch for when I add the > .jobs option back in. > > > Thanks. I'll take a look at the newly added/copied over patches next. > > - Emily
