Re: [PATCH 3/3] sequencer: run post-commit hook

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2019, 04:27 UTC
Message-ID: <xmqqftjzj49u.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.1910102324140.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 13 quoted lines

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d898a57f5d..adb8c89c60 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1653,7 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>
>>  	repo_rerere(the_repository, 0);
>>  	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>> -	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
>> +	run_post_commit_hook(use_editor, get_index_file());
>
> Does it really make sense to abstract the hook name away? It adds a lot
> of churn for just two callers...

After looking at the three patches, I do not think so.

Show 18 quoted lines

>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index d2f1d5bd23..d9217235b6 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1467,4 +1467,21 @@ test_expect_success 'valid author header when author contains single quote' '
>>  	test_cmp expected actual
>>  '
>>
>> +test_expect_success 'post-commit hook is called' '
>> +	test_when_finished "rm -f .git/hooks/post-commit commits" &&
>> +	mkdir -p .git/hooks &&
>> +	write_script .git/hooks/post-commit <<-\EOS &&
>> +	git rev-parse HEAD >>commits
>
> Should `commits` be initialized before this script is written, e.g.
> using
>
> 	>commits &&

Yes.

Show 8 quoted lines

>> +	git rev-list --no-walk=unsorted HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} \
>> +		HEAD@{1} HEAD >expected &&
>
> Wouldn't this be better as:
>
> 	git rev-parse HEAD@{5} HEAD@{4} HEAD@{3} HEAD@{2} HEAD@{1} HEAD \
> 		>expect &&
>

Yes.

>> +	test_cmp expected commits
>
> We usually use the name `expect` instead of `expected` in the test
> suite.

And the actual output file is called 'actual'.

Thanks.