Re: [PATCH] commit: use config-based hooks (config-based hooks part II)

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

> @@ -983,7 +984,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		return 0;
>  	}
>  
> -	if (!no_verify && find_hook("pre-commit")) {
> +	if (!no_verify && hook_exists("pre-commit", configured_hookdir_opt())) {

So, the migration from find_hook(), which is the traditional "only
on the filesystem in $GIT_DIR/hooks", to hook_exists(), which knows
the ones defined in the configuration files, is the same as the
previous round's.  Understood.

> diff --git a/commit.c b/commit.c
> index f53429c0ac..b338f50103 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -21,6 +21,7 @@
>  #include "commit-reach.h"
>  #include "run-command.h"
>  #include "shallow.h"
> +#include "hook.h"
>  
>  static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
>  
> @@ -1635,8 +1636,11 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>  {
>  	struct strvec hook_env = STRVEC_INIT;
>  	va_list args;
> +	const char *arg;
> +	struct strvec hook_args = STRVEC_INIT;
>  	int ret;
>  
> +
>  	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);

Noise.

> @@ -1646,9 +1650,13 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>  		strvec_push(&hook_env, "GIT_EDITOR=:");
>  
>  	va_start(args, name);
> -	ret = run_hook_ve(hook_env.v, name, args);
> +	while ((arg = va_arg(args, const char *)))
> +		strvec_push(&hook_args, arg);
>  	va_end(args);
> +
> +	ret = run_hooks(hook_env.v, name, &hook_args, configured_hookdir_opt());

And this is the calling convention change.  Generally speaking,
run_hook_ve() and friends (does it still have friends?) are on their
way out, and run_hooks() will be the only thing we need to learn in
the future.

> diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> index b3485450a2..fc93bc3d23 100755
> --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> @@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
>  . ./test-lib.sh
>  
>  HOOKDIR="$(git rev-parse --git-dir)/hooks"
> -PRECOMMIT="$HOOKDIR/pre-commit"
> -PREMERGE="$HOOKDIR/pre-merge-commit"
> +PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
> +PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"

What makes this change necessary?  Are we going to chdir around or
something?  Isn't the configuration file the ONLY thing that needs
to know their full location?

Should and does hook.*.command configuration honor ~ expansion?  I
think our $TRASH_DIRECTORY is our $HOME during tests, so 

	git config hook.foo.command "~/.git/hooks/success.sample"

might be a worthy thing to test.  Also, do we have a clear
definition on what happens to a relative pathname specified for the
hook commands, or is it left "undefined--do not do that"?  If the
former, that would also be worth testing.  I'd imagine that majority
of hooks defined in ~/.gitconfig WILL point at full path so testing
relative path may not matter too much for that particular use case,
but for ones defined in .git/config, I suspect it would be most
natural to take it as relative to the root of the working tree.

It may be a good change in the longer term, but it felt surprising
to see such a change that would affect 103-5=98 lines of existing
tests was made centrally here, without any explanation in the
proposed log message.

Thanks.

>  # Prepare sample scripts that write their $0 to actual_hooks
>  test_expect_success 'sample script setup' '
> @@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' '
>  	test_cmp expected_hooks actual_hooks
>  '
>  
> +# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
> +# instead
> +test_expect_success 'with succeeding hook (config-based)' '
> +	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
> +	test_when_finished "rm -f expected_hooks actual_hooks" &&
> +	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
> +	echo "$HOOKDIR/success.sample" >expected_hooks &&
> +	echo "more" >>file &&
> +	git add file &&
> +	git commit -m "more" &&
> +	test_cmp expected_hooks actual_hooks
> +'
> +
>  test_expect_success 'with succeeding hook (merge)' '
>  	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
>  	cp "$HOOKDIR/success.sample" "$PREMERGE" &&