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

> On Fri, Mar 12, 2021 at 09:20:05AM +0100, �var Arnfj�r� Bjarmason wrote:
>> 
>> 
>> On Thu, Mar 11 2021, Emily Shaffer wrote:
>> 
>> > 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 :)
>
>> > +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.

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