Re: [PATCH] commit: use config-based hooks (config-based hooks part II)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 16, 2020, 18:34 UTC
- Message-ID
- <xmqq4kmuc951.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20201014232517.3068298-1-emilyshaffer@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
Show 6 quoted lines
> @@ -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.
Show 22 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.
Show 12 quoted lines
> 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.
Show 22 quoted lines
> # 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" &&