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

On Fri, Mar 12, 2021 at 09:30:04AM +0100, Ævar Arnfjörð Bjarmason wrote:

On Thu, Mar 11 2021, Emily Shaffer wrote:

quoted

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 <redacted>
---

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.

It's not a comment for translators. It's a comment for someone helpful
who comes later and says "oh, none of this is marked for translation,
I'd better fix that."

quoted

+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?

The output is checked in the run tests later on. I can remove it for
this commit if you want.

quoted

[...]
+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...