Re: [PATCH v4 3/9] hook: add list command

On Wed, Sep 23, 2020 at 04:04:51PM -0700, Jonathan Tan wrote:

quoted

  $ git hook list pre-commit
  ~/baz/from/hookcmd.sh
  ~/bar.sh

In the tests below, there is a "local:" prefix (or similar). It's
clearer if the commit message has that too.

Also, looking at a later commit, the "list" command probably should
include the legacy hook if it exists.

Have added it as a separate patch for v5, hopefully that will make more
sense.

quoted

+static void emplace_hook(struct list_head *pos, const char *command)
+{
+	struct hook *to_add = malloc(sizeof(struct hook));
+	to_add->origin = current_config_scope();
+	strbuf_init(&to_add->command, 0);
+	/* even with use_shell, run_command() needs quotes */
+	strbuf_addf(&to_add->command, "'%s'", command);
+
+	list_add_tail(&to_add->list, pos);
+}

It might be odd to a programmer reading this that an existing "struct
hook" with the same name is not reused - the scanning of the list done
in hook_config_lookup() could probably go here instead.

Sure, done.

quoted

+test_expect_success 'git hook list orders by config order' '
+	setup_hooks &&
+
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi

Will the "global" strings etc. be translated? If yes, it's probably not
worth it to align the paths in this way.

Asked more offline. Jonathan was saying that translation might result in
scope name + tab character leaving the path in different columns
depending on the scope anyways, so there's no point in using a tab
character instead of a space character here. That seems reasonable; I'll
switch.

 - Emily