Re: [PATCH v8 04/37] hook: include hookdir hook in list

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 12, 2021, 08:30 UTC
Message-ID: <87lfashj03.fsf@evledraar.gmail.com>
In-Reply-To: <20210311021037.3001235-5-emilyshaffer@google.com>

On Thu, Mar 11 2021, Emily Shaffer wrote:

Show 33 quoted lines

> Historically, hooks are declared by placing an executable into
> $GIT_DIR/hooks/$HOOKNAME (or $HOOKDIR/$HOOKNAME). Although hooks taken
> from the config are more featureful than hooks placed in the $HOOKDIR,
> those hooks should not stop working for users who already have them.
> Let's list them to the user, but instead of displaying a config scope
> (e.g. "global: blah") we can prefix them with "hookdir:".
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>
> Notes:
>     Since v7, fix some nits from Jonathan Tan. The largest is to move reference to
>     "hookdir annotation" from this commit to the next one which introduces the
>     hook.runHookDir option.
>
>  builtin/hook.c                | 11 +++++++++--
>  hook.c                        | 17 +++++++++++++++++
>  hook.h                        |  1 +
>  t/t1360-config-based-hooks.sh | 19 +++++++++++++++++++
>  4 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/hook.c b/builtin/hook.c
> index bb64cd77ca..c8fbfbb39d 100644
> --- a/builtin/hook.c
> +++ b/builtin/hook.c
> @@ -40,10 +40,15 @@ static int list(int argc, const char **argv, const char *prefix)
>  
>  	list_for_each(pos, head) {
>  		struct hook *item = list_entry(pos, struct hook, list);
> -		if (item)
> +		item = list_entry(pos, struct hook, list);
> +		if (item) {
> +			/* Don't translate 'hookdir' - it matches the config */

Let's prefix comments for translators with /* TRANSLATORS: .., see the coding style doc. That's what they'll see, and this is useful to them.

Better yet have a note here about the first argument being 'system', 'local' etc., which I had to source spelunge for, and translators won't have any idea about unless the magic parameter is documented.

> +setup_hookdir () {
> +	mkdir .git/hooks
> +	write_script .git/hooks/pre-commit <<-EOF
> +	echo \"Legacy Hook\"

Nit, "'s not needed, but it also seems nothing uses this, so if it's just a pass-through script either "exit 0", or actually check if it's run or something?

Show 11 quoted lines

> [...]
> +test_expect_success 'git hook list shows hooks from the hookdir' '
> +	setup_hookdir &&
> +
> +	cat >expected <<-EOF &&
> +	hookdir: $(pwd)/.git/hooks/pre-commit
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
> +'

Ah, so it's just checking if it exists...