Re: [PATCH v8 03/37] hook: add list command

On Wed, Mar 24 2021, Emily Shaffer wrote:

On Fri, Mar 12, 2021 at 09:20:05AM +0100, �var Arnfj�r� Bjarmason wrote:

quoted

On Thu, Mar 11 2021, Emily Shaffer wrote:

quoted

diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
new file mode 100644
index 0000000000..71449ecbc7
--- /dev/null
+++ b/Documentation/config/hook.txt

@@ -0,0 +1,9 @@
+hook.<command>.command::
+	A command to execute during the <command> hook event. This can be an
+	executable on your device, a oneliner for your shell, or the name of a
+	hookcmd. See linkgit:git-hook[1].
+
+hookcmd.<name>.command::
+	A command to execute during a hook for which <name> has been specified
+	as a command. This can be an executable on your device or a oneliner for
+	your shell. See linkgit:git-hook[1].

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 9eeab0009d..f19875ed68 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt

@@ -8,12 +8,65 @@ git-hook - Manage configured hooks
 SYNOPSIS
 --------
 [verse]
-'git hook'
+'git hook' list <hook-name>

Having just read this far (maybe this pattern is shared in the rest of
the series): Let's just squash this and the 2nd patch together.

Sometimes it's worth doing the scaffolding first, but adding a new
built-in is so trivial that I don't think it's worth it, and it just
results in back & forth churn like the above...

Yeah, I think you are right here :)

quoted

quoted

+void free_hook(struct hook *ptr)
+{
+	if (ptr) {
+		strbuf_release(&ptr->command);
+		free(ptr);
+	}
+}

Neither strbuf_release() nor free() need or should have a "if (ptr)" guard.

I'll take free() out of the if guard, but I think
'strbuf_release(&<null>->command)' will go poorly - dereferencing the
NULL to even invoke strbuf_release will not be a happy time, and
strbuf_release internally is not NULL-resistant.

Sorry I meant something like:

    if (ptr) strbuf_release(&ptr->command);
    free(ptr);

But maybe even more idiomatic would be:

    if (!ptr)
	return;
    strbuf_release(&ptr->command);
    free(ptr);

Or some other variant of checking teh container struct early. Anyway,
this doesn't really matter, per a below comment I had more meaningful
feedback in [1]. Most of my other traffic on this topic (including this)
was some stream-of-consciousness notes as I went along.

quoted

quoted

+struct list_head* hook_list(const struct strbuf* hookname)
+{
+	struct strbuf hook_key = STRBUF_INIT;
+	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
+	struct hook_config_cb cb_data = { &hook_key, hook_head };
+
+	INIT_LIST_HEAD(hook_head);
+
+	if (!hookname)
+		return NULL;

...if a strbuf being passed in is NULL?

Yeah, I think this is misplaced. But since it sounds like generally
folks don't like having the strbuf at the input here, I will address the
error checking then also.

quoted

quoted

[...]
+ROOT=
+if test_have_prereq MINGW
+then
+	# In Git for Windows, Unix-like paths work only in shell scripts;
+	# `git.exe`, however, will prefix them with the pseudo root directory
+	# (of the Unix shell). Let's accommodate for that.
+	ROOT="$(cd / && pwd)"
+fi

I didn't read up on previous rounds, but if we're squashing this into 02
having a seperate commit summarizing this little hack would be most
welcome, or have it in this commit message.

Sure. I squashed it in from a commit dscho sent, so I can preserve that
commit in tree instead.

quoted

Isn't this sort of thing generally usable, maybe we can add it under a
longer variable name to test-lib.sh?

I wonder. `git grep cd \/ &&` shows me that this hack also happens in
t1509-root-work-tree.sh. I think most tests must use relative paths, so
this must not be in broad use? But since it's not used elsewhere I feel
ambivalent about adding a helper to test-lib.sh. I can if you feel
strongly :)

After I sent this I saw that pretty much the same thing is happening in
t1300-config.sh for the --show-origin option.

    ! test_have_prereq MINGW ||
    HOME="$(pwd)" # convert to Windows path

I don't feel strongly about this at all, but per the outstanding
feedback I had in[1] I wondered whether this whole thing wouln't be
better as some variant of "git config --show-origin",

1. https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/#t (local)