Re: [PATCH v2 3/4] hook: add list command

> On Fri, May 22, 2020 at 11:27:44AM +0100, Phillip Wood wrote:
>>
>> Hi Emily
>>
>> On 21/05/2020 19:54, Emily Shaffer wrote:
>>> [...]
>>> +static int hook_config_lookup(const char *key, const char *value, void *hook_key_cb)
>>> +{
>>> +	const char *hook_key = hook_key_cb;
>>> +
>>> +	if (!strcmp(key, hook_key)) {
>>> +		const char *command = value;
>>> +		struct strbuf hookcmd_name = STRBUF_INIT;
>>> +		struct list_head *pos = NULL, *tmp = NULL;
>>> +
>>> +		/* Check if a hookcmd with that name exists. */
>>> +		strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
>>> +		git_config_get_value(hookcmd_name.buf, &command);
>>
>> This looks dodgy to me. This code is called by git_config() as it parses
>> the config files, so it has not had a chance to fully populate the
>> config cache used by git_config_get_value(). I think the test below
>> passes because the hookcmd setting is set in the global file and the
>> hook setting is set in the local file so when we have already parsed the
>> hookcmd setting when we come to look it up. The same comment applies to
>> the hypothetical ordering config mentioned below. I think it would be
>> better to collect the list of hook.<event>.command settings in this
>> callback and then look up any hookcmd settings for those hook commands
>> after we've finished reading all of the config files.
> 
> git_config_get_value() calls repo_read_config(the_repository) if the
> config hasn't been fully parsed yet, so I think what you're worrying
> about is not an issue. It's ugly, I agree, but since the new hotness
> (git_config_get_value() and friends) doesn't offer the same
> functionality as the old solution (config origin) this seemed like an
> okay approach. As I understand it, moving this hookcmd lookup section
> outside of the config callback will save us up to one additional pass
> through the configs, at the expense of a more convoluted code path.

>>
>>> +
>>> +		if (!command)
>>> +			BUG("git_config_get_value overwrote a string it shouldn't have");
>>> +
>>> +		/*
>>> +		 * TODO: implement an option-getting callback, e.g.
>>> +		 *   get configs by pattern hookcmd.$value.*
>>> +		 *   for each key+value, do_callback(key, value, cb_data)
>>> +		 */
>>> +
>>> +		list_for_each_safe(pos, tmp, &hook_head) {
>>> +			struct hook *hook = list_entry(pos, struct hook, list);
>>> +			/*
>>> +			 * The list of hooks to run can be reordered by being redeclared
>>> +			 * in the config. Options about hook ordering should be checked
>>> +			 * here.
>>> +			 */
>>> +			if (0 == strcmp(hook->command.buf, command))
>>> +				remove_hook(pos);
>>> +		}
>>> +		emplace_hook(pos, command);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +struct list_head* hook_list(const struct strbuf* hookname)
>>> +{
>>> +	struct strbuf hook_key = STRBUF_INIT;
>>> +
>>> +	if (!hookname)
>>> +		return NULL;
>>> +
>>> +	strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
>>> +
>>> +	git_config(hook_config_lookup, (void*)hook_key.buf);
>>> +
>>> +	return &hook_head;
>>> +}
>>> diff --git a/hook.h b/hook.h
>>> new file mode 100644
>>> index 0000000000..aaf6511cff
>>> --- /dev/null
>>> +++ b/hook.h
>>> @@ -0,0 +1,15 @@
>>> +#include "config.h"
>>> +#include "list.h"
>>> +#include "strbuf.h"
>>> +
>>> +struct hook
>>> +{
>>> +	struct list_head list;
>>> +	enum config_scope origin;
>>> +	struct strbuf command;
>>> +};
>>> +
>>> +struct list_head* hook_list(const struct strbuf *hookname);
>>> +
>>> +void free_hook(struct hook *ptr);
>>> +void clear_hook_list(void);
>>> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
>>> index 34b0df5216..4e46d7dd4e 100755
>>> --- a/t/t1360-config-based-hooks.sh
>>> +++ b/t/t1360-config-based-hooks.sh
>>> @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
>>>   
>>>   . ./test-lib.sh
>>>   
>>> -test_expect_success 'git hook command does not crash' '
>>> -	git hook
>>> +test_expect_success 'git hook rejects commands without a mode' '
>>> +	test_must_fail git hook pre-commit
>>> +'
>>> +
>>> +
>>> +test_expect_success 'git hook rejects commands without a hookname' '
>>> +	test_must_fail git hook list
>>> +'
>>> +
>>> +test_expect_success 'setup hooks in global, and local' '
>>> +	git config --add --local hook.pre-commit.command "/path/ghi" &&
>>
>> Can I make a plea for the use of test_config please. Writing tests which
>> rely on previous tests for their set-up creates a chain of hidden
>> dependencies that make it hard to add/alter tests later or run a subset
>> of the tests when developing a new patch. t3404-rebase-interactive.sh is
>> a prime example of this and I dread touching it.
> 
> Sure. I'll redo them.

>>
>>> +	git config --add --global hook.pre-commit.command "/path/def"
>>> +'
>>> +
>>> +test_expect_success 'git hook list orders by config order' '
>>> +	cat >expected <<-\EOF &&
>>> +	global:	/path/def
>>> +	local:	/path/ghi
>>> +	EOF
>>> +
>>> +	git hook list pre-commit >actual &&
>>> +	test_cmp expected actual
>>> +'
>>> +
>>> +test_expect_success 'git hook list dereferences a hookcmd' '
>>> +	git config --add --local hook.pre-commit.command "abc" &&
>>> +	git config --add --global hookcmd.abc.command "/path/abc" &&
>>> +
>>> +	cat >expected <<-\EOF &&
>>> +	global:	/path/def
>>> +	local:	/path/ghi
>>> +	local:	/path/abc
>>
>> We should make it clear in the documentation that the config origin
>> applies to the hook setting, even though we display the hookcmd command
>> which is set globally here for the last hook.
> 
> One of the suggestions from our UX team last week was to make this list
> output clearer to indicate the origin of the command plus the origin of
> the hookcmd object; I'll try to straighten this out and make sure the
> doc agrees.
> 
>   - Emily
>