Re: [PATCH 3/5] sequencer: add support for multiple hooks

[Phillip Wood <phillip.wood123@gmail.com>]

On 24/04/2019 23:46, brian m. carlson wrote:
> On Wed, Apr 24, 2019 at 10:51:56AM +0100, Phillip Wood wrote:
>> On 24/04/2019 01:49, brian m. carlson wrote:
>>> Add support for multiple post-rewrite hooks, both for "git commit
>>> --amend" and "git rebase".
>>>
>>> Additionally add support for multiple prepare-commit-msg hooks.
>>>
>>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>>> ---
>>>   builtin/am.c                       | 28 ++++++---
>>
>> Having read the patch subject I was surprised to see this touching
>> bulitin/am.c
> 
> I can rewrite the subject. Unfortunately, the same hook for rebase goes
> through two wildly different modules, so in order to completely convert
> the post-rewrite hook, I have to update both at the same time.

It makes sense to convert both in the same commit, it's just that when I 
see "sequencer" I think of the subset of rebase (-k/-i/-m/-r/-x) that 
uses sequencer.c

>>> +static void run_interactive_rewrite_hook(void)
>>> +{
>>> +	struct string_list *hooks;
>>> +	struct string_list_item *p;
>>> +	struct child_process child;
>>> +
>>> +	hooks = find_hooks("post-rewrite");
>>> +	if (!hooks)
>>> +		return;
>>> +
>>> +	for_each_string_list_item(p, hooks) {
>>> +		child_process_init(&child);
>>> +
>>> +		child.in = open(rebase_path_rewritten_list(),
>>> +			O_RDONLY);
>>> +		child.stdout_to_stderr = 1;
>>> +		child.trace2_hook_name = "post-rewrite";
>>> +		argv_array_push(&child.args, p->string);
>>> +		argv_array_push(&child.args, "rebase");
>>> +		if (run_command(&child))
>>> +			break;
>>> +	}
>>> +	free_hooks(hooks);
>>>   }
>>
>> If you're adding a function to do this it would be nice to use it from
>> am.c as well rather than duplicating essentially the same code. Is there
>> any way to use a helper to run all the hooks, rather than introducing a
>> similar loop everywhere where we call a hook?
> 
> It's becoming clear to me that a helper is probably going to be cleaner,
> so I'll add one in for v2.
> 
>>>   void commit_post_rewrite(struct repository *r,
>>> @@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,
>>>   	char *amend_author = NULL;
>>>   	const char *hook_commit = NULL;
>>>   	enum commit_msg_cleanup_mode cleanup;
>>> +	struct string_list *hooks;
>>>   	int res = 0;
>>>   
>>>   	if (parse_head(r, &current_head))
>>> @@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,
>>>   		goto out;
>>>   	}
>>>   
>>> -	if (find_hook("prepare-commit-msg")) {
>>> +	hooks = find_hooks("prepare-commit-msg");
>>> +	if (hooks) {
>>> +		free_hooks(hooks);
>>
>> I think you forgot to update run_prepare_commit_msg_hook(), it should
>> probably be passed this list now. It might be outside the scope of this
>> series but unifying this with builtin/commit.c
> 
> run_prepare_commit_msg_hook calls run_hook_le, which looks up that value
> itself. It's unfortunate that we have to do it twice, but we need to
> know whether we need to re-read the commit msg or not. I can explain
> this further in the commit message.

Thanks for explaining that, it would be nice to have that in the commit 
message (I probably should have read the previous two patches to see 
what run_hook_le() was doing).

Best Wishes

Phillip


>>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
>>> index ba8bd1b514..5b83f037b5 100755
>>> --- a/t/t7505-prepare-commit-msg-hook.sh
>>> +++ b/t/t7505-prepare-commit-msg-hook.sh
>>> @@ -3,6 +3,7 @@
>>>   test_description='prepare-commit-msg hook'
>>>   
>>>   . ./test-lib.sh
>>> +. "$TEST_DIRECTORY/lib-hooks.sh"
>>>   
>>>   test_expect_success 'set up commits for rebasing' '
>>>   	test_commit root &&
>>> @@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
>>>   	test $(grep -c prepare-commit-msg actual) = 1
>>>   '
>>>   
>>> +commit_command () {
>>> +	echo "$1" >>file &&
>>> +	git add file &&
>>> +	git commit -m "$1"
>>> +}
>>> +
>>> +test_multiple_hooks prepare-commit-msg commit_command
>>
>> It's not clear to me that this is testing the sequencer
> 
> You're right. I need to adopt a different approach here.
>