Thread (581 messages) flat view 581 messages, 16 authors, 2021-10-12

Re: [PATCH v4 00/36] Run hooks via "git run hook" & hook library

From: Emily Shaffer <hidden>
Date: 2021-08-19 23:40:25 

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?

    ++	 * 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.

    ++ */
    ++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 [off-list ref]
     
      ## 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".

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 [off-list ref]
     
      ## 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 am a little bummed that the patch reordering effectively removes the
justification for parallel/serial execution for each hook from 'git
blame'. Oh well.

    +    Move these hooks hook away from run-command.h to and over to the new
    +    hook.h library.
     
         Signed-off-by: Emily Shaffer [off-list ref]
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## 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 [off-list ref]
     
      ## 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 :)

     
         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?

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 [off-list ref]
     
      ## 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 [off-list ref]
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
    @@ 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
  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help